fix: reject QUIC packets with reserved bits set

[Soundcreates]

Fixes #2526.

Preserve the reserved-bit state after header protection is removed, authenticate the packet, then reject packets whose reserved bits are non-zero. This follows RFC 9000's requirement that the check happen after both protections are removed.

Tests:

• cargo test -p quiche --lib reserved_bits_
• cargo test -p quiche --lib

[xhon-pelushi]

Checked this against RFC 9000 and built it. The change is conformant, and several of the non-obvious choices in it are load-bearing in ways worth recording so a future cleanup doesn't undo them. The one gap is that it ships without a test, and the PR body cites a test that belongs to #2574.

Conformance

RFC 9000 §17.2 (long header) and §17.3.1 (1-RTT) both say the reserved bits "MUST be set to 0" and that an endpoint "MUST treat receipt of a packet that has a non-zero value for these bits, after removing both packet and header protection, as a connection error of type PROTOCOL_VIOLATION."

The patch satisfies all three parts:

• Masks. 0x0c for long headers (§17.2) and 0x18 for short (§17.3.1). Correct.
• Ordering. The bits are captured immediately after packet::decrypt_hdr (lib.rs:3250) and checked after packet::decrypt_pkt (lib.rs:3312) — i.e. after both protections are removed, which is exactly the qualifier the spec attaches to the requirement. Capturing early and checking late is the right way round, since header protection is reapplied nowhere but the value has to survive to the post-authentication point.
• The error really is PROTOCOL_VIOLATION. recv() turns any non-Done error from recv_single into a connection close (lib.rs:2882-2886, self.close(false, e.to_wire(), b"")), and Error::InvalidPacket falls through to_wire's catch-all (error.rs:200) to WireErrorCode::ProtocolViolation. Confirmed rather than assumed:

assert_eq!(Error::InvalidPacket.to_wire(), 0x0a);
assert_eq!(WireErrorCode::ProtocolViolation as u64, 0x0a);
// test result: ok. 1 passed

Three things that look wrong but are right

Flagging these because each one reads like a defect and a future refactor could plausibly "fix" it into a spec violation.

1. The bare return Err(...) instead of drop_pkt_on_err. Every neighbouring fallible call in recv_single wraps its error via .map_err(|e| drop_pkt_on_err(e, ...)), and this new check deliberately does not. That's necessary: drop_pkt_on_err returns Error::Done for everything except a server with recv_count == 0 (lib.rs:9344-9352), and recv() treats Done as "silently ignore this packet". Routing the reserved-bits check through it would downgrade the spec's MUST-be-a-connection-error into a silent drop. Worth a one-line comment saying so, because the inconsistency with the surrounding lines is otherwise an invitation to harmonise it.

2. b.buf()[0] is byte 0 of the packet, not of the remaining buffer. Octets::buf() returns self.buf wholesale (octets/src/lib.rs:387); it's as_ref()/as_mut() that are offset-relative. So this reads the real first byte even though the cursor has moved past the header. Correct, but easy to misread.

3. Retry and Version Negotiation are excluded, and must be. For those the spec says the opposite — §17.2.1's Unused bits and §17.2.5's Unused field MUST be ignored, so rejecting them for non-zero low bits would itself be non-conformant. They can't reach the new check: Version Negotiation returns at lib.rs:2993 and Retry at lib.rs:3084, both before decrypt_hdr at 3250. Worth stating explicitly since the long-header branch would otherwise look over-broad.

I also checked the replay path: buffered undecryptable 0-RTT packets are re-fed through self.recv(...) in process_undecrypted_0rtt_packets (lib.rs:2911), so they get the same check rather than bypassing it.

Test coverage

cargo test -p quiche --lib gives 1089 passed, 0 failed on both master and this branch — identical counts, so nothing was added and no existing test changed behaviour. The PR body's "Tests: cargo test -p quiche packet::tests::fixed_bit_must_be_set" refers to the test added by #2574, not to this change.

To be fair to the author, this is genuinely awkward to test, and it's worth spelling out why rather than just asking for a test: the first byte is AEAD associated data, so an attacker cannot flip a reserved bit without failing decrypt_pkt, which returns before the new check. The only way to reach it is a peer that sets the bits legitimately — so a test needs to make quiche's own send path emit them.

I tried building that end-to-end test and did not land it in the time I gave it. The hook has to go in before the seal in packet::encrypt_pkt so the bit is covered by the AAD, and getting at the right byte there took more care than I expected — my attempt ended up modifying the payload region rather than the header. So: a #[cfg(test)] knob on the send path plus a test_utils::Pipe handshake would give a real end-to-end conformance test that asserts both Err(Error::InvalidPacket) and local_error().error_code == 0x0a, and I think it's worth having given this is a MUST. If that's more than the change warrants, a narrower unit test over the two masks would at least pin the 0x0c/0x18 choice.

What I did not test

cargo test -p quiche --lib only; I did not run the full workspace, the FFI/H3 crates, or any interop suite, so I have not observed an actual CONNECTION_CLOSE(0x0a) frame on the wire — the error-code claim above is from the mapping plus the assertion I ran, not from a captured packet. I also did not check the long-header (0x0c) path against a real Initial or Handshake packet for the same reason the short-header case was hard to construct.

[Soundcreates]

thanks for the deep dive on this, appreciated the RFC walkthrough. pushed a small update, added a comment explaining why the reserved bits check skips drop_pkt_on_err on purpose (so it stays a hard connection error instead of getting quietly downgraded to Done like the calls around it), and pulled the mask computation out into its own function with a couple tests pinning 0x0c and 0x18 to the values you verified. didn't attempt the full send path AEAD test you described, that sounded like a bigger undertaking to get right and I don't want to rush it, but if it's worth doing properly happy to take a pass at it

[Soundcreates]

Thanks for checking this against the RFC so carefully. You’re right about the test reference, that command belongs to #2574, and this PR doesn’t currently have its own regression coverage. I’ll correct the PR description and add a focused test for the reserved-bit failure path before asking for another review.

[Soundcreates]

I corrected the test section in the PR description. The branch already includes focused mask coverage for both long and short headers, and the command now targets the reserved-bits tests instead of #2574. I also kept the full quiche lib test listed separately. I could not run cargo locally here because Rust is not installed in this environment.