testing + bug fixes for TLS ECH

Author: sebastian-carpenter

src/internal.c

@@ -8634,10 +8634,9 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl)
     ForceZero(&ssl->serverSecret, sizeof(ssl->serverSecret));
 
 #if defined(HAVE_ECH)
-    if (ssl->options.useEch == 1) {
+    if (ssl->echConfigs != NULL) {
         FreeEchConfigs(ssl->echConfigs, ssl->heap);
         ssl->echConfigs = NULL;
-        ssl->options.useEch = 0;
     }
 #endif /* HAVE_ECH */
 #endif /* WOLFSSL_TLS13 */

src/ssl_ech.c

@@ -36,7 +36,6 @@ int wolfSSL_CTX_GenerateEchConfig(WOLFSSL_CTX* ctx, const char* publicName,
     int ret = 0;
     word16 encLen = DHKEM_X25519_ENC_LEN;
     WOLFSSL_EchConfig* newConfig;
-    WOLFSSL_EchConfig* parentConfig;
 #ifdef WOLFSSL_SMALL_STACK
     Hpke* hpke = NULL;
     WC_RNG* rng;
@@ -63,7 +62,9 @@ int wolfSSL_CTX_GenerateEchConfig(WOLFSSL_CTX* ctx, const char* publicName,
     else
         XMEMSET(newConfig, 0, sizeof(WOLFSSL_EchConfig));
 
-    /* set random config id */
+    /* set random configId */
+    /* TODO: if an equal configId is found should the old config be removed from
+     * the LL? Prevents growth beyond 255+ items */
     if (ret == 0)
         ret = wc_RNG_GenerateByte(rng, &newConfig->configId);
 
@@ -139,17 +140,14 @@ int wolfSSL_CTX_GenerateEchConfig(WOLFSSL_CTX* ctx, const char* publicName,
         }
     }
     else {
-        parentConfig = ctx->echConfigs;
-
-        if (parentConfig == NULL) {
+        /* insert new configs at beginning of LL as preference should be given
+         * to the most recently generated configs */
+        if (ctx->echConfigs == NULL) {
             ctx->echConfigs = newConfig;
         }
         else {
-            while (parentConfig->next != NULL) {
-                parentConfig = parentConfig->next;
-            }
-
-            parentConfig->next = newConfig;
+            newConfig->next = ctx->echConfigs;
+            ctx->echConfigs = newConfig;
         }
     }
 
@@ -242,7 +240,7 @@ void wolfSSL_CTX_SetEchEnable(WOLFSSL_CTX* ctx, byte enable)
 
 /* set the ech config from base64 for our client ssl object, base64 is the
  * format ech configs are sent using dns records */
-int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, char* echConfigs64,
+int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, const char* echConfigs64,
     word32 echConfigs64Len)
 {
     int ret = 0;
@@ -253,7 +251,7 @@ int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, char* echConfigs64,
         return BAD_FUNC_ARG;
 
     /* already have ech configs */
-    if (ssl->options.useEch == 1) {
+    if (ssl->echConfigs != NULL) {
         return WOLFSSL_FATAL_ERROR;
     }
 
@@ -266,7 +264,7 @@ int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, char* echConfigs64,
     decodedConfigs[decodedLen - 1] = 0;
 
     /* decode the echConfigs */
-    ret = Base64_Decode((byte*)echConfigs64, echConfigs64Len,
+    ret = Base64_Decode((const byte*)echConfigs64, echConfigs64Len,
       decodedConfigs, &decodedLen);
 
     if (ret != 0) {
@@ -292,7 +290,7 @@ int wolfSSL_SetEchConfigs(WOLFSSL* ssl, const byte* echConfigs,
         return BAD_FUNC_ARG;
 
     /* already have ech configs */
-    if (ssl->options.useEch == 1) {
+    if (ssl->echConfigs != NULL) {
         return WOLFSSL_FATAL_ERROR;
     }
 
@@ -301,7 +299,6 @@ int wolfSSL_SetEchConfigs(WOLFSSL* ssl, const byte* echConfigs,
 
     /* if we found valid configs */
     if (ret == 0) {
-        ssl->options.useEch = 1;
         return WOLFSSL_SUCCESS;
     }
 
@@ -459,7 +456,7 @@ int wolfSSL_GetEchConfigs(WOLFSSL* ssl, byte* output, word32* outputLen)
         return BAD_FUNC_ARG;
 
     /* if we don't have ech configs */
-    if (ssl->options.useEch != 1) {
+    if (ssl->echConfigs == NULL) {
         return WOLFSSL_FATAL_ERROR;
     }
 

src/tls.c

@@ -2233,9 +2233,10 @@ static int TLSX_SNI_Parse(WOLFSSL* ssl, const byte* input, word16 length,
     byte type;
     byte matched;
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+    TLSX* echX = NULL;
     WOLFSSL_ECH* ech = NULL;
     WOLFSSL_EchConfig* workingConfig;
-    TLSX* echX;
+    word16 privateNameLen;
 #endif
 #endif /* !NO_WOLFSSL_SERVER */
     TLSX *extension = TLSX_Find(ssl->extensions, TLSX_SERVER_NAME);
@@ -2266,6 +2267,15 @@ static int TLSX_SNI_Parse(WOLFSSL* ssl, const byte* input, word16 length,
         #endif
     }
 
+#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+    if (!ssl->options.disableECH) {
+        echX = TLSX_Find(ssl->extensions, TLSX_ECH);
+        if (echX != NULL) {
+            ech = (WOLFSSL_ECH*)(echX->data);
+        }
+    }
+#endif
+
 #ifndef NO_WOLFSSL_SERVER
     if (!extension || !extension->data) {
         /* This will keep SNI even though TLSX_UseSNI has not been called.
@@ -2282,11 +2292,32 @@ static int TLSX_SNI_Parse(WOLFSSL* ssl, const byte* input, word16 length,
         if (cacheOnly) {
             WOLFSSL_MSG("Forcing SSL object to store SNI parameter");
         }
+    #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+        else if (ech != NULL && ech->sniState != ECH_OUTER_SNI) {
+            /* verifying the outer SNI is not necessary, but the inner SNI does
+             * need to be verified - so error when server SNI does not exist and
+             * ECH is being used */
+            SendAlert(ssl, alert_fatal, UNKNOWN_SNI_HOST_NAME_E);
+            WOLFSSL_ERROR_VERBOSE(UNKNOWN_SNI_HOST_NAME_E);
+            return UNKNOWN_SNI_HOST_NAME_E;
+        }
+    #endif
         else {
             /* Skipping, SNI not enabled at server side. */
             return 0;
         }
     }
+#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+    else if (ech != NULL && ech->sniState == ECH_INNER_SNI) {
+        if (ssl->ctx->sniRecvCb) {
+            cacheOnly = 1;
+        }
+
+        if (cacheOnly) {
+            WOLFSSL_MSG("Forcing SSL object to store SNI parameter (inner)");
+        }
+    }
+#endif
 
     if (OPAQUE16_LEN > length)
         return BUFFER_ERROR;
@@ -2315,24 +2346,58 @@ static int TLSX_SNI_Parse(WOLFSSL* ssl, const byte* input, word16 length,
     if (!cacheOnly && !(sni = TLSX_SNI_Find((SNI*)extension->data, type)))
         return 0; /* not using this type of SNI. */
 
-#ifdef WOLFSSL_TLS13
+#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+    if (ech != NULL && ech->sniState == ECH_INNER_SNI){
+        /* SNI status is carried over from processing the outer hello so it is
+        * necessary to clear it before processing the inner hello */
+        ech->sniState = ECH_INNER_SNI_ATTEMPT;
+        if (sni != NULL){
+            sni->status = WOLFSSL_SNI_NO_MATCH;
+        }
+    }
+    else if (ech != NULL && ech->sniState == ECH_OUTER_SNI &&
+            ech->privateName == NULL && sni != NULL){
+        /* save the private SNI before it is overwritten by the public SNI */
+        privateNameLen = (word16)XSTRLEN(sni->data.host_name) + 1;
+        ech->privateName = (char*)XMALLOC(privateNameLen, ssl->heap,
+            DYNAMIC_TYPE_TMP_BUFFER);
+        if (ech->privateName == NULL)
+            return MEMORY_E;
+        XMEMCPY((char*)ech->privateName, sni->data.host_name,
+            privateNameLen);
+    }
+#endif
+
+#if defined(WOLFSSL_TLS13)
     /* Don't process the second ClientHello SNI extension if there
      * was problems with the first.
      */
-    if (!cacheOnly && sni->status != 0)
+    if (!cacheOnly && sni->status != WOLFSSL_SNI_NO_MATCH)
         return 0;
 #endif
-    matched = cacheOnly || (XSTRLEN(sni->data.host_name) == size &&
-         XSTRNCMP(sni->data.host_name, (const char*)input + offset, size) == 0);
 
-#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
-    echX = TLSX_Find(ssl->extensions, TLSX_ECH);
-    if (echX != NULL)
-        ech = (WOLFSSL_ECH*)(echX->data);
+#if defined(HAVE_ECH)
+    if (ech != NULL && ech->sniState == ECH_INNER_SNI_ATTEMPT) {
+        if (ech->privateName != NULL) {
+            matched = cacheOnly || (XSTRLEN(ech->privateName) == size &&
+                XSTRNCMP(ech->privateName, (const char*)input + offset,
+                                                                    size) == 0);
+        }
+        else {
+            matched = cacheOnly;
+        }
+    }
+    else
+#endif
+    {
+        matched = cacheOnly || (XSTRLEN(sni->data.host_name) == size &&
+            XSTRNCMP(sni->data.host_name, (const char*)input + offset,
+                                                                    size) == 0);
+    }
 
-    if (!matched && ech != NULL) {
+#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
+    if (!matched && ech != NULL && ech->sniState == ECH_OUTER_SNI) {
         workingConfig = ech->echConfig;
-
         while (workingConfig != NULL) {
             matched = XSTRLEN(workingConfig->publicName) == size &&
                 XSTRNCMP(workingConfig->publicName,
@@ -2350,6 +2415,7 @@ static int TLSX_SNI_Parse(WOLFSSL* ssl, const byte* input, word16 length,
         int matchStat;
         int r = TLSX_UseSNI(&ssl->extensions, type, input + offset, size,
                                                                      ssl->heap);
+
         if (r != WOLFSSL_SUCCESS)
             return r; /* throws error. */
 
@@ -13886,12 +13952,12 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size,
     byte* aadCopy;
     byte* readBuf_p = (byte*)readBuf;
     WOLFSSL_MSG("TLSX_ECH_Parse");
-    if (size == 0)
-        return BAD_FUNC_ARG;
     if (ssl->options.disableECH) {
         WOLFSSL_MSG("TLSX_ECH_Parse: ECH disabled. Ignoring.");
         return 0;
     }
+    if (size == 0)
+        return BAD_FUNC_ARG;
     /* retry configs */
     if (msgType == encrypted_extensions) {
         ret = wolfSSL_SetEchConfigs(ssl, readBuf, size);
@@ -13900,7 +13966,8 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size,
             ret = 0;
     }
     /* HRR with special confirmation */
-    else if (msgType == hello_retry_request && ssl->options.useEch) {
+    else if (msgType == hello_retry_request && ssl->echConfigs != NULL &&
+            !ssl->options.disableECH) {
         /* length must be 8 */
         if (size != ECH_ACCEPT_CONFIRMATION_SZ)
             return BAD_FUNC_ARG;
@@ -14000,6 +14067,7 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size,
         if (ret == 0) {
             i = 0;
             /* decrement until before the padding */
+            /* TODO: verify padding is 0, abort with illegal_parameter */
             while (ech->innerClientHello[ech->innerClientHelloLen +
                 HANDSHAKE_HEADER_SZ - i - 1] != ECH_TYPE_INNER) {
                 i++;
@@ -14035,6 +14103,8 @@ static void TLSX_ECH_Free(WOLFSSL_ECH* ech, void* heap)
         XFREE(ech->hpke, heap, DYNAMIC_TYPE_TMP_BUFFER);
     if (ech->hpkeContext != NULL)
         XFREE(ech->hpkeContext, heap, DYNAMIC_TYPE_TMP_BUFFER);
+    if (ech->privateName != NULL)
+        XFREE((char*)ech->privateName, heap, DYNAMIC_TYPE_TMP_BUFFER);
 
     XFREE(ech, heap, DYNAMIC_TYPE_TMP_BUFFER);
     (void)heap;
@@ -15814,7 +15884,7 @@ int TLSX_GetRequestSize(WOLFSSL* ssl, byte msgType, word32* pLength)
     }
     #endif
 #if defined(HAVE_ECH)
-    if (ssl->options.useEch == 1 && !ssl->options.disableECH
+    if (ssl->echConfigs != NULL && !ssl->options.disableECH
             && msgType == client_hello) {
         ret = TLSX_GetSizeWithEch(ssl, semaphore, msgType, &length);
         if (ret != 0)
@@ -16000,7 +16070,7 @@ int TLSX_WriteRequest(WOLFSSL* ssl, byte* output, byte msgType, word32* pOffset)
     #endif
 #endif
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
-    if (ssl->options.useEch == 1 && !ssl->options.disableECH
+    if (ssl->echConfigs != NULL && !ssl->options.disableECH
             && msgType == client_hello) {
         ret = TLSX_WriteWithEch(ssl, output, semaphore,
                          msgType, &offset);
@@ -17280,7 +17350,8 @@ int TLSX_Parse(WOLFSSL* ssl, const byte* input, word16 length, byte msgType,
 
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
     /* If client used ECH, server HRR must include ECH confirmation */
-    if (ret == 0 && msgType == hello_retry_request && ssl->options.useEch == 1) {
+    if (ret == 0 && msgType == hello_retry_request && ssl->echConfigs != NULL &&
+            !ssl->options.disableECH) {
         TLSX* echX = TLSX_Find(ssl->extensions, TLSX_ECH);
         if (echX == NULL || ((WOLFSSL_ECH*)echX->data)->confBuf == NULL) {
             WOLFSSL_MSG("ECH used but HRR missing ECH confirmation");