Skip to content

quic: a second ClientHello naming a different host than the first is refused instead of answered - #376

Open
MDA2AV wants to merge 1 commit into
mainfrom
fix/349-quic-second-hello-sni
Open

MDA2AV wants to merge 1 commit into
mainfrom
fix/349-quic-second-hello-sni

Conversation

@MDA2AV

@MDA2AV MDA2AV commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner

Fixes #349.

What was wrong. After a HelloRetryRequest, a QUIC client could send a second ClientHello that named a different host than its first, and the server carried on. picotls runs on_client_hello for the first ClientHello only, so the certificate is chosen from that one. picotls does compare the second ClientHello's server_name with the first and fails the handshake on a mismatch. It does so only when the callback saved the name with ptls_set_server_name, and iq_on_client_hello never did. So the check never ran.

What changed. iq_on_client_hello saves the name when the name selects a host context. A retry that changes the name now fails inside picotls (PTLS_ALERT_HANDSHAKE_FAILURE), and the peer gets a CONNECTION_CLOSE.

Saving the name has one visible side effect: picotls acknowledges it with an empty server_name extension in EncryptedExtensions (RFC 6066 section 3). That is why the name is saved only when it selected a host. TCP acknowledges under the same rule: ServerNameCallback returns SSL_TLSEXT_ERR_OK only for a name it served.

I read picotls at the pinned ref for anything else on the server that reads server_name. None of it applies here:

  • The certificate-verify callback is passed NULL on the server side.
  • This engine issues no session tickets, so the ticket server-name check never runs.
  • No HelloRetryRequest cookie is configured.
  • There is no ECH.

Native build. libioxide_ngtcp2.so was rebuilt with scripts/build-ngtcp2-native.sh at its pinned refs (ngtcp2 7f7e24b, picotls f07f1c8, BoringSSL 5b83e5d), using the script's default WORK.

  • Reproduction. Built the same way from unchanged main, the script reproduces the shipped .so byte for byte (sha256 4ce1b7a8…). With a different WORK path, the only difference is the 193 assert() source paths compiled into the binary. So this .so differs from main's only by the shim change, and it embeds no local path.
  • Result. 2,974,032 bytes, the same as before.
    • The exported symbols are identical in name and size.
    • 842 of the 888 exports moved by exactly +16 bytes: the edited function grew by one 16-byte alignment unit. The 46 exports that come before it in the link did not move.
    • No export was added or changed, so the ABI is unchanged.

Proof. The pending sni/quic: a second ClientHello naming a different host is refused is now a Test.

  • Without the fix (test promoted, main's .so): FAIL ... the second ClientHello asked for beta.test where the first asked for alpha.test, and the certificate was already chosen from the first - the server should have refused the change instead of continuing; it sent a ServerHello.
  • With the fix: it passes.
  • Control: the existing control: a second ClientHello keeping the name continues the handshake passes both ways, so the refusal comes from the name change alone. All 25 sni/quic tests pass, including the validating-client ones that now receive the acknowledgement.

Suites.

  • E2E: 293 passed, 0 failed, 4 pending (main: 292 passed, 5 pending).
  • Http: 88 passed (main: 88).
  • Chaos: 79 passed, 1 pending (main: the same).
  • No new build warnings.

Overlap with #377 (the fix for #200). That PR also rebuilds libioxide_ngtcp2.so. git merge-tree of the two branches auto-merges the shim (the result carries both fixes and compiles), and only the .so conflicts, since it is binary. Whichever PR merges second needs scripts/build-ngtcp2-native.sh rerun on the merged shim, and the resulting .so committed.

All eight together. This batch's PRs (#376 to #383) were merged onto main 71aab91 in one scratch tree. The only conflict is the binary libioxide_ngtcp2.so between #376 and #377, rebuilt there from the merged shim with the script's defaults. Everything builds, and Unit 102, E2E 309, Http 88, Chaos 80, Tls 169 (14 kTLS cases skipped) and File 5 pass, with 0 failed and 0 pending. The repo's only pending test left is the Redis suite's (#336).

Noticed, not changed.

  • If the first ClientHello carries no SNI, or a name no host holds, nothing is saved. A retry can then add or change the name, and the server still serves the default certificate, which is what the first hello got. picotls gives the shim no hook on the second ClientHello.
  • TCP does not refuse a renamed retry either. BoringSSL negotiates everything from the first ClientHello and ignores the second one's server_name (tls13_server.cc, "We perform all our negotiation based on the first ClientHello"), which is what QUIC did until now. Both stacks serve the certificate chosen from the first hello. After this change, QUIC is the stricter of the two.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sni/quic: a second ClientHello naming a different host is not refused

1 participant