Skip to content

Fix: prevent public SNI from being installed on ctx #10678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sebastian-carpenter wants to merge 1 commit into from
+208 −59
Closed

Fix: prevent public SNI from being installed on ctx #10678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions src/tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16737,9 +16737,9 @@ static int TLSX_EchChangeSNI(WOLFSSL* ssl, TLSX** pEchX,
}

if (serverNameX == NULL && ssl->ctx && ssl->ctx->extensions) {
/* do not set extensions to ctx->extensions otherwise the shared
* list would be mutated */
serverNameX = TLSX_Find(ssl->ctx->extensions, TLSX_SERVER_NAME);
if (serverNameX != NULL)
extensions = &ssl->ctx->extensions;
}

/* ECH requires an inner SNI to be present for ClientHelloInner.
Expand All @@ -16759,14 +16759,21 @@ static int TLSX_EchChangeSNI(WOLFSSL* ssl, TLSX** pEchX,
XMEMCPY(serverName, hostName, hostNameSz);
}

/* only swap the SNI if one was found; extensions is non-NULL if an
* SNI entry was found on ssl->extensions or ctx->extensions */
if (ret == 0 && extensions != NULL) {
/* remove the inner server name */
TLSX_Remove(extensions, TLSX_SERVER_NAME, ssl->heap);
/* only swap the SNI if one was found, but only install onto
* ssl->extensions */
if (ret == 0) {

@dgarske dgarske Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [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 sebastian-carpenter Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (extensions != NULL) {
/* remove the inner server name */
TLSX_Remove(extensions, TLSX_SERVER_NAME, ssl->heap);
}
else {
/* inner SNI stays on the ctx; nothing to restore on ssl */
serverNameX = NULL;
extensions = &ssl->extensions;
}

@dgarske dgarske Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [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.


/* set the public name as the server name */
if ((ret = TLSX_UseSNI(extensions, WOLFSSL_SNI_HOST_NAME,
if ((ret = TLSX_UseSNI(&ssl->extensions, WOLFSSL_SNI_HOST_NAME,
((WOLFSSL_ECH*)echX->data)->echConfig->publicName,
XSTRLEN(((WOLFSSL_ECH*)echX->data)->echConfig->publicName),
ssl->heap)) == WOLFSSL_SUCCESS)
Expand Down
244 changes: 193 additions & 51 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14061,6 +14061,63 @@ static word16 echCbTestKemID = 0;
static word16 echCbTestKdfID = 0;
static word16 echCbTestAeadID = 0;

/* return the index of the first extension
* extLen will be updated with the length of the extensions field */
static int ech_seek_extensions(byte* buf, word16* extLen)
{
word16 idx;
byte sessionIdLen;
word16 cipherSuitesLen;
byte compressionLen;

idx = OPAQUE16_LEN + RAN_LEN;

sessionIdLen = buf[idx++];
idx += sessionIdLen;

ato16(buf + idx, &cipherSuitesLen);
idx += OPAQUE16_LEN + cipherSuitesLen;

compressionLen = buf[idx++];
idx += compressionLen;

ato16(buf + idx, extLen);
idx += OPAQUE16_LEN;

return idx;
}

/* locate a particular extension:
* idx_p is updated with the location of that extension
* -> idx_p should start just after the handshake header
* 0 returned on success, error otherwise */
static int ech_find_extension(byte* buf, word16* idx_p, word16 extType)
{
word16 idx;
word16 extIdx;
word16 extLen;

extIdx = ech_seek_extensions(buf + *idx_p, &extLen) + *idx_p;
idx = extIdx;

while (idx - extIdx < extLen) {
word16 type;
word16 len;

ato16(buf + idx, &type);
if (type == extType) {
*idx_p = idx;
return 0;
}

idx += OPAQUE16_LEN;
ato16(buf + idx, &len);
idx += OPAQUE16_LEN + len;
}

return BAD_FUNC_ARG;
}

/* the arg is whether the client has ech enabled or not */
static int test_ech_server_sni_callback(WOLFSSL* ssl, int* ad, void* arg)
{
Expand Down Expand Up @@ -14999,6 +15056,141 @@ static int test_wolfSSL_Tls13_ECH_disable_conn(void)
return EXPECT_RESULT();
}

/* The public name must be visible and the private name must not be visible.
* useCtx installs the inner SNI on the shared ctx instead of the per-connection
* ssl, to exercise both branches of TLSX_EchChangeSNI. */
static int test_wolfSSL_Tls13_ECH_wire_sni_ex(int accept, int useCtx)
{
EXPECT_DECLS;
test_ssl_memio_ctx test_ctx;
word16 idx;
word16 nameLen;
word16 publicLen = (word16)XSTRLEN(echCbTestPublicName);

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;

ExpectIntEQ(test_ssl_memio_setup(&test_ctx), TEST_SUCCESS);

/* Accept path uses the correct configs; reject path installs bad configs
* (with the correct public name) so the client still offers ECH. */
if (accept) {
ExpectIntEQ(wolfSSL_SetEchConfigs(test_ctx.c_ssl, echCbTestConfigs,
echCbTestConfigsLen), WOLFSSL_SUCCESS);
}
else {
WOLFSSL_CTX* tempCtx = NULL;
byte badConfig[128];
word32 badConfigLen = sizeof(badConfig);

ExpectNotNull(tempCtx = wolfSSL_CTX_new(wolfTLSv1_3_server_method()));
ExpectIntEQ(wolfSSL_CTX_GenerateEchConfig(tempCtx, echCbTestPublicName,
0, 0, 0), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_CTX_GetEchConfigs(tempCtx, badConfig,
&badConfigLen), WOLFSSL_SUCCESS);
wolfSSL_CTX_free(tempCtx);
ExpectIntEQ(wolfSSL_SetEchConfigs(test_ctx.c_ssl, badConfig,
badConfigLen), WOLFSSL_SUCCESS);
}

/* install the private (inner) SNI on the shared ctx or the per-connection

@dgarske dgarske Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [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 sebastian-carpenter Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported over to #10568

* ssl */
if (useCtx) {
ExpectIntEQ(wolfSSL_CTX_UseSNI(test_ctx.c_ctx, WOLFSSL_SNI_HOST_NAME,
echCbTestPrivateName, (word16)XSTRLEN(echCbTestPrivateName)),
WOLFSSL_SUCCESS);
}
else {
ExpectIntEQ(wolfSSL_UseSNI(test_ctx.c_ssl, WOLFSSL_SNI_HOST_NAME,
echCbTestPrivateName, (word16)XSTRLEN(echCbTestPrivateName)),
WOLFSSL_SUCCESS);
}

/* force HelloRetryRequest */
ExpectIntEQ(wolfSSL_NoKeyShares(test_ctx.c_ssl), WOLFSSL_SUCCESS);

/* On reject, client aborts with ech_required and won't send a cert. */
if (!accept) {
wolfSSL_set_verify(test_ctx.s_ssl, WOLFSSL_VERIFY_NONE, NULL);
wolfSSL_set_verify(test_ctx.c_ssl, WOLFSSL_VERIFY_PEER, NULL);
}

/* client writes CH1 into s_buff */
ExpectIntEQ(wolfSSL_connect(test_ctx.c_ssl), WOLFSSL_FATAL_ERROR);
ExpectIntEQ(wolfSSL_get_error(test_ctx.c_ssl, WOLFSSL_FATAL_ERROR),
WOLFSSL_ERROR_WANT_READ);

/* check sent SNI is correct */
idx = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ;
ExpectIntEQ(ech_find_extension(test_ctx.s_buff, &idx, TLSXT_SERVER_NAME),
0);
ExpectIntEQ(test_ctx.s_buff[idx + 6], WOLFSSL_SNI_HOST_NAME);
ato16(test_ctx.s_buff + idx + 7, &nameLen);
ExpectIntEQ(nameLen, publicLen);
ExpectIntEQ(XMEMCMP(test_ctx.s_buff + idx + 9, echCbTestPublicName,
publicLen), 0);
Comment thread
sebastian-carpenter marked this conversation as resolved.

/* server consumes CH1 and writes HRR into c_buff */
ExpectIntEQ(wolfSSL_accept(test_ctx.s_ssl), WOLFSSL_FATAL_ERROR);
ExpectIntEQ(wolfSSL_get_error(test_ctx.s_ssl, WOLFSSL_FATAL_ERROR),
WOLFSSL_ERROR_WANT_READ);
ExpectIntEQ(test_ctx.s_ssl->options.serverState,
SERVER_HELLO_RETRY_REQUEST_COMPLETE);

/* client reads HRR from c_buff and writes CH2 into s_buff */
ExpectIntEQ(wolfSSL_connect(test_ctx.c_ssl), WOLFSSL_FATAL_ERROR);
ExpectIntEQ(wolfSSL_get_error(test_ctx.c_ssl, WOLFSSL_FATAL_ERROR),
WOLFSSL_ERROR_WANT_READ);

/* check sent SNI is correct */
idx = RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ;
ExpectIntEQ(ech_find_extension(test_ctx.s_buff, &idx, TLSXT_SERVER_NAME),
0);
ExpectIntEQ(test_ctx.s_buff[idx + 6], WOLFSSL_SNI_HOST_NAME);
ato16(test_ctx.s_buff + idx + 7, &nameLen);
ExpectIntEQ(nameLen, publicLen);
ExpectIntEQ(XMEMCMP(test_ctx.s_buff + idx + 9, echCbTestPublicName,
publicLen), 0);
Comment thread
sebastian-carpenter marked this conversation as resolved.

/* sanity check: finish the handshake and verify ECH acceptance */
if (accept) {
ExpectIntEQ(test_ssl_memio_do_handshake(&test_ctx, 10, NULL),
TEST_SUCCESS);
ExpectIntEQ(wolfSSL_GetEchStatus(test_ctx.c_ssl),
WOLFSSL_ECH_STATUS_ACCEPTED);
ExpectIntEQ(wolfSSL_GetEchStatus(test_ctx.s_ssl),
WOLFSSL_ECH_STATUS_ACCEPTED);
}
else {
ExpectIntNE(test_ssl_memio_do_handshake(&test_ctx, 10, NULL),
TEST_SUCCESS);
ExpectIntEQ(wolfSSL_GetEchStatus(test_ctx.c_ssl),
WOLFSSL_ECH_STATUS_REJECTED);
ExpectIntEQ(wolfSSL_GetEchStatus(test_ctx.s_ssl),
WOLFSSL_ECH_STATUS_REJECTED);
}

test_ssl_memio_cleanup(&test_ctx);

return EXPECT_RESULT();
}

static int test_wolfSSL_Tls13_ECH_wire_sni(void)
{
EXPECT_DECLS;
/* {reject, accept} x inner SNI on {ssl, ctx} */
ExpectIntEQ(test_wolfSSL_Tls13_ECH_wire_sni_ex(0, 0), TEST_SUCCESS);
ExpectIntEQ(test_wolfSSL_Tls13_ECH_wire_sni_ex(1, 0), TEST_SUCCESS);
ExpectIntEQ(test_wolfSSL_Tls13_ECH_wire_sni_ex(0, 1), TEST_SUCCESS);
ExpectIntEQ(test_wolfSSL_Tls13_ECH_wire_sni_ex(1, 1), TEST_SUCCESS);
return EXPECT_RESULT();
}

/* Regression test: an inner SNI hostname >= MAX_PUBLIC_NAME_SZ (256) bytes
* must not cause a stack-buffer-overflow in TLSX_EchRestoreSNI. Before the
* fix, the truncated copy omitted the NUL terminator and XSTRLEN read past
Expand Down Expand Up @@ -15045,57 +15237,6 @@ static int test_wolfSSL_Tls13_ECH_long_SNI(void)
return EXPECT_RESULT();
}

static int ech_seek_extensions(byte* buf, word16* innerExtLen)
{
word16 idx;
byte sessionIdLen;
word16 cipherSuitesLen;
byte compressionLen;

idx = OPAQUE16_LEN + RAN_LEN;

sessionIdLen = buf[idx++];
idx += sessionIdLen;

ato16(buf + idx, &cipherSuitesLen);
idx += OPAQUE16_LEN + cipherSuitesLen;

compressionLen = buf[idx++];
idx += compressionLen;

ato16(buf + idx, innerExtLen);
idx += OPAQUE16_LEN;

return idx;
}

static int ech_find_extension(byte* buf, word16* idx_p, word16 extType)
{
word16 idx;
word16 innerExtIdx;
word16 innerExtLen;

innerExtIdx = ech_seek_extensions(buf + *idx_p, &innerExtLen) + *idx_p;
idx = innerExtIdx;

while (idx - innerExtIdx < innerExtLen) {
word16 type;
word16 len;

ato16(buf + idx, &type);
if (type == extType) {
*idx_p = idx;
return 0;
}

idx += OPAQUE16_LEN;
ato16(buf + idx, &len);
idx += OPAQUE16_LEN + len;
}

return BAD_FUNC_ARG;
}

/* Test the HRR ECH rejection fallback path:
* client offers ECH, HRR is triggered, server sends HRR without ECH extension,
* client falls back to the outer transcript, then aborts with ech_required.
Expand Down Expand Up @@ -35242,6 +35383,7 @@ TEST_CASE testCases[] = {
TEST_DECL(test_wolfSSL_Tls13_ECH_trial_decrypt),
TEST_DECL(test_wolfSSL_Tls13_ECH_GREASE),
TEST_DECL(test_wolfSSL_Tls13_ECH_disable_conn),
TEST_DECL(test_wolfSSL_Tls13_ECH_wire_sni),
TEST_DECL(test_wolfSSL_Tls13_ECH_long_SNI),
TEST_DECL(test_wolfSSL_Tls13_ECH_HRR_rejection),
TEST_DECL(test_wolfSSL_Tls13_ECH_ch2_no_ech),
Expand Down
Loading