Browse code

Allow tls-crypt-v2 to be setup only on initial packet of a session

This fixes an internal server error condition that can be triggered by a
malicous authenticated client, a very unlucky corruption of packets in
transit or by an attacker that is able to inject a specially created
packet at the right time and is able to observe the traffic to construct
the packet.

The error condition results in an ASSERT statement being triggered,

NOTE: due to the security sensitive nature, this patch was prepared
under embargo on the security@openvpn.net mailing list, and thus has
no publically available "mailing list discussion before merge" URL.

CVE: 2025-2704
Change-Id: I07c1352204d308e5bde5f0b85e561a5dd0bc63c8
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <385d88f0-d7c9-4330-82ff-9f5931183afd@rfc2549.org>
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 82ee2fe4b42d9988c59ae3f83bd56a54d54e8c76)

Arne Schwabe authored on 2025/04/02 02:30:37
Showing 7 changed files
... ...
@@ -848,6 +848,9 @@ state_name(int state)
848 848
         case S_INITIAL:
849 849
             return "S_INITIAL";
850 850
 
851
+        case S_PRE_START_SKIP:
852
+            return "S_PRE_START_SKIP";
853
+
851 854
         case S_PRE_START:
852 855
             return "S_PRE_START";
853 856
 
... ...
@@ -2598,7 +2601,7 @@ session_move_pre_start(const struct tls_session *session,
2598 2598
     }
2599 2599
     INCR_GENERATED;
2600 2600
 
2601
-    ks->state = S_PRE_START;
2601
+    ks->state = skip_initial_send ? S_PRE_START_SKIP : S_PRE_START;
2602 2602
 
2603 2603
     struct gc_arena gc = gc_new();
2604 2604
     dmsg(D_TLS_DEBUG, "TLS: Initial Handshake, sid=%s",
... ...
@@ -3801,7 +3804,7 @@ tls_pre_decrypt(struct tls_multi *multi,
3801 3801
         }
3802 3802
 
3803 3803
         if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id), from,
3804
-                               session->opt))
3804
+                               session->opt, true))
3805 3805
         {
3806 3806
             goto error;
3807 3807
         }
... ...
@@ -3871,7 +3874,7 @@ tls_pre_decrypt(struct tls_multi *multi,
3871 3871
         if (op == P_CONTROL_SOFT_RESET_V1 && ks->state >= S_GENERATED_KEYS)
3872 3872
         {
3873 3873
             if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id),
3874
-                                   from, session->opt))
3874
+                                   from, session->opt, false))
3875 3875
             {
3876 3876
                 goto error;
3877 3877
             }
... ...
@@ -3884,6 +3887,15 @@ tls_pre_decrypt(struct tls_multi *multi,
3884 3884
         }
3885 3885
         else
3886 3886
         {
3887
+            bool initial_packet = false;
3888
+            if (ks->state == S_PRE_START_SKIP)
3889
+            {
3890
+                /* When we are coming from the session_skip_to_pre_start
3891
+                 * method, we allow this initial packet to setup the
3892
+                 * tls-crypt-v2 peer specific key */
3893
+                initial_packet = true;
3894
+                ks->state = S_PRE_START;
3895
+            }
3887 3896
             /*
3888 3897
              * Remote responding to our key renegotiation request?
3889 3898
              */
... ...
@@ -3893,8 +3905,14 @@ tls_pre_decrypt(struct tls_multi *multi,
3893 3893
             }
3894 3894
 
3895 3895
             if (!read_control_auth(buf, tls_session_get_tls_wrap(session, key_id),
3896
-                                   from, session->opt))
3896
+                                   from, session->opt, initial_packet))
3897 3897
             {
3898
+                /* if an initial packet in read_control_auth, we rather
3899
+                 * error out than anything else */
3900
+                if (initial_packet)
3901
+                {
3902
+                    multi->n_hard_errors++;
3903
+                }
3898 3904
                 goto error;
3899 3905
             }
3900 3906
 
... ...
@@ -80,22 +80,25 @@
80 80
 #define S_INITIAL         1     /**< Initial \c key_state state after
81 81
                                  *   initialization by \c key_state_init()
82 82
                                  *   before start of three-way handshake. */
83
-#define S_PRE_START       2     /**< Waiting for the remote OpenVPN peer
83
+#define S_PRE_START_SKIP  2     /**< Waiting for the remote OpenVPN peer
84 84
                                  *   to acknowledge during the initial
85 85
                                  *   three-way handshake. */
86
-#define S_START           3     /**< Three-way handshake is complete,
86
+#define S_PRE_START       3     /**< Waiting for the remote OpenVPN peer
87
+                                 *   to acknowledge during the initial
88
+                                 *   three-way handshake. */
89
+#define S_START           4     /**< Three-way handshake is complete,
87 90
                                  *   start of key exchange. */
88
-#define S_SENT_KEY        4     /**< Local OpenVPN process has sent its
91
+#define S_SENT_KEY        5     /**< Local OpenVPN process has sent its
89 92
                                  *   part of the key material. */
90
-#define S_GOT_KEY         5     /**< Local OpenVPN process has received
93
+#define S_GOT_KEY         6     /**< Local OpenVPN process has received
91 94
                                  *   the remote's part of the key
92 95
                                  *   material. */
93
-#define S_ACTIVE          6     /**< Operational \c key_state state
96
+#define S_ACTIVE          7     /**< Operational \c key_state state
94 97
                                  *   immediately after negotiation has
95 98
                                  *   completed while still within the
96 99
                                  *   handshake window.  Deferred auth and
97 100
                                  *   client connect can still be pending. */
98
-#define S_GENERATED_KEYS  7     /**< The data channel keys have been generated
101
+#define S_GENERATED_KEYS  8     /**< The data channel keys have been generated
99 102
                                  *  The TLS session is fully authenticated
100 103
                                  *  when reaching this state. */
101 104
 
... ...
@@ -200,7 +200,8 @@ bool
200 200
 read_control_auth(struct buffer *buf,
201 201
                   struct tls_wrap_ctx *ctx,
202 202
                   const struct link_socket_actual *from,
203
-                  const struct tls_options *opt)
203
+                  const struct tls_options *opt,
204
+                  bool initial_packet)
204 205
 {
205 206
     struct gc_arena gc = gc_new();
206 207
     bool ret = false;
... ...
@@ -208,7 +209,7 @@ read_control_auth(struct buffer *buf,
208 208
     const uint8_t opcode = *(BPTR(buf)) >> P_OPCODE_SHIFT;
209 209
     if ((opcode == P_CONTROL_HARD_RESET_CLIENT_V3
210 210
          || opcode == P_CONTROL_WKC_V1)
211
-        && !tls_crypt_v2_extract_client_key(buf, ctx, opt))
211
+        && !tls_crypt_v2_extract_client_key(buf, ctx, opt, initial_packet))
212 212
     {
213 213
         msg(D_TLS_ERRORS,
214 214
             "TLS Error: can not extract tls-crypt-v2 client key from %s",
... ...
@@ -373,7 +374,7 @@ tls_pre_decrypt_lite(const struct tls_auth_standalone *tas,
373 373
      * into newbuf or just setting newbuf to point to the start of control
374 374
      * message */
375 375
     bool status = read_control_auth(&state->newbuf, &state->tls_wrap_tmp,
376
-                                    from, NULL);
376
+                                    from, NULL, true);
377 377
 
378 378
     if (!status)
379 379
     {
... ...
@@ -207,14 +207,22 @@ write_control_auth(struct tls_session *session,
207 207
                    bool prepend_ack);
208 208
 
209 209
 
210
-/*
210
+
211
+/**
211 212
  * Read a control channel authentication record.
213
+ * @param buf               buffer that holds the incoming packet
214
+ * @param ctx               control channel security context
215
+ * @param from              incoming link socket address
216
+ * @param opt               tls options struct for the session
217
+ * @param initial_packet    whether this is the initial packet for the connection
218
+ * @return                  if the packet was successfully processed
212 219
  */
213 220
 bool
214 221
 read_control_auth(struct buffer *buf,
215 222
                   struct tls_wrap_ctx *ctx,
216 223
                   const struct link_socket_actual *from,
217
-                  const struct tls_options *opt);
224
+                  const struct tls_options *opt,
225
+                  bool initial_packet);
218 226
 
219 227
 
220 228
 /**
... ...
@@ -612,7 +612,8 @@ cleanup:
612 612
 bool
613 613
 tls_crypt_v2_extract_client_key(struct buffer *buf,
614 614
                                 struct tls_wrap_ctx *ctx,
615
-                                const struct tls_options *opt)
615
+                                const struct tls_options *opt,
616
+                                bool initial_packet)
616 617
 {
617 618
     if (!ctx->tls_crypt_v2_server_key.cipher)
618 619
     {
... ...
@@ -641,6 +642,27 @@ tls_crypt_v2_extract_client_key(struct buffer *buf,
641 641
         return false;
642 642
     }
643 643
 
644
+    if (!initial_packet)
645
+    {
646
+        /* This might be a harmless resend of the packet but it is better to
647
+         * just ignore the WKC part than trying to setup tls-crypt keys again.
648
+         *
649
+         * A CONTROL_WKC_V1 packets has a normal packet part and an appended
650
+         * wrapped control key. These are authenticated individually. We already
651
+         * set up tls-crypt with the wrapped key, so we are ignoring this part
652
+         * of the message but we return the normal packet part as the normal
653
+         * part of the message might have been corrupted earlier and discarded
654
+         * and this is resend. So return the normal part of the packet,
655
+         * basically transforming the CONTROL_WKC_V1 into a normal CONTROL_V1
656
+         * packet*/
657
+        msg(D_TLS_ERRORS, "control channel security already setup ignoring "
658
+            "wrapped key part of packet.");
659
+
660
+        /* Remove client key from buffer so tls-crypt code can unwrap message */
661
+        ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key))));
662
+        return true;
663
+    }
664
+
644 665
     ctx->tls_crypt_v2_metadata = alloc_buf(TLS_CRYPT_V2_MAX_METADATA_LEN);
645 666
     if (!tls_crypt_v2_unwrap_client_key(&ctx->original_wrap_keydata,
646 667
                                         &ctx->tls_crypt_v2_metadata,
... ...
@@ -207,11 +207,16 @@ void tls_crypt_v2_init_client_key(struct key_ctx_bi *key,
207 207
  *              message.
208 208
  * @param ctx   tls-wrap context to be initialized with the client key.
209 209
  *
210
+ * @param initial_packet    whether this is the initial packet of the
211
+ *                          connection. Only in these scenarios unwrapping
212
+ *                          of a tls-crypt-v2 key is allowed
213
+ *
210 214
  * @returns true if a key was successfully extracted.
211 215
  */
212 216
 bool tls_crypt_v2_extract_client_key(struct buffer *buf,
213 217
                                      struct tls_wrap_ctx *ctx,
214
-                                     const struct tls_options *opt);
218
+                                     const struct tls_options *opt,
219
+                                     bool initial_packet);
215 220
 
216 221
 /**
217 222
  * Generate a tls-crypt-v2 server key, and write to file.
... ...
@@ -533,7 +533,7 @@ tls_crypt_v2_wrap_unwrap_max_metadata(void **state)
533 533
         .mode = TLS_WRAP_CRYPT,
534 534
         .tls_crypt_v2_server_key = ctx->server_keys.encrypt,
535 535
     };
536
-    assert_true(tls_crypt_v2_extract_client_key(&ctx->wkc, &wrap_ctx, NULL));
536
+    assert_true(tls_crypt_v2_extract_client_key(&ctx->wkc, &wrap_ctx, NULL, true));
537 537
     tls_wrap_free(&wrap_ctx);
538 538
 }
539 539