Simplify TLSX_SupportedCurve_Parse

Server only uses curves that are supported by both the client and the server. If no common groups are found, the connection will fail in TLS 1.2 and below. In TLS 1.3, HRR may still be used to resolve the group mismatch.

Author: julek-wolfssl

src/tls.c

@@ -5042,12 +5042,14 @@ static word16 TLSX_PointFormat_Write(PointFormat* list, byte* output)
 
 #if !defined(NO_WOLFSSL_SERVER) || (defined(WOLFSSL_TLS13) && \
                                          !defined(WOLFSSL_NO_SERVER_GROUPS_EXT))
+
 int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
                               word16 length, byte isRequest, TLSX** extensions)
 {
     word16 offset;
     word16 name;
     int ret;
+    TLSX* extension;
 
     if(!isRequest && !IsAtLeastTLSv1_3(ssl->version)) {
 #ifdef WOLFSSL_ALLOW_SERVER_SC_EXT
@@ -5066,74 +5068,47 @@ int TLSX_SupportedCurve_Parse(const WOLFSSL* ssl, const byte* input,
     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);
-            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;
-        }
-    }
-#endif
-    if(!IsAtLeastTLSv1_3(ssl->version)) {
-        TLSX* existingExt;
-        TLSX* newExt = NULL;
-        SupportedCurve* intersectionCurve;
-
-        existingExt = TLSX_Find(*extensions, TLSX_SUPPORTED_GROUPS);
+    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);
-            if (existingExt != NULL) {
-                /* Check if this curve exists in our current list */
-                intersectionCurve = (SupportedCurve*)existingExt->data;
-                while (intersectionCurve != NULL) {
-                    if (intersectionCurve->name == name) {
-                        ret = TLSX_UseSupportedCurve(&newExt, name, ssl->heap);
-                        if (ret != WOLFSSL_SUCCESS && ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG))
-                            return ret;
-                        break;
-                    }
-                    intersectionCurve = intersectionCurve->next;
-                }
-            }
-            else {
-                ret = TLSX_UseSupportedCurve(extensions, name, ssl->heap);
-                if (ret != WOLFSSL_SUCCESS && ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG))
-                    return ret;
-            }
+
+            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 (existingExt != NULL) {
-            if (newExt == NULL){
-                return ECC_CURVE_ERROR;
+            if (ret != WOLFSSL_SUCCESS &&
+                    ret != WC_NO_ERR_TRACE(BAD_FUNC_ARG)) {
+                return ret;
             }
-            /* Replace existing extension data with intersection */
-            intersectionCurve = (SupportedCurve*)existingExt->data;
-            TLSX_SupportedCurve_FreeAll(intersectionCurve, ssl->heap);
-            existingExt->data = newExt->data;
         }
-    }else{
+    }
+    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);
 
-            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;
+            while (foundCurve != NULL && foundCurve->name != name)
+                foundCurve = foundCurve->next;
+
+            if (foundCurve != NULL) {
+                ret = commonCurves == NULL ?
+                      TLSX_SupportedCurve_New(&commonCurves, name, ssl->heap) :
+                      TLSX_SupportedCurve_Append(commonCurves, name, ssl->heap);
+                if (ret != 0)
+                    return ret;
             }
         }
+        /* If no common curves return error. In TLS 1.3 we can still try to save
+         * this by using HRR. */
+        if (commonCurves == NULL && !IsAtLeastTLSv1_3(ssl->version))
+            return ECC_CURVE_ERROR;
+        /* Now swap out the curves in the extension */
+        TLSX_SupportedCurve_FreeAll((SupportedCurve*)extension->data,
+                                    ssl->heap);
+        extension->data = commonCurves;
     }
 
     return 0;
@@ -10828,15 +10803,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))
@@ -10853,8 +10830,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
@@ -10878,7 +10873,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;

tests/api.c

@@ -30634,11 +30634,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

tests/api/test_tls.c

@@ -188,7 +188,7 @@ int test_tls12_curve_intersection(void) {
     ret = wolfSSL_get_error(ssl_s, WOLFSSL_FATAL_ERROR);
 
     // Fix: Use proper constant or define HANDSHAKE_FAILURE
-    ExpectTrue(ret == ECC_CURVE_ERROR);
+    ExpectTrue(ret == WC_NO_ERR_TRACE(ECC_CURVE_ERROR));
 
     wolfSSL_free(ssl_c);
     wolfSSL_free(ssl_s);