Skip to content

dtls: filter pre-ClientHello queue poison#140

Open
zRedShift wants to merge 1 commit into
algesten:mainfrom
zRedShift:fix/pre-clienthello-queue-poison
Open

dtls: filter pre-ClientHello queue poison#140
zRedShift wants to merge 1 commit into
algesten:mainfrom
zRedShift:fix/pre-clienthello-queue-poison

Conversation

@zRedShift
Copy link
Copy Markdown
Contributor

Before a server has accepted a ClientHello, records that cannot be useful at
that stage could still occupy receive-queue slots. A run of future-epoch or
otherwise non-ClientHello records could fill the queue before a valid
ClientHello arrives.

This filters records in the pre-ClientHello server path before they are queued:
DTLS 1.2 keeps only epoch-0 alerts and current-or-previous ClientHello
handshakes, and DTLS 1.3 keeps only plaintext alerts and current-or-previous
ClientHello handshakes. Filtered records still count against the raw
per-datagram parsed-record cap.

The auto-server fallback path retains only sanitized ClientHello-shaped packets
needed for later fallback, with the retained-packet cap checked before parsing
can mutate the DTLS 1.3 engine.

This keeps the change to the pre-ClientHello queue filter. It does not change
public error taxonomy, queue-full semantics, or broader malformed-tail/replay
policy.

Line delta:

area added removed
non-test code 241 36
tests 204 0
docs/changelog 1 0
total 446 36

Validation:

  • git diff --check
  • cargo fmt --check
  • cargo test --all-targets --features rcgen
  • cargo clippy --all-targets --features rcgen -- -D warnings
  • /home/ronen/.codex/skills/dimpl/scripts/check-snowflake-local.pl upstream/main
  • cargo test --no-default-features --features rust-crypto
  • cargo clippy --no-default-features --features rust-crypto -- -D warnings
  • cargo test --doc --features rcgen

@algesten
Copy link
Copy Markdown
Owner

This is adding yet another filter which looks very similar to classify_records but in a new place.

I'm also not sure what we are trying to solve here.

@zRedShift
Copy link
Copy Markdown
Contributor Author

The bug I was aiming at is pre-ClientHello queue poisoning: records that can’t start the handshake can fill queue_rx before a valid ClientHello arrives

If this seems worth solving to you, I can rework this to reuse/extend the existing classification path instead of adding another similar filter.

@algesten
Copy link
Copy Markdown
Owner

Thinking about this more — the framing isn't really pre-ClientHello specific. The same attack works through the whole plaintext handshake window. As soon as the legit client moves on from CH the server sits in AwaitCertificate/AwaitClientKeyExchange/AwaitCertificateVerify/AwaitChangeCipherSpec/AwaitFinished with peer_encryption_enabled = false, so Record::parse still short-circuits AEAD and insert_incoming_handshake's only seq guard is "drop msg_seq < peer_handshake_seq_no" — no upper bound. Attacker stuffs seq = N+1, N+2, … and they all queue, queue fills, ReceiveQueueFull (now fatal) tears down the connection.

This is exactly the same shape as OpenSSL's CVE-2016-2179 (DTLS buffered message DoS, commits 26f2c5774f / 00a4c14214 from 2016). Their fix had two arms:

  1. Forward bound at insert: cap msg_seq ≤ handshake_read_seq + 10.
  2. Per-message size cap (max_cert_list, ~100KB).

That bounds the orphan memory per connection to ~1.5MB.

I'd suggest we mirror that, in classify_record rather than a new filter path:

  • Drop handshake records where all msg_seq fall outside [peer_handshake_seq_no, peer_handshake_seq_no + 10].
  • Cap individual handshake message length to something like max_cert_list.

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.

2 participants