Addressing review feedback

Author: embhorn

src/tls.c

@@ -2135,7 +2135,7 @@ static void TLSX_SNI_FreeAll(SNI* list, void* heap)
 }
 
 /** Tells the buffered size of the SNI objects in a list. */
-static word16 TLSX_SNI_GetSize(SNI* list)
+WOLFSSL_TEST_VIS word16 TLSX_SNI_GetSize(SNI* list)
 {
     SNI* sni;
     word32 length = OPAQUE16_LEN; /* list length */
@@ -3241,15 +3241,19 @@ word16 TLSX_CSR_GetSize_ex(CertificateStatusRequest* csr, byte isRequest,
         if (csr->ssl != NULL && SSL_CM(csr->ssl) != NULL &&
                 SSL_CM(csr->ssl)->ocsp_stapling != NULL &&
                 SSL_CM(csr->ssl)->ocsp_stapling->statusCb != NULL) {
+            if (WOLFSSL_MAX_16BIT - OPAQUE8_LEN - OPAQUE24_LEN <
+                    csr->ssl->ocspCsrResp[idx].length) {
+                return 0;
+            }
             size = OPAQUE8_LEN + OPAQUE24_LEN +
                     csr->ssl->ocspCsrResp[idx].length;
-            if (size > WOLFSSL_MAX_16BIT)
-                return 0;
             return (word16)size;
         }
-        size = OPAQUE8_LEN + OPAQUE24_LEN + csr->responses[idx].length;
-        if (size > WOLFSSL_MAX_16BIT)
+        if (WOLFSSL_MAX_16BIT - OPAQUE8_LEN - OPAQUE24_LEN <
+                csr->responses[idx].length) {
             return 0;
+        }
+        size = OPAQUE8_LEN + OPAQUE24_LEN + csr->responses[idx].length;
         return (word16)size;
     }
 #else

tests/api/test_tls_ext.c

@@ -546,66 +546,8 @@ int test_certificate_authorities_client_hello(void) {
     return EXPECT_RESULT();
 }
 
-#if defined(HAVE_SNI) && !defined(NO_WOLFSSL_CLIENT) && !defined(NO_TLS)
-/* Walk the TLSX list to find an extension by type. */
-static TLSX* test_TLSX_find_ext_sni(TLSX* list, TLSX_Type type)
-{
-    while (list) {
-        if (list->type == type)
-            return list;
-        list = list->next;
-    }
-    return NULL;
-}
-
-/* Replicate the SNI size calculation to test overflow protection.
- * The fixed TLSX_SNI_GetSize uses word32 internally and returns 0 on
- * overflow. This helper mirrors that logic so we can verify the behavior. */
-static word16 test_SNI_GetSize(SNI* list)
-{
-    SNI* sni;
-    word32 length = OPAQUE16_LEN;
-
-    while ((sni = list)) {
-        list = sni->next;
-        length += ENUM_LEN + OPAQUE16_LEN;
-        switch (sni->type) {
-            case WOLFSSL_SNI_HOST_NAME:
-                length += (word16)XSTRLEN((char*)sni->data.host_name);
-                break;
-        }
-        if (length > WOLFSSL_MAX_16BIT)
-            return 0;
-    }
-    return (word16)length;
-}
-
-/* Replicate the old (vulnerable) SNI size calculation using word16 to
- * demonstrate the overflow that the fix prevents. */
-static word16 test_SNI_GetSize_old(SNI* list)
-{
-    SNI* sni;
-    word16 length = OPAQUE16_LEN;
-
-    while ((sni = list)) {
-        list = sni->next;
-        length += ENUM_LEN + OPAQUE16_LEN;
-        switch (sni->type) {
-            case WOLFSSL_SNI_HOST_NAME:
-                /* Intentional truncation to word16 to mirror the original
-                 * vulnerable implementation for testing overflow behavior. */
-                length += (word16)XSTRLEN((char*)sni->data.host_name);
-                break;
-        }
-    }
-    return length;
-}
-
 /* Test that the SNI size calculation returns 0 on overflow instead of
  * wrapping around to a small value (integer overflow vulnerability). */
-/* closing #if for helper functions */
-#endif /* HAVE_SNI && !NO_WOLFSSL_CLIENT && !NO_TLS */
-
 int test_TLSX_SNI_GetSize_overflow(void)
 {
     EXPECT_DECLS;
@@ -616,8 +558,6 @@ int test_TLSX_SNI_GetSize_overflow(void)
     SNI* head = NULL;
     SNI* sni = NULL;
     int i;
-    word16 fixed_size;
-    word16 old_size;
     /* Each SNI adds ENUM_LEN(1) + OPAQUE16_LEN(2) + hostname_len to the size.
      * With a 1-byte hostname, each entry adds 4 bytes. Starting from
      * OPAQUE16_LEN(2) base, we need enough entries to exceed UINT16_MAX. */
@@ -632,7 +572,7 @@ int test_TLSX_SNI_GetSize_overflow(void)
 
     /* Find the SNI extension and manually build a long chain */
     if (EXPECT_SUCCESS()) {
-        sni_ext = test_TLSX_find_ext_sni(ssl->extensions, TLSX_SERVER_NAME);
+        sni_ext = TLSX_Find(ssl->extensions, TLSX_SERVER_NAME);
         ExpectNotNull(sni_ext);
     }
 
@@ -661,17 +601,7 @@ int test_TLSX_SNI_GetSize_overflow(void)
 
     if (EXPECT_SUCCESS()) {
         /* The fixed calculation should return 0 (overflow detected) */
-        fixed_size = test_SNI_GetSize((SNI*)sni_ext->data);
-        ExpectIntEQ(fixed_size, 0);
-
-        /* The old (vulnerable) calculation would wrap around to a small value.
-         * The unwrapped total is approximately OPAQUE16_LEN + num_sni * 4,
-         * which is ~65,542. After wrapping past UINT16_MAX the result should
-         * be drastically smaller than that expected total. */
-        old_size = test_SNI_GetSize_old((SNI*)sni_ext->data);
-        ExpectIntNE(old_size, 0);
-        ExpectIntLT(old_size,
-            OPAQUE16_LEN + num_sni * (ENUM_LEN + OPAQUE16_LEN + 1));
+        ExpectIntEQ(TLSX_SNI_GetSize((SNI*)sni_ext->data), 0);
     }
 
     wolfSSL_free(ssl);

wolfssl/internal.h

@@ -3169,7 +3169,7 @@ struct TLSX {
     struct TLSX* next; /* List Behavior   */
 };
 
-WOLFSSL_LOCAL TLSX* TLSX_Find(TLSX* list, TLSX_Type type);
+WOLFSSL_TEST_VIS TLSX* TLSX_Find(TLSX* list, TLSX_Type type);
 WOLFSSL_LOCAL void  TLSX_Remove(TLSX** list, TLSX_Type type, void* heap);
 WOLFSSL_LOCAL void  TLSX_FreeAll(TLSX* list, void* heap);
 WOLFSSL_LOCAL int   TLSX_SupportExtensions(WOLFSSL* ssl);
@@ -3237,6 +3237,7 @@ WOLFSSL_LOCAL int TLSX_UseSNI(TLSX** extensions, byte type, const void* data,
 WOLFSSL_LOCAL byte TLSX_SNI_Status(TLSX* extensions, byte type);
 WOLFSSL_LOCAL word16 TLSX_SNI_GetRequest(TLSX* extensions, byte type,
                                                 void** data, byte ignoreStatus);
+WOLFSSL_TEST_VIS word16 TLSX_SNI_GetSize(SNI* list);
 
 #ifndef NO_WOLFSSL_SERVER
 WOLFSSL_LOCAL void   TLSX_SNI_SetOptions(TLSX* extensions, byte type,