From 73a814240c5db6fae261a6e4ab567b0b094a35db Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum <55108558+WesleyRosenblum@users.noreply.github.com> Date: Mon, 24 Jul 2023 17:05:22 -0400 Subject: [PATCH] Merge pull request from GHSA-rfhg-rjfp-9q8q * fix(s2n-quic-transport): close connection when no available connection ids * allow all connection migrations when testing * add generator to stateless reset token * fix partial eq impl for connection IDs * add new connection ID to fuzz test * implement hash and fix merge issues * Impl PartialOrd and Ord * PR feedback * clippy --- quic/s2n-quic-core/src/connection/id.rs | 26 +- quic/s2n-quic-core/src/path/migration.rs | 15 + .../src/stateless_reset/token.rs | 4 + quic/s2n-quic-transport/src/endpoint/mod.rs | 4 +- quic/s2n-quic-transport/src/path/manager.rs | 9 +- .../src/path/manager/fuzz_target.rs | 280 ++++++------------ .../src/path/manager/tests.rs | 20 +- .../src/recovery/manager/tests.rs | 2 +- 8 files changed, 150 insertions(+), 210 deletions(-) diff --git a/quic/s2n-quic-core/src/connection/id.rs b/quic/s2n-quic-core/src/connection/id.rs index b5bab0f953..3424668b5a 100644 --- a/quic/s2n-quic-core/src/connection/id.rs +++ b/quic/s2n-quic-core/src/connection/id.rs @@ -35,7 +35,7 @@ pub const MAX_LIFETIME: Duration = Duration::from_secs(24 * 60 * 60); // one day macro_rules! id { ($type:ident, $min_len:expr) => { /// Uniquely identifies a QUIC connection between 2 peers - #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] + #[derive(Copy, Clone, Eq)] #[cfg_attr(any(feature = "generator", test), derive(TypeGenerator))] pub struct $type { bytes: [u8; MAX_LEN], @@ -49,6 +49,30 @@ macro_rules! id { } } + impl PartialEq for $type { + fn eq(&self, other: &Self) -> bool { + self.as_bytes() == other.as_bytes() + } + } + + impl core::hash::Hash for $type { + fn hash(&self, state: &mut H) { + self.as_bytes().hash(state); + } + } + + impl PartialOrd for $type { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + + impl Ord for $type { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + self.as_bytes().cmp(&other.as_bytes()) + } + } + impl $type { /// The minimum length for this connection ID type pub const MIN_LEN: usize = $min_len; diff --git a/quic/s2n-quic-core/src/path/migration.rs b/quic/s2n-quic-core/src/path/migration.rs index 75d8f9b5da..ab28d16e2a 100644 --- a/quic/s2n-quic-core/src/path/migration.rs +++ b/quic/s2n-quic-core/src/path/migration.rs @@ -219,3 +219,18 @@ pub mod disabled { } } } + +#[cfg(any(test, feature = "testing"))] +pub mod allow_all { + use super::*; + + #[derive(Debug, Default)] + pub struct Validator; + + impl super::Validator for Validator { + fn on_migration_attempt(&mut self, _attempt: &Attempt) -> Outcome { + // allow all migration attempts + Outcome::Allow + } + } +} diff --git a/quic/s2n-quic-core/src/stateless_reset/token.rs b/quic/s2n-quic-core/src/stateless_reset/token.rs index 6e56b7a7d7..4683306c31 100644 --- a/quic/s2n-quic-core/src/stateless_reset/token.rs +++ b/quic/s2n-quic-core/src/stateless_reset/token.rs @@ -20,6 +20,10 @@ pub const LEN: usize = 128 / 8; // Hash can still be derived. #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Copy, Clone, Debug, Eq, Hash)] +#[cfg_attr( + any(test, feature = "generator"), + derive(bolero_generator::TypeGenerator) +)] pub struct Token([u8; LEN]); impl Token { diff --git a/quic/s2n-quic-transport/src/endpoint/mod.rs b/quic/s2n-quic-transport/src/endpoint/mod.rs index dde6060b7d..9e6daf31f7 100644 --- a/quic/s2n-quic-transport/src/endpoint/mod.rs +++ b/quic/s2n-quic-transport/src/endpoint/mod.rs @@ -1172,7 +1172,7 @@ pub mod testing { type StreamManager = crate::stream::DefaultStreamManager; type ConnectionCloseFormatter = s2n_quic_core::connection::close::Development; type EventSubscriber = Subscriber; - type PathMigrationValidator = path::migration::default::Validator; + type PathMigrationValidator = path::migration::allow_all::Validator; type PacketInterceptor = s2n_quic_core::packet::interceptor::Disabled; type DatagramEndpoint = s2n_quic_core::datagram::Disabled; @@ -1202,7 +1202,7 @@ pub mod testing { type StreamManager = crate::stream::DefaultStreamManager; type ConnectionCloseFormatter = s2n_quic_core::connection::close::Development; type EventSubscriber = Subscriber; - type PathMigrationValidator = path::migration::default::Validator; + type PathMigrationValidator = path::migration::allow_all::Validator; type PacketInterceptor = s2n_quic_core::packet::interceptor::Disabled; type DatagramEndpoint = s2n_quic_core::datagram::Disabled; diff --git a/quic/s2n-quic-transport/src/path/manager.rs b/quic/s2n-quic-transport/src/path/manager.rs index 7e307f83a2..c1a6902b59 100644 --- a/quic/s2n-quic-transport/src/path/manager.rs +++ b/quic/s2n-quic-transport/src/path/manager.rs @@ -705,11 +705,10 @@ impl Manager { active_path_connection_id, publisher, ) - .expect( - "since we are only checking the active path and new id was delivered \ - via the new_connection_id frames, there will always be a new id available \ - to consume if necessary", - ); + .ok_or(transport::Error::PROTOCOL_VIOLATION.with_reason( + "A NEW_CONNECTION_ID frame was received that retired the active path's \ + connection ID and no unused connection IDs remain to replace it", + ))? } Ok(()) diff --git a/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs b/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs index 03a97af092..4393892404 100644 --- a/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs +++ b/quic/s2n-quic-transport/src/path/manager/fuzz_target.rs @@ -5,7 +5,6 @@ use super::{tests::helper_path, *}; use crate::{ connection::{ConnectionIdMapper, InternalConnectionIdGenerator}, endpoint::testing::Server as Config, - path, }; use bolero::{check, generator::*}; use core::time::Duration; @@ -17,118 +16,12 @@ use s2n_quic_core::{ random, random::testing::Generator, time::{testing::Clock, Clock as _}, + transport, }; -use std::collections::HashMap; type Manager = super::Manager; type Handle = path::RemoteAddress; -/// Path information stored for modeling the path::Manager -#[derive(Debug)] -struct PathInfo { - state: path::State, - handle: Handle, -} - -impl PathInfo { - pub fn new(path: Path) -> PathInfo { - PathInfo { - handle: path.handle, - state: path.state, - } - } - - fn remote_address(&self) -> Handle { - self.handle - } - - fn is_validated(&self) -> bool { - self.state == path::State::Validated - } -} - -#[derive(Debug)] -struct Oracle { - paths: HashMap, - active: Handle, - last_known_active_validated_path: Option, - prev_state_amplification_limited: bool, -} - -impl Oracle { - fn new(handle: Handle, path: PathInfo) -> Oracle { - let mut paths = HashMap::new(); - paths.insert(handle, path); - - Oracle { - paths, - active: handle, - last_known_active_validated_path: None, - prev_state_amplification_limited: false, - } - } - - fn get_active_path(&self) -> &PathInfo { - self.paths.get(&self.active).unwrap() - } - - fn path(&mut self, handle: &Handle) -> Option<(&Handle, &mut PathInfo)> { - self.paths - .iter_mut() - .find(|(path_handle, _path)| path_handle.eq(&handle)) - } - - /// Insert a new path while adhering to limits - fn insert_new_path(&mut self, new_handle: Handle, new_path: PathInfo) { - // We only store max of 4 paths because of limits imposed by Active cid limits - if self.paths.len() < 4 { - self.paths.insert(new_handle, new_path); - } - } - - /// Insert new path if the remote address doesnt match any existing path - fn on_datagram_received( - &mut self, - handle: &Handle, - payload_len: u16, - still_amplification_limited: bool, - ) { - match self.path(handle) { - Some(_path) => (), - None => { - let new_path = PathInfo { - handle: *handle, - state: path::State::Validated, - }; - - self.insert_new_path(*handle, new_path); - } - } - - // Verify receiving bytes unblocks amplification limited. - // - // Amplification is calculated in terms of packets rather than bytes. Therefore any - // payload len that is > 0 will unblock amplification. - if self.prev_state_amplification_limited && payload_len > 0 { - assert!(!still_amplification_limited); - } - } - - fn on_timeout(&self, _millis: &u16) { - // TODO implement - // call timeout on each Path - // - // if active path is not validated and validation failed - // set the last_known_active_validated_path as active path - } - - fn on_processed_packet(&mut self, handle: &Handle, probe: &path_validation::Probe) { - if !probe.is_probing() { - self.active = *handle; - } - } -} - #[derive(Debug, TypeGenerator)] enum Operation { // on_datagram_received @@ -165,6 +58,13 @@ enum Operation { bytes: u16, }, + OnNewConnectionId { + sequence_number: u32, + retire_prior_to: u32, + id: connection::PeerId, + stateless_reset_token: stateless_reset::Token, + }, + // on_timeout IncrementTime { /// The milli-second value by which to increase the timestamp @@ -174,7 +74,6 @@ enum Operation { #[derive(Debug)] struct Model { - oracle: Oracle, subject: Manager, /// A monotonically increasing timestamp @@ -184,44 +83,28 @@ struct Model { impl Model { pub fn new() -> Model { let zero_conn_id = connection::PeerId::try_from_bytes(&[0]).unwrap(); - let new_addr = Handle::default(); - let oracle = { - let zero_path = PathInfo::new(helper_path(zero_conn_id)); - Oracle::new(new_addr, zero_path) - }; let manager = { let zero_path = helper_path(zero_conn_id); let mut random_generator = random::testing::Generator(123); - let mut peer_id_registry = + let peer_id_registry = ConnectionIdMapper::new(&mut random_generator, endpoint::Type::Server) .create_server_peer_id_registry( InternalConnectionIdGenerator::new().generate_id(), zero_path.peer_connection_id, ); - // register 3 more ids which means a total of 4 paths. cid 0 is retired as part of - // on_new_connection_id call - for i in 1..=3 { - let cid = connection::PeerId::try_from_bytes(&[i]).unwrap(); - let token = &[i; 16].into(); - peer_id_registry - .on_new_connection_id(&cid, i.into(), 0, token) - .unwrap(); - } - Manager::new(zero_path, peer_id_registry) }; let clock = Clock::default(); Model { - oracle, subject: manager, timestamp: clock.get_time(), } } - pub fn apply(&mut self, operation: &Operation) { + pub fn apply(&mut self, operation: &Operation) -> Result<(), transport::Error> { match operation { Operation::IncrementTime { millis } => self.on_timeout(millis), Operation::OnDatagramReceived { @@ -234,15 +117,24 @@ impl Model { path_id_generator, bytes, } => self.on_bytes_transmitted(*path_id_generator, *bytes), + Operation::OnNewConnectionId { + id, + sequence_number, + retire_prior_to, + stateless_reset_token, + } => self.on_new_connection_id( + id, + *sequence_number, + *retire_prior_to, + *stateless_reset_token, + ), } } - fn on_timeout(&mut self, millis: &u16) { + fn on_timeout(&mut self, millis: &u16) -> Result<(), transport::Error> { // timestamp should be monotonically increasing self.timestamp += Duration::from_millis(*millis as u64); - self.oracle.on_timeout(millis); - self.subject .on_timeout( self.timestamp, @@ -250,6 +142,8 @@ impl Model { &mut Publisher::no_snapshot(), ) .unwrap(); + + Ok(()) } fn on_datagram_received( @@ -258,7 +152,7 @@ impl Model { probing: &Option, local_id: LocalId, payload_len: u16, - ) { + ) -> Result<(), transport::Error> { // Handle is an alias to RemoteAddress so does not inherit the PathHandle // eq implementation which unmaps an ipv6 address into a ipv4 address let handle = path::RemoteAddress(handle.unmap()); @@ -269,15 +163,10 @@ impl Model { destination_connection_id: local_id, source_connection_id: None, }; - let mut migration_validator = path::migration::default::Validator; + let mut migration_validator = path::migration::allow_all::Validator; let mut random_generator = Generator::default(); let mut publisher = Publisher::no_snapshot(); - self.oracle.prev_state_amplification_limited = self - .subject - .path(&handle) - .map_or(false, |(_id, path)| path.at_amplification_limit()); - match self.subject.on_datagram_received( &handle, &datagram, @@ -287,27 +176,16 @@ impl Model { MaxMtu::default(), &mut publisher, ) { - Ok((id, _)) => { - // Only call oracle if the subject can process on_datagram_received without errors - self.oracle.on_datagram_received( - &handle, - payload_len, - self.subject[id].at_amplification_limit(), - ); - + Ok(_) => { if let Some(probe) = probing { - self.oracle.on_processed_packet(&handle, probe); - let (path_id, _path) = self.subject.path(&handle).unwrap(); - self.subject - .on_processed_packet( - path_id, - None, - *probe, - &mut random_generator, - &mut publisher, - ) - .unwrap(); + self.subject.on_processed_packet( + path_id, + None, + *probe, + &mut random_generator, + &mut publisher, + )? } } Err(datagram_drop_reason) => { @@ -315,55 +193,68 @@ impl Model { // Ignore errors emitted by the migration::validator and peer_id_registry DatagramDropReason::InsufficientConnectionIds => {} DatagramDropReason::RejectedConnectionMigration => {} + DatagramDropReason::PathLimitExceeded => {} datagram_drop_reason => panic!("{:?}", datagram_drop_reason), }; } } + + Ok(()) } - fn on_bytes_transmitted(&mut self, path_id_generator: u8, bytes: u16) { + fn on_new_connection_id( + &mut self, + connection_id: &connection::PeerId, + sequence_number: u32, + retire_prior_to: u32, + stateless_reset_token: stateless_reset::Token, + ) -> Result<(), transport::Error> { + // These validations are performed when decoding the `NEW_CONNECTION_ID` frame + // see s2n-quic-core/src/frame/new_connection_id.rs + if retire_prior_to > sequence_number { + return Ok(()); + } + + if !((1..=20).contains(&connection_id.len())) { + return Ok(()); + } + + if sequence_number == 0 { + return Ok(()); + } + + let mut publisher = Publisher::no_snapshot(); + + self.subject.on_new_connection_id( + connection_id, + sequence_number, + retire_prior_to, + &stateless_reset_token, + &mut publisher, + ) + } + + fn on_bytes_transmitted( + &mut self, + path_id_generator: u8, + bytes: u16, + ) -> Result<(), transport::Error> { let id = path_id_generator as usize % self.subject.paths.len(); let path = &mut self.subject[path_id(id as u8)]; if !path.at_amplification_limit() { path.on_bytes_transmitted(bytes as usize); } + + Ok(()) } /// Check that the subject and oracle match. pub fn invariants(&self) { - // compare total paths - assert_eq!(self.oracle.paths.len(), self.subject.paths.len()); - - // compare active path - assert_eq!( - self.oracle.get_active_path().remote_address(), - self.subject.active_path().remote_address() - ); - - // compare last known valid path - assert_eq!( - self.oracle.last_known_active_validated_path, - self.subject.last_known_active_validated_path - ); - - // compare path properties - for (path_id, s_path) in self.subject.paths.iter().enumerate() { - let o_path = self.oracle.paths.get(&s_path.remote_address()).unwrap(); - - assert_eq!( - o_path.remote_address(), - s_path.remote_address(), - "path_id: {}", - path_id - ); - assert_eq!( - o_path.is_validated(), - s_path.is_validated(), - "path_id: {}", - path_id - ); - } + let peer_id = self.subject.active_path().peer_connection_id; + + // The active path should be using an active connection ID + assert!(self.subject.peer_id_registry.is_active(&peer_id)); } } @@ -373,10 +264,17 @@ fn cm_model_test() { .with_type::>() .for_each(|operations| { let mut model = Model::new(); + let mut error = false; + for operation in operations.iter() { - model.apply(operation); + if model.apply(operation).is_err() { + error = true; + break; + } } - model.invariants(); + if !error { + model.invariants(); + } }) } diff --git a/quic/s2n-quic-transport/src/path/manager/tests.rs b/quic/s2n-quic-transport/src/path/manager/tests.rs index eafa57cefe..bcf92b2f64 100644 --- a/quic/s2n-quic-transport/src/path/manager/tests.rs +++ b/quic/s2n-quic-transport/src/path/manager/tests.rs @@ -718,7 +718,7 @@ fn test_adding_new_path() { &datagram, true, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -777,7 +777,7 @@ fn do_not_add_new_path_if_handshake_not_confirmed() { &datagram, handshake_confirmed, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ); @@ -837,7 +837,7 @@ fn do_not_add_new_path_if_client() { &datagram, true, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ); @@ -926,7 +926,7 @@ fn limit_number_of_connection_migrations() { &new_addr, &datagram, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ); @@ -983,7 +983,7 @@ fn connection_migration_challenge_behavior() { &new_addr, &datagram, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -1077,7 +1077,7 @@ fn connection_migration_use_max_ack_delay_from_active_path() { &new_addr, &datagram, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -1154,7 +1154,7 @@ fn connection_migration_new_path_abandon_timer() { &new_addr, &datagram, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -1426,7 +1426,7 @@ fn temporary_until_authenticated() { &datagram, true, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -1448,7 +1448,7 @@ fn temporary_until_authenticated() { &datagram, true, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) @@ -1485,7 +1485,7 @@ fn temporary_until_authenticated() { &datagram, true, &mut Default::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, &mut publisher, ) diff --git a/quic/s2n-quic-transport/src/recovery/manager/tests.rs b/quic/s2n-quic-transport/src/recovery/manager/tests.rs index a7b9734a83..b357e81e6f 100644 --- a/quic/s2n-quic-transport/src/recovery/manager/tests.rs +++ b/quic/s2n-quic-transport/src/recovery/manager/tests.rs @@ -3152,7 +3152,7 @@ fn helper_generate_multi_path_manager( &datagram, true, &mut Endpoint::default(), - &mut migration::default::Validator, + &mut migration::allow_all::Validator, DEFAULT_MAX_MTU, publisher, )