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 <maximilian.fillinger@sentyron.com>
Github: OpenVPN/openvpn-private-issues#137
Github: OpenVPN/openvpn-private-issues#138

Change-Id: I3b5e4e84762aa253d46e69103f7b1e84ebefca1d
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-By: Max Fillinger <maximilian.fillinger@sentyron.com>

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 85546020da8a84a769f46981f7059672177abd4c..60df7ce84a9990b638b5f6576164583b2e926c5e 100644 (file)
--- 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 f90b24f1e4b55d80e84dff0c5a2bd8ec722e879f..7ddf9657e9faff1d0a6dfedcc4a069c6fccf70e6 100644 (file)
--- 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 2922f96fb4c3cd1c1b5534af68bc4a9d59936247..f84444514b6a441eb939b799188f76611419d11f 100644 (file)
--- 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 ac9d4be2df34ccc4ea8f1b3017455066c167d289..640885e8c45c6b39080aa50fc862e63fb94c31e0 100644 (file)
--- 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 c188cef136df5bb45d3d8f0ebbab6557803b5d4a..3ed72db6ad9477641f064adbaf1d8f543039e157 100644 (file)
--- 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 b9b15c2e0b471e88819d1f04fc15ca806d451be4..d179cbb21c82d7008a64e2e6125fd96196f84d31 100644 (file)
--- 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 4044cd79e12423d377031871cd241903ca1ae06e..82b1dc89b2708cfbdaafa9951b8cf4e8e1b20faf 100644 (file)
--- 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);