From cac441b7853ef7f26486bf0ade2e1f9203649a60 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 09:20:00 +0300 Subject: [PATCH 01/13] fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden --- neqo-transport/src/connection/mod.rs | 23 ++++++++-- neqo-transport/tests/connection.rs | 64 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 156c7de815..b35af3b0ba 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2862,7 +2862,7 @@ impl Connection { ecn_count, ack_delay, now, - ); + )?; } Frame::Crypto { offset, data } => { qtrace!( @@ -3055,15 +3055,29 @@ impl Connection { ack_ecn: Option, ack_delay: u64, now: Instant, - ) where + ) -> Res<()> + where R: IntoIterator> + Debug, R::IntoIter: ExactSizeIterator, { qdebug!([self], "Rx ACK space={}, ranges={:?}", space, ack_ranges); - let Some(path) = self.paths.primary() else { - return; + 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.) + let next_pn = self + .crypto + .states + .select_tx(self.version, space) + .map_or_else(|| Err(Error::InternalError), |(_, tx)| Ok(tx.next_pn()))?; + 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, @@ -3097,6 +3111,7 @@ impl Connection { let stats = &mut self.stats.borrow_mut().frame_rx; stats.ack += 1; stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + Ok(()) } /// Tell 0-RTT packets that they were "lost". diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 6b670cdc3c..feb5095a3a 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -132,6 +132,70 @@ fn reorder_server_initial() { assert_eq!(*client.state(), State::Confirmed); } +/// +#[test] +fn ack_for_unsent() { + // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. + const ACK_FRAME: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; + + let mut client = new_client( + ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), + ); + let mut server = default_server(); + + let client_initial = client.process_output(now()); + let (_, client_dcid, _, _) = + decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap(); + let client_dcid = client_dcid.to_owned(); + + let server_packet = server.process(client_initial.as_dgram_ref(), now()).dgram(); + let (server_initial, server_hs) = split_datagram(server_packet.as_ref().unwrap()); + let (protected_header, _, _, payload) = + decode_initial_header(&server_initial, Role::Server).unwrap(); + + // Now decrypt the packet. + let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server); + let (header, pn) = remove_header_protection(&hp, protected_header, payload); + assert_eq!(pn, 0); + let pn_len = header.len() - protected_header.len(); + let mut buf = vec![0; payload.len()]; + let mut plaintext = aead + .decrypt(pn, &header, &payload[pn_len..], &mut buf) + .unwrap() + .to_owned(); + + // Now we need to find the frames. Make some really strong assumptions. + let mut dec = Decoder::new(&plaintext[..]); + assert_eq!(dec.decode(ACK_FRAME.len()), Some(ACK_FRAME)); + assert_eq!(dec.decode_varint(), Some(0x06)); // CRYPTO + assert_eq!(dec.decode_varint(), Some(0x00)); // offset + dec.skip_vvec(); // Skip over the payload. + + // Overwrite larget_acked in the ACK frame with 3 (a packet that was never sent). + plaintext[1] = 0x3; + + // And rebuild a packet. + let mut packet = header.clone(); + packet.resize(MIN_INITIAL_PACKET_SIZE, 0); + aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) + .unwrap(); + apply_header_protection(&hp, &mut packet, protected_header.len()..header.len()); + let spoofed = Datagram::new( + server_initial.source(), + server_initial.destination(), + server_initial.tos(), + packet, + ); + + // 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)) + ); +} + fn set_payload(server_packet: &Option, client_dcid: &[u8], payload: &[u8]) -> Datagram { let (server_initial, _server_hs) = split_datagram(server_packet.as_ref().unwrap()); let (protected_header, _, _, orig_payload) = From b65957bd293b839d23e5216153ca8e0614a74829 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 10:16:41 +0300 Subject: [PATCH 02/13] Fix --- neqo-transport/src/connection/mod.rs | 29 ++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index b35af3b0ba..86f9cec187 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) { @@ -2857,6 +2876,7 @@ impl Connection { Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; self.handle_ack( space, + next_pn, largest_acknowledged, ranges, ecn_count, @@ -3047,9 +3067,11 @@ impl Connection { ) } + #[allow(clippy::too_many_arguments)] fn handle_ack( &mut self, space: PacketNumberSpace, + next_pn: PacketNumber, largest_acknowledged: PacketNumber, ack_ranges: R, ack_ecn: Option, @@ -3068,11 +3090,6 @@ impl Connection { // 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.) - let next_pn = self - .crypto - .states - .select_tx(self.version, space) - .map_or_else(|| Err(Error::InternalError), |(_, tx)| Ok(tx.next_pn()))?; if largest_acknowledged >= next_pn { qwarn!("Largest ACKed {} was never sent", largest_acknowledged,); return Err(Error::AckedUnsentPacket); From e03c825d40e068d9d5be75cbc12739fdbad66c2e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 7 Oct 2024 12:18:22 +0200 Subject: [PATCH 03/13] 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 04/13] 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 { From c08f47f4fb50602e5874684a8981f2bc8254c7a5 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 14:51:02 +0300 Subject: [PATCH 05/13] Tweaks --- neqo-transport/src/connection/mod.rs | 16 +++++++--------- neqo-transport/src/recovery/mod.rs | 4 +--- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 48d4045cde..f87d114986 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1664,7 +1664,7 @@ impl Connection { // 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, + // TODO: Once PR #2118 lands, this can move to `input_frame`. 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. @@ -2873,8 +2873,8 @@ impl Connection { 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 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); @@ -2882,7 +2882,7 @@ impl Connection { let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; - self.handle_ack(space, ranges, ecn_count, ack_delay, now)?; + self.handle_ack(space, ranges, ecn_count, ack_delay, now); } Frame::Crypto { offset, data } => { qtrace!( @@ -3067,7 +3067,6 @@ impl Connection { ) } - #[allow(clippy::too_many_arguments)] fn handle_ack( &mut self, space: PacketNumberSpace, @@ -3075,14 +3074,14 @@ impl Connection { ack_ecn: Option, ack_delay: u64, now: Instant, - ) -> Res<()> - where + ) where R: IntoIterator> + Debug, R::IntoIter: ExactSizeIterator, { qdebug!([self], "Rx ACK space={}, ranges={:?}", space, ack_ranges); + let Some(path) = self.paths.primary() else { - return Ok(()); + return; }; let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( @@ -3120,7 +3119,6 @@ impl Connection { if let Some(la) = largest_acknowledged { stats.largest_acknowledged = max(stats.largest_acknowledged, la); } - Ok(()) } /// Tell 0-RTT packets that they were "lost". diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index c104518d22..0a4fb20096 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -587,8 +587,6 @@ impl LossRecovery { R: IntoIterator>, R::IntoIter: ExactSizeIterator, { - qdebug!([self], "ACK for {}.", pn_space,); - let Some(space) = self.spaces.get_mut(pn_space) else { qinfo!("ACK on discarded space"); return (Vec::new(), Vec::new()); @@ -621,7 +619,7 @@ impl LossRecovery { qdebug!( [self], - "ACK pn_space={} largest_acked={}.", + "ACK for {} - largest_acked={}", pn_space, largest_acked_pkt.pn() ); From 0478506cc68a4b1d2398c2447efb246eec765fb5 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 14:52:56 +0300 Subject: [PATCH 06/13] Typo --- neqo-transport/src/connection/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index f87d114986..5fae1dad81 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2873,7 +2873,7 @@ impl Connection { 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 + // (If we ever start using non-contiguous 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,); From dae5faf8261be83fbfbb6bbafdd3334b49d136a6 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 14:55:48 +0300 Subject: [PATCH 07/13] Tweaks --- neqo-transport/src/connection/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 5fae1dad81..09f9f00373 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -3083,7 +3083,6 @@ impl Connection { let Some(path) = self.paths.primary() else { return; }; - let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( &path, space, @@ -3116,8 +3115,8 @@ impl Connection { qlog::packets_lost(&self.qlog, &lost_packets); let stats = &mut self.stats.borrow_mut().frame_rx; stats.ack += 1; - if let Some(la) = largest_acknowledged { - stats.largest_acknowledged = max(stats.largest_acknowledged, la); + if let Some(largest_acknowledged) = largest_acknowledged { + stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); } } From ffa064baa4515911927996e864b8081fccb42ac9 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 14:58:50 +0300 Subject: [PATCH 08/13] Tweaks --- neqo-transport/tests/connection.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index feb5095a3a..604737ed8e 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); } -/// +/// Receiving an ACK frame for a packet number that was never sent is an error. #[test] fn ack_for_unsent() { // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. @@ -171,10 +171,10 @@ fn ack_for_unsent() { assert_eq!(dec.decode_varint(), Some(0x00)); // offset dec.skip_vvec(); // Skip over the payload. - // Overwrite larget_acked in the ACK frame with 3 (a packet that was never sent). + // Overwrite largest_acked in the ACK frame with 3 (a packet that was never sent). plaintext[1] = 0x3; - // And rebuild a packet. + // And rebuild the packet. let mut packet = header.clone(); packet.resize(MIN_INITIAL_PACKET_SIZE, 0); aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) From 42b0e165ad8e830a9237b684cb2fc11c44e0fe3c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 15:10:33 +0300 Subject: [PATCH 09/13] Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden Signed-off-by: Lars Eggert --- neqo-transport/src/connection/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 3aed604c9c..eb8ff77881 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2876,7 +2876,7 @@ impl Connection { // (If we ever start using non-contiguous 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,); + qwarn!("Largest ACKed {} was never sent", largest_acknowledged); return Err(Error::AckedUnsentPacket); } From b10dab11b6c83f32868dc097ac741735cadf2537 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 17:08:57 +0300 Subject: [PATCH 10/13] Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden Signed-off-by: Lars Eggert --- neqo-transport/tests/connection.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index f3e4be31c5..6287acc28c 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -189,7 +189,6 @@ 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)) From 0c43ce0249945986ee4ab50087ed93cfa7c6af4c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 17:09:11 +0300 Subject: [PATCH 11/13] Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden Signed-off-by: Lars Eggert --- neqo-transport/tests/connection.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 6287acc28c..f3c2391854 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -172,6 +172,7 @@ fn ack_for_unsent() { dec.skip_vvec(); // Skip over the payload. // Overwrite largest_acked in the ACK frame with 3 (a packet that was never sent). + assert_eq!(plaintext[1], 0x0); plaintext[1] = 0x3; // And rebuild the packet. From 91a66830941fbd3585d883f2b996de167c569b46 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 7 Oct 2024 18:16:45 +0300 Subject: [PATCH 12/13] Nit --- neqo-transport/tests/connection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index f3c2391854..d5ccd6bc60 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -149,7 +149,7 @@ fn ack_for_unsent() { let client_dcid = client_dcid.to_owned(); let server_packet = server.process(client_initial.as_dgram_ref(), now()).dgram(); - let (server_initial, server_hs) = split_datagram(server_packet.as_ref().unwrap()); + let (server_initial, _) = split_datagram(server_packet.as_ref().unwrap()); let (protected_header, _, _, payload) = decode_initial_header(&server_initial, Role::Server).unwrap(); From 771d4dbe1c5a49bb9427dcaa93f14fc928b38f3e Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 8 Oct 2024 08:34:12 +0300 Subject: [PATCH 13/13] Simplify test --- .../src/connection/tests/recovery.rs | 41 +++++++++++- neqo-transport/tests/connection.rs | 64 ------------------- 2 files changed, 39 insertions(+), 66 deletions(-) diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index a97df1ca64..5b50e8be0a 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -24,7 +24,9 @@ use super::{ POST_HANDSHAKE_CWND, }; use crate::{ - connection::tests::cwnd_min, + connection::{test_internal::FrameWriter, tests::cwnd_min}, + frame::FRAME_TYPE_ACK, + packet::PacketBuilder, recovery::{ FAST_PTO_SCALE, MAX_OUTSTANDING_UNACK, MAX_PTO_PACKET_COUNT, MIN_OUTSTANDING_UNACK, }, @@ -32,7 +34,7 @@ use crate::{ stats::MAX_PTO_COUNTS, tparams::TransportParameter, tracking::DEFAULT_ACK_DELAY, - Pmtud, StreamType, + CloseReason, Error, Pmtud, StreamType, }; #[test] @@ -812,3 +814,38 @@ fn fast_pto_persistent_congestion() { client.process_input(&ack.unwrap(), now); assert_eq!(cwnd(&client), cwnd_min(&client)); } + +/// Receiving an ACK frame for a packet number that was never sent is an error. +#[test] +fn ack_for_unsent() { + /// This inserts an ACK frame into packets that ACKs a packet that was never sent. + struct AckforUnsentWriter {} + + impl FrameWriter for AckforUnsentWriter { + fn write_frames(&mut self, builder: &mut PacketBuilder) { + builder.encode_varint(FRAME_TYPE_ACK); + builder.encode_varint(666u16); // Largest ACKed + builder.encode_varint(0u8); // ACK delay + builder.encode_varint(0u8); // ACK block count + builder.encode_varint(0u8); // ACK block length + } + } + + let mut client = default_client(); + let mut server = default_server(); + connect_force_idle(&mut client, &mut server); + + server.test_frame_writer = Some(Box::new(AckforUnsentWriter {})); + let spoofed = server.process_output(now()).dgram().unwrap(); + server.test_frame_writer = None; + + // Now deliver the packet with the spoofed ACK frame + client.process_input(&spoofed, now()); + assert!(matches!( + client.state(), + State::Closing { + error: CloseReason::Transport(Error::AckedUnsentPacket), + .. + } + )); +} diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index d5ccd6bc60..934199022b 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -132,70 +132,6 @@ fn reorder_server_initial() { assert_eq!(*client.state(), State::Confirmed); } -/// Receiving an ACK frame for a packet number that was never sent is an error. -#[test] -fn ack_for_unsent() { - // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. - const ACK_FRAME: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; - - let mut client = new_client( - ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), - ); - let mut server = default_server(); - - let client_initial = client.process_output(now()); - let (_, client_dcid, _, _) = - decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap(); - let client_dcid = client_dcid.to_owned(); - - let server_packet = server.process(client_initial.as_dgram_ref(), now()).dgram(); - let (server_initial, _) = split_datagram(server_packet.as_ref().unwrap()); - let (protected_header, _, _, payload) = - decode_initial_header(&server_initial, Role::Server).unwrap(); - - // Now decrypt the packet. - let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server); - let (header, pn) = remove_header_protection(&hp, protected_header, payload); - assert_eq!(pn, 0); - let pn_len = header.len() - protected_header.len(); - let mut buf = vec![0; payload.len()]; - let mut plaintext = aead - .decrypt(pn, &header, &payload[pn_len..], &mut buf) - .unwrap() - .to_owned(); - - // Now we need to find the frames. Make some really strong assumptions. - let mut dec = Decoder::new(&plaintext[..]); - assert_eq!(dec.decode(ACK_FRAME.len()), Some(ACK_FRAME)); - assert_eq!(dec.decode_varint(), Some(0x06)); // CRYPTO - assert_eq!(dec.decode_varint(), Some(0x00)); // offset - dec.skip_vvec(); // Skip over the payload. - - // Overwrite largest_acked in the ACK frame with 3 (a packet that was never sent). - assert_eq!(plaintext[1], 0x0); - plaintext[1] = 0x3; - - // And rebuild the packet. - let mut packet = header.clone(); - packet.resize(MIN_INITIAL_PACKET_SIZE, 0); - aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) - .unwrap(); - apply_header_protection(&hp, &mut packet, protected_header.len()..header.len()); - let spoofed = Datagram::new( - server_initial.source(), - server_initial.destination(), - server_initial.tos(), - packet, - ); - - // Now deliver the packet with the spoofed ACK frame - client.process_input(&spoofed, now()); - assert_eq!( - client.state(), - &State::Closed(CloseReason::Transport(Error::AckedUnsentPacket)) - ); -} - fn set_payload(server_packet: Option<&Datagram>, client_dcid: &[u8], payload: &[u8]) -> Datagram { let (server_initial, _server_hs) = split_datagram(server_packet.as_ref().unwrap()); let (protected_header, _, _, orig_payload) =