From da6390a31338d79b12a056fd5c0b1890d4f8aea7 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Mon, 31 Jul 2023 18:23:09 -0700 Subject: [PATCH 1/5] feat(s2n-quic-transport): discard initial sooner --- .../src/connection/transmission.rs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/quic/s2n-quic-transport/src/connection/transmission.rs b/quic/s2n-quic-transport/src/connection/transmission.rs index 00787dfcff..810a881aa5 100644 --- a/quic/s2n-quic-transport/src/connection/transmission.rs +++ b/quic/s2n-quic-transport/src/connection/transmission.rs @@ -120,6 +120,14 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< "the amplification limit should be checked before trying to transmit" ); + if Config::ENDPOINT_TYPE.is_client() && space_manager.handshake().is_some() { + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# a client MUST discard Initial keys when it first sends a + //# Handshake packet + let path = &mut self.context.path_manager[self.context.path_id]; + space_manager.discard_initial(path, self.context.path_id, self.context.publisher); + } + // limit the number of retries to the MAX_BURST_PACKETS for _ in 0..MAX_BURST_PACKETS { let encoder = EncoderBuffer::new(&mut buffer[..mtu]); @@ -274,19 +282,6 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< encoder, ) { Ok((outcome, encoder)) => { - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# a client MUST discard Initial keys when it first sends a - //# Handshake packet - - if Config::ENDPOINT_TYPE.is_client() { - let path = &mut self.context.path_manager[self.context.path_id]; - space_manager.discard_initial( - path, - self.context.path_id, - self.context.publisher, - ); - } - *self.context.outcome += outcome; encoder } From 696d8bae58a05d75fee608f5c59dfed3ef77dfb4 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Tue, 1 Aug 2023 17:15:48 -0700 Subject: [PATCH 2/5] Discard initial more aggressively and ensure PTO is armed --- .../src/connection/connection_impl.rs | 24 ++++ .../src/connection/transmission.rs | 14 +- .../s2n-quic-transport/src/space/handshake.rs | 14 +- quic/s2n-quic-transport/src/space/mod.rs | 135 +++++++++++------- 4 files changed, 129 insertions(+), 58 deletions(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 87ac407017..f35ef2f99b 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -785,6 +785,7 @@ impl connection::Trait for ConnectionImpl { error, self.path_manager.active_path_mut(), active_path_id, + timestamp, &mut publisher, ); } @@ -1218,6 +1219,28 @@ impl connection::Trait for ConnectionImpl { packet_interceptor, datagram_endpoint, )?; + + if Config::ENDPOINT_TYPE.is_client() && self.space_manager.handshake().is_some() { + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# Packets protected with Initial secrets (Section 5.2) are not + //# authenticated, meaning that an attacker could spoof packets with the + //# intent to disrupt a connection. To limit these attacks, Initial + //# packet protection keys are discarded more aggressively than other + //# keys. + + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# a client MUST discard Initial keys when it first sends a + //# Handshake packet + + // We aggressively discard initial keys as soon as we have installed + // the handshake keys to prevent the attacks mentioned above. + self.space_manager.discard_initial( + &mut self.path_manager[path_id], + path_id, + datagram.timestamp, + &mut self.event_context.publisher(datagram.timestamp, subscriber), + ); + } } Ok(()) @@ -1339,6 +1362,7 @@ impl connection::Trait for ConnectionImpl { self.space_manager.discard_initial( self.path_manager.active_path_mut(), path_id, + datagram.timestamp, &mut publisher, ); } diff --git a/quic/s2n-quic-transport/src/connection/transmission.rs b/quic/s2n-quic-transport/src/connection/transmission.rs index 810a881aa5..44d2d00ddd 100644 --- a/quic/s2n-quic-transport/src/connection/transmission.rs +++ b/quic/s2n-quic-transport/src/connection/transmission.rs @@ -120,14 +120,6 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< "the amplification limit should be checked before trying to transmit" ); - if Config::ENDPOINT_TYPE.is_client() && space_manager.handshake().is_some() { - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# a client MUST discard Initial keys when it first sends a - //# Handshake packet - let path = &mut self.context.path_manager[self.context.path_id]; - space_manager.discard_initial(path, self.context.path_id, self.context.publisher); - } - // limit the number of retries to the MAX_BURST_PACKETS for _ in 0..MAX_BURST_PACKETS { let encoder = EncoderBuffer::new(&mut buffer[..mtu]); @@ -282,6 +274,12 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< encoder, ) { Ok((outcome, encoder)) => { + if Config::ENDPOINT_TYPE.is_client() { + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# a client MUST discard Initial keys when it first sends a + //# Handshake packet + debug_assert!(space_manager.initial().is_none()); + } *self.context.outcome += outcome; encoder } diff --git a/quic/s2n-quic-transport/src/space/handshake.rs b/quic/s2n-quic-transport/src/space/handshake.rs index 6919e3e690..51b8fb968f 100644 --- a/quic/s2n-quic-transport/src/space/handshake.rs +++ b/quic/s2n-quic-transport/src/space/handshake.rs @@ -274,8 +274,7 @@ impl HandshakeSpace { //# datagram unblocks it, even if none of the packets in the datagram are //# successfully processed. In such a case, the PTO timer will need to //# be re-armed. - self.recovery_manager - .update_pto_timer(path, timestamp, is_handshake_confirmed); + self.update_pto_timer(path, timestamp, is_handshake_confirmed); } /// Called when the connection timer expired @@ -309,6 +308,17 @@ impl HandshakeSpace { .on_packet_number_space_discarded(path, path_id, publisher); } + /// Updates the probe timeout timer + pub fn update_pto_timer( + &mut self, + path: &Path, + now: Timestamp, + is_handshake_confirmed: bool, + ) { + self.recovery_manager + .update_pto_timer(path, now, is_handshake_confirmed); + } + pub fn requires_probe(&self) -> bool { self.recovery_manager.requires_probe() } diff --git a/quic/s2n-quic-transport/src/space/mod.rs b/quic/s2n-quic-transport/src/space/mod.rs index 956ffab9b2..d522927962 100644 --- a/quic/s2n-quic-transport/src/space/mod.rs +++ b/quic/s2n-quic-transport/src/space/mod.rs @@ -89,52 +89,16 @@ impl fmt::Debug for PacketSpaceManager { } macro_rules! packet_space_api { - ($ty:ty, $field:ident, $get_mut:ident $(, $discard:ident $(, $assert_discard:ident)? )?) => { + ($ty:ty, $field:ident, $get_mut:ident) => { #[allow(dead_code)] pub fn $field(&self) -> Option<&$ty> { - self.$field - .as_ref() - .map(Box::as_ref) + self.$field.as_ref().map(Box::as_ref) } pub fn $get_mut(&mut self) -> Option<(&mut $ty, &mut HandshakeStatus)> { - let space = self.$field - .as_mut() - .map(Box::as_mut)?; + let space = self.$field.as_mut().map(Box::as_mut)?; Some((space, &mut self.handshake_status)) } - - $( - pub fn $discard( - &mut self, - path: &mut Path, - path_id: path::Id, - publisher: &mut Pub, - ) { - //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2 - //# When Initial or Handshake keys are discarded, the PTO and loss - //# detection timers MUST be reset, because discarding keys indicates - //# forward progress and the loss detection timer might have been set for - //# a now discarded packet number space. - if let Some(mut space) = self.$field.take() { - // reset the PTO backoff value as part of resetting the PTO timer. - path.reset_pto_backoff(); - - space.on_discard(path, path_id, publisher); - } - - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# Endpoints MUST NOT send - //# Initial packets after this point. - // By discarding a space, we are no longer capable of sending packets with those - // keys. - - debug_assert!(self.$field.is_none(), "space should have been discarded"); - $( - debug_assert!(self.$assert_discard.is_none(), "space should have been discarded"); - )? - } - )? }; } @@ -174,15 +138,9 @@ impl PacketSpaceManager { } } - packet_space_api!(InitialSpace, initial, initial_mut, discard_initial); + packet_space_api!(InitialSpace, initial, initial_mut); - packet_space_api!( - HandshakeSpace, - handshake, - handshake_mut, - discard_handshake, - initial - ); + packet_space_api!(HandshakeSpace, handshake, handshake_mut); packet_space_api!(ApplicationSpace, application, application_mut); @@ -193,6 +151,86 @@ impl PacketSpaceManager { self.zero_rtt_crypto.as_ref().map(Box::as_ref) } + /// Discard the initial packet space + pub fn discard_initial( + &mut self, + path: &mut Path, + path_id: path::Id, + now: Timestamp, + publisher: &mut Pub, + ) { + //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2 + //# When Initial or Handshake keys are discarded, the PTO and loss + //# detection timers MUST be reset, because discarding keys indicates + //# forward progress and the loss detection timer might have been set for + //# a now discarded packet number space. + if let Some(mut space) = self.initial.take() { + path.reset_pto_backoff(); + space.on_discard(path, path_id, publisher); + + if let Some((handshake, handshake_status)) = self.handshake_mut() { + //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.1 + //# A sender SHOULD restart its PTO timer every time an ack-eliciting + //# packet is sent or acknowledged, or when Initial or Handshake keys are + //# discarded (Section 4.9 of [QUIC-TLS]). + + //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2.1 + //# Since the server could be blocked until more datagrams are received + //# from the client, it is the client's responsibility to send packets to + //# unblock the server until it is certain that the server has finished + //# its address validation + + // Arm the PTO timer so the handshake can make progress + handshake.update_pto_timer(path, now, handshake_status.is_confirmed()); + } + } + + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# Endpoints MUST NOT send + //# Initial packets after this point. + // By discarding a space, we are no longer capable of sending packets with those + // keys. + + debug_assert!( + self.initial.is_none(), + "initial space should have been discarded" + ); + } + + /// Discard the handshake packet space + pub fn discard_handshake( + &mut self, + path: &mut Path, + path_id: path::Id, + publisher: &mut Pub, + ) { + //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2 + //# When Initial or Handshake keys are discarded, the PTO and loss + //# detection timers MUST be reset, because discarding keys indicates + //# forward progress and the loss detection timer might have been set for + //# a now discarded packet number space. + if let Some(mut space) = self.handshake.take() { + path.reset_pto_backoff(); + space.on_discard(path, path_id, publisher); + // Dropping handshake will clear the PTO timer for the handshake space. + // The PTO timer for the application space will be reset when the + // handshake is confirmed. + + //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.1 + //# An endpoint MUST NOT set its PTO timer for the Application Data + //# packet number space until the handshake is confirmed. + } + + debug_assert!( + self.initial.is_none(), + "initial space should have been discarded" + ); + debug_assert!( + self.handshake.is_none(), + "handshake space should have been discarded" + ); + } + pub fn discard_zero_rtt_crypto(&mut self) { self.zero_rtt_crypto = None; } @@ -450,11 +488,12 @@ impl PacketSpaceManager { error: connection::Error, path: &mut path::Path, path_id: path::Id, + now: Timestamp, publisher: &mut Pub, ) { self.session_info = None; self.retry_cid = None; - self.discard_initial(path, path_id, publisher); + self.discard_initial(path, path_id, now, publisher); self.discard_handshake(path, path_id, publisher); self.discard_zero_rtt_crypto(); From 168e44aa3748c9e78eb87f5a7dca70670e622d22 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Wed, 2 Aug 2023 10:35:57 -0700 Subject: [PATCH 3/5] Allow for the initial ack to be written --- .../src/connection/connection_impl.rs | 22 ------------------- .../src/connection/transmission.rs | 21 +++++++++++------- quic/s2n-quic-transport/src/space/mod.rs | 5 +++-- 3 files changed, 16 insertions(+), 32 deletions(-) diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index f35ef2f99b..7b4d435294 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -1219,28 +1219,6 @@ impl connection::Trait for ConnectionImpl { packet_interceptor, datagram_endpoint, )?; - - if Config::ENDPOINT_TYPE.is_client() && self.space_manager.handshake().is_some() { - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# Packets protected with Initial secrets (Section 5.2) are not - //# authenticated, meaning that an attacker could spoof packets with the - //# intent to disrupt a connection. To limit these attacks, Initial - //# packet protection keys are discarded more aggressively than other - //# keys. - - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# a client MUST discard Initial keys when it first sends a - //# Handshake packet - - // We aggressively discard initial keys as soon as we have installed - // the handshake keys to prevent the attacks mentioned above. - self.space_manager.discard_initial( - &mut self.path_manager[path_id], - path_id, - datagram.timestamp, - &mut self.event_context.publisher(datagram.timestamp, subscriber), - ); - } } Ok(()) diff --git a/quic/s2n-quic-transport/src/connection/transmission.rs b/quic/s2n-quic-transport/src/connection/transmission.rs index 44d2d00ddd..6039853a60 100644 --- a/quic/s2n-quic-transport/src/connection/transmission.rs +++ b/quic/s2n-quic-transport/src/connection/transmission.rs @@ -274,12 +274,6 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< encoder, ) { Ok((outcome, encoder)) => { - if Config::ENDPOINT_TYPE.is_client() { - //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 - //# a client MUST discard Initial keys when it first sends a - //# Handshake packet - debug_assert!(space_manager.initial().is_none()); - } *self.context.outcome += outcome; encoder } @@ -301,13 +295,24 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission< } }; + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.1 + //# a client MUST discard Initial keys when it first sends a + //# Handshake packet + if Config::ENDPOINT_TYPE.is_client() { + space_manager.discard_initial( + &mut self.context.path_manager[self.context.path_id], + self.context.path_id, + self.context.timestamp, + self.context.publisher, + ); + } + //= https://www.rfc-editor.org/rfc/rfc9001#section-4.9.2 //# An endpoint MUST discard its handshake keys when the TLS handshake is //# confirmed (Section 4.1.2). if space_manager.is_handshake_confirmed() { - let path = &mut self.context.path_manager[self.context.path_id]; space_manager.discard_handshake( - path, + &mut self.context.path_manager[self.context.path_id], self.context.path_id, self.context.publisher, ); diff --git a/quic/s2n-quic-transport/src/space/mod.rs b/quic/s2n-quic-transport/src/space/mod.rs index d522927962..a29d518f9a 100644 --- a/quic/s2n-quic-transport/src/space/mod.rs +++ b/quic/s2n-quic-transport/src/space/mod.rs @@ -180,7 +180,8 @@ impl PacketSpaceManager { //# unblock the server until it is certain that the server has finished //# its address validation - // Arm the PTO timer so the handshake can make progress + // Arm the PTO timer on the handshake space so the handshake can make progress + // even if no handshake packets have been transmitted or received yet handshake.update_pto_timer(path, now, handshake_status.is_confirmed()); } } @@ -213,7 +214,7 @@ impl PacketSpaceManager { path.reset_pto_backoff(); space.on_discard(path, path_id, publisher); // Dropping handshake will clear the PTO timer for the handshake space. - // The PTO timer for the application space will be reset when the + // The PTO timer for the application space is reset when the // handshake is confirmed. //= https://www.rfc-editor.org/rfc/rfc9002#section-6.2.1 From 62c041457386529539091aec351d3fe5dd298718 Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Wed, 2 Aug 2023 15:19:07 -0700 Subject: [PATCH 4/5] Add integration test --- quic/s2n-quic/src/tests.rs | 1 + quic/s2n-quic/src/tests/pto.rs | 87 ++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 quic/s2n-quic/src/tests/pto.rs diff --git a/quic/s2n-quic/src/tests.rs b/quic/s2n-quic/src/tests.rs index 6022c14d22..e69583536c 100644 --- a/quic/s2n-quic/src/tests.rs +++ b/quic/s2n-quic/src/tests.rs @@ -31,6 +31,7 @@ mod blackhole; mod interceptor; mod mtu; mod no_tls; +mod pto; mod self_test; // TODO: https://github.com/aws/s2n-quic/issues/1726 diff --git a/quic/s2n-quic/src/tests/pto.rs b/quic/s2n-quic/src/tests/pto.rs new file mode 100644 index 0000000000..e2251600c2 --- /dev/null +++ b/quic/s2n-quic/src/tests/pto.rs @@ -0,0 +1,87 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +use super::*; +use s2n_codec::EncoderBuffer; +use s2n_quic_core::{ + event::api::{PacketHeader, Subject}, + packet::interceptor::{Interceptor, Packet}, +}; + +/// This test ensures the PTO timer in the Handshake space is armed even +/// when the client does not otherwise receive or send any handshake +/// packets +#[test] +fn handshake_pto_timer_is_armed() { + let model = Model::default(); + let pto_subscriber = recorder::Pto::new(); + let packet_sent_subscriber = recorder::PacketSent::new(); + let pto_events = pto_subscriber.events(); + let packet_sent_events = packet_sent_subscriber.events(); + + test(model.clone(), |handle| { + let mut server = Server::builder() + .with_io(handle.builder().build()?)? + .with_tls(SERVER_CERTS)? + .with_packet_interceptor(DropHandshakeTx)? + .start()?; + + let addr = server.local_addr()?; + spawn(async move { + // We would expect this connection to time out since the server + // is not able to send any handshake packets + assert!(server.accept().await.is_none()); + }); + + let client = Client::builder() + .with_io(handle.builder().build().unwrap())? + .with_tls(certificates::CERT_PEM)? + .with_event((pto_subscriber, packet_sent_subscriber))? + .start()?; + + primary::spawn(async move { + let connect = Connect::new(addr).with_server_name("localhost"); + // We would expect this connection to time out since the server + // is not able to send any handshake packets + assert!(client.connect(connect).await.is_err()); + }); + + Ok(addr) + }) + .unwrap(); + + let pto_events = pto_events.lock().unwrap(); + let pto_count = *pto_events.iter().max().unwrap_or(&0) as usize; + + // Assert that we sent some PTOs + assert!(pto_count > 0); + + let packet_sent_events = packet_sent_events.lock().unwrap(); + let handshake_packets_sent = packet_sent_events + .iter() + .filter(|&packet_sent| matches!(packet_sent.packet_header, PacketHeader::Handshake { .. })) + .count(); + + // Assert that all handshake packets that were sent were due to the PTO timer firing. + // The first PTO that fires will send a single packet, since there are no packets + // in flight. Subsequent PTOs will send two packets. + let expected_handshake_packet_count = pto_count * 2 - 1; + assert_eq!(expected_handshake_packet_count, handshake_packets_sent); +} + +/// Drops all outgoing handshake packets +struct DropHandshakeTx; + +impl Interceptor for DropHandshakeTx { + #[inline] + fn intercept_tx_payload( + &mut self, + _subject: &Subject, + packet: &Packet, + payload: &mut EncoderBuffer, + ) { + if packet.number.space().is_handshake() { + payload.set_position(0); + } + } +} From 81ded56c0082fa1442676f78812dadb44940397a Mon Sep 17 00:00:00 2001 From: Wesley Rosenblum Date: Wed, 2 Aug 2023 17:03:10 -0700 Subject: [PATCH 5/5] Assert two initial packets are sent --- quic/s2n-quic/src/tests/pto.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/quic/s2n-quic/src/tests/pto.rs b/quic/s2n-quic/src/tests/pto.rs index e2251600c2..0bd629619c 100644 --- a/quic/s2n-quic/src/tests/pto.rs +++ b/quic/s2n-quic/src/tests/pto.rs @@ -53,15 +53,22 @@ fn handshake_pto_timer_is_armed() { let pto_events = pto_events.lock().unwrap(); let pto_count = *pto_events.iter().max().unwrap_or(&0) as usize; - // Assert that we sent some PTOs + // Assert that the client sent some PTOs assert!(pto_count > 0); let packet_sent_events = packet_sent_events.lock().unwrap(); + let initial_packets_sent = packet_sent_events + .iter() + .filter(|&packet_sent| matches!(packet_sent.packet_header, PacketHeader::Initial { .. })) + .count(); let handshake_packets_sent = packet_sent_events .iter() .filter(|&packet_sent| matches!(packet_sent.packet_header, PacketHeader::Handshake { .. })) .count(); + // Assert that only 2 initial packets were sent (the Initial[ClientHello] and the Initial[ACK]) + assert_eq!(2, initial_packets_sent); + // Assert that all handshake packets that were sent were due to the PTO timer firing. // The first PTO that fires will send a single packet, since there are no packets // in flight. Subsequent PTOs will send two packets.