fix(gateway): refuse an SNI that is not a host name - #1284
Merged
Merged
Conversation
An anonymous ClientHello on port 443 chooses this string, and the gateway does two things with it before anything has authenticated the client: it concatenates it into a DNS query name, and -- when that lookup fails, which is the whole point of sending a name the gateway does not serve -- it writes it to the journal inside an `error!` line that repeats it once per layer of `anyhow` context. Nothing bounded it. The sniff buffer is 4096 bytes, so ~4000 of them could be the name, and the rejection echoed it about three times: roughly 12 KB of attacker-chosen bytes per connection, at whatever rate the client chooses, with no restriction on content. `\n` and terminal escapes go straight through, so the lines between the real ones are the caller's to write too. A host name is at most 253 bytes and is letters, digits, `-`, `.` and (in practice) `_`. Anything else could never have resolved, so refuse it where it is decoded, without naming the value -- quoting it in the rejection would be half of what is being refused. Signed-off-by: Kevin Wang <fremontkevin@icloud.com>
kvinwang
force-pushed
the
fix/unauth-exposure
branch
from
September 24, 2026 08:34
e3c2de3 to
bdf668f
Compare
kvinwang
changed the title
# Conflicts: # dstack/gateway/src/proxy.rs
kvinwang
added a commit
that referenced
this pull request
Sep 25, 2026
…te abort PR #1284 refuses an SNI that is not a DNS name without echoing it, PR #1245 reads a ClientHello past the first 4096 bytes up to one record and requires a handshake record, and PR #1278 fixes a parser abort on a length byte that counts itself. tc-gw-proxy-prot-003 drives the first two through the live proxy; tc-gw-internal-004 loads the parser with #[path], since its inner doc comment broke include!, adds the self-counting and non-handshake rows, and sends the abort probe to the live listener. Signed-off-by: Kevin Wang <wy721@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
take_snipassed the ClientHelloserver_nameon unvalidated. Before any authentication it goes into DNS query names (_dstack-app-address.{sni}and similar) and, when resolution fails, into theerror!log line several times. An anonymous client could send a ~4 KB SNI containing newlines or terminal escapes and write it into the gateway's log on every connection.Fix
Refuse an SNI that is not a valid DNS name (
rustls::pki_types::DnsName, which also enforces the 253-byte limit), without quoting it in the error.Pre-fix:
a 4000-byte SNI was accepted.Verification
cargo test -p dstack-gateway,cargo clippy -p dstack-gateway --all-features -- -D warnings --allow unused_variables,cargo fmt --all -- --check.Merging with #1245
Conflicts only in the
proxy.rstest module.take_snimerges cleanly (the check sits right afterString::from_utf8). Resolution: keep #1245'sclient_hello(sni, filler)andsniff()helpers, drop this PR'sclient_hello/sniff, and rewrite the test with them:Verified on a trial merge: 326 tests pass.