src/tls.c, wolfssl/ssl.h, tests/api.c: followup to ff7a32d (#10182):

* Fix OOB heap reads via TLSX_ExtractEch() by preemptively rejecting oversized
  SNI names in TLSX_UseSNI().

* In TLSX_EchChangeSNI(), don't attempt to truncate if an oversized name is
  seen, just return error.

* Move definition of WOLFSSL_HOST_NAME_MAX to an ungated context in ssl.h, and
  use it consistently in tls.c, eliminating the duplicative
  WOLFSSL_HOST_NAME_MAX.

Author: douzzer

src/tls.c

@@ -2613,6 +2613,9 @@ int TLSX_UseSNI(TLSX** extensions, byte type, const void* data, word16 size,
     if (extensions == NULL || data == NULL)
         return BAD_FUNC_ARG;
 
+    if ((type == WOLFSSL_SNI_HOST_NAME) && (size >= WOLFSSL_HOST_NAME_MAX))
+        return BAD_LENGTH_E;
+
     if ((sni = TLSX_SNI_New(type, data, size, heap)) == NULL)
         return MEMORY_E;
 
@@ -13445,7 +13448,6 @@ void TLSX_Remove(TLSX** list, TLSX_Type type, void* heap)
 
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
 #define GREASE_ECH_SIZE 160
-#define MAX_PUBLIC_NAME_SZ 256
 #define TLS_INFO_CONST_STRING "tls ech"
 #define TLS_INFO_CONST_STRING_SZ 7
 
@@ -16101,14 +16103,10 @@ static int TLSX_EchChangeSNI(WOLFSSL* ssl, TLSX** pEchX,
             char* hostName = ((SNI*)serverNameX->data)->data.host_name;
             word32 hostNameSz = (word32)XSTRLEN(hostName) + 1;
 
-            /* truncate if too long */
-            if (hostNameSz > MAX_PUBLIC_NAME_SZ)
-                hostNameSz = MAX_PUBLIC_NAME_SZ;
-
-            XMEMCPY(serverName, hostName, hostNameSz);
-            /* Guarantee NUL termination after truncation so that
-             * TLSX_EchRestoreSNI's XSTRLEN cannot read past the buffer. */
-            serverName[hostNameSz - 1] = '\0';
+            if (hostNameSz > WOLFSSL_HOST_NAME_MAX)
+                ret = BAD_LENGTH_E;
+            else
+                XMEMCPY(serverName, hostName, hostNameSz);
         }
 
         /* only swap the SNI if one was found; extensions is non-NULL if an
@@ -16161,9 +16159,9 @@ static int TLSX_GetSizeWithEch(WOLFSSL* ssl, byte* semaphore, byte msgType,
     TLSX* echX = NULL;
     TLSX* serverNameX = NULL;
     TLSX** extensions = NULL;
-    WC_DECLARE_VAR(serverName, char, MAX_PUBLIC_NAME_SZ, 0);
+    WC_DECLARE_VAR(serverName, char, WOLFSSL_HOST_NAME_MAX, 0);
 
-    WC_ALLOC_VAR_EX(serverName, char, MAX_PUBLIC_NAME_SZ, NULL,
+    WC_ALLOC_VAR_EX(serverName, char, WOLFSSL_HOST_NAME_MAX, NULL,
                     DYNAMIC_TYPE_TMP_BUFFER, return MEMORY_E);
     r = TLSX_EchChangeSNI(ssl, &echX, serverName, &serverNameX, &extensions);
     if (r == 0 && ssl->extensions)
@@ -16303,9 +16301,9 @@ static int TLSX_WriteWithEch(WOLFSSL* ssl, byte* output, byte* semaphore,
     TLSX* echX = NULL;
     TLSX* serverNameX = NULL;
     TLSX** extensions = NULL;
-    WC_DECLARE_VAR(serverName, char, MAX_PUBLIC_NAME_SZ, 0);
+    WC_DECLARE_VAR(serverName, char, WOLFSSL_HOST_NAME_MAX, 0);
 
-    WC_ALLOC_VAR_EX(serverName, char, MAX_PUBLIC_NAME_SZ, NULL,
+    WC_ALLOC_VAR_EX(serverName, char, WOLFSSL_HOST_NAME_MAX, NULL,
                     DYNAMIC_TYPE_TMP_BUFFER, return MEMORY_E);
     r = TLSX_EchChangeSNI(ssl, &echX, serverName, &serverNameX, &extensions);
     ret = r;

tests/api.c

@@ -15202,13 +15202,15 @@ static int test_wolfSSL_Tls13_ECH_long_SNI(void)
     ExpectIntEQ(wolfSSL_SetEchConfigs(test_ctx.c_ssl, echCbTestConfigs,
         echCbTestConfigsLen), WOLFSSL_SUCCESS);
 
-    /* Set the over-long SNI as the inner hostname */
+    /* Try to set the over-long SNI as the inner hostname -- after the fix, this
+     * is expected to fail.
+     */
     ExpectIntEQ(wolfSSL_UseSNI(test_ctx.c_ssl, WOLFSSL_SNI_HOST_NAME,
-        longName, (word16)XSTRLEN(longName)), WOLFSSL_SUCCESS);
+        longName, (word16)XSTRLEN(longName)), BAD_LENGTH_E);
 
-    /* The handshake triggers TLSX_EchChangeSNI / TLSX_EchRestoreSNI.
-     * Before the fix this would stack-buffer-overflow in XSTRLEN.
-     * The connection may fail (SNI mismatch) but must not crash. */
+    /* Before the fix, the handshake would trigger TLSX_EchChangeSNI /
+     * TLSX_EchRestoreSNI, which would then stack-buffer-overflow in XSTRLEN.
+     */
     (void)test_ssl_memio_do_handshake(&test_ctx, 10, NULL);
 
     test_ssl_memio_cleanup(&test_ctx);

wolfssl/ssl.h

@@ -455,6 +455,7 @@ struct WOLFSSL_ASN1_STRING {
 
 #define WOLFSSL_MAX_SNAME 40
 
+#define WOLFSSL_HOST_NAME_MAX  256
 
 #define WOLFSSL_ASN1_DYNAMIC 0x1
 #define WOLFSSL_ASN1_DYNAMIC_DATA 0x2
@@ -861,7 +862,6 @@ struct WOLFSSL_X509_STORE {
 #define WOLFSSL_USE_CHECK_TIME 0x2
 #define WOLFSSL_NO_CHECK_TIME  0x200000
 #define WOLFSSL_PARTIAL_CHAIN  0x80000
-#define WOLFSSL_HOST_NAME_MAX  256
 
 #define WOLFSSL_VPARAM_DEFAULT          0x1
 #define WOLFSSL_VPARAM_OVERWRITE        0x2