cloudflare / cloudflare/quiche

reserved packet bits are accepted

Open
#2,526 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.