From: Arne Schwabe Date: Wed, 20 May 2026 23:07:34 +0000 (+0000) Subject: Ensure we only get the session from valid tokens for external-auth X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=423afa8e265a21527838540529a41fef834a4033;p=thirdparty%2Fopenvpn.git Ensure we only get the session from valid tokens for external-auth This also improves is_auth_token to check for the correct length and adds a few unit tests. This fixes an assertion when a user would send auth token with the right prefix but overall too short length. Reported-By: Haiyang Huang CVE: 2026-13122 Github: OpenVPN/openvpn-private-issues#118 Github: OpenVPN/openvpn-private-issues#128 Change-Id: I053cca5f42e3841cef8934c2114a8a159fbc0c29 Acked-by: MaxF --- diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst index eb8e27366..f420588d8 100644 --- a/doc/man-sections/server-options.rst +++ b/doc/man-sections/server-options.rst @@ -87,7 +87,9 @@ fast hardware. SSL/TLS authentication must be used in this mode. external-auth option unless the client could authenticate in another acceptable way (e.g. client certificate), otherwise returning success will lead to authentication bypass (as does returning success on a wrong - password from a script). + password from a script). In the case that Expired token is accepted + the token will keep the session id and start time from the original + (expired) token. **Note:** the username for ``--auth-gen-token`` can be overridden by ``--override-username``. In this case the client will be pushed also the diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c index d8ca12523..bd9021232 100644 --- a/src/openvpn/auth_token.c +++ b/src/openvpn/auth_token.c @@ -19,14 +19,33 @@ const char *auth_token_pem_name = "OpenVPN auth-token server key"; #define AUTH_TOKEN_SESSION_ID_LEN 12 -#define AUTH_TOKEN_SESSION_ID_BASE64_LEN (AUTH_TOKEN_SESSION_ID_LEN * 8 / 6) +#define AUTH_TOKEN_SESSION_ID_BASE64_LEN (OPENVPN_BASE64_LENGTH(AUTH_TOKEN_SESSION_ID_LEN)) -#if AUTH_TOKEN_SESSION_ID_LEN % 3 -#error AUTH_TOKEN_SESSION_ID_LEN needs to be multiple a 3 -#endif +/* We want our token to be a multiple of 3 bytes to avoid the base64 padding */ +static_assert(AUTH_TOKEN_SESSION_ID_LEN % 3 == 0, "AUTH_TOKEN_SESSION_ID_LEN needs to be multiple a 3"); +#define AUTH_TOKEN_HMAC_LEN SHA256_DIGEST_LENGTH /* Size of the data of the token (not b64 encoded and without prefix) */ -#define TOKEN_DATA_LEN (2 * sizeof(int64_t) + AUTH_TOKEN_SESSION_ID_LEN + 32) +#define TOKEN_DATA_LEN (2 * sizeof(int64_t) + AUTH_TOKEN_SESSION_ID_LEN + AUTH_TOKEN_HMAC_LEN) +#define TOKEN_DATA_BASE64_LEN (OPENVPN_BASE64_LENGTH(TOKEN_DATA_LEN)) + + +#define TOTAL_SESSION_TOKEN_LEN (strlen(SESSION_ID_PREFIX) + TOKEN_DATA_BASE64_LEN) + +/* Ensure that TOKEN_DATA_LEN is a multiple of 3 so the we avoid the base64 + * padding */ +static_assert(TOKEN_DATA_LEN % 3 == 0, "TOKEN_DATA_BASE64_LEN is not a multiple of 3"); + +bool +is_auth_token(const char *password) +{ + if (strlen(password) != TOTAL_SESSION_TOKEN_LEN) + { + return false; + } + + return (memcmp_constant_time(SESSION_ID_PREFIX, password, strlen(SESSION_ID_PREFIX)) == 0); +} static struct key_type auth_token_kt(void) @@ -106,7 +125,6 @@ add_session_token_env(struct tls_session *session, struct tls_multi *multi, * and being a multiple of 4 ensure that it a multiple of bytes * in the encoding */ - char session_id[AUTH_TOKEN_SESSION_ID_LEN * 2] = { 0 }; memcpy(session_id, session_id_source + strlen(SESSION_ID_PREFIX), AUTH_TOKEN_SESSION_ID_LEN * 8 / 6); @@ -172,7 +190,7 @@ generate_auth_token(const struct user_pass *up, struct tls_multi *multi) if (multi->auth_token_initial) { - /* Just enough space to fit 8 bytes+ 1 extra to decode a non-padded + /* Just enough space to fit 8 bytes + 1 extra to decode a non-padded * base64 string (multiple of 3 bytes). 9 bytes => 12 bytes base64 * bytes */ @@ -182,7 +200,7 @@ generate_auth_token(const struct user_pass *up, struct tls_multi *multi) char *initial_token_copy = string_alloc(multi->auth_token_initial, &gc); char *old_sessid = initial_token_copy + strlen(SESSION_ID_PREFIX); - char *old_tstamp_initial = old_sessid + AUTH_TOKEN_SESSION_ID_LEN * 8 / 6; + char *old_tstamp_initial = old_sessid + AUTH_TOKEN_SESSION_ID_BASE64_LEN; /* * We null terminate the old token just after the session ID to let @@ -284,7 +302,7 @@ check_hmac_token(hmac_ctx_t *ctx, const uint8_t *b64decoded, const char *usernam hmac_ctx_final(ctx, hmac_output); const uint8_t *hmac = b64decoded + TOKEN_DATA_LEN - 256 / 8; - return memcmp_constant_time(&hmac_output, hmac, 32) == 0; + return memcmp_constant_time(&hmac_output, hmac, AUTH_TOKEN_HMAC_LEN) == 0; } unsigned int diff --git a/src/openvpn/auth_token.h b/src/openvpn/auth_token.h index 7b66a8189..fd8317d80 100644 --- a/src/openvpn/auth_token.h +++ b/src/openvpn/auth_token.h @@ -117,11 +117,8 @@ void wipe_auth_token(struct tls_multi *multi); * @param password * @return whether the password string starts with the session token prefix */ -static inline bool -is_auth_token(const char *password) -{ - return (memcmp_constant_time(SESSION_ID_PREFIX, password, strlen(SESSION_ID_PREFIX)) == 0); -} +bool +is_auth_token(const char *password); /** * Checks if a client should be sent a new auth token to update its * current auth-token diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 31ecf13ad..9197adfba 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1651,10 +1651,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, struct tls_sessi { ks->auth_token_state_flags = verify_auth_token(up, multi, session); - /* If this is the first time we see an auth-token in this multi session, - * save it as initial auth token. This ensures using the - * same session ID and initial timestamp in new tokens */ - if (!multi->auth_token_initial) + /* If this is a valid token and the first time we see an auth-token + * in this multi session, save it as initial auth token. This ensures + * using the same session ID and initial timestamp in new tokens */ + if (!multi->auth_token_initial && (ks->auth_token_state_flags & AUTH_TOKEN_HMAC_OK)) { multi->auth_token_initial = strdup(up->password); } diff --git a/tests/unit_tests/openvpn/test_auth_token.c b/tests/unit_tests/openvpn/test_auth_token.c index c7ba5fd82..9e7b5b929 100644 --- a/tests/unit_tests/openvpn/test_auth_token.c +++ b/tests/unit_tests/openvpn/test_auth_token.c @@ -83,6 +83,8 @@ static const char *random_key = static const char *random_token = "SESS_ID_AT_ThhRItzOKNKrh3dfAAAAAFwzHpwAAAAAXDMenDdrq0RoH3dkA1f7O3wO+7kZcx2DusVZrRmFlWQM9HOb"; +static const char *last_session_state_value; +static char last_session_id_value[64]; static int setup(void **state) @@ -113,6 +115,10 @@ setup(void **state) init_key_ctx(&ctx->multi.opt.auth_token_key, &key, &ctx->kt, false, "TEST"); now = 0; + + CLEAR(last_session_id_value); + last_session_state_value = NULL; + return 0; } @@ -257,13 +263,17 @@ auth_token_test_known_keys(void **state) assert_int_equal(verify_auth_token(&ctx->up, &ctx->multi, ctx->session), AUTH_TOKEN_HMAC_OK); } -static const char *lastsesion_statevalue; + void setenv_str(struct env_set *es, const char *name, const char *value) { if (streq(name, "session_state")) { - lastsesion_statevalue = value; + last_session_state_value = value; + } + else if (streq(name, "session_id")) + { + strncpy(last_session_id_value, value, sizeof(last_session_id_value)); } } @@ -337,29 +347,50 @@ auth_token_test_env(void **state) ks->auth_token_state_flags = 0; ctx->multi.auth_token = NULL; add_session_token_env(ctx->session, &ctx->multi, &ctx->up); - assert_string_equal(lastsesion_statevalue, "Initial"); + assert_string_equal(last_session_state_value, "Initial"); + /* This generated a fresh token with a new session ID, + * check that this session id is part of the multi initial token and + * found at the right index */ + char *idx = strstr(ctx->multi.auth_token_initial, last_session_id_value); + assert_non_null(idx); + + ptrdiff_t offset = idx - ctx->multi.auth_token_initial; + assert_int_equal(offset, 11); + + CLEAR(last_session_id_value); ks->auth_token_state_flags = 0; - strcpy(ctx->up.password, now0key0); + strcpy(ctx->up.password, "somepassword"); + + free(ctx->multi.auth_token_initial); + ctx->multi.auth_token_initial = strdup(now0key0); + add_session_token_env(ctx->session, &ctx->multi, &ctx->up); - assert_string_equal(lastsesion_statevalue, "Invalid"); + assert_string_equal(last_session_state_value, "Initial"); + assert_string_equal(last_session_id_value, "0123456789abcdef"); + + CLEAR(last_session_id_value); + free(ctx->multi.auth_token_initial); + ctx->multi.auth_token_initial = NULL; ks->auth_token_state_flags = AUTH_TOKEN_HMAC_OK; + strcpy(ctx->up.password, now0key0); add_session_token_env(ctx->session, &ctx->multi, &ctx->up); - assert_string_equal(lastsesion_statevalue, "Authenticated"); + assert_string_equal(last_session_state_value, "Authenticated"); + assert_string_equal(last_session_id_value, "0123456789abcdef"); 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"); + assert_string_equal(last_session_state_value, "Expired"); 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"); + assert_string_equal(last_session_state_value, "AuthenticatedEmptyUser"); 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"); + assert_string_equal(last_session_state_value, "ExpiredEmptyUser"); } static void @@ -387,6 +418,45 @@ auth_token_test_random_keys(void **state) } +static void +auth_token_test_is_auth_token(void **state) +{ + /* Known-good tokens are recognised */ + assert_true(is_auth_token(now0key0)); + assert_true(is_auth_token(random_token)); + + /* Empty and clearly too-short strings are rejected */ + assert_false(is_auth_token("")); + assert_false(is_auth_token("foobar")); + assert_false(is_auth_token(SESSION_ID_PREFIX)); + assert_false(is_auth_token("SESS_ID_AT_Something_wild")); + + /* Since all auth token have the same length, get the length from + * now0key0 */ + const size_t auth_token_len = strlen(now0key0); + + /* Right length, incorrect prefix */ + char buf[256] = { 0 }; + assert_true(auth_token_len < sizeof(buf)); + memset(buf, 'A', auth_token_len); + buf[auth_token_len] = '\0'; + assert_false(is_auth_token(buf)); + + /* Correct prefix to ensure the previous test is valid */ + memcpy(buf, "SESS_ID_AT_", strlen("SESS_ID_AT_")); + assert_true(is_auth_token(buf)); + + /* Correct prefix, one byte too short */ + buf[auth_token_len - 1] = '\0'; + assert_false(is_auth_token(buf)); + + /* Correct prefix, one byte too long */ + strcpy(buf, now0key0); + buf[auth_token_len] = 'A'; + buf[auth_token_len + 1] = '\0'; + assert_false(is_auth_token(buf)); +} + static void auth_token_test_key_load(void **state) { @@ -415,7 +485,8 @@ main(void) cmocka_unit_test_setup_teardown(auth_token_test_random_keys, setup, teardown), cmocka_unit_test_setup_teardown(auth_token_test_key_load, setup, teardown), cmocka_unit_test_setup_teardown(auth_token_test_timeout, setup, teardown), - cmocka_unit_test_setup_teardown(auth_token_test_session_mismatch, setup, teardown) + cmocka_unit_test_setup_teardown(auth_token_test_session_mismatch, setup, teardown), + cmocka_unit_test(auth_token_test_is_auth_token), }; return cmocka_run_group_tests_name("auth-token tests", tests, NULL, NULL);