More review comments

Author: Frauschi

src/internal.c

@@ -18566,8 +18566,12 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx,
                ((defined(WOLFSSL_SM2) && defined(WOLFSSL_SM3)) || \
                 (defined(HAVE_ED25519) && !defined(NO_ED25519_CLIENT_AUTH)) || \
                 (defined(HAVE_ED448) && !defined(NO_ED448_CLIENT_AUTH)))
-        if (ssl->options.resuming || !ssl->options.verifyPeer || \
-                     !IsAtLeastTLSv1_2(ssl) || IsAtLeastTLSv1_3(ssl->version)) {
+        if ((ssl->options.resuming || !ssl->options.verifyPeer ||
+               !IsAtLeastTLSv1_2(ssl) || IsAtLeastTLSv1_3(ssl->version))
+        #ifdef WOLFSSL_DTLS
+               && !ssl->options.dtls
+        #endif
+               ) {
         #if defined(WOLFSSL_ASYNC_CRYPT) || defined(WOLFSSL_NONBLOCK_OCSP)
             if (ret != WC_NO_ERR_TRACE(WC_PENDING_E) &&
                 ret != WC_NO_ERR_TRACE(OCSP_WANT_READ))

src/tls.c

@@ -7268,7 +7268,7 @@ static int TLSX_Cookie_RestoreHrrGroup(WOLFSSL* ssl)
 #elif defined(WOLFSSL_SM3)
     byte macSz = WC_SM3_DIGEST_SIZE;
 #else
-    #error "No digest to available to use with HMAC for cookies."
+    #error "No digest available to use with HMAC for cookies."
 #endif /* NO_SHA */
 
     extension = TLSX_Find(ssl->extensions, TLSX_COOKIE);
@@ -10275,18 +10275,6 @@ 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

src/tls13.c

@@ -7509,7 +7509,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

@@ -24317,11 +24317,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 */
 
@@ -27783,105 +27894,6 @@ static int test_wolfSSL_dtls_stateless_downgrade(void)
 }
 #endif /* !defined(NO_OLD_TLS) */
 
-/* DTLS stateless API handling multiple CHs with different HRR groups */
-static int test_wolfSSL_dtls_stateless_hrr_group(void)
-{
-    EXPECT_DECLS;
-    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
-    };
-    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);
-
-        /* 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);
-        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_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);
-    }
-    return EXPECT_RESULT();
-}
-
 #endif /* defined(WOLFSSL_DTLS) && !defined(WOLFSSL_NO_TLS12) && \
     !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER)*/
 
@@ -33285,6 +33297,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
@@ -33322,7 +33335,6 @@ TEST_CASE testCases[] = {
 #if !defined(NO_OLD_TLS)
     TEST_DECL(test_wolfSSL_dtls_stateless_downgrade),
 #endif /* !defined(NO_OLD_TLS) */
-    TEST_DECL(test_wolfSSL_dtls_stateless_hrr_group),
 #endif /* ! NO_RSA */
 #endif /* defined(WOLFSSL_DTLS) && !defined(WOLFSSL_NO_TLS12) &&     \
         *  !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) */