improve ECH tests even further

fix HPKE error propagation in CH2

Author: sebastian-carpenter

src/ssl_ech.c

@@ -454,13 +454,14 @@ int wolfSSL_GetEchConfigs(WOLFSSL* ssl, byte* output, word32* outputLen)
 }
 
 /* wrapper function to get retry configs
+ * a client should only call this after the 'wolfSSL_connect()' call fails
  * returns error if retry configs were not received or were malformed */
 int wolfSSL_GetEchRetryConfigs(WOLFSSL* ssl, byte* output, word32* outputLen)
 {
     if (ssl == NULL || outputLen == NULL)
         return BAD_FUNC_ARG;
 
-    if (ssl->echRetryConfigs == NULL || !ssl->echRetryConfigsAccepted) {
+    if (ssl->echRetryConfigs == NULL || !ssl->options.echRetryConfigsAccepted) {
         return WOLFSSL_FATAL_ERROR;
     }
 

src/tls.c

@@ -13915,7 +13915,7 @@ static const byte* TLSX_ECH_FindOuterExtension(const byte* outerCh,
  * newInnerCh       The inner ClientHello buffer.
  * newInnerChLen    Inner ClientHello length.
  * numOuterRefs     Number of references described by OuterExtensions extension.
- * numOuterTypes    References described by OuterExtensions extension.
+ * OuterRefTypes    References described by OuterExtensions extension.
  * returns 0 on success and otherwise failure.
  */
 static int TLSX_ECH_CopyOuterExtensions(const byte* outerCh, word32 outerChLen,
@@ -13930,6 +13930,10 @@ static int TLSX_ECH_CopyOuterExtensions(const byte* outerCh, word32 outerChLen,
     word16 extsLen = 0;
     const byte* outerExtData;
 
+    if (newInnerCh == NULL) {
+        *newInnerChLen = 0;
+    }
+
     while (numOuterRefs-- > 0) {
         ato16(outerRefTypes, &refType);
 
@@ -13995,7 +13999,7 @@ static int TLSX_ECH_ExpandOuterExtensions(WOLFSSL* ssl, WOLFSSL_ECH* ech,
     int foundEchOuter = 0;
     word16 numOuterRefs = 0;
     const byte* outerRefTypes = NULL;
-    word32 extraSize = 0;
+    word32 extraSize;
     byte* newInnerCh = NULL;
     byte* newInnerChRef;
     word32 newInnerChLen;
@@ -14166,6 +14170,7 @@ static int TLSX_ExtractEch(WOLFSSL_ECH* ech, WOLFSSL_EchConfig* echConfig,
 {
     int ret = 0;
     int i;
+    int allocatedHpke = 0;
     word32 rawConfigLen = 0;
     byte* info = NULL;
     word32 infoLen = 0;
@@ -14186,6 +14191,7 @@ static int TLSX_ExtractEch(WOLFSSL_ECH* ech, WOLFSSL_EchConfig* echConfig,
     }
     /* check if hpke already exists, may if HelloRetryRequest */
     if (ech->hpke == NULL) {
+        allocatedHpke = 1;
         ech->hpke = (Hpke*)XMALLOC(sizeof(Hpke), heap, DYNAMIC_TYPE_TMP_BUFFER);
         if (ech->hpke == NULL)
             ret = MEMORY_E;
@@ -14234,8 +14240,9 @@ static int TLSX_ExtractEch(WOLFSSL_ECH* ech, WOLFSSL_EchConfig* echConfig,
             ech->outerClientPayload, ech->innerClientHelloLen,
             ech->innerClientHello + HANDSHAKE_HEADER_SZ);
     }
-    /* free the hpke and context on failure */
-    if (ret != 0) {
+    /* only free hpke/hpkeContext if allocated in this call; otherwise preserve
+     * them for clientHello2 */
+    if (ret != 0 && allocatedHpke) {
         XFREE(ech->hpke, heap, DYNAMIC_TYPE_TMP_BUFFER);
         ech->hpke = NULL;
         XFREE(ech->hpkeContext, heap, DYNAMIC_TYPE_TMP_BUFFER);
@@ -14295,7 +14302,7 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size,
 
         /* retry configs may only be accepted at the point when ECH_REQUIRED is
          * sent */
-        ssl->echRetryConfigsAccepted = 0;
+        ssl->options.echRetryConfigsAccepted = 0;
     }
     /* HRR with special confirmation */
     else if (msgType == hello_retry_request && ssl->echConfigs != NULL) {

src/tls13.c

@@ -14131,7 +14131,9 @@ int wolfSSL_connect_TLSv13(WOLFSSL* ssl)
              * send ech_required alert and abort before returning to the app */
             if (ssl->echConfigs != NULL && !ssl->options.disableECH &&
                     !ssl->options.echAccepted) {
-                ssl->echRetryConfigsAccepted = 1;
+                if (ssl->echRetryConfigs != NULL) {
+                    ssl->options.echRetryConfigsAccepted = 1;
+                }
                 SendAlert(ssl, alert_fatal, ech_required);
                 ssl->error = ECH_REQUIRED_E;
                 WOLFSSL_ERROR_VERBOSE(ECH_REQUIRED_E);

tests/api.c

@@ -15069,7 +15069,8 @@ static int test_wolfSSL_Tls13_ECH_retry_configs_auth_fail_ex(int hrr)
     WOLFSSL_CTX* tempCtx = NULL;
     byte badConfigs[256];
     word32 badConfigsLen = sizeof(badConfigs);
-    word32 retryConfigsLen = sizeof(badConfigs); /* non-zero sentinel */
+    word32 retryConfigsLen = sizeof(badConfigs);
+    const char* badPublicName = "ech-public-name.com";
 
     XMEMSET(&test_ctx, 0, sizeof(test_ctx));
     test_ctx.s_cb.method = wolfTLSv1_3_server_method;
@@ -15078,10 +15079,8 @@ static int test_wolfSSL_Tls13_ECH_retry_configs_auth_fail_ex(int hrr)
 
     ExpectIntEQ(test_ssl_memio_setup(&test_ctx), TEST_SUCCESS);
 
-    /* wrong ECH config so server sends retry_configs in EncryptedExtensions;
-     * use a public name that does NOT appear in the server cert so that the
-     * outer handshake cert check fails with DOMAIN_NAME_MISMATCH */
-    const char* badPublicName = "ech-public-name.com";
+    /* generate mismatched ECH configs so retry_configs are sent
+     * and use a bad public name so auth fails in outer hello */
     ExpectNotNull(tempCtx = wolfSSL_CTX_new(wolfTLSv1_3_server_method()));
     ExpectIntEQ(wolfSSL_CTX_GenerateEchConfig(tempCtx, badPublicName,
         0, 0, 0), WOLFSSL_SUCCESS);
@@ -15101,15 +15100,15 @@ static int test_wolfSSL_Tls13_ECH_retry_configs_auth_fail_ex(int hrr)
     wolfSSL_set_verify(test_ctx.s_ssl, WOLFSSL_VERIFY_NONE, NULL);
     wolfSSL_set_verify(test_ctx.c_ssl, WOLFSSL_VERIFY_PEER, NULL);
 
-    /* outer handshake uses badPublicName, which doesn't match the server cert */
+    /* use badPublicName so ECH public name matches */
     ExpectIntEQ(wolfSSL_UseSNI(test_ctx.s_ssl, WOLFSSL_SNI_HOST_NAME,
         badPublicName, (word16)XSTRLEN(badPublicName)),
         WOLFSSL_SUCCESS);
 
     if (hrr)
         ExpectIntEQ(wolfSSL_NoKeyShares(test_ctx.c_ssl), WOLFSSL_SUCCESS);
 
-    /* handshake fails due to auth failure in outer handshake, not ech_required */
+    /* auth failure in outer handshake, not ech_required */
     ExpectIntNE(test_ssl_memio_do_handshake(&test_ctx, 10, NULL), TEST_SUCCESS);
     ExpectIntEQ(wolfSSL_get_error(test_ctx.c_ssl, 0),
         WC_NO_ERR_TRACE(DOMAIN_NAME_MISMATCH));
@@ -15410,14 +15409,8 @@ static int test_wolfSSL_Tls13_ECH_long_SNI(void)
 }
 
 /* Test the HRR ECH rejection fallback path:
- * client offers ECH, HRR is triggered, server sends HRR without ECH extension
- * (confBuf == NULL), client frees hsHashesEch and falls back to the outer
- * transcript, then aborts with ech_required.
- *
- * When disableECH is set on the server the ECH-aware SNI matching in
- * TLSX_SNI_Parse (which normally accepts the ECH public name) is bypassed, so
- * the server SNI must be set explicitly to avoid an unrecognized_name fatal
- * alert before HRR is even sent. */
+ * client offers ECH, HRR is triggered, server sends HRR without ECH extension,
+ * client falls back to the outer transcript, then aborts with ech_required. */
 static int test_wolfSSL_Tls13_ECH_HRR_rejection(void)
 {
     EXPECT_DECLS;
@@ -15428,9 +15421,7 @@ static int test_wolfSSL_Tls13_ECH_HRR_rejection(void)
     test_ctx.s_cb.method = wolfTLSv1_3_server_method;
     test_ctx.c_cb.method = wolfTLSv1_3_client_method;
 
-    /* Server generates ECH config (public name = echCbTestPublicName =
-     * "example.com", which appears in the server cert SAN so cert
-     * verification passes when ECH is rejected and the outer name is used) */
+    /* Server generates ECH config with good public name */
     test_ctx.s_cb.ctx_ready = test_ech_server_ctx_ready;
     /* Client sets the correct ECH config and private SNI */
     test_ctx.c_cb.ssl_ready = test_ech_client_ssl_ready;
@@ -15442,20 +15433,18 @@ static int test_wolfSSL_Tls13_ECH_HRR_rejection(void)
     wolfSSL_set_verify(test_ctx.c_ssl, WOLFSSL_VERIFY_PEER, NULL);
 
     /* Disable ECH on the server SSL object: the server ignores ECH in CH1 and
-     * sends HRR without an ECH extension (confBuf stays NULL on the client).
-     * Because ECH is disabled, the SNI code's ECH-aware public-name fallback
-     * is skipped; set the server SNI to the public name explicitly. */
+     * sends HRR without an ECH extension (confBuf stays NULL on the client) */
     wolfSSL_SetEchEnable(test_ctx.s_ssl, 0);
     ExpectIntEQ(wolfSSL_UseSNI(test_ctx.s_ssl, WOLFSSL_SNI_HOST_NAME,
         echCbTestPublicName, (word16)XSTRLEN(echCbTestPublicName)),
         WOLFSSL_SUCCESS);
 
-    /* Force HRR so the code path at tls13.c:5717-5725 is exercised:
-     * client receives HRR with no ECH extension, detects confBuf == NULL
-     * and frees hsHashesEch, falling back to the outer transcript */
+    /* Force HRR so client receives HRR with no ECH extension,
+     * detects confBuf == NULL and frees hsHashesEch, falling back to the
+     * outer transcript */
     ExpectIntEQ(wolfSSL_NoKeyShares(test_ctx.c_ssl), WOLFSSL_SUCCESS);
 
-    /* Handshake must fail: client aborts with ech_required after Finished */
+    /* Handshake must fail: client aborts with ech_required */
     ExpectIntNE(test_ssl_memio_do_handshake(&test_ctx, 10, NULL), TEST_SUCCESS);
     ExpectIntEQ(test_ctx.c_ssl->options.echAccepted, 0);
     /* hsHashesEch must have been freed by the HRR rejection code path */
@@ -15468,9 +15457,9 @@ static int test_wolfSSL_Tls13_ECH_HRR_rejection(void)
     return EXPECT_RESULT();
 }
 
-/* RFC 9849 §6.1.5: server must abort if CH2 omits the ECH extension after the
- * server accepted ECH in the HRR round. */
-static int test_wolfSSL_Tls13_ECH_HRR_ch2_no_ech(void)
+/* verify the server aborts if CH2 omits the ECH extension after the server
+ * accepted ECH in the HRR round */
+static int test_wolfSSL_Tls13_ECH_ch2_no_ech(void)
 {
     EXPECT_DECLS;
     test_ssl_memio_ctx test_ctx;
@@ -15500,7 +15489,7 @@ static int test_wolfSSL_Tls13_ECH_HRR_ch2_no_ech(void)
     /* disable ECH on the client so CH2 omits the ECH extension entirely */
     wolfSSL_SetEchEnable(test_ctx.c_ssl, 0);
 
-    /* rest of handshake must fail: server enforces RFC 9849 §6.1.5 */
+    /* rest of handshake must fail */
     ExpectIntNE(test_ssl_memio_do_handshake(&test_ctx, 10, NULL), TEST_SUCCESS);
     ExpectIntEQ(wolfSSL_get_error(test_ctx.s_ssl, 0),
         WC_NO_ERR_TRACE(INCOMPLETE_DATA));
@@ -15510,6 +15499,65 @@ static int test_wolfSSL_Tls13_ECH_HRR_ch2_no_ech(void)
     return EXPECT_RESULT();
 }
 
+/* verify that a decryption failure in CH2 is caught
+ * this also verifies that HPKE context is correctly reused */
+static int test_wolfSSL_Tls13_ECH_ch2_decrypt_error(void)
+{
+    EXPECT_DECLS;
+    test_ssl_memio_ctx test_ctx;
+    int i;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+
+    test_ctx.s_cb.method = wolfTLSv1_3_server_method;
+    test_ctx.c_cb.method = wolfTLSv1_3_client_method;
+
+    test_ctx.s_cb.ctx_ready = test_ech_server_ctx_ready;
+    test_ctx.s_cb.ssl_ready = test_ech_server_ssl_ready;
+    test_ctx.c_cb.ssl_ready = test_ech_client_ssl_ready;
+
+    ExpectIntEQ(test_ssl_memio_setup(&test_ctx), TEST_SUCCESS);
+
+    /* Withhold key shares so the server is forced to send HRR */
+    ExpectIntEQ(wolfSSL_NoKeyShares(test_ctx.c_ssl), WOLFSSL_SUCCESS);
+
+    /* One round: client sends CH1, server processes it and sends HRR */
+    (void)test_ssl_memio_do_handshake(&test_ctx, 1, NULL);
+
+    ExpectIntEQ(test_ctx.s_ssl->options.serverState,
+        SERVER_HELLO_RETRY_REQUEST_COMPLETE);
+    ExpectIntEQ(test_ctx.s_ssl->options.echAccepted, 1);
+
+    if (EXPECT_SUCCESS()) {
+        /* Client reads HRR and writes CH2 into s_buff */
+        (void)wolfSSL_connect(test_ctx.c_ssl);
+
+        /* Corrupt one byte of the ECH ciphertext in the CH2 record in s_buff.
+         * ECH outer extension layout after the 0xFE0D type marker:
+         *   extLen(2) + outerType(1) + kdfId(2) + aeadId(2) + configId(1)
+         *   + encLen(2, always 0 in CH2) + payloadLen(2) = 12 bytes, so the
+         * ciphertext starts 14 bytes past the first 0xFE byte. */
+        for (i = 0; i < test_ctx.s_len - 1; i++) {
+            if (test_ctx.s_buff[i] == 0xFE && test_ctx.s_buff[i + 1] == 0x0D) {
+                if (i + 14 < test_ctx.s_len)
+                    test_ctx.s_buff[i + 14] ^= 0xFF;
+                break;
+            }
+        }
+
+        /* Server processes the corrupted CH2.
+         * hpkeContext is preserved, TLSX_ECH_Parse correctly identifies the CH2
+         *   round and sends decrypt_error. */
+        (void)wolfSSL_accept(test_ctx.s_ssl);
+        ExpectIntEQ(wolfSSL_get_error(test_ctx.s_ssl, 0),
+            WC_NO_ERR_TRACE(DECRYPT_ERROR));
+    }
+
+    test_ssl_memio_cleanup(&test_ctx);
+
+    return EXPECT_RESULT();
+}
+
 /* when ECH is rejected the certificate must match the public name of the chosen
  * ech config
  * the cert check should pass and the client aborts with ech_required */
@@ -36998,7 +37046,8 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_wolfSSL_Tls13_ECH_disable_conn),
     TEST_DECL(test_wolfSSL_Tls13_ECH_long_SNI),
     TEST_DECL(test_wolfSSL_Tls13_ECH_HRR_rejection),
-    TEST_DECL(test_wolfSSL_Tls13_ECH_HRR_ch2_no_ech),
+    TEST_DECL(test_wolfSSL_Tls13_ECH_ch2_no_ech),
+    TEST_DECL(test_wolfSSL_Tls13_ECH_ch2_decrypt_error),
     TEST_DECL(test_wolfSSL_Tls13_ECH_rejected_cert_valid),
     TEST_DECL(test_wolfSSL_Tls13_ECH_rejected_empty_client_cert),
 #endif

wolfssl/internal.h

@@ -5163,8 +5163,9 @@ struct Options {
 #endif /* WOLFSSL_DTLS_CID */
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
     word16            echAccepted:1;
-    word16            disableECH:1;           /* Did the user disable ech */
-    word16            echProcessingInner:1;   /* Processing the inner hello */
+    word16            disableECH:1;             /* Did the user disable ech */
+    word16            echProcessingInner:1;     /* Processing the inner hello */
+    word16            echRetryConfigsAccepted:1;
 #endif
 #ifdef WOLFSSL_SEND_HRR_COOKIE
     word16            cookieGood:1;
@@ -6484,7 +6485,6 @@ struct WOLFSSL {
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH)
     WOLFSSL_EchConfig* echConfigs;
     WOLFSSL_EchConfig* echRetryConfigs;
-    byte               echRetryConfigsAccepted:1;
 #endif
 #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) && defined(WOLFSSL_TEST_ECH)
     /* Test-only hook: called on the client before ECH encryption, after the