Fix for DTLS1.3 HRR group handling

When a server uses a HRR to negotiate the key exchange group to use, the
selected group is advertised in the HRR key share extension.
Furthermore, this group is also stored in the Cookie that is sent to the
client. When the server receives the second CH, the group used in the
key share extension MUST be the one of the HRR.

For stateless DTLS servers, the handling of this check had a bug. The
key share group of the HRR is stored in the ssl->hrr_keyshare_group
variable and is checked against the received key share of the second CH.
However, in the stateless server case, another CH message may be
received inbetween the two CH message of the desired client, potentially
overwriting the ssl->hrr_keyshare_group variable. This then causes
handshake failures when the ssl->hrr_keyshare_group variable contains
another group than the second CH message of the desired client.

To fix this, the following changes are conducted:
1. Disable the ssl->hrr_keyshare_group check for stateless DTLS 1.3
   servers. As long as the server is stateless, CHs from multiple
   clients may be received that individually cause HRRs with different
   groups. For each of these clients, the HRR group is properly stored
   in the cookie.
2. When a valid cookie is received from the client, the server becomes
   stateful. In this case, we now parse the cookie for a stored HRR
   group in the RestartHandshakeHashWithCookie() method. If present,
   we restore the ssl->hrr_keyshare_group variable to this group to
   ensure the error checks succeed.
3. Move the check of ssl->hrr_keyshare_group of the the KeyShare
   extension parsing logic into the general TLS1.3 ClientHello parsing
   after extension handling. This ensures that the order of the cookie
   and key share extensions does not matter.

A new test is added to check for this behavior.

Author: Frauschi

src/tls.c

@@ -10216,20 +10216,6 @@ int TLSX_KeyShare_Parse_ClientHello(const WOLFSSL* ssl,
         offset += ret;
     }
 
-    if (ssl->hrr_keyshare_group != 0) {
-        /*
-         * https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.8
-         *   when sending the new ClientHello, the client MUST
-         *   replace the original "key_share" extension with one containing only a
-         *   new KeyShareEntry for the group indicated in the selected_group field
-         *   of the triggering HelloRetryRequest
-         */
-        if (seenGroupsCnt != 1 || seenGroups[0] != ssl->hrr_keyshare_group) {
-            WOLFSSL_ERROR_VERBOSE(BAD_KEY_SHARE_DATA);
-            return BAD_KEY_SHARE_DATA;
-        }
-    }
-
     return 0;
 }
 

src/tls13.c

@@ -6502,6 +6502,8 @@ static int RestartHandshakeHashWithCookie(WOLFSSL* ssl, Cookie* cookie)
         hrrIdx += 2;
         c16toa(OPAQUE16_LEN, hrr + hrrIdx);
         hrrIdx += 2;
+        /* Restore the HRR key share group from the cookie. */
+        ato16(cookieData + idx, &ssl->hrr_keyshare_group);
         hrr[hrrIdx++] = cookieData[idx++];
         hrr[hrrIdx++] = cookieData[idx++];
     }
@@ -7015,6 +7017,29 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
     }
 #endif
 
+#ifdef HAVE_SUPPORTED_CURVES
+    if (ssl->hrr_keyshare_group != 0) {
+        /*
+         * https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.8
+         *   when sending the new ClientHello, the client MUST
+         *   replace the original "key_share" extension with one containing only
+         *   a new KeyShareEntry for the group indicated in the selected_group
+         *   field of the triggering HelloRetryRequest.
+         */
+        TLSX* extension = TLSX_Find(ssl->extensions, TLSX_KEY_SHARE);
+        if (extension != NULL) {
+            KeyShareEntry* kse = (KeyShareEntry*)extension->data;
+            /* Exactly one KeyShareEntry with the HRR group must be present. */
+            if (kse == NULL || kse->next != NULL ||
+                                        kse->group != ssl->hrr_keyshare_group) {
+                ERROR_OUT(BAD_KEY_SHARE_DATA, exit_dch);
+            }
+        }
+        else
+            ERROR_OUT(BAD_KEY_SHARE_DATA, exit_dch);
+    }
+#endif
+
 #if defined(HAVE_ECH)
     /* hash clientHelloInner to hsHashesEch independently since it can't include
      * the HRR */
@@ -7509,7 +7534,18 @@ int SendTls13ServerHello(WOLFSSL* ssl, byte extMsgType)
     if (ret != 0)
         return ret;
 
-    if (extMsgType == hello_retry_request) {
+    /* When we send a HRR, we store the selected key share group to later check
+     * that the client uses the same group in the second ClientHello.
+     *
+     * In case of stateless DTLS, we do not store the group, however, as it is
+     * already stored in the cookie that is sent to the client. We later recover
+     * the group from the cookie to prevent storing a state in a stateless
+     * server. */
+    if (extMsgType == hello_retry_request
+#if defined(WOLFSSL_DTLS13) && defined(WOLFSSL_SEND_HRR_COOKIE)
+        && (!ssl->options.dtls || ssl->options.dtlsStateful)
+#endif
+    ) {
         TLSX* ksExt = TLSX_Find(ssl->extensions, TLSX_KEY_SHARE);
         if (ksExt != NULL) {
             KeyShareEntry* kse = (KeyShareEntry*)ksExt->data;

tests/api.c

@@ -24180,6 +24180,12 @@ static word32 test_wolfSSL_dtls_stateless_HashWOLFSSL(const WOLFSSL* ssl)
     sslCopy.keys.dtls_peer_handshake_number = 0;
     XMEMSET(&sslCopy.alert_history, 0, sizeof(sslCopy.alert_history));
     sslCopy.hsHashes = NULL;
+#if !defined(WOLFSSL_NO_CLIENT_AUTH) && \
+    ((defined(WOLFSSL_SM2) && defined(WOLFSSL_SM3)) || \
+     (defined(HAVE_ED25519) && !defined(NO_ED25519_CLIENT_AUTH)) || \
+     (defined(HAVE_ED448) && !defined(NO_ED448_CLIENT_AUTH)))
+    sslCopy.options.cacheMessages = 0;
+#endif
 #ifdef WOLFSSL_ASYNC_IO
 #ifdef WOLFSSL_ASYNC_CRYPT
     sslCopy.asyncDev = NULL;
@@ -24317,11 +24323,122 @@ static int test_wolfSSL_dtls_stateless(void)
 
     return TEST_SUCCESS;
 }
+
+/* DTLS stateless API handling multiple CHs with different HRR groups */
+static int test_wolfSSL_dtls_stateless_hrr_group(void)
+{
+    EXPECT_DECLS;
+#if defined(WOLFSSL_SEND_HRR_COOKIE)
+    size_t i;
+    word32 initHash;
+    struct {
+        method_provider client_meth;
+        method_provider server_meth;
+    } params[] = {
+#if defined(WOLFSSL_TLS13) && defined(WOLFSSL_DTLS13)
+        { wolfDTLSv1_3_client_method, wolfDTLSv1_3_server_method },
+#endif
+#if !defined(WOLFSSL_NO_TLS12) && defined(WOLFSSL_DTLS)
+        { wolfDTLSv1_2_client_method, wolfDTLSv1_2_server_method },
+#endif
+    };
+    for (i = 0; i < XELEM_CNT(params) && !EXPECT_FAIL(); i++) {
+        WOLFSSL_CTX *ctx_s = NULL, *ctx_c = NULL;
+        WOLFSSL *ssl_s = NULL, *ssl_c = NULL, *ssl_c2 = NULL;
+        struct test_memio_ctx test_ctx;
+        int groups_1[] = {
+            WOLFSSL_ECC_SECP256R1,
+            WOLFSSL_ECC_SECP384R1,
+            WOLFSSL_ECC_SECP521R1
+        };
+        int groups_2[] = {
+            WOLFSSL_ECC_SECP384R1,
+            WOLFSSL_ECC_SECP521R1
+        };
+        char hrrBuf[1000];
+        int hrrSz = sizeof(hrrBuf);
+
+        XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+
+        ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+                params[i].client_meth, params[i].server_meth), 0);
+
+        ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, NULL, &ssl_c2, NULL,
+                params[i].client_meth, params[i].server_meth), 0);
+
+
+        wolfSSL_SetLoggingPrefix("server");
+        wolfSSL_dtls_set_using_nonblock(ssl_s, 1);
+
+        initHash = test_wolfSSL_dtls_stateless_HashWOLFSSL(ssl_s);
+
+        /* Set groups and disable key shares. This ensures that only the given
+         * groups are in the SupportedGroups extension and that an empty key
+         * share extension is sent in the initial ClientHello of each session.
+         * This triggers the server to send a HelloRetryRequest with the first
+         * group in the SupportedGroups extension selected. */
+        wolfSSL_SetLoggingPrefix("client1");
+        ExpectIntEQ(wolfSSL_set_groups(ssl_c, groups_1, 3), WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_NoKeyShares(ssl_c), WOLFSSL_SUCCESS);
+
+        wolfSSL_SetLoggingPrefix("client2");
+        ExpectIntEQ(wolfSSL_set_groups(ssl_c2, groups_2, 2), WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_NoKeyShares(ssl_c2), WOLFSSL_SUCCESS);
+
+        /* Start handshake, send first ClientHello */
+        wolfSSL_SetLoggingPrefix("client1");
+        ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+        ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+
+        /* Read first ClientHello, send HRR with WOLFSSL_ECC_SECP256R1 */
+        wolfSSL_SetLoggingPrefix("server");
+        ExpectIntEQ(wolfDTLS_accept_stateless(ssl_s), 0);
+        ExpectIntEQ(test_memio_copy_message(&test_ctx, 1, hrrBuf, &hrrSz, 0), 0);
+        ExpectIntGT(hrrSz, 0);
+        ExpectIntEQ(initHash, test_wolfSSL_dtls_stateless_HashWOLFSSL(ssl_s));
+        test_memio_clear_buffer(&test_ctx, 1);
+
+        /* Send second ClientHello */
+        wolfSSL_SetLoggingPrefix("client2");
+        ExpectIntEQ(wolfSSL_connect(ssl_c2), -1);
+        ExpectIntEQ(wolfSSL_get_error(ssl_c2, -1), WOLFSSL_ERROR_WANT_READ);
+
+        /* Read second ClientHello, send HRR now with WOLFSSL_ECC_SECP384R1 */
+        wolfSSL_SetLoggingPrefix("server");
+        ExpectIntEQ(wolfDTLS_accept_stateless(ssl_s), 0);
+        ExpectIntEQ(initHash, test_wolfSSL_dtls_stateless_HashWOLFSSL(ssl_s));
+        test_memio_clear_buffer(&test_ctx, 1);
+
+        /* Complete first handshake with WOLFSSL_ECC_SECP256R1 */
+        wolfSSL_SetLoggingPrefix("client1");
+        ExpectIntEQ(test_memio_inject_message(&test_ctx, 1, hrrBuf, hrrSz), 0);
+        ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+        ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+
+        wolfSSL_SetLoggingPrefix("server");
+        ExpectIntEQ(wolfDTLS_accept_stateless(ssl_s), WOLFSSL_SUCCESS);
+
+        ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+        wolfSSL_free(ssl_s);
+        wolfSSL_free(ssl_c);
+        wolfSSL_free(ssl_c2);
+        wolfSSL_CTX_free(ctx_s);
+        wolfSSL_CTX_free(ctx_c);
+    }
+#endif /* WOLFSSL_SEND_HRR_COOKIE */
+    return EXPECT_RESULT();
+}
 #else
 static int test_wolfSSL_dtls_stateless(void)
 {
     return TEST_SKIPPED;
 }
+
+static int test_wolfSSL_dtls_stateless_hrr_group(void)
+{
+    return TEST_SKIPPED;
+}
 #endif /* WOLFSSL_DTLS13 && WOLFSSL_SEND_HRR_COOKIE &&
         * HAVE_IO_TESTS_DEPENDENCIES && !SINGLE_THREADED */
 
@@ -33186,6 +33303,7 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_wolfSSL_dtls_bad_record),
     /* Uses Assert in handshake callback. */
     TEST_DECL(test_wolfSSL_dtls_stateless),
+    TEST_DECL(test_wolfSSL_dtls_stateless_hrr_group),
     TEST_DECL(test_generate_cookie),
 
 #ifndef NO_BIO