packet: never emit 1-byte truncated packet numbers#2449
Open
dave wants to merge 1 commit intocloudflare:masterfrom
Open
packet: never emit 1-byte truncated packet numbers#2449dave wants to merge 1 commit intocloudflare:masterfrom
dave wants to merge 1 commit intocloudflare:masterfrom
Conversation
`pkt_num_len` returns the number of bytes used to truncate an outgoing
packet number. The current implementation can return 1 when fewer than
128 packets are unacked. RFC 9000 §17.1 permits this, but it is unsafe:
if the receiver observes more than 128 packets reordered, the entire
valid range of a 1-byte truncation collapses, and the decoded full
packet number can land on the wrong candidate. The AEAD nonce is
derived from the full packet number, so the receiver fails to decrypt
an otherwise-good packet.
The reference Go implementation, quic-go, refuses 1-byte truncation
for the same reason — see PacketNumberLengthForHeader in
quic-go/internal/protocol/packet_number.go ("it never chooses a
PacketNumberLen of 1 byte, since this is too short under certain
circumstances").
This change floors the result at 2 bytes. Cost: 1 byte per outgoing
1-RTT packet during the brief window where num_unacked < 128 (typically
just after an ACK clears a large batch). quic-go has accepted this cost
since at least 2017.
Fixes the AEAD-decryption-failure pattern reproducible at
https://github.com/dave/quiche-bug — that repo includes a Docker-based
server (this code, with optional patch toggle) and a Go HTTP/3 client
that scans qlog for payload_decrypt_error events.
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.
Fixes #2448
What
Floor
pkt_num_lenat 2 bytes. Previously returned 1 whennum_unacked < 128.pub fn pkt_num_len(pn: u64, largest_acked: u64) -> usize { let num_unacked: u64 = pn.saturating_sub(largest_acked) + 1; // computes ceil of num_unacked.log2() + 1 let min_bits = u64::BITS - num_unacked.leading_zeros() + 1; - min_bits.div_ceil(8) as usize + (min_bits.div_ceil(8) as usize).max(2) }(commit also expands the inline comment to explain the floor, and updates the existing
pkt_num_encode_decodetest, which asserted the 1-byte case.)Why
1-byte truncation is unambiguous only with ≤128-packet reorder windows. Any path that produces >128-packet reordering — common on mobile data, behind some VPN egress, anywhere pacing meets aggregator hardware — collapses the decode space, AEAD nonce is wrong, decryption fails on the otherwise-correct ciphertext.
quic-go refuses for the same reason —
PacketNumberLengthForHeader.Reproducer
https://github.com/dave/quiche-bug — same hardware, same client, 90s:
Cost
1 extra byte per 1-RTT packet during the brief window where
num_unacked < 128. Steady-state high-throughput connections see zero impact; post-ACK-burst conditions see at most 1 byte per packet for a small number of packets.quic-go has shipped this floor since 2017 with no field regressions.
Test changes
pkt_num_encode_decodepreviously assertedpkt_num_len(0, 0) == 1with a roundtrip loop branched on the result. Updated: asserts== 2, roundtrip loop simplified to test 2 bytes uniformly.