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

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

Commit Message

Arne Schwabe May 20, 2021, 3:11 p.m.
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 11, 2021, 12:23 a.m. | #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>

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