fix: Check that the largest_acked was sent (mozilla#2150)

* fix: Check that the largest_acked was sent This is a test and fix for the issue we're discussing with Avast. CC @mxinden * Fix * Do not use untrusted largest_ack * Return Error::AckedUnsentPacket * Tweaks * Typo * Tweaks * Tweaks * Update neqo-transport/src/connection/mod.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/tests/connection.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Nit * Simplify test --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Max Inden <[email protected]>
mxinden · Oct 15, 2024 · d02e34f · d02e34f
1 parent 80db3a0
commit d02e34f
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 40 deletions.
diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml
@@ -8,11 +8,16 @@ license = "MIT/Apache-2.0"
 build = "build.rs"
 
 [dependencies]
-log = {version = "0.4.0", default-features = false}
+log = {version = "=0.4.17", default-features = false}
 env_logger = {version = "0.9", default-features = false}
 lazy_static = "1.3.0"
 qlog = "0.4.0"
 chrono = {version = "0.4.10", default-features = false, features = ["std"]}
+# Pinning dependency for CI
+# error: package `enumset v1.1.5` cannot be built because it requires rustc 1.61 or newer, while the currently active rustc version is 1.57.0
+enumset = "=1.0.12"
+# error[E0658]: use of unstable library feature 'mixed_integer_ops'
+unicode-width = "=0.1.10"
 
 [features]
 default = ["deny-warnings"]

diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs
@@ -1468,6 +1468,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 `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.
+        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[..]);
@@ -1492,7 +1503,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))?;
             }
         }
@@ -2560,6 +2578,7 @@ impl Connection {
         packet_version: Version,
         packet_type: PacketType,
         frame: Frame,
+        next_pn: PacketNumber,
         now: Instant,
     ) -> Res<()> {
         if !frame.is_allowed(packet_type) {
@@ -2594,9 +2613,17 @@ impl Connection {
                 first_ack_range,
                 ack_ranges,
             } => {
+                // Ensure that the largest acknowledged packet number was actually sent.
+                // (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);
+                    return Err(Error::AckedUnsentPacket);
+                }
+
                 let ranges =
                     Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?;
-                self.handle_ack(space, largest_acknowledged, ranges, ack_delay, now);
+                self.handle_ack(space, ranges, ack_delay, now);
             }
             Frame::Crypto { offset, data } => {
                 qtrace!(
@@ -2771,7 +2798,6 @@ impl Connection {
     fn handle_ack<R>(
         &mut self,
         space: PacketNumberSpace,
-        largest_acknowledged: u64,
         ack_ranges: R,
         ack_delay: u64,
         now: Instant,
@@ -2784,11 +2810,11 @@ impl Connection {
         let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received(
             &self.paths.primary(),
             space,
-            largest_acknowledged,
             ack_ranges,
             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 {
@@ -2812,7 +2838,9 @@ impl Connection {
         qlog::packets_lost(&mut 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(largest_acknowledged) = largest_acknowledged {
+            stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged);
+        }
     }
 
     /// When the server rejects 0-RTT we need to drop a bunch of stuff.

diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs
@@ -808,3 +808,38 @@ fn fast_pto_persistent_congestion() {
     client.process_input(ack.unwrap(), now);
     assert_eq!(cwnd(&client), CWND_MIN);
 }
+
+/// 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/src/recovery.rs b/neqo-transport/src/recovery.rs
@@ -652,7 +652,6 @@ impl LossRecovery {
         &mut self,
         primary_path: &PathRef,
         pn_space: PacketNumberSpace,
-        largest_acked: u64,
         acked_ranges: R,
         ack_delay: Duration,
         now: Instant,
@@ -661,36 +660,25 @@ impl LossRecovery {
         R: IntoIterator<Item = RangeInclusive<u64>>,
         R::IntoIter: ExactSizeIterator,
     {
-        qdebug!(
-            [self],
-            "ACK for {} - largest_acked={}.",
-            pn_space,
-            largest_acked
-        );
-
-        let space = self.spaces.get_mut(pn_space);
-        let space = if let Some(sp) = space {
-            sp
-        } else {
+        let Some(space) = self.spaces.get_mut(pn_space) else {
             qinfo!("ACK on discarded space");
             return (Vec::new(), Vec::new());
         };
 
         let (acked_packets, any_ack_eliciting) =
             space.remove_acked(acked_ranges, &mut self.stats.borrow_mut());
-        if acked_packets.is_empty() {
+        let Some(largest_acked_pkt) = acked_packets.first() else {
             // No new information.
             return (Vec::new(), Vec::new());
-        }
+        };
 
         // 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)
-            let largest_acked_pkt = acked_packets.first().expect("must be there");
             space.largest_acked_sent_time = Some(largest_acked_pkt.time_sent);
             if any_ack_eliciting && largest_acked_pkt.on_primary_path() {
                 self.rtt_sample(
@@ -702,6 +690,13 @@ impl LossRecovery {
             }
         }
 
+        qdebug!(
+            [self],
+            "ACK for {} - 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
@@ -1035,19 +1030,12 @@ mod tests {
         pub fn on_ack_received(
             &mut self,
             pn_space: PacketNumberSpace,
-            largest_acked: u64,
-            acked_ranges: Vec<RangeInclusive<u64>>,
+            acked_ranges: Vec<RangeInclusive<PacketNumber>>,
             ack_delay: Duration,
             now: Instant,
         ) -> (Vec<SentPacket>, Vec<SentPacket>) {
-            self.lr.on_ack_received(
-                &self.path,
-                pn_space,
-                largest_acked,
-                acked_ranges,
-                ack_delay,
-                now,
-            )
+            self.lr
+                .on_ack_received(&self.path, pn_space, acked_ranges, ack_delay, now)
         }
 
         pub fn on_packet_sent(&mut self, sent_packet: SentPacket) {
@@ -1191,7 +1179,6 @@ mod tests {
     fn ack(lr: &mut Fixture, pn: u64, delay: Duration) {
         lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            pn,
             vec![pn..=pn],
             ACK_DELAY,
             pn_time(pn) + delay,
@@ -1338,7 +1325,6 @@ mod tests {
         ));
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            1,
             vec![1..=1],
             ACK_DELAY,
             pn_time(0) + (TEST_RTT * 5 / 4),
@@ -1361,7 +1347,6 @@ mod tests {
 
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            2,
             vec![2..=2],
             ACK_DELAY,
             pn2_ack_time,
@@ -1390,7 +1375,6 @@ mod tests {
         assert_eq!(super::PACKET_THRESHOLD, 3);
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            4,
             vec![2..=4],
             ACK_DELAY,
             pn_time(4),
@@ -1418,7 +1402,6 @@ mod tests {
         lr.discard(PacketNumberSpace::Initial, now());
         let (acked, lost) = lr.on_ack_received(
             PacketNumberSpace::Initial,
-            0,
             vec![],
             Duration::from_millis(0),
             pn_time(0),
@@ -1464,7 +1447,7 @@ mod tests {
             let sent_pkt = SentPacket::new(*sp, 1, pn_time(3), true, Vec::new(), ON_SENT_SIZE);
             let pn_space = PacketNumberSpace::from(sent_pkt.pt);
             lr.on_packet_sent(sent_pkt);
-            lr.on_ack_received(pn_space, 1, vec![1..=1], Duration::from_secs(0), pn_time(3));
+            lr.on_ack_received(pn_space, vec![1..=1], Duration::from_secs(0), pn_time(3));
             let mut lost = Vec::new();
             lr.spaces.get_mut(pn_space).unwrap().detect_lost_packets(
                 pn_time(3),
@@ -1510,7 +1493,6 @@ mod tests {
         let rtt = lr.path.borrow().rtt().estimate();
         lr.on_ack_received(
             PacketNumberSpace::Initial,
-            0,
             vec![0..=0],
             Duration::new(0, 0),
             now() + rtt,

diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs
@@ -169,6 +169,11 @@ impl SentPacket {
         }
     }
 
+    /// The number of the packet.
+    pub const fn pn(&self) -> PacketNumber {
+        self.pn
+    }
+
     /// Returns `true` if the packet will elicit an ACK.
     pub fn ack_eliciting(&self) -> bool {
         self.ack_eliciting