Fix: prevent public SNI from being installed on ctx

[sebastian-carpenter]

Description

Reworked the changeSNI logic so it never installs the public SNI onto the shared ctx.

Testing

Added a regression test to check the plaintext SNI is always the public_name. Added matrix to verify ctx SNI swap works.

• test_wolfSSL_Tls13_ECH_wire_sni

Checklist

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

[Copilot]

Pull request overview

This PR adjusts TLS 1.3 ECH SNI-swapping so the public (outer) SNI is never installed onto a shared WOLFSSL_CTX extension list, preventing cross-connection state mutation when the inner SNI was configured at the context level.

Changes:

• Update TLSX_EchChangeSNI() to avoid mutating ssl->ctx->extensions while still ensuring the public SNI is used on the wire for ClientHelloOuter.
• Add a regression test that inspects ClientHello(1/2) bytes to validate the plaintext SNI equals public_name across {accept,reject} × {inner SNI on ssl, ctx}.
• Move/centralize small ClientHello extension-parsing helpers used by multiple ECH memio tests.

Reviewed changes

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

File	Description
src/tls.c	Prevents ECH SNI swap from installing the public SNI on the shared ctx extension list.
tests/api.c	Adds a memio regression test to validate wire SNI behavior for ECH across multiple branches.

---

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

[github-actions]

MemBrowse Memory Report

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
• linuxkm-standard
• stm32-sim-stm32h753

[sebastian-carpenter]

Jenkins retest this please.

[Copilot]

Pull request overview

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10678

Scan targets checked: wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

[dgarske]

Skoll Multi-Scan Review

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

Posted findings

• [Medium] [review] useCtx test does not directly assert the shared ctx SNI is left unmutated — tests/api.c:15101-15157
• [Low] [review+review-security] Redundant inner-SNI copy into serverName buffer in the ctx-only branch (harmless dead work) — src/tls.c:16751-16773
• [Low] [review] TLSX_UseSNI hardcodes &ssl-extensions instead of the extensions local — src/tls.c:16764-16780

Review generated by Skoll

[dgarske]

🟠 [Medium] useCtx test does not directly assert the shared ctx SNI is left unmutated · test

The useCtx matrix arm is intended to prove the fix (public SNI is never installed on the shared ctx). However, the test only inspects the outer ClientHello on the wire. The pre-fix code mutated ssl->ctx->extensions transiently and then restored it inside the same size/write cycle, so for a single sequential connection the outer SNI on the wire was still the public name. That means these wire assertions would also PASS against the old buggy code, so this arm does not actually guard against re-introducing the ctx-mutation regression -- it only confirms the new else-branch produces correct wire output. To make it a true regression test, additionally assert that test_ctx.c_ctx's SERVER_NAME extension still holds echCbTestPrivateName after CH1/CH2 (e.g. via TLSX_Find(c_ctx->extensions, TLSX_SERVER_NAME) or a public SNI getter), confirming the shared list was not swapped to the public name.

Fix: Add a direct assertion that the shared client CTX SNI is unchanged (still the private name) after the ClientHello is produced, so the useCtx arm actually protects against the fixed regression rather than only exercising the new branch. For example, after the CH1 wire check verify TLSX_Find(test_ctx.c_ctx->extensions, TLSX_SERVER_NAME) resolves to echCbTestPrivateName.

[sebastian-carpenter]

Ported over to #10568

[dgarske]

🔵 [Low] Redundant inner-SNI copy into serverName buffer in the ctx-only branch (harmless dead work) · Logic

When the inner SNI is found only on the shared ssl->ctx->extensions, the 'store the inner server name' step still runs (serverNameX is non-NULL at that point), copying the private name into serverName and enforcing hostNameSz > WOLFSSL_HOST_NAME_MAX -> BAD_LENGTH_E. Immediately afterward, the new else branch sets serverNameX = NULL, which causes TLSX_EchRestoreSNI to skip restoring from serverName entirely (the inner SNI legitimately stays on the untouched ctx list), so the copied buffer is unused on the ctx path. This is not a memory-safety defect: the copy is bounded (rejected with BAD_LENGTH_E before the copy) and the destination is a WOLFSSL_HOST_NAME_MAX allocation. One notable side effect flagged by the review mode: a ctx inner SNI at/above WOLFSSL_HOST_NAME_MAX would abort the handshake with BAD_LENGTH_E even though its value is never used on this path (only reachable with a pathologically long >= ~256 byte inner SNI, so not a practical defect). Severity views differ across modes -- review: Low, review-security: Info; the stricter Low is kept. The core change itself is correct: the BEFORE code mutated the shared ctx->extensions (removed the private SNI and installed the public name on it), which the AFTER code correctly avoids by installing the public name only on ssl->extensions and relying on the ssl-before-ctx semaphore dedup in TLSX_GetSize/TLSX_Write so the public name masks the private SNI still on ctx.

Fix: No action strictly required. Optionally, scope the inner-name copy/length-check to the ssl branch where the restore actually needs it (i.e., skip the serverName copy when the SNI originates from ctx), or add a comment noting the copy is intentionally unused on the ctx path. This makes the intent explicit and avoids an unnecessary BAD_LENGTH_E abort and dead work.

[dgarske]

🔵 [Low] TLSX_UseSNI hardcodes &ssl-extensions instead of the extensions local · style

Inside the if (ret == 0) block, the code removes the inner SNI via TLSX_Remove(extensions, ...) (using the extensions local) but then adds the public SNI via TLSX_UseSNI(&ssl->extensions, ...) (hardcoded). In both branches extensions is guaranteed to equal &ssl->extensions at that point (the else-branch sets extensions = &ssl->extensions), so the behavior is correct, but mixing extensions and &ssl->extensions in the same block reads inconsistently and invites confusion if the code is later modified. Using extensions throughout would make the intent (always operate on ssl-level) explicit.

Fix: Use the extensions local in the TLSX_UseSNI call for consistency with the adjacent TLSX_Remove, since both refer to &ssl->extensions.

[sebastian-carpenter]

Will not change. &ssl->extensions is a necessary change since it explicitly states the list to mount onto. Using extensions instead encourages the issue.

[sebastian-carpenter]

This logic is all reworked by #10568, so this PR is no longer necessary. Novel developments have been moved over.