TLS cipher suite: improvements

wolfSSL_clear: check return from InitSSL_Suites() call.
TLS13: check ClientHello cipher suite length is even.
Silently remove duplicate cipher suites from user input.
Add tests of duplicate cipher suite removal.

src/internal.c

@@ -24208,6 +24208,8 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list)
     #endif /* OPENSSL_EXTRA */
 
         for (i = 0; i < suiteSz; i++) {
+            int j;
+
             if (XSTRNCMP(name, cipher_names[i].name, sizeof(name)) == 0
             #ifndef NO_ERROR_STRINGS
                 || XSTRNCMP(name, cipher_names[i].name_iana, sizeof(name)) == 0
@@ -24225,6 +24227,17 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list)
                 }
             #endif /* WOLFSSL_DTLS */
 
+                for (j = 0; j < idx; j += 2) {
+                    if ((suites->suites[j+0] == cipher_names[i].cipherSuite0) &&
+                        (suites->suites[j+1] == cipher_names[i].cipherSuite)) {
+                        break;
+                    }
+                }
+                /* Silently drop duplicates from list. */
+                if (j != idx) {
+                    break;
+                }
+
                 if (idx + 1 >= WOLFSSL_MAX_SUITE_SZ) {
                     WOLFSSL_MSG("WOLFSSL_MAX_SUITE_SZ set too low");
                     return 0; /* suites buffer not large enough, error out */
@@ -24341,10 +24354,15 @@ int SetCipherListFromBytes(WOLFSSL_CTX* ctx, Suites* suites, const byte* list,
         return 0;
     }
 
+    if ((listSz % 2) != 0) {
+        return 0;
+    }
+
     for (i = 0; (i + 1) < listSz; i += 2) {
         const byte firstByte = list[i];
         const byte secondByte = list[i + 1];
         const char* name = NULL;
+        int j;
 
         name = GetCipherNameInternal(firstByte, secondByte);
         if (XSTRCMP(name, "None") == 0) {
@@ -24362,6 +24380,17 @@ int SetCipherListFromBytes(WOLFSSL_CTX* ctx, Suites* suites, const byte* list,
         }
     #endif /* WOLFSSL_DTLS */
 
+        for (j = 0; j < idx; j += 2) {
+            if ((suites->suites[j+0] == firstByte) &&
+                    (suites->suites[j+1] == secondByte)) {
+                break;
+            }
+        }
+        /* Silently drop duplicates from list. */
+        if (j != idx) {
+            continue;
+        }
+
         if (idx + 1 >= WOLFSSL_MAX_SUITE_SZ) {
             WOLFSSL_MSG("WOLFSSL_MAX_SUITE_SZ set too low");
             return 0; /* suites buffer not large enough, error out */

src/ssl.c

@@ -18718,7 +18718,8 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out,
         ssl->keys.encryptionOn = 0;
         XMEMSET(&ssl->msgsReceived, 0, sizeof(ssl->msgsReceived));
 
-        InitSSL_Suites(ssl);
+        if (InitSSL_Suites(ssl) != WOLFSSL_SUCCESS)
+            return WOLFSSL_FAILURE;
 
         if (InitHandshakeHashes(ssl) != 0)
             return WOLFSSL_FAILURE;

src/tls13.c

@@ -4764,26 +4764,30 @@ static void RefineSuites(WOLFSSL* ssl, Suites* peerSuites)
         /* Server order refining. */
         for (i = 0; i < ssl->suites->suiteSz; i += 2) {
             for (j = 0; j < peerSuites->suiteSz; j += 2) {
-                if (ssl->suites->suites[i+0] == peerSuites->suites[j+0] &&
+                if ((ssl->suites->suites[i+0] == peerSuites->suites[j+0]) &&
-                    ssl->suites->suites[i+1] == peerSuites->suites[j+1]) {
+                    (ssl->suites->suites[i+1] == peerSuites->suites[j+1])) {
                     suites[suiteSz++] = peerSuites->suites[j+0];
                     suites[suiteSz++] = peerSuites->suites[j+1];
                     break;
                 }
             }
+            if (suiteSz == WOLFSSL_MAX_SUITE_SZ)
+                break;
         }
     }
     else {
         /* Client order refining. */
         for (j = 0; j < peerSuites->suiteSz; j += 2) {
             for (i = 0; i < ssl->suites->suiteSz; i += 2) {
-                if (ssl->suites->suites[i+0] == peerSuites->suites[j+0] &&
+                if ((ssl->suites->suites[i+0] == peerSuites->suites[j+0]) &&
-                    ssl->suites->suites[i+1] == peerSuites->suites[j+1]) {
+                    (ssl->suites->suites[i+1] == peerSuites->suites[j+1])) {
                     suites[suiteSz++] = peerSuites->suites[j+0];
                     suites[suiteSz++] = peerSuites->suites[j+1];
                     break;
                 }
             }
+            if (suiteSz == WOLFSSL_MAX_SUITE_SZ)
+                break;
         }
     }
 
@@ -5788,6 +5792,9 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
         ERROR_OUT(BUFFER_ERROR, exit_dch);
     ato16(&input[args->idx], &args->clSuites->suiteSz);
     args->idx += OPAQUE16_LEN;
+    if ((args->clSuites->suiteSz % 2) != 0) {
+        ERROR_OUT(INVALID_PARAMETER, exit_dch);
+    }
     /* suites and compression length check */
     if ((args->idx - args->begin) + args->clSuites->suiteSz + OPAQUE8_LEN > helloSz)
         ERROR_OUT(BUFFER_ERROR, exit_dch);

tests/api.c

@@ -51033,6 +51033,19 @@ static int test_tls13_cipher_suites(void)
     const int csOff = 78;
     /* Server cipher list. */
     const char* serverCs = "TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256";
+    /* Suite list with duplicates. */
+    const char* dupCs = "TLS13-AES128-GCM-SHA256:"
+                        "TLS13-AES128-GCM-SHA256:"
+                        "TLS13-AES256-GCM-SHA384:"
+                        "TLS13-AES256-GCM-SHA384:"
+                        "TLS13-AES128-GCM-SHA256";
+#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_SET_CIPHER_BYTES)
+    const byte dupCsBytes[] = { TLS13_BYTE, TLS_AES_256_GCM_SHA384,
+                                TLS13_BYTE, TLS_AES_256_GCM_SHA384,
+                                TLS13_BYTE, TLS_AES_128_GCM_SHA256,
+                                TLS13_BYTE, TLS_AES_128_GCM_SHA256,
+                                TLS13_BYTE, TLS_AES_256_GCM_SHA384 };
+#endif
 
     printf(testingFmt, "test_tls13_cipher_suites");
 
@@ -51076,14 +51089,11 @@ static int test_tls13_cipher_suites(void)
     msg.length = (unsigned int)sizeof(clientHello);
     wolfSSL_SetIOReadCtx(ssl, &msg);
     /* Server order: TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256 */
-#ifdef OPENSSL_EXTRA
     AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), WOLFSSL_SUCCESS);
-#else
-    AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), 1);
-#endif
     /* Negotiate cipher suites in server order: TLS13-AES256-GCM-SHA384 */
     wolfSSL_accept_TLSv13(ssl);
     /* Check refined order - server order. */
+    AssertIntEQ(ssl->suites->suiteSz, 4);
     AssertIntEQ(ssl->suites->suites[0], TLS13_BYTE);
     AssertIntEQ(ssl->suites->suites[1], TLS_AES_256_GCM_SHA384);
     AssertIntEQ(ssl->suites->suites[2], TLS13_BYTE);
@@ -51096,21 +51106,36 @@ static int test_tls13_cipher_suites(void)
     msg.length = (unsigned int)sizeof(clientHello);
     wolfSSL_SetIOReadCtx(ssl, &msg);
     /* Server order: TLS13-AES256-GCM-SHA384:TLS13-AES128-GCM-SHA256 */
-#ifdef OPENSSL_EXTRA
     AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), WOLFSSL_SUCCESS);
-#else
-    AssertIntEQ(wolfSSL_set_cipher_list(ssl, serverCs), 1);
-#endif
     AssertIntEQ(wolfSSL_UseClientSuites(ssl), 0);
     /* Negotiate cipher suites in client order: TLS13-AES128-GCM-SHA256 */
     wolfSSL_accept_TLSv13(ssl);
     /* Check refined order - client order. */
+    AssertIntEQ(ssl->suites->suiteSz, 4);
     AssertIntEQ(ssl->suites->suites[0], TLS13_BYTE);
     AssertIntEQ(ssl->suites->suites[1], TLS_AES_128_GCM_SHA256);
     AssertIntEQ(ssl->suites->suites[2], TLS13_BYTE);
     AssertIntEQ(ssl->suites->suites[3], TLS_AES_256_GCM_SHA384);
     wolfSSL_free(ssl);
 
+    /* Check duplicate detection is working. */
+    AssertIntEQ(wolfSSL_CTX_set_cipher_list(ctx, dupCs), WOLFSSL_SUCCESS);
+    AssertIntEQ(ctx->suites->suiteSz, 4);
+    AssertIntEQ(ctx->suites->suites[0], TLS13_BYTE);
+    AssertIntEQ(ctx->suites->suites[1], TLS_AES_128_GCM_SHA256);
+    AssertIntEQ(ctx->suites->suites[2], TLS13_BYTE);
+    AssertIntEQ(ctx->suites->suites[3], TLS_AES_256_GCM_SHA384);
+
+#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_SET_CIPHER_BYTES)
+    AssertIntEQ(wolfSSL_CTX_set_cipher_list_bytes(ctx, dupCsBytes,
+        sizeof(dupCsBytes)), WOLFSSL_SUCCESS);
+    AssertIntEQ(ctx->suites->suiteSz, 4);
+    AssertIntEQ(ctx->suites->suites[0], TLS13_BYTE);
+    AssertIntEQ(ctx->suites->suites[1], TLS_AES_256_GCM_SHA384);
+    AssertIntEQ(ctx->suites->suites[2], TLS13_BYTE);
+    AssertIntEQ(ctx->suites->suites[3], TLS_AES_128_GCM_SHA256);
+#endif
+
     wolfSSL_CTX_free(ctx);
 
     printf(resultFmt, passed);