Enhancement: Public extension manager for ECH

[sebastian-carpenter]

Description

Reworks ECH public-vs-private extension handling. The old path swapped a single SNI string in place on the live extension list - this was fragile and SNI-only. This PR generalizes it to a public-extension list on the ECH struct that's swapped by extension type, so the outer ClientHello can carry arbitrary public extensions.

ZD#21931

Public extension manager

• Add TLSX* extensions to WOLFSSL_ECH holding the public extensions (e.g. the public-name SNI), populated in TLSX_ECH_Use from echConfig->publicName.
• Replace TLSX_EchChangeSNI/TLSX_EchRestoreSNI with:
  • TLSX_EchShouldHideInner - true when ECH is the outer type.
  • TLSX_EchSwapExtensions - swaps matching extension types between ssl->extensions and ech->extensions; unmatched public exts are prepended; popCount reverses leading nodes to preserve OuterExtensions ordering.
  • TLSX_EchReplaceExtensions - accept: free the public list; reject: swap public exts into ssl->extensions.
• TLSX_GetSizeWithEch/TLSX_WriteWithEch install the public exts before size/write and swap back after.
• ForceZero the hpke; free ech->extensions in TLSX_ECH_Free.

SNI changes

• Remove the old SNI-state machinery: drop the EchStateSNI enum and the sniState/privateName fields, plus the sniState transitions in DoTls13HandShakeMsgType.
• TLSX_SNI_Parse: drop the sniState-based private-name save/restore. On outer-CH parse, match against any echConfig->publicName and write a matched public SNI to ech->extensions, not ssl->extensions; simplify the matched/cacheOnly and response-flag logic.
• Inner SNI is now optional - removed the client-side "inner SNI must be present" requirement enforced by TLSX_EchChangeSNI.
• SendTls13ClientHello: read padding length via TLSX_SNI_GetRequest instead of ech->privateName.
• Call TLSX_EchReplaceExtensions at the accept/reject points (EchCheckAcceptance, DoTls13ServerHello reject fallback, DoTls13ClientHello). This installs the correct SNI for use later in the handshake.

Padding

• If the client ssl had no SNI then it would take the generic no SNI path. Updated padding calculation to fallback to ctx's SNI if it is available.

Testing

• Add test_wolfSSL_Tls13_ECH_wire_sni: drive the handshake manually and assert the public name is present and the private name absent in raw CH1/CH2 bytes, across accept/reject.
• Update test_wolfSSL_Tls13_ECH_no_private_name: no-inner-SNI now succeeds against a permissive server (ACCEPTED); rejected only with ABORT_ON_ABSENCE.
• Add b64MandatoryFirst config case; test_ech_server_sni_callback rejects a wrong public name; add a double-public-SNI scenario to bad_configs_ex.
• Remove obsolete test_wolfSSL_Tls13_ECH_long_SNI (its overflow target - the deleted swap helpers - no longer exist) and relocate the over-long-SNI BAD_LENGTH_E check into test_wolfSSL_UseSNI_params.
• Testing for the new TLSX_EchSwapExtensions function: test_TLSX_EchSwapExtensions. This emulates the swap between ssl->extensions and ech->extensions with various configs: 'match', 'no match', 'match and non-match', 'match and no-match + out of order'. This required qualifying the function with WOLFSSL_TEST_VIS.
• SNI test vectors. Enumerated many possible SNI configurations / possibilities for ECH connections (test_wolfSSL_Tls13_ECH_sni_parse).

Added testing from #10542:

• More efficient *_wire_sni test
  • Updated further to test with SNI installed on CTX and to strictly check for Hostname SNI only.
• Interop testing for when ECH is rejected

Follow-ups

• Lays groundwork for managing multiple public extensions and multiple SNI's in the outer ClientHello.

Checklist

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

[Copilot]

Pull request overview

This PR refactors TLS 1.3 ECH public-vs-private extension handling by introducing a dedicated “public extensions” list on the WOLFSSL_ECH object and swapping extensions by type, replacing the previous SNI-only in-place string swap approach. It also updates SNI/ECH parsing and handshake flow to install the correct extensions on accept/reject paths, and adds/updates tests to validate wire visibility of public vs private SNI.

Changes:

• Add WOLFSSL_ECH::extensions to hold “public” extensions and implement swapping/replacement helpers used during ClientHello size/write and accept/reject handling.
• Rework SNI parsing and ECH handshake transitions to remove the old sniState/privateName machinery and support public-name matching for outer ClientHello.
• Update API tests to cover new behaviors (wire-SNI visibility, no-inner-SNI behavior, new bad config cases) and relocate long-SNI length validation coverage.

Reviewed changes

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

File	Description
wolfssl/internal.h	Removes old SNI-state fields and adds WOLFSSL_ECH::extensions plus TLSX_EchReplaceExtensions() prototype.
src/tls.c	Implements public-extension swapping/replacement, updates SNI parsing, and updates ECH lifecycle management.
src/tls13.c	Adjusts ClientHello padding derivation and installs correct extension sets at ECH accept/reject points (incl. HRR).
tests/api.c	Adds/updates ECH/SNI tests (wire visibility, no-private-name behavior, new config cases) and relocates length checks.

---

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

[github-actions]

retest this please

[github-actions]

MemBrowse Memory Report

linuxkm-standard

• Data: __patchable_function_entries +16 B (+0.0%, 48,176 B)
  No memory changes detected for:
• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m3
• gcc-arm-cortex-m4
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-dtls13
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-openssl-compat
• gcc-arm-cortex-m4-pkcs7
• gcc-arm-cortex-m4-pq
• gcc-arm-cortex-m4-rsa-only
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m4-tls13
• gcc-arm-cortex-m7
• gcc-arm-cortex-m7-pq
• gcc-arm-cortex-m7-tls13
• linuxkm-pie
• stm32-sim-stm32h753

[Copilot]

Pull request overview

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

[sebastian-carpenter]

Jenkins retest this please.

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] Multi-public-extension swap/reversal path is untested — src/tls.c:16682-16734
• [Low] ECH padding length only considers ssl-extensions, not ctx-level inner SNI — src/tls13.c:4844-4855
• [Info] Pointer declaration style inconsistent with repo convention — src/tls.c:2378
• [Info] TLSX_EchShouldHideInner can be a single boolean expression — src/tls.c:16665-16671

Review generated by Skoll

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] TLSX_FreeAll echList tracks only a single ECH extension per list — src/tls.c:15190-15216
• [Low] BAD_STATE_E on restore-swap leaves ssl-extensions inconsistent without repair — src/tls.c:16895-16901, 17111-17117

Review generated by Skoll

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 6 total — 6 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Servers with WOLFSSL_SNI_ABORT_ON_ABSENCE now abort every ECH handshake at the outer ClientHello, even when a valid inner SNI is present — src/tls.c:2520-2557
• [Medium] test_wolfSSL_Tls13_ECH_no_private_name ABORT_ON_ABSENCE case encodes the outer-parse abort; missing test for ABORT_ON_ABSENCE with inner SNI present — tests/api.c:14625-14639
• [Low] BAD_STATE_E restore-failure paths log with WOLFSSL_MSG only, no WOLFSSL_ERROR_VERBOSE — src/tls.c:16900-16907, src/tls.c:17117-17124
• [Low] Inline reimplementation of TLSX_SetResponse in TLSX_SNI_Parse — src/tls.c:2550-2557
• [Low] EchCheckAcceptance calls TLSX_EchReplaceExtensions even when EchCalcAcceptance failed — src/tls13.c:5310-5314
• [Info] PUB_NAME changed to example.com for all interop scenarios, not just the new reject tests — .github/scripts/openssl-ech.sh:97-99

Review generated by Skoll

[dgarske]

See these skoll review items -> #10568 (review)

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

tests/api.c:1

• This helper will underflow storage-- when called with count == 0, causing an out-of-bounds write to storage->next. Even if current call sites always pass count > 0, it’s brittle for future edits. Add an early return when count <= 0.

[dgarske]

This PR has many changes too late in release process. There are a few minor good things that @sebastian-carpenter is going to pull out into a smaller PR to see if it can go into the release.

[Copilot]

Pull request overview

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

[sebastian-carpenter]

Jenkins retest this please.

[dgarske]

Jenkins retest this please "StreamCorruptedException"

[dgarske]

Skoll Multi-Scan Review

Modes: review + review-securityOverall recommendation: COMMENT
Findings: 4 total — 3 posted, 1 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] [review] Test helper ech_seek_extensions dropped its malformed-buffer bounds checks — tests/api.c:14551-14606
• [Low] [review] Dead defensive branch for WOLFSSL_FAILURE from TLSX_UseSNI — src/tls.c:13905-13908
• [Info] [review-security] TLSX_EchSwapExtensions: popCount larger than the ssl list length would relocate the entire list (latent, not currently reachable) — src/tls.c:16770-16780

Skipped findings

• [Low] workingConfig pointer reused as an implicit 'public name matched' flag

Review generated by Skoll

[dgarske]

🔵 [Low] Test helper ech_seek_extensions dropped its malformed-buffer bounds checks

The relocated ech_seek_extensions/ech_find_extension helpers lost the defensive bounds checks that the deleted copies had (if (cipherSuitesLen > MAX_RECORD_SIZE) return BAD_FUNC_ARG;, the *innerExtLen > MAX_RECORD_SIZE guard, and the per-extension len > MAX_RECORD_SIZE || wrap check in the walk loop). These parse raw ClientHello bytes by advancing idx with lengths read straight from the buffer. In the current tests the buffers are self-generated valid ClientHellos so this is safe, but if a future test feeds a truncated/hostile buffer the helper can read out of bounds instead of returning BAD_FUNC_ARG.

Fix: Restore the MAX_RECORD_SIZE sanity checks (and the length/wrap check in ech_find_extension's loop) so the helpers fail closed on malformed input; low priority since inputs are currently trusted test-generated buffers.

[dgarske]

🔵 [Low] Dead defensive branch for WOLFSSL_FAILURE from TLSX_UseSNI

TLSX_UseSNI returns only WOLFSSL_SUCCESS, BAD_FUNC_ARG, BAD_LENGTH_E, or MEMORY_E (see src/tls.c:2653-2698); it never returns WOLFSSL_FAILURE. The else if (ret == WC_NO_ERR_TRACE(WOLFSSL_FAILURE)) ret = BAD_STATE_E; branch is therefore unreachable. It is harmless (the generic if (ret != 0) cleanup handles the real error returns), but it is dead code that can mislead readers into thinking WOLFSSL_FAILURE is a possible result.

Fix: Remove the unreachable WOLFSSL_FAILURE else-if, or convert to if (ret != WOLFSSL_SUCCESS) and rely on the existing ret != 0 cleanup.

[dgarske]

⚪ [Info] TLSX_EchSwapExtensions: popCount larger than the ssl list length would relocate the entire list (latent, not currently… · Logic

The new TLSX_EchSwapExtensions computes the pop point from popCount with while (len > popCount) { sslTail = &(*sslTail)->next; len--; }. If popCount were ever greater than or equal to the number of nodes in *sslExts, the loop never advances, sslTail stays at the head, and chunk = *sslTail captures the ENTIRE ssl->extensions list (then *sslTail = NULL empties it), silently moving all of ssl->extensions onto ech->extensions. Today this cannot be triggered: the only callers (TLSX_EchExposeExtensions via TLSX_GetSizeWithEch/TLSX_WriteWithEch) pass popCount = appended, where appended was produced by the paired conceal call and the list only grows (never shrinks) between conceal and expose, so popCount <= len always holds. This is a defense-in-depth/robustness note on newly introduced code, not an exploitable condition. Because the function is now exported via WOLFSSL_TEST_VIS/WOLFSSL_API_PREFIX_MAP, a future or external caller could violate the invariant.

Fix: Clamp defensively, e.g. only pop when popCount < len (or add if (popCount >= len) popCount = len ? len : 0; / an assert), so an out-of-range popCount cannot relocate the whole extension list. Optional hardening; no functional change for the current callers.

[dgarske]

#10568 (review)