TLS ECH Compliance Fixes

[sebastian-carpenter]

Description

This PR fixes several ECH (Encrypted Client Hello) protocol compliance issues in the TLS implementation, improving correctness of ECH rejection handling, ECHConfig parsing, and inner ClientHello validation per RFC 9849.

ECH rejection handling (client, §6.1.6 / §6.1.7)

• After a successful outer handshake where ECH was offered but rejected, the client now sends the mandatory ech_required fatal alert (121) and aborts with a new ECH_REQUIRED_E error code.
• Certificate verification now uses ECHConfig.contents.public_name instead of the SNI hostname when ECH is rejected.
• Responds to CertificateRequest with an empty Certificate message on ECH rejection.
• Session tickets and PSK resumption are now ignored/rejected during rejected-ECH handshakes.
• ECH processing in ClientHello2 is now gated on the outcome of ClientHello1.
• Added wolfSSL_GetEchRetryConfigs() public API to retrieve server-supplied retry configs after ECH rejection; retry configs are stored separately from active configs and properly freed on session teardown.

ECH rejection through HRR (§6.1.4 / §6.1.5)

• If the HRR contains no ECH extension, the ECH transcript is freed and processing falls back to the outer hello.
• ServerHello accepting ECH after an HRR rejection is now treated as an error.
• HPKE state lifetime is now correctly scoped to the handshake leg that established it.

Inner ClientHello integrity (§6.1)

• Inner ClientHello is now built with downgrade disabled, preventing TLS < 1.3 from being advertised or negotiated.
• Added echProcessingInner flag to correctly validate INNER/OUTER ECH extension types in their respective contexts.
• The server now verifies the inner ClientHello contains an ECH extension of type inner after decryption, and enforces its presence in ClientHello2 when ECH was accepted in ClientHello1.

ECHConfig parsing (§4.2 / §6.2)

• Rewrote SetEchConfigsEx with a single index variable, eliminating fragile dual-pointer bounds tracking.
• Added EchConfigCheckExtensions: configs with unsupported mandatory extensions (high bit set in type) are now correctly skipped.
• Retry configs in EncryptedExtensions are rejected when ECH was already accepted, and discarded on GREASE connections.
• SetEchConfigsEx now returns distinct error codes for malformed input, unsupported algorithms, and unsupported versions, rather than a generic failure. Incompatible retry configs from the server are treated as non-fatal.

ech_outer_extensions handling (§5.1)

• Merged two near-identical copy loops in TLSX_ECH_CopyOuterExtensions into one, with a proper check that ech_outer_extensions never references the ECH extension itself.

Error codes and alerts (§11.2)

• Added ECH_REQUIRED_E = -518 to wolfSSL_ErrorCodes.
• Added ech_required = 121 to AlertDescription.
• Added string representations in AlertTypeToString and wolfSSL_ERR_reason_error_string.

Partly addresses zd#21504

Tests

• test_wolfSSL_Tls13_ECH_rejected_cert_valid: cert authentication against public_name on rejection; expected to pass with a matching CN and fail with a mismatched one (§6.1.7).
• test_wolfSSL_Tls13_ECH_rejected_empty_client_cert: empty certificate sent on rejection during mutual auth (§6.1.7).
• Tests for downgrade prevention in the inner ClientHello via a WOLFSSL_TEST_ECH-only echInnerHelloCb hook (§6.1).
• Tests for mandatory-extension skipping in ECHConfig parsing (§4.2).
• Tests for ECHConfig error handling across malformed, unsupported-algorithm, and unsupported-version inputs.
• HRR+ECH scenario tests, including retry config retrieval via wolfSSL_GetEchRetryConfigs().

Checklist

• added tests
• updated/added doxygen
• updated appropriate READMEs
• Updated manual and documentation

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates wolfSSL’s TLS 1.3 ECH implementation to better match RFC 9849 requirements across ECH rejection handling, HRR behavior, inner ClientHello integrity checks, and ECHConfig parsing.

Changes:

• Adds ech_required alert (121) + ECH_REQUIRED_E error code and wires them into alert/error-string mapping.
• Tightens ECH acceptance/rejection behavior across HRR and outer vs inner ClientHello processing, including session ticket / PSK restrictions on rejected ECH.
• Reworks ECHConfig parsing to correctly skip configs with unsupported mandatory extensions and adds/updates tests for the new compliance behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
wolfssl/ssl.h	Adds ech_required alert value and updates comment pointing to AlertTypeToString location.
wolfssl/internal.h	Adds echProcessingInner state flag and a WOLFSSL_TEST-only hook for tampering inner ClientHello.
wolfssl/error-ssl.h	Adds ECH_REQUIRED_E and updates WOLFSSL_LAST_E.
tests/api.c	Adds new ECH compliance tests and updates GREASE expectations and tamper tests.
src/tls13.c	Enforces no downgrade in inner CH, adds ECH rejection fatal alert behavior, and tightens HRR/PSK/session-ticket handling.
src/tls.c	Refines ECH parsing, retry-config behavior, and ech_outer_extensions validation; refactors outer extension copying.
src/ssl_ech.c	Rewrites SetEchConfigsEx parsing and adds mandatory-extension detection.
src/internal.c	Uses ECH public_name for cert name checks on rejection; adds alert/error string mappings.

Comments suppressed due to low confidence (2)

tests/api.c:1

• This assertion appears to check echConfigs on the client context (c_ctx), but the implementation changes in this PR free/discard configs via ssl->echConfigs (connection-level). If the intent is to ensure the client connection did not retain retry configs on a GREASE connection, this should assert against the appropriate per-connection storage (or use the public getter and expect failure/empty output).
  src/internal.c:1
• Using pointer inequality against ssl->buffers.domainName.buffer to decide whether to run name checks is brittle and non-obvious (it depends on whether domainName was reassigned and on pointer identity rather than intent). Prefer an explicit boolean (e.g., usingEchPublicName) that is set when switching to ECHConfig.public_name, and use that to drive the conditional.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/internal.c:1

• This condition changes behavior by potentially enforcing hostname/SAN matching even when verifyNone is set, as long as domainName was overridden (ECH rejection → public_name). That’s an observable semantic change for callers who intentionally disable verification. To preserve existing API behavior, keep hostname validation gated on !ssl->options.verifyNone, and only swap domainName to public_name within that verification-enabled path (or track an explicit flag instead of relying on pointer inequality).
  tests/api.c:1
• This test now asserts an internal struct field (c_ssl->echConfigs) instead of validating observable behavior via the public API (previously wolfSSL_GetEchConfigs(...)). Since internal storage can change without breaking the public contract, consider restoring an assertion based on wolfSSL_GetEchConfigs (and/or another public-facing signal) to make the test resilient to internal refactors while still verifying 'no configs received' on GREASE.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/internal.c:1

• On ECH rejection, domainName is overwritten with NULL when echX != NULL but ech->echConfig is not set. That disables hostname verification entirely (if (!verifyNone && domainName)), potentially accepting any server certificate. Keep the previously selected ssl->buffers.domainName.buffer as a fallback; only override domainName when ech->echConfig (and its publicName) is present.
  src/internal.c:1
• On ECH rejection, domainName is overwritten with NULL when echX != NULL but ech->echConfig is not set. That disables hostname verification entirely (if (!verifyNone && domainName)), potentially accepting any server certificate. Keep the previously selected ssl->buffers.domainName.buffer as a fallback; only override domainName when ech->echConfig (and its publicName) is present.
  tests/api.c:1
• This test asserts the server sent CertificateRequest and then failed with NO_PEER_CERT (comment says FAIL_IF_NO_PEER_CERT is set), but within this function there is no explicit server-side wolfSSL_set_verify(..., WOLFSSL_VERIFY_PEER | WOLFSSL_VERIFY_FAIL_IF_NO_PEER_CERT, ...) (or equivalent) to ensure a client cert is requested/required. If test_ssl_memio_setup() doesn’t enforce this by default, the handshake may not exercise the intended path and the expected server error becomes unreliable. Set the server verify mode (and any required trust setup) in this test, or adjust the expectations/comment to match the actual configured behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 7 total — 6 posted, 1 skipped

Posted findings

• [Medium] Missing fatal alert for inner hello padding validation failure — src/tls.c:13819
• [Low] Dead null-check ternary for domainName inside non-null guard — src/internal.c:16878
• [Medium] No test for HRR ECH rejection fallback path — src/tls13.c:5717-5722
• [Medium] Retry configs not stored for real ECH clients on rejection — src/tls.c:14275-14289
• [Low] Inconsistent bit-field base type for ECH flags — wolfssl/internal.h:5164-5166
• [Medium] GREASE encrypted_extensions: error from wolfSSL_SetEchConfigs not distinguished — src/tls.c:14280-14289

Skipped findings

• [Low] Removed test_wolfSSL_SubTls13_ECH without full replacement

Review generated by Skoll via openclaw

[sebastian-carpenter]

Should I try and break this down into multiple PR's?

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4

• FLASH: .rodata.wolfSSL_ERR_reason_error_string.str1.1 +35 B (+0.0%, 195,391 B / 262,144 B, total: 75% used)

gcc-arm-cortex-m4-tls12

• FLASH: +17 B (+0.0%, 119,435 B / 262,144 B, total: 46% used)
  No memory changes detected for:
• gcc-arm-cortex-m4-baremetal- gcc-arm-cortex-m4-min-ecc