Merge pull request wolfSSL#8824 from JeremiahM37/tlsCurveFix

JacobBarthelmeh · web-flow · commit 570c1fc39064 · 2025-06-06T10:47:06.000-06:00
tls fix for set_groups
diff --git a/src/tls.c b/src/tls.c
@@ -5048,7 +5048,8 @@ int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
 {
     word16 offset;
     word16 name;
-    int ret;
+    int ret = 0;
+    TLSX* extension;
 
     if(!isRequest && !IsAtLeastTLSv1_3(ssl->version)) {
 #ifdef WOLFSSL_ALLOW_SERVER_SC_EXT
@@ -5057,57 +5058,66 @@ int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
         return BUFFER_ERROR; /* servers doesn't send this extension. */
 #endif
     }
-
     if (OPAQUE16_LEN > length || length % OPAQUE16_LEN)
         return BUFFER_ERROR;
-
     ato16(input, &offset);
-
     /* validating curve list length */
     if (length != OPAQUE16_LEN + offset)
         return BUFFER_ERROR;
-
     offset = OPAQUE16_LEN;
     if (offset == length)
         return 0;
 
-#if defined(WOLFSSL_TLS13) && !defined(WOLFSSL_NO_SERVER_GROUPS_EXT)
-    if (!isRequest) {
-        TLSX* extension;
-        SupportedCurve* curve;
-
-        extension = TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS);
-        if (extension != NULL) {
-            /* Replace client list with server list of supported groups. */
-            curve = (SupportedCurve*)extension->data;
-            extension->data = NULL;
-            TLSX_SupportedCurve_FreeAll(curve, ssl->heap);
-
+    extension = TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS);
+    if (extension == NULL) {
+        /* Just accept what the peer wants to use */
+        for (; offset < length; offset += OPAQUE16_LEN) {
             ato16(input + offset, &name);
-            offset += OPAQUE16_LEN;
 
-            ret = TLSX_SupportedCurve_New(&curve, name, ssl->heap);
-            if (ret != 0)
-                return ret; /* throw error */
-            extension->data = (void*)curve;
+            ret = TLSX_UseSupportedCurve(extensions, name, ssl->heap);
+            /* If it is BAD_FUNC_ARG then it is a group we do not support, but
+             * that is fine. */
+            if (ret != WOLFSSL_SUCCESS &&
+                    ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG))
+                break;
+            ret = 0;
         }
     }
-#endif
+    else {
+        /* Find the intersection with what the user has set */
+        SupportedCurve* commonCurves = NULL;
+        for (; offset < length; offset += OPAQUE16_LEN) {
+            SupportedCurve* foundCurve = (SupportedCurve*)extension->data;
+            ato16(input + offset, &name);
 
-    for (; offset < length; offset += OPAQUE16_LEN) {
-        ato16(input + offset, &name);
+            while (foundCurve != NULL && foundCurve->name != name)
+                foundCurve = foundCurve->next;
 
-        ret = TLSX_UseSupportedCurve(extensions, name, ssl->heap);
-        /* If it is BAD_FUNC_ARG then it is a group we do not support, but
-         * that is fine. */
-        if (ret != WOLFSSL_SUCCESS && ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
-            return ret;
+            if (foundCurve != NULL) {
+                ret = commonCurves == NULL ?
+                      TLSX_SupportedCurve_New(&commonCurves, name, ssl->heap) :
+                      TLSX_SupportedCurve_Append(commonCurves, name, ssl->heap);
+                if (ret != 0)
+                    break;
+            }
         }
+        /* If no common curves return error. In TLS 1.3 we can still try to save
+         * this by using HRR. */
+        if (ret == 0 && commonCurves == NULL &&
+                !IsAtLeastTLSv1_3(ssl->version))
+            ret = ECC_CURVE_ERROR;
+        if (ret == 0) {
+            /* Now swap out the curves in the extension */
+            TLSX_SupportedCurve_FreeAll((SupportedCurve*)extension->data,
+                                        ssl->heap);
+            extension->data = commonCurves;
+            commonCurves = NULL;
+        }
+        TLSX_SupportedCurve_FreeAll(commonCurves, ssl->heap);
     }
 
-    return 0;
+    return ret;
 }
-
 #endif
 
 #if !defined(NO_WOLFSSL_SERVER)
@@ -10798,15 +10808,17 @@ int TLSX_KeyShare_SetSupported(const WOLFSSL* ssl, TLSX** extensions)
     TLSX*           extension;
     SupportedCurve* curve = NULL;
     SupportedCurve* preferredCurve = NULL;
+    word16          name = WOLFSSL_NAMED_GROUP_INVALID;
     KeyShareEntry*  kse = NULL;
     int             preferredRank = WOLFSSL_MAX_GROUP_COUNT;
     int             rank;
 
     extension = TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS);
     if (extension != NULL)
         curve = (SupportedCurve*)extension->data;
-    /* Use server's preference order. */
     for (; curve != NULL; curve = curve->next) {
+        /* Use server's preference order. Common group was found but key share
+         * was missing */
         if (!TLSX_IsGroupSupported(curve->name))
             continue;
         if (wolfSSL_curve_is_disabled(ssl, curve->name))
@@ -10823,8 +10835,26 @@ int TLSX_KeyShare_SetSupported(const WOLFSSL* ssl, TLSX** extensions)
     curve = preferredCurve;
 
     if (curve == NULL) {
-        WOLFSSL_ERROR_VERBOSE(BAD_KEY_SHARE_DATA);
-        return BAD_KEY_SHARE_DATA;
+        byte i;
+        /* Fallback to user selected group */
+        preferredRank = WOLFSSL_MAX_GROUP_COUNT;
+        for (i = 0; i < ssl->numGroups; i++) {
+            rank = TLSX_KeyShare_GroupRank(ssl, ssl->group[i]);
+            if (rank == -1)
+                continue;
+            if (rank < preferredRank) {
+                name = ssl->group[i];
+                preferredRank = rank;
+            }
+        }
+        if (name == WOLFSSL_NAMED_GROUP_INVALID) {
+            /* No group selected or specified by the server */
+            WOLFSSL_ERROR_VERBOSE(BAD_KEY_SHARE_DATA);
+            return BAD_KEY_SHARE_DATA;
+        }
+    }
+    else {
+        name = curve->name;
     }
 
     #ifdef WOLFSSL_ASYNC_CRYPT
@@ -10848,7 +10878,7 @@ int TLSX_KeyShare_SetSupported(const WOLFSSL* ssl, TLSX** extensions)
     /* Extension got pushed to head */
     extension = *extensions;
     /* Push the selected curve */
-    ret = TLSX_KeyShare_New((KeyShareEntry**)&extension->data, curve->name,
+    ret = TLSX_KeyShare_New((KeyShareEntry**)&extension->data, name,
                             ssl->heap, &kse);
     if (ret != 0)
         return ret;
diff --git a/tests/api.c b/tests/api.c
@@ -30635,11 +30635,16 @@ static int test_wolfSSL_curves_mismatch(void)
     } test_params[] = {
 #ifdef WOLFSSL_TLS13
         {wolfTLSv1_3_client_method, wolfTLSv1_3_server_method, "TLS 1.3",
-                WC_NO_ERR_TRACE(FATAL_ERROR), WC_NO_ERR_TRACE(BAD_KEY_SHARE_DATA)},
+                /* Client gets error because server will attempt HRR */
+                WC_NO_ERR_TRACE(BAD_KEY_SHARE_DATA),
+                WC_NO_ERR_TRACE(FATAL_ERROR)
+        },
 #endif
 #ifndef WOLFSSL_NO_TLS12
         {wolfTLSv1_2_client_method, wolfTLSv1_2_server_method, "TLS 1.2",
                 WC_NO_ERR_TRACE(FATAL_ERROR),
+                /* Server gets error because <=1.2 doesn't have a mechanism
+                 * to negotiate curves. */
 #ifdef OPENSSL_EXTRA
                 WC_NO_ERR_TRACE(WOLFSSL_ERROR_SYSCALL)
 #else
@@ -68270,6 +68275,8 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_ocsp_certid_enc_dec),
     TEST_DECL(test_tls12_unexpected_ccs),
     TEST_DECL(test_tls13_unexpected_ccs),
+    TEST_DECL(test_tls12_curve_intersection),
+    TEST_DECL(test_tls13_curve_intersection),
     TEST_DECL(test_wc_DhSetNamedKey),
     /* This test needs to stay at the end to clean up any caches allocated. */
     TEST_DECL(test_wolfSSL_Cleanup)
diff --git a/tests/api/test_tls.c b/tests/api/test_tls.c
@@ -141,3 +141,116 @@ int test_tls13_unexpected_ccs(void)
 #endif
     return EXPECT_RESULT();
 }
+int test_tls12_curve_intersection(void) {
+    EXPECT_DECLS;
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
+    !defined(WOLFSSL_NO_TLS12) && defined(HAVE_ECC) && \
+    defined(HAVE_CURVE25519)
+    WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    int ret;
+    const char* curve_name;
+    int test1[] = {WOLFSSL_ECC_SECP256R1};
+    int test2[] = {WOLFSSL_ECC_SECP384R1};
+    int test3[] = {WOLFSSL_ECC_SECP256R1, WOLFSSL_ECC_SECP384R1};
+    int test4[] = {WOLFSSL_ECC_SECP384R1, WOLFSSL_ECC_SECP256R1};
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+                    wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_c,
+                    test1, 1), WOLFSSL_SUCCESS);
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    /* Fix: Get curve name and compare with string comparison or use curve
+     * ID function */
+    curve_name = wolfSSL_get_curve_name(ssl_s);
+    /* or use appropriate string comparison */
+    ExpectStrEQ(curve_name, "SECP256R1");
+    curve_name = wolfSSL_get_curve_name(ssl_c);
+    ExpectStrEQ(curve_name, "SECP256R1");
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+    ssl_c = NULL;
+    ssl_s = NULL;
+    ctx_c = NULL;
+    ctx_s = NULL;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+                    wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_c,
+                   test2, 1), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_s,
+                    test1, 1), WOLFSSL_SUCCESS);
+    ExpectIntNE(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+    ret = wolfSSL_get_error(ssl_s, WOLFSSL_FATAL_ERROR);
+
+    /* Fix: Use proper constant or define HANDSHAKE_FAILURE */
+    ExpectTrue(ret == WC_NO_ERR_TRACE(ECC_CURVE_ERROR));
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+    ssl_c = NULL;
+    ssl_s = NULL;
+    ctx_c = NULL;
+    ctx_s = NULL;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+                    wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_c,
+                    test3, 2),
+                    WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_s,
+                    test4, 2),
+                    WOLFSSL_SUCCESS);
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    curve_name = wolfSSL_get_curve_name(ssl_s);
+    ExpectStrEQ(curve_name, "SECP256R1");
+    curve_name = wolfSSL_get_curve_name(ssl_c);
+    ExpectStrEQ(curve_name, "SECP256R1");
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+#endif
+    return EXPECT_RESULT();
+}
+
+int test_tls13_curve_intersection(void) {
+    EXPECT_DECLS;
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
+    defined(WOLFSSL_TLS13) && defined(HAVE_ECC) && defined(HAVE_CURVE25519)
+    WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    const char* curve_name;
+    int test1[] ={WOLFSSL_ECC_SECP256R1};
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+                    wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0);
+    ExpectIntEQ(wolfSSL_set_groups(ssl_c,
+                    test1, 1), WOLFSSL_SUCCESS);
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    curve_name = wolfSSL_get_curve_name(ssl_s);
+    ExpectStrEQ(curve_name, "SECP256R1");
+    curve_name = wolfSSL_get_curve_name(ssl_c);
+    ExpectStrEQ(curve_name, "SECP256R1");
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+#endif
+    return EXPECT_RESULT();
+}
diff --git a/tests/api/test_tls.h b/tests/api/test_tls.h
@@ -24,5 +24,7 @@
 
 int test_tls12_unexpected_ccs(void);
 int test_tls13_unexpected_ccs(void);
+int test_tls12_curve_intersection(void);
+int test_tls13_curve_intersection(void);
 
 #endif /* TESTS_API_TEST_TLS_EMS_H */
