From 3aca477a1b58714754fea3a26d0892fffc51db6b Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Sat, 27 Mar 2021 18:47:24 +0100 Subject: [PATCH] Move auth_token_state from multi to key_state 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. 2nd patch, squashed into the first one: 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. CVE: 2020-15078 Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli Message-Id: URL: non-public, embargoed Signed-off-by: Gert Doering --- doc/man-sections/example-fingerprint.rst | 0 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 +++++++++++----------- 5 files changed, 60 insertions(+), 55 deletions(-) create mode 100644 doc/man-sections/example-fingerprint.rst diff --git a/doc/man-sections/example-fingerprint.rst b/doc/man-sections/example-fingerprint.rst new file mode 100644 index 000000000..e69de29bb diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c index cc70c06c3..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 06c32ac1d..d6fd50bd3 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -166,6 +166,8 @@ enum ks_auth_state { 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. @@ -582,8 +584,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 33115eb6c..6fd51505e 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1269,7 +1269,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) { /* @@ -1278,7 +1278,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 @@ -1373,8 +1373,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