diff mbox series

[Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state

Message ID 20210520151148.2565578-1-arne@rfc2549.org
State Accepted
Delegated to: Antonio Quartulli
Headers show
Series [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state | expand

Commit Message

Arne Schwabe May 20, 2021, 5:11 a.m. UTC 
The auth-token check is tied to the username/password that is coming
via a specific SSL session, so keep the state also in the key_state
structure.

This also ensures the auth_token_state is always set to 0 on a new
session since we clear the key_state object at the start of a new
SSL session.

This is a prerequisite patch to fix 2020-15078 in the following two
commits.

This also applies the changes to the auth_token_test.c. The change of
tls_session to a pointer is necessary since before that we had tls_session
not tied to the multi and had two tls_session used in the test. One
implicitly in tls_multi and one explicit one. Merge these to one.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/auth_token.c                   | 12 +--
 src/openvpn/ssl_common.h                   |  4 +-
 src/openvpn/ssl_verify.c                   |  8 +-
 tests/unit_tests/openvpn/test_auth_token.c | 91 +++++++++++-----------
 4 files changed, 60 insertions(+), 55 deletions(-)

Comments

Antonio Quartulli June 10, 2021, 2:23 p.m. UTC | #1 
Hi,

On 20/05/2021 17:11, Arne Schwabe wrote:
> The auth-token check is tied to the username/password that is coming
> via a specific SSL session, so keep the state also in the key_state
> structure.
> 
> This also ensures the auth_token_state is always set to 0 on a new
> session since we clear the key_state object at the start of a new
> SSL session.
> 
> This is a prerequisite patch to fix 2020-15078 in the following two
> commits.
> 
> This also applies the changes to the auth_token_test.c. The change of
> tls_session to a pointer is necessary since before that we had tls_session
> not tied to the multi and had two tls_session used in the test. One
> implicitly in tls_multi and one explicit one. Merge these to one.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Looks good and does what it says. The key-state object is initialized by
key_state_init() which wipes the entire thing before setting fields,
therefore the auth-token-flags are indeed erased as expected during the
key-state life cycle.

No functional change, as expected.

Compile zoo is happy.

Basic connectivity tests with auth-user-pass along with auth-gen-token
have worked fine, including renegotiation.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering June 24, 2021, 1:21 a.m. UTC | #2 
Your patch has been applied to the master branch.

I did some stare-at-code and testing of my own, and most of this patch
is fairly straightforward - it's only that long because the auth-token-test
module is really extensive, and due to the pointer change, lots of lines
change.

Also tested with the "renegotiate, then reconnect from same port" nuisance
client, and that seems to be a bit worse now (this re-introduces "we send 
a PUSH_REPLY right away on client-reconnect-same-port") but I understand 
that this is fixed in 3/9+4/9 (and verified that it is).

commit 716049923e3e70c3de938d6da5d05f529ec515b5
Author: Arne Schwabe
Date:   Thu May 20 17:11:40 2021 +0200

     Move auth_token_state from multi to key_state

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210520151148.2565578-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22415.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index d571b686e..0ea6d1832 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -57,6 +57,7 @@  add_session_token_env(struct tls_session *session, struct tls_multi *multi,
         return;
     }
 
+    int auth_token_state_flags = session->key[KS_PRIMARY].auth_token_state_flags;
 
     const char *state;
 
@@ -64,9 +65,9 @@  add_session_token_env(struct tls_session *session, struct tls_multi *multi,
     {
         state = "Initial";
     }
-    else if (multi->auth_token_state_flags & AUTH_TOKEN_HMAC_OK)
+    else if (auth_token_state_flags & AUTH_TOKEN_HMAC_OK)
     {
-        switch (multi->auth_token_state_flags & (AUTH_TOKEN_VALID_EMPTYUSER|AUTH_TOKEN_EXPIRED))
+        switch (auth_token_state_flags & (AUTH_TOKEN_VALID_EMPTYUSER|AUTH_TOKEN_EXPIRED))
         {
             case 0:
                 state = "Authenticated";
@@ -98,8 +99,8 @@  add_session_token_env(struct tls_session *session, struct tls_multi *multi,
 
     /* We had a valid session id before */
     const char *session_id_source;
-    if (multi->auth_token_state_flags & AUTH_TOKEN_HMAC_OK
-        && !(multi->auth_token_state_flags & AUTH_TOKEN_EXPIRED))
+    if (auth_token_state_flags & AUTH_TOKEN_HMAC_OK
+        && !(auth_token_state_flags & AUTH_TOKEN_EXPIRED))
     {
         session_id_source = up->password;
     }
@@ -236,7 +237,8 @@  generate_auth_token(const struct user_pass *up, struct tls_multi *multi)
      * a new token with the empty username since we do not want to loose
      * the information that the username cannot be trusted
      */
-    if (multi->auth_token_state_flags & AUTH_TOKEN_VALID_EMPTYUSER)
+    struct key_state *ks = &multi->session[TM_ACTIVE].key[KS_PRIMARY];
+    if (ks->auth_token_state_flags & AUTH_TOKEN_VALID_EMPTYUSER)
     {
         hmac_ctx_update(ctx, (const uint8_t *) "", 0);
     }
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 61cae7419..53325e556 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -182,6 +182,8 @@  enum auth_deferred_result {
 struct key_state
 {
     int state;
+    /** The state of the auth-token sent from the client */
+    int auth_token_state_flags;
 
     /**
      * Key id for this key_state,  inherited from struct tls_session.
@@ -598,8 +600,6 @@  struct tls_multi
      * OpenVPN 3 clients sometimes wipes or replaces the username with a
      * username hint from their config.
      */
-    int auth_token_state_flags;
-    /**< The state of the auth-token sent from the client last time */
 
     /* For P_DATA_V2 */
     uint32_t peer_id;
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 0b41eea2d..7992a6eb9 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1484,7 +1484,7 @@  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
      */
     if (session->opt->auth_token_generate && is_auth_token(up->password))
     {
-        multi->auth_token_state_flags = verify_auth_token(up, multi, session);
+        ks->auth_token_state_flags = verify_auth_token(up, multi, session);
         if (session->opt->auth_token_call_auth)
         {
             /*
@@ -1493,7 +1493,7 @@  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
              * decide what to do with the result
              */
         }
-        else if (multi->auth_token_state_flags == AUTH_TOKEN_HMAC_OK)
+        else if (ks->auth_token_state_flags == AUTH_TOKEN_HMAC_OK)
         {
             /*
              * We do not want the EXPIRED or EMPTY USER flags here so check
@@ -1592,8 +1592,8 @@  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
              * the initial timestamp and session id can be extracted from it
              */
             if (!multi->auth_token
-                && (multi->auth_token_state_flags & AUTH_TOKEN_HMAC_OK)
-                && !(multi->auth_token_state_flags & AUTH_TOKEN_EXPIRED))
+                && (ks->auth_token_state_flags & AUTH_TOKEN_HMAC_OK)
+                && !(ks->auth_token_state_flags & AUTH_TOKEN_EXPIRED))
             {
                 multi->auth_token = strdup(up->password);
             }
diff --git a/tests/unit_tests/openvpn/test_auth_token.c b/tests/unit_tests/openvpn/test_auth_token.c
index dbde86318..69fc1f8c9 100644
--- a/tests/unit_tests/openvpn/test_auth_token.c
+++ b/tests/unit_tests/openvpn/test_auth_token.c
@@ -45,7 +45,7 @@  struct test_context {
     struct tls_multi multi;
     struct key_type kt;
     struct user_pass up;
-    struct tls_session session;
+    struct tls_session *session;
 };
 
 /* Dummy functions that do nothing to mock the functionality */
@@ -100,10 +100,11 @@  setup(void **state)
     }
     ctx->multi.opt.auth_token_generate = true;
     ctx->multi.opt.auth_token_lifetime = 3000;
+    ctx->session = &ctx->multi.session[TM_ACTIVE];
 
-    ctx->session.opt = calloc(1, sizeof(struct tls_options));
-    ctx->session.opt->renegotiate_seconds = 120;
-    ctx->session.opt->auth_token_lifetime = 3000;
+    ctx->session->opt = calloc(1, sizeof(struct tls_options));
+    ctx->session->opt->renegotiate_seconds = 120;
+    ctx->session->opt->auth_token_lifetime = 3000;
 
     strcpy(ctx->up.username, "test user name");
     strcpy(ctx->up.password, "ignored");
@@ -122,7 +123,7 @@  teardown(void **state)
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     wipe_auth_token(&ctx->multi);
 
-    free(ctx->session.opt);
+    free(ctx->session->opt);
     free(ctx);
 
     return 0;
@@ -135,7 +136,7 @@  auth_token_basic_test(void **state)
 
     generate_auth_token(&ctx->up, &ctx->multi);
     strcpy(ctx->up.password, ctx->multi.auth_token);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 }
 
@@ -146,7 +147,7 @@  auth_token_fail_invalid_key(void **state)
 
     generate_auth_token(&ctx->up, &ctx->multi);
     strcpy(ctx->up.password, ctx->multi.auth_token);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
     /* Change auth-token key */
@@ -155,13 +156,13 @@  auth_token_fail_invalid_key(void **state)
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     init_key_ctx(&ctx->multi.opt.auth_token_key, &key, &ctx->kt, false, "TEST");
 
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session), 0);
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), 0);
 
     /* Load original test key again */
     memset(&key, 0, sizeof(key));
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     init_key_ctx(&ctx->multi.opt.auth_token_key, &key, &ctx->kt, false, "TEST");
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
 }
@@ -176,32 +177,32 @@  auth_token_test_timeout(void **state)
     strcpy(ctx->up.password, ctx->multi.auth_token);
 
     /* No time has passed */
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
     /* Token before validity, should be rejected */
     now = 100000 - 100;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
 
     /* Token still in validity, should be accepted */
-    now = 100000 + 2*ctx->session.opt->renegotiate_seconds - 20;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    now = 100000 + 2*ctx->session->opt->renegotiate_seconds - 20;
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
     /* Token past validity, should be rejected */
-    now = 100000 + 2*ctx->session.opt->renegotiate_seconds + 20;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    now = 100000 + 2*ctx->session->opt->renegotiate_seconds + 20;
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
 
     /* Check if the mode for a client that never updates its token works */
     ctx->multi.auth_token_initial = strdup(ctx->up.password);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
     /* But not when we reached our timeout */
-    now = 100000 + ctx->session.opt->auth_token_lifetime + 1;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    now = 100000 + ctx->session->opt->auth_token_lifetime + 1;
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
 
     free(ctx->multi.auth_token_initial);
@@ -209,22 +210,22 @@  auth_token_test_timeout(void **state)
 
     /* regenerate the token util it hits the expiry */
     now = 100000;
-    while (now < 100000 + ctx->session.opt->auth_token_lifetime + 1)
+    while (now < 100000 + ctx->session->opt->auth_token_lifetime + 1)
     {
-        assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+        assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                          AUTH_TOKEN_HMAC_OK);
         generate_auth_token(&ctx->up, &ctx->multi);
         strcpy(ctx->up.password, ctx->multi.auth_token);
-        now += ctx->session.opt->renegotiate_seconds;
+        now += ctx->session->opt->renegotiate_seconds;
     }
 
 
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
     ctx->multi.opt.auth_token_lifetime = 0;
 
     /* Non expiring token should be fine */
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 }
 
@@ -253,7 +254,7 @@  auth_token_test_known_keys(void **state)
     assert_string_equal(now0key0, ctx->multi.auth_token);
 
     strcpy(ctx->up.password, ctx->multi.auth_token);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 }
 
@@ -277,25 +278,25 @@  auth_token_test_empty_user(void **state)
 
     generate_auth_token(&ctx->up, &ctx->multi);
     strcpy(ctx->up.password, ctx->multi.auth_token);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK);
 
     now = 100000;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED);
     strcpy(ctx->up.username, "test user name");
 
     now = 0;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_VALID_EMPTYUSER);
 
     strcpy(ctx->up.username, "test user name");
     now = 100000;
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED|AUTH_TOKEN_VALID_EMPTYUSER);
 
     zerohmac(ctx->up.password);
-    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session),
+    assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session),
                      0);
 }
 
@@ -304,30 +305,32 @@  auth_token_test_env(void **state)
 {
     struct test_context *ctx = (struct test_context *) *state;
 
-    ctx->multi.auth_token_state_flags = 0;
+    struct key_state *ks = &ctx->multi.session[TM_ACTIVE].key[KS_PRIMARY];
+    
+    ks->auth_token_state_flags = 0;
     ctx->multi.auth_token = NULL;
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "Initial");
 
-    ctx->multi.auth_token_state_flags = 0;
+    ks->auth_token_state_flags = 0;
     strcpy(ctx->up.password, now0key0);
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "Invalid");
 
-    ctx->multi.auth_token_state_flags = AUTH_TOKEN_HMAC_OK;
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    ks->auth_token_state_flags = AUTH_TOKEN_HMAC_OK;
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "Authenticated");
 
-    ctx->multi.auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED;
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    ks->auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED;
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "Expired");
 
-    ctx->multi.auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_VALID_EMPTYUSER;
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    ks->auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_VALID_EMPTYUSER;
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "AuthenticatedEmptyUser");
 
-    ctx->multi.auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED|AUTH_TOKEN_VALID_EMPTYUSER;
-    add_session_token_env(&ctx->session, &ctx->multi, &ctx->up);
+    ks->auth_token_state_flags = AUTH_TOKEN_HMAC_OK|AUTH_TOKEN_EXPIRED|AUTH_TOKEN_VALID_EMPTYUSER;
+    add_session_token_env(ctx->session, &ctx->multi, &ctx->up);
     assert_string_equal(lastsesion_statevalue, "ExpiredEmptyUser");
 }
 
@@ -351,7 +354,7 @@  auth_token_test_random_keys(void **state)
     assert_string_equal(random_token, ctx->multi.auth_token);
 
     strcpy(ctx->up.password, ctx->multi.auth_token);
-    assert_true(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session));
+    assert_true(verify_auth_token(&ctx->up, &ctx->multi, ctx->session));
 }
 
 
@@ -363,11 +366,11 @@  auth_token_test_key_load(void **state)
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     auth_token_init_secret(&ctx->multi.opt.auth_token_key, zeroinline, true);
     strcpy(ctx->up.password, now0key0);
-    assert_true(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session));
+    assert_true(verify_auth_token(&ctx->up, &ctx->multi, ctx->session));
 
     free_key_ctx(&ctx->multi.opt.auth_token_key);
     auth_token_init_secret(&ctx->multi.opt.auth_token_key, allx01inline, true);
-    assert_false(verify_auth_token(&ctx->up, &ctx->multi, &ctx->session));
+    assert_false(verify_auth_token(&ctx->up, &ctx->multi, ctx->session));
 }
 
 int