From e03c825d40e068d9d5be75cbc12739fdbad66c2e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 7 Oct 2024 12:18:22 +0200 Subject: [PATCH 1/2] Do not use untrusted largest_ack --- neqo-transport/src/connection/mod.rs | 47 ++++------------------------ neqo-transport/src/recovery/mod.rs | 38 +++++++--------------- neqo-transport/tests/connection.rs | 7 ++--- 3 files changed, 20 insertions(+), 72 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 86f9cec187..07cdfe304f 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1663,17 +1663,6 @@ impl Connection { // on the assert for doesn't exist. // OK, we have a valid packet. - // Get the next packet number we'll send, for ACK verification. - // TODO: Once PR #2118 lands, this can move to handle_ack. For now, it needs to be here, - // because we can drop packet number spaces as we parse throught the packet, and if an ACK - // frame follows a CRYPTO frame that makes us drop a space, we need to know this - // packet number to verify the ACK against. - let next_pn = self - .crypto - .states - .select_tx(self.version, PacketNumberSpace::from(packet.packet_type())) - .map_or(0, |(_, tx)| tx.next_pn()); - let mut ack_eliciting = false; let mut probing = true; let mut d = Decoder::from(&packet[..]); @@ -1686,14 +1675,7 @@ impl Connection { ack_eliciting |= f.ack_eliciting(); probing &= f.path_probing(); let t = f.get_type(); - if let Err(e) = self.input_frame( - path, - packet.version(), - packet.packet_type(), - f, - next_pn, - now, - ) { + if let Err(e) = self.input_frame(path, packet.version(), packet.packet_type(), f, now) { self.capture_error(Some(Rc::clone(path)), now, t, Err(e))?; } } @@ -2840,7 +2822,6 @@ impl Connection { packet_version: Version, packet_type: PacketType, frame: Frame, - next_pn: PacketNumber, now: Instant, ) -> Res<()> { if !frame.is_allowed(packet_type) { @@ -2874,15 +2855,7 @@ impl Connection { } => { let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; - self.handle_ack( - space, - next_pn, - largest_acknowledged, - ranges, - ecn_count, - ack_delay, - now, - )?; + self.handle_ack(space, ranges, ecn_count, ack_delay, now)?; } Frame::Crypto { offset, data } => { qtrace!( @@ -3071,8 +3044,6 @@ impl Connection { fn handle_ack( &mut self, space: PacketNumberSpace, - next_pn: PacketNumber, - largest_acknowledged: PacketNumber, ack_ranges: R, ack_ecn: Option, ack_delay: u64, @@ -3087,23 +3058,15 @@ impl Connection { return Ok(()); }; - // Ensure that the largest acknowledged packet number was actually sent. - // (If we ever start using non-contigous packet numbers, we need to check all the packet - // numbers in the ACKed ranges.) - if largest_acknowledged >= next_pn { - qwarn!("Largest ACKed {} was never sent", largest_acknowledged,); - return Err(Error::AckedUnsentPacket); - } - let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( &path, space, - largest_acknowledged, ack_ranges, ack_ecn, self.decode_ack_delay(ack_delay), now, ); + let largest_acknowledged = acked_packets.first().map(SentPacket::pn); for acked in acked_packets { for token in acked.tokens() { match token { @@ -3127,7 +3090,9 @@ impl Connection { qlog::packets_lost(&self.qlog, &lost_packets); let stats = &mut self.stats.borrow_mut().frame_rx; stats.ack += 1; - stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + if let Some(la) = largest_acknowledged { + stats.largest_acknowledged = max(stats.largest_acknowledged, la); + } Ok(()) } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index a5753e6c84..c104518d22 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -578,7 +578,6 @@ impl LossRecovery { &mut self, primary_path: &PathRef, pn_space: PacketNumberSpace, - largest_acked: PacketNumber, acked_ranges: R, ack_ecn: Option, ack_delay: Duration, @@ -588,12 +587,7 @@ impl LossRecovery { R: IntoIterator>, R::IntoIter: ExactSizeIterator, { - qdebug!( - [self], - "ACK for {} - largest_acked={}.", - pn_space, - largest_acked - ); + qdebug!([self], "ACK for {}.", pn_space,); let Some(space) = self.spaces.get_mut(pn_space) else { qinfo!("ACK on discarded space"); @@ -609,8 +603,8 @@ impl LossRecovery { // Track largest PN acked per space let prev_largest_acked = space.largest_acked_sent_time; - if Some(largest_acked) > space.largest_acked { - space.largest_acked = Some(largest_acked); + if Some(largest_acked_pkt.pn()) > space.largest_acked { + space.largest_acked = Some(largest_acked_pkt.pn()); // If the largest acknowledged is newly acked and any newly acked // packet was ack-eliciting, update the RTT. (-recovery 5.1) @@ -625,6 +619,13 @@ impl LossRecovery { } } + qdebug!( + [self], + "ACK pn_space={} largest_acked={}.", + pn_space, + largest_acked_pkt.pn() + ); + // Perform loss detection. // PTO is used to remove lost packets from in-flight accounting. // We need to ensure that we have sent any PTO probes before they are removed @@ -978,21 +979,13 @@ mod tests { pub fn on_ack_received( &mut self, pn_space: PacketNumberSpace, - largest_acked: PacketNumber, acked_ranges: Vec>, ack_ecn: Option, ack_delay: Duration, now: Instant, ) -> (Vec, Vec) { - self.lr.on_ack_received( - &self.path, - pn_space, - largest_acked, - acked_ranges, - ack_ecn, - ack_delay, - now, - ) + self.lr + .on_ack_received(&self.path, pn_space, acked_ranges, ack_ecn, ack_delay, now) } pub fn on_packet_sent(&mut self, sent_packet: SentPacket) { @@ -1145,7 +1138,6 @@ mod tests { fn ack(lr: &mut Fixture, pn: u64, delay: Duration) { lr.on_ack_received( PacketNumberSpace::ApplicationData, - pn, vec![pn..=pn], None, ACK_DELAY, @@ -1299,7 +1291,6 @@ mod tests { )); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 1, vec![1..=1], None, ACK_DELAY, @@ -1323,7 +1314,6 @@ mod tests { let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 2, vec![2..=2], None, ACK_DELAY, @@ -1353,7 +1343,6 @@ mod tests { assert_eq!(super::PACKET_THRESHOLD, 3); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 4, vec![2..=4], None, ACK_DELAY, @@ -1375,7 +1364,6 @@ mod tests { lr.discard(PacketNumberSpace::Initial, now()); let (acked, lost) = lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![], None, Duration::from_millis(0), @@ -1435,7 +1423,6 @@ mod tests { lr.on_packet_sent(sent_pkt); lr.on_ack_received( pn_space, - 1, vec![1..=1], None, Duration::from_secs(0), @@ -1488,7 +1475,6 @@ mod tests { let rtt = lr.path.borrow().rtt().estimate(); lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![0..=0], None, Duration::new(0, 0), diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index feb5095a3a..cc077e1e4f 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -132,7 +132,7 @@ fn reorder_server_initial() { assert_eq!(*client.state(), State::Confirmed); } -/// +/// Ignore ACK for unsent package. #[test] fn ack_for_unsent() { // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. @@ -190,10 +190,7 @@ fn ack_for_unsent() { // Now deliver the packet with the spoofed ACK frame client.process_input(&spoofed, now()); client.process_input(&server_hs.unwrap(), now()); - assert_eq!( - client.state(), - &State::Closed(CloseReason::Transport(Error::AckedUnsentPacket)) - ); + assert_eq!(client.state(), &State::Handshaking); } fn set_payload(server_packet: &Option, client_dcid: &[u8], payload: &[u8]) -> Datagram { From 6551357cb4dab649ea05e0d15d220cdcb3edbd7b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 7 Oct 2024 12:46:32 +0200 Subject: [PATCH 2/2] Return Error::AckedUnsentPacket --- neqo-transport/src/connection/mod.rs | 29 +++++++++++++++++++++++++++- neqo-transport/tests/connection.rs | 7 +++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 07cdfe304f..48d4045cde 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1663,6 +1663,17 @@ impl Connection { // on the assert for doesn't exist. // OK, we have a valid packet. + // Get the next packet number we'll send, for ACK verification. + // TODO: Once PR #2118 lands, this can move to handle_ack. For now, it needs to be here, + // because we can drop packet number spaces as we parse throught the packet, and if an ACK + // frame follows a CRYPTO frame that makes us drop a space, we need to know this + // packet number to verify the ACK against. + let next_pn = self + .crypto + .states + .select_tx(self.version, PacketNumberSpace::from(packet.packet_type())) + .map_or(0, |(_, tx)| tx.next_pn()); + let mut ack_eliciting = false; let mut probing = true; let mut d = Decoder::from(&packet[..]); @@ -1675,7 +1686,14 @@ impl Connection { ack_eliciting |= f.ack_eliciting(); probing &= f.path_probing(); let t = f.get_type(); - if let Err(e) = self.input_frame(path, packet.version(), packet.packet_type(), f, now) { + if let Err(e) = self.input_frame( + path, + packet.version(), + packet.packet_type(), + f, + next_pn, + now, + ) { self.capture_error(Some(Rc::clone(path)), now, t, Err(e))?; } } @@ -2822,6 +2840,7 @@ impl Connection { packet_version: Version, packet_type: PacketType, frame: Frame, + next_pn: PacketNumber, now: Instant, ) -> Res<()> { if !frame.is_allowed(packet_type) { @@ -2853,6 +2872,14 @@ impl Connection { ack_ranges, ecn_count, } => { + // Ensure that the largest acknowledged packet number was actually sent. + // (If we ever start using non-contigous packet numbers, we need to check all the packet + // numbers in the ACKed ranges.) + if largest_acknowledged >= next_pn { + qwarn!("Largest ACKed {} was never sent", largest_acknowledged,); + return Err(Error::AckedUnsentPacket); + } + let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; self.handle_ack(space, ranges, ecn_count, ack_delay, now)?; diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index cc077e1e4f..feb5095a3a 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -132,7 +132,7 @@ fn reorder_server_initial() { assert_eq!(*client.state(), State::Confirmed); } -/// Ignore ACK for unsent package. +/// #[test] fn ack_for_unsent() { // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. @@ -190,7 +190,10 @@ fn ack_for_unsent() { // Now deliver the packet with the spoofed ACK frame client.process_input(&spoofed, now()); client.process_input(&server_hs.unwrap(), now()); - assert_eq!(client.state(), &State::Handshaking); + assert_eq!( + client.state(), + &State::Closed(CloseReason::Transport(Error::AckedUnsentPacket)) + ); } fn set_payload(server_packet: &Option, client_dcid: &[u8], payload: &[u8]) -> Datagram {