From: Arne Schwabe Date: Thu, 18 Jun 2026 14:33:43 +0000 (+0200) Subject: Ensure tls-crypt keys are not setup twice X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=01ee1693f960a696a613e7b83f9960002264a379;p=thirdparty%2Fopenvpn.git Ensure tls-crypt keys are not setup twice Commit 82ee2fe4b42d already did this when the session id stayed the same but forgot the other code path that could also lead to tls-crypt keys to be setup. This approach was a bit too fragile as it missed some other code path that might trigger the same behaviour. This commit changes the logic to directly infer if the key is already initialised instead of taking a proxy (like key state as the previous commit did). The fix in commit 7a6ab5773 (#121, #127) made sure that we do not try to extract the tls-crypt-v2 key multiple times and made the unit test basically not work as the extraction was skipped and then could also not fail anymore. I could work around it in the unit test but improving tls_wrap_free felt preferable. CVE: 2026-13698 Reported-By: Max Fillinger Github: OpenVPN/openvpn-private-issues#137 Github: OpenVPN/openvpn-private-issues#138 Change-Id: I3b5e4e84762aa253d46e69103f7b1e84ebefca1d Signed-off-by: Arne Schwabe Acked-By: Max Fillinger --- diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 85546020d..60df7ce84 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -3716,8 +3716,7 @@ tls_pre_decrypt(struct tls_multi *multi, const struct link_socket_actual *from, goto error; } - if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), from, session->opt, - true)) + if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), from, session->opt)) { goto error; } @@ -3775,7 +3774,7 @@ tls_pre_decrypt(struct tls_multi *multi, const struct link_socket_actual *from, if (op == P_CONTROL_SOFT_RESET_V1 && ks->state >= S_GENERATED_KEYS) { if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), from, - session->opt, false)) + session->opt)) { goto error; } @@ -3804,8 +3803,8 @@ tls_pre_decrypt(struct tls_multi *multi, const struct link_socket_actual *from, do_burst = true; } - if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), from, - session->opt, initial_packet)) + if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), + from, session->opt)) { /* if an initial packet in read_control_auth, we rather * error out than anything else */ diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index f90b24f1e..7ddf9657e 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -480,6 +480,7 @@ tls_wrap_free(struct tls_wrap_ctx *tls_wrap) if (tls_wrap->cleanup_key_ctx) { free_key_ctx_bi(&tls_wrap->opt.key_ctx_bi); + tls_wrap->cleanup_key_ctx = false; } free_buf(&tls_wrap->work); diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 2922f96fb..f84444514 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -192,15 +192,14 @@ write_control_auth(struct tls_session *session, struct key_state *ks, struct buf bool read_control_auth(struct buffer *buf, struct tls_wrap_ctx *ctx, - const struct link_socket_actual *from, const struct tls_options *opt, - bool initial_packet) + const struct link_socket_actual *from, const struct tls_options *opt) { struct gc_arena gc = gc_new(); bool ret = false; const uint8_t opcode = *(BPTR(buf)) >> P_OPCODE_SHIFT; if ((opcode == P_CONTROL_HARD_RESET_CLIENT_V3 || opcode == P_CONTROL_WKC_V1) - && !tls_crypt_v2_extract_client_key(buf, ctx, opt, initial_packet)) + && !tls_crypt_v2_extract_client_key(buf, ctx, opt)) { msg(D_TLS_ERRORS, "TLS Error: can not extract tls-crypt-v2 client key from %s", print_link_socket_actual(from, &gc)); @@ -350,7 +349,7 @@ tls_pre_decrypt_lite(const struct tls_auth_standalone *tas, struct tls_pre_decry /* HMAC test and unwrapping the encrypted part of the control message * into newbuf or just setting newbuf to point to the start of control * message */ - bool status = read_control_auth(&state->newbuf, &state->tls_wrap_tmp, from, NULL, true); + bool status = read_control_auth(&state->newbuf, &state->tls_wrap_tmp, from, NULL); if (!status) { diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h index ac9d4be2d..640885e8c 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -211,8 +211,7 @@ void write_control_auth(struct tls_session *session, struct key_state *ks, struc * @return if the packet was successfully processed */ bool read_control_auth(struct buffer *buf, struct tls_wrap_ctx *ctx, - const struct link_socket_actual *from, const struct tls_options *opt, - bool initial_packet); + const struct link_socket_actual *from, const struct tls_options *opt); /** diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index c188cef13..3ed72db6a 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -608,7 +608,7 @@ cleanup: bool tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, - const struct tls_options *opt, bool initial_packet) + const struct tls_options *opt) { if (!ctx->tls_crypt_v2_server_key.cipher) { @@ -636,7 +636,8 @@ tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, return false; } - if (!initial_packet) + /* Check if this context already owns an initialised key */ + if (ctx->cleanup_key_ctx == true) { /* This might be a harmless resend of the packet but it is better to * just ignore the WKC part than trying to setup tls-crypt keys again. @@ -649,7 +650,7 @@ tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, * and this is resend. So return the normal part of the packet, * basically transforming the CONTROL_WKC_V1 into a normal CONTROL_V1 * packet*/ - msg(D_TLS_ERRORS, "control channel security already setup ignoring " + msg(D_TLS_ERRORS, "Control channel security already setup. Ignoring " "wrapped key part of packet."); /* Remove client key from buffer so tls-crypt code can unwrap message */ diff --git a/src/openvpn/tls_crypt.h b/src/openvpn/tls_crypt.h index b9b15c2e0..d179cbb21 100644 --- a/src/openvpn/tls_crypt.h +++ b/src/openvpn/tls_crypt.h @@ -201,14 +201,10 @@ void tls_crypt_v2_init_client_key(struct key_ctx_bi *key, struct key2 *original_ * @param ctx tls-wrap context to be initialized with the client key. * @param opt TLS options, used for \c tls-crypt-v2-verify script. * - * @param initial_packet whether this is the initial packet of the - * connection. Only in these scenarios unwrapping - * of a tls-crypt-v2 key is allowed - * * @returns true if a key was successfully extracted. */ bool tls_crypt_v2_extract_client_key(struct buffer *buf, struct tls_wrap_ctx *ctx, - const struct tls_options *opt, bool initial_packet); + const struct tls_options *opt); /** * Generate a tls-crypt-v2 server key, and write to file. diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c index 4044cd79e..82b1dc89b 100644 --- a/tests/unit_tests/openvpn/test_tls_crypt.c +++ b/tests/unit_tests/openvpn/test_tls_crypt.c @@ -545,13 +545,13 @@ tls_crypt_v2_wrap_unwrap_max_metadata(void **state) }; /* a buffer that only contains the wrapped key should fail */ - assert_false(tls_crypt_v2_extract_client_key(&ctx->wkc, &wrap_ctx, NULL, true)); + assert_false(tls_crypt_v2_extract_client_key(&ctx->wkc, &wrap_ctx, NULL)); /* add a opcode in front of the key to make it valid to extract */ struct buffer wkcop = alloc_buf_gc(buf_len(&ctx->wkc) + 1, &ctx->gc); buf_write_u8(&wkcop, 0x50); buf_copy(&wkcop, &ctx->wkc); - assert_true(tls_crypt_v2_extract_client_key(&wkcop, &wrap_ctx, NULL, true)); + assert_true(tls_crypt_v2_extract_client_key(&wkcop, &wrap_ctx, NULL)); tls_wrap_free(&wrap_ctx); } @@ -683,7 +683,7 @@ tls_crypt_v2_wrap_unwrap_invalid(void **state) /* Make the wrapped key invalid by flipping a few bits */ buf_bptr(&tmp)[buf_len(&tmp) - 20] ^= 0x55; - assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, NULL, true)); + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, NULL)); tls_wrap_free(&wrap_ctx); } @@ -707,7 +707,7 @@ tls_crypt_v2_unwrap_checks(void **state) struct buffer tmp = create_client_key_input(ctx, 29); - assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options)); tls_wrap_free(&wrap_ctx); @@ -737,7 +737,7 @@ tls_crypt_v2_unwrap_checks(void **state) will_return(__wrap_buffer_write_file, true); tmp = create_client_key_input(ctx, 29); - assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + assert_true(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options)); tls_wrap_free(&wrap_ctx); tls_options.tls_crypt_v2_verify_script = "/usr/bin/false"; @@ -752,7 +752,7 @@ tls_crypt_v2_unwrap_checks(void **state) will_return(__wrap_buffer_write_file, true); tmp = create_client_key_input(ctx, 29); - assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options)); tls_wrap_free(&wrap_ctx); @@ -760,7 +760,7 @@ tls_crypt_v2_unwrap_checks(void **state) /* An outdated time should fail */ tmp = create_client_key_input(ctx, 31); - assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options, true)); + assert_false(tls_crypt_v2_extract_client_key(&tmp, &wrap_ctx, &tls_options)); script_security_set(0); tls_wrap_free(&wrap_ctx);