cloudflare / cloudflare/quiche
reserved packet bits are accepted
- Dominant language
- Rust
- Stars
- 11.8k
- Forks
- 1.1k
- Avg merge
- 21h 9m
- Merged PRs (30d)
- 6
Description
After header protection is removed, quiche does not validate QUIC reserved
packet bits. A packet with valid packet protection and non-zero reserved bits
is accepted instead of closing with `PROTOCOL_VIOLATION`.
## Relevant code
`src/packet.rs`
```rust
let first = b.get_u8()?;
hdr.pkt_num_len = (first & 0x03) as usize + 1;
```
```rust
hdr.key_phase = first & 0x04 != 0;
hdr.pkt_num_len = (first & 0x03) as usize + 1;
```
`src/lib.rs`
```rust
let mut payload =
packet::decrypt_pkt(&mut b, pn, hdr.pkt_num_len, payload_len, aead)?;
```
There is no reserved-bit state carried from `decrypt_hdr()` to `recv_single()`.
## RFC 9000
Exact text:
- Section 17.2: "mask of 0x0c"
- Section 17.3.1: "mask of 0x18"
- Sections 17.2 and 17.3.1: "MUST be set to 0"
- Sections 17.2 and 17.3.1: "after removing both packet and header protection"
- Sections 17.2 and 17.3.1: "connection error of type PROTOCOL_VIOLATION"
- Sections 17.2 and 17.3.1: "only removing header protection can expose"
So the value must be read after header protection is removed, and acted on
after packet protection is also removed.
## Reproducer
Paste into `src/tests.rs`:
```rust
fn encode_short_pkt_with_reserved_bits(
conn: &mut Connection, frames: &[frame::Frame], buf: &mut [u8],
) -> Result {
let mut b = octets::OctetsMut::with_slice(buf);
let pkt_type = Type::Short;
let epoch = pkt_type.to_epoch()?;
let crypto_ctx = &mut conn.crypto_ctx[epoch];
let pn = conn.next_pkt_num;
let pn_len = 4;
let send_path = conn.paths.get_active()?;
let active_dcid_seq = send_path
.active_dcid_seq
.as_ref()
.ok_or(Error::InvalidState)?;
let active_scid_seq = send_path
.active_scid_seq
.as_ref()
.ok_or(Error::InvalidState)?;
let hdr = Header {
ty: pkt_type,
version: conn.version,
dcid: ConnectionId::from_ref(
conn.ids.get_dcid(*active_dcid_seq)?.cid.as_ref(),
),
scid: ConnectionId::from_ref(
conn.ids.get_scid(*active_scid_seq)?.cid.as_ref(),
),
pkt_num: pn,
pkt_num_len: pn_len,
token: conn.token.clone(),
versions: None,
key_phase: conn.key_phase,
};
hdr.to_bytes(&mut b)?;
{
let (mut first, _) = b.split_at(1)?;
first.as_mut()[0] |= 0x18;
}
b.put_u32(pn as u32)?;
let payload_offset = b.off();
for frame in frames {
frame.to_bytes(&mut b)?;
}
let payload_len = frames.iter().fold(0, |acc, x| acc + x.wire_len());
let aead = crypto_ctx.crypto_seal.as_mut().ok_or(Error::InvalidState)?;
let written = packet::encrypt_pkt(
&mut b,
pn,
pn_len,
payload_len,
payload_offset,
None,
aead,
)?;
conn.next_pkt_num += 1;
Ok(written)
}
#[test]
fn nonzero_short_reserved_bits_are_protocol_violation() {
let mut buf = [0; 65535];
let mut pipe = test_utils::Pipe::new("cubic").unwrap();
assert_eq!(pipe.handshake(), Ok(()));
let frames = [frame::Frame::Ping { mtu_probe: None }];
let written =
encode_short_pkt_with_reserved_bits(&mut pipe.client, &frames, &mut buf)
.unwrap();
assert_eq!(
test_utils::recv_send(&mut pipe.server, &mut buf, written),
Err(Error::InvalidPacket)
);
assert_eq!(
pipe.server.local_error.unwrap().error_code,
WireErrorCode::ProtocolViolation as u64
);
}
```
Result on `0.29.2`:
```text
left: Ok(39)
right: Err(InvalidPacket)
```
## Fix suggestion
Preserve whether reserved bits were non-zero in `Header` after
`packet::decrypt_hdr()` unprotects byte 0. After `packet::decrypt_pkt()`
authenticates the packet, close with `Error::InvalidPacket` /
`PROTOCOL_VIOLATION` if that state is set.
Contributor guide
Research direction
Start in src/packet.rs at decrypt_hdr() and decrypt_pkt(), then follow recv_single() in src/lib.rs to understand how Header state and authentication errors flow. Add the reserved-bit state described in the issue and use the reproducer in src/tests.rs as the first test. Done means the test returns InvalidPacket and reports PROTOCOL_VIOLATION for non-zero reserved bits.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100