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, two 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. If present, we reset the ssl->hrr_keyshare_group variable to
   this group to ensure the error checks succeed.

A new test is added to check for this behavior.

Author: Frauschi

src/dtls.c

@@ -295,6 +295,42 @@ static int CheckDtlsCookie(const WOLFSSL* ssl, WolfSSL_CH* ch,
     return ret;
 }
 
+#if defined(WOLFSSL_DTLS13) && defined(WOLFSSL_SEND_HRR_COOKIE)
+static int ParseCookieExt(WOLFSSL* ssl, WolfSSL_CH* ch)
+{
+    int ret = 0;
+    word16 len;
+    const byte* cookie = NULL;
+
+    if (ch->cookieExt.size < OPAQUE16_LEN + 1)
+        return BUFFER_E;
+    ato16(ch->cookieExt.elements, &len);
+    if (ch->cookieExt.size - OPAQUE16_LEN != len)
+        return BUFFER_E;
+    cookie = ch->cookieExt.elements + OPAQUE16_LEN;
+    ret = TlsCheckCookie(ssl, cookie, len);
+    if (ret < 0)
+        return ret;
+    len = (word16)ret;
+
+    /* Cookie Data = Hash Len | Hash | CS | KeyShare Group (optional) */
+
+    /* Check if the cookie has a key share group */
+    if (len > cookie[0] + 4) {
+        word16 keyShareGroup = 0;
+        ato16(cookie + 3 + cookie[0], &keyShareGroup);
+
+        /* The key share group in the cookie is the group selected by the
+         * server in the HelloRetryRequest. Hence, the client must use this
+         * group in the second ClientHello.
+         */
+        ssl->hrr_keyshare_group = keyShareGroup;
+    }
+
+    return 0;
+}
+#endif /* WOLFSSL_DTLS13 && WOLFSSL_SEND_HRR_COOKIE */
+
 static int ParseClientHello(const byte* input, word32 helloSz, WolfSSL_CH* ch,
         byte isFirstCHFrag)
 {
@@ -1041,6 +1077,10 @@ int DoClientHelloStateless(WOLFSSL* ssl, const byte* input, word32 helloSz,
                 e = Dtls13GetEpoch(ssl, ssl->keys.curEpoch64);
                 if (e != NULL)
                     XMEMSET(e->window, 0xFF, sizeof(e->window));
+
+                ret = ParseCookieExt(ssl, &ch);
+                if (ret != 0)
+                    return ret;
             }
             else
 #endif

src/tls.c

@@ -10217,6 +10217,18 @@ int TLSX_KeyShare_Parse_ClientHello(const WOLFSSL* ssl,
     }
 
     if (ssl->hrr_keyshare_group != 0) {
+#ifdef WOLFSSL_DTLS
+        /* In case of stateless DTLSv1.3, the ssl->hrr_keyshare_group variable
+         * may have already been set without further proceeding the handshake,
+         * so we never received the second ClientHello with the selected group.
+         * Hence, when we receive another ClientHello message, we do not
+         * consider that as an error here.
+         */
+        if (ssl->options.dtls && IsAtLeastTLSv1_3(ssl->version) &&
+            !ssl->options.dtlsStateful) {
+            return 0;
+        }
+#endif
         /*
          * https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.8
          *   when sending the new ClientHello, the client MUST

tests/api/test_dtls.c

@@ -2567,3 +2567,117 @@ int test_dtls13_min_rtx_interval(void)
 #endif
     return EXPECT_RESULT();
 }
+
+/* DTLS stateless API handling multiple CHs with different HRR groups */
+int test_dtls_stateless_hrr_group(void)
+{
+    EXPECT_DECLS;
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_DTLS)
+    size_t i;
+    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
+    };
+    XMEMSET(&test_memio_wolfio_ctx, 0, sizeof(test_memio_wolfio_ctx));
+    for (i = 0; i < XELEM_CNT(params) && !EXPECT_FAIL(); i++) {
+        WOLFSSL_CTX *ctx_s = NULL, *ctx_c = NULL;
+        WOLFSSL *ssl_s = NULL, *ssl_c1 = 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_c1, &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);
+
+        test_memio_wolfio_ctx.test_ctx = &test_ctx;
+        test_memio_wolfio_ctx.ssl_s = ssl_s;
+        /* Large number to error out if any syscalls are called with it */
+        test_memio_wolfio_ctx.fd = 6000;
+        XMEMSET(&test_memio_wolfio_ctx.peer_addr, 0,
+                sizeof(test_memio_wolfio_ctx.peer_addr));
+        test_memio_wolfio_ctx.peer_addr.ss_family = AF_INET;
+
+        wolfSSL_SetLoggingPrefix("server");
+        wolfSSL_dtls_set_using_nonblock(ssl_s, 1);
+        wolfSSL_SetRecvFrom(ssl_s, test_memio_wolfio_recvfrom);
+        /* Restore default functions */
+        wolfSSL_SSLSetIORecv(ssl_s, EmbedReceiveFrom);
+        ExpectIntEQ(wolfSSL_set_read_fd(ssl_s, test_memio_wolfio_ctx.fd),
+                    WOLFSSL_SUCCESS);
+
+        /* 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_c1, groups_1, 3), WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_NoKeyShares(ssl_c1), 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_c1), -1);
+        ExpectIntEQ(wolfSSL_get_error(ssl_c1, -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);
+        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);
+        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_c1), -1);
+        ExpectIntEQ(wolfSSL_get_error(ssl_c1, -1), WOLFSSL_ERROR_WANT_READ);
+
+        wolfSSL_SetLoggingPrefix("server");
+        ExpectIntEQ(wolfDTLS_accept_stateless(ssl_s), WOLFSSL_SUCCESS);
+
+        ExpectIntEQ(test_memio_do_handshake(ssl_c1, ssl_s, 10, NULL), 0);
+
+        wolfSSL_free(ssl_s);
+        wolfSSL_free(ssl_c1);
+        wolfSSL_free(ssl_c2);
+        wolfSSL_CTX_free(ctx_s);
+        wolfSSL_CTX_free(ctx_c);
+    }
+#endif
+    return EXPECT_RESULT();
+}

tests/api/test_dtls.h

@@ -50,6 +50,7 @@ int test_dtls_memio_wolfio_stateless(void);
 int test_dtls_mtu_fragment_headroom(void);
 int test_dtls_mtu_split_messages(void);
 int test_dtls13_min_rtx_interval(void);
+int test_dtls_stateless_hrr_group(void);
 
 #define TEST_DTLS_DECLS                                                        \
         TEST_DECL_GROUP("dtls", test_dtls12_basic_connection_id),              \
@@ -79,5 +80,6 @@ int test_dtls13_min_rtx_interval(void);
         TEST_DECL_GROUP("dtls", test_dtls_mtu_fragment_headroom),              \
         TEST_DECL_GROUP("dtls", test_dtls_mtu_split_messages),                 \
         TEST_DECL_GROUP("dtls", test_dtls_memio_wolfio_stateless),             \
-        TEST_DECL_GROUP("dtls", test_dtls13_min_rtx_interval)
+        TEST_DECL_GROUP("dtls", test_dtls13_min_rtx_interval),                 \
+        TEST_DECL_GROUP("dtls", test_dtls_stateless_hrr_group)
 #endif /* TESTS_API_DTLS_H */