Skip to content

Commit

Permalink
proto: change PartialDecode::new to require a cid parser
Browse files Browse the repository at this point in the history
This commit made `PartialDecode::new` a pub function, since
`PartialDecode` was private, this is not a breaking change.

This commit also expose `FixedLengthConnectionIdParser` to public
for to make the fuzzer be able to compile.
  • Loading branch information
thynson committed May 18, 2024
1 parent 07590af commit ce39cf5
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 33 deletions.
4 changes: 2 additions & 2 deletions fuzz/fuzz_targets/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ extern crate proto;
use libfuzzer_sys::fuzz_target;
use proto::{
fuzzing::{PacketParams, PartialDecode},
DEFAULT_SUPPORTED_VERSIONS,
FixedLengthConnectionIdParser, DEFAULT_SUPPORTED_VERSIONS,
};

fuzz_target!(|data: PacketParams| {
let len = data.buf.len();
let supported_versions = DEFAULT_SUPPORTED_VERSIONS.to_vec();
if let Ok(decoded) = PartialDecode::new(
data.buf,
data.local_cid_len,
&FixedLengthConnectionIdParser::new(data.local_cid_len),
&supported_versions,
data.grease_quic_bit,
) {
Expand Down
4 changes: 2 additions & 2 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::{
range_set::ArrayRangeSet,
shared::{
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
EndpointEvent, EndpointEventInner,
EndpointEvent, EndpointEventInner, FixedLengthConnectionIdParser,
},
token::ResetToken,
transport_parameters::TransportParameters,
Expand Down Expand Up @@ -2120,7 +2120,7 @@ impl Connection {
while let Some(data) = remaining {
match PartialDecode::new(
data,
self.local_cid_state.cid_len(),
&FixedLengthConnectionIdParser::new(self.local_cid_state.cid_len()),
&[self.version],
self.endpoint_config.grease_quic_bit,
) {
Expand Down
4 changes: 2 additions & 2 deletions quinn-proto/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::{
},
shared::{
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
EndpointEvent, EndpointEventInner, IssuedCid,
EndpointEvent, EndpointEventInner, FixedLengthConnectionIdParser, IssuedCid,
},
token::TokenDecodeError,
transport_parameters::{PreferredAddress, TransportParameters},
Expand Down Expand Up @@ -144,7 +144,7 @@ impl Endpoint {
let datagram_len = data.len();
let (first_decode, remaining) = match PartialDecode::new(
data,
self.local_cid_generator.cid_len(),
&FixedLengthConnectionIdParser::new(self.local_cid_generator.cid_len()),
&self.config.supported_versions,
self.config.grease_quic_bit,
) {
Expand Down
1 change: 1 addition & 0 deletions quinn-proto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ pub use crate::endpoint::{
mod shared;
pub use crate::shared::{
ConnectionEvent, ConnectionId, ConnectionIdParser, EcnCodepoint, EndpointEvent,
FixedLengthConnectionIdParser,
};

mod transport_error;
Expand Down
53 changes: 28 additions & 25 deletions quinn-proto/src/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,22 @@ use thiserror::Error;
use crate::{
coding::{self, BufExt, BufMutExt},
crypto,
shared::{ConnectionIdParser, FixedLengthConnectionIdParser},
shared::ConnectionIdParser,
ConnectionId,
};

// Due to packet number encryption, it is impossible to fully decode a header
// (which includes a variable-length packet number) without crypto context.
// The crypto context (represented by the `Crypto` type in Quinn) is usually
// part of the `Connection`, or can be derived from the destination CID for
// Initial packets.
//
// To cope with this, we decode the invariant header (which should be stable
// across QUIC versions), which gives us the destination CID and allows us
// to inspect the version and packet type (which depends on the version).
// This information allows us to fully decode and decrypt the packet.
/// Decodes a QUIC packet's invariant header
///
/// Due to packet number encryption, it is impossible to fully decode a header
/// (which includes a variable-length packet number) without crypto context.
/// The crypto context (represented by the `Crypto` type in Quinn) is usually
/// part of the `Connection`, or can be derived from the destination CID for
/// Initial packets.
///
/// To cope with this, we decode the invariant header (which should be stable
/// across QUIC versions), which gives us the destination CID and allows us
/// to inspect the version and packet type (which depends on the version).
/// This information allows us to fully decode and decrypt the packet.
#[allow(unreachable_pub)] // fuzzing only
#[cfg_attr(test, derive(Clone))]
#[derive(Debug)]
Expand All @@ -30,20 +32,16 @@ pub struct PartialDecode {

#[allow(clippy::len_without_is_empty)]
impl PartialDecode {
#[allow(unreachable_pub)] // fuzzing only
/// Begin decoding a QUIC packet from `bytes`, returning any trailing data not part of that packet
pub fn new(
bytes: BytesMut,
local_cid_len: usize,
cid_parser: &impl ConnectionIdParser,
supported_versions: &[u32],
grease_quic_bit: bool,
) -> Result<(Self, Option<BytesMut>), PacketDecodeError> {
let mut buf = io::Cursor::new(bytes);
let plain_header = PlainHeader::decode(
&mut buf,
&FixedLengthConnectionIdParser::new(local_cid_len),
supported_versions,
grease_quic_bit,
)?;
let plain_header =
PlainHeader::decode(&mut buf, cid_parser, supported_versions, grease_quic_bit)?;
let dgram_len = buf.get_ref().len();
let packet_len = plain_header
.payload_len()
Expand Down Expand Up @@ -548,7 +546,7 @@ impl PlainHeader {
/// Decode a plain header from given buffer, with given [`ConnectionIdParser`].
pub fn decode(
buf: &mut io::Cursor<BytesMut>,
cid_validator: &impl ConnectionIdParser,
cid_parser: &impl ConnectionIdParser,
supported_versions: &[u32],
grease_quic_bit: bool,
) -> Result<Self, PacketDecodeError> {
Expand All @@ -561,7 +559,7 @@ impl PlainHeader {

Ok(Self::Short {
spin,
dst_cid: cid_validator.parse(buf)?,
dst_cid: cid_parser.parse(buf)?,
})
} else {
let version = buf.get::<u32>()?;
Expand Down Expand Up @@ -832,7 +830,7 @@ impl SpaceId {
mod tests {
use super::*;
#[cfg(feature = "rustls")]
use crate::DEFAULT_SUPPORTED_VERSIONS;
use crate::{shared::FixedLengthConnectionIdParser, DEFAULT_SUPPORTED_VERSIONS};
use hex_literal::hex;
use std::io;

Expand Down Expand Up @@ -913,9 +911,14 @@ mod tests {

let server = initial_keys(Version::V1, &dcid, Side::Server, &suite);
let supported_versions = DEFAULT_SUPPORTED_VERSIONS.to_vec();
let decode = PartialDecode::new(buf.as_slice().into(), 0, &supported_versions, false)
.unwrap()
.0;
let decode = PartialDecode::new(
buf.as_slice().into(),
&FixedLengthConnectionIdParser::new(0),
&supported_versions,
false,
)
.unwrap()
.0;
let mut packet = decode.finish(Some(&*server.header.remote)).unwrap();
assert_eq!(
packet.header_data[..],
Expand Down
6 changes: 4 additions & 2 deletions quinn-proto/src/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ use crate::{
ResetToken, MAX_CID_SIZE,
};

pub(crate) struct FixedLengthConnectionIdParser {
/// An [`ConnectionIdParser`] implementation that assumes the connection id is of fixed length
pub struct FixedLengthConnectionIdParser {
expected_len: usize,
}

impl FixedLengthConnectionIdParser {
pub(crate) fn new(expected_len: usize) -> Self {
/// Create a new instance of `FixedLengthConnectionIdParser`
pub fn new(expected_len: usize) -> Self {
Self { expected_len }
}
}
Expand Down

0 comments on commit ce39cf5

Please sign in to comment.