fix: Check that the largest_acked was sent

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 `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[..]);
@@ -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,16 +2872,17 @@ impl Connection {
                 ack_ranges,
                 ecn_count,
             } => {
+                // 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,
-                    ecn_count,
-                    ack_delay,
-                    now,
-                );
+                self.handle_ack(space, ranges, ecn_count, ack_delay, now);
             }
             Frame::Crypto { offset, data } => {
                 qtrace!(
@@ -3050,7 +3070,6 @@ impl Connection {
     fn handle_ack<R>(
         &mut self,
         space: PacketNumberSpace,
-        largest_acknowledged: PacketNumber,
         ack_ranges: R,
         ack_ecn: Option<EcnCount>,
         ack_delay: u64,
@@ -3067,12 +3086,12 @@ impl Connection {
         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 {
@@ -3096,7 +3115,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(largest_acknowledged) = largest_acknowledged {
+            stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged);
+        }
     }
 
     /// Tell 0-RTT packets that they were "lost".

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<EcnCount>,
         ack_delay: Duration,
@@ -588,13 +587,6 @@ impl LossRecovery {
         R: IntoIterator<Item = RangeInclusive<PacketNumber>>,
         R::IntoIter: ExactSizeIterator,
     {
-        qdebug!(
-            [self],
-            "ACK for {} - largest_acked={}.",
-            pn_space,
-            largest_acked
-        );
-
         let Some(space) = self.spaces.get_mut(pn_space) else {
             qinfo!("ACK on discarded space");
             return (Vec::new(), Vec::new());
@@ -609,8 +601,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 +617,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
@@ -978,21 +977,13 @@ mod tests {
         pub fn on_ack_received(
             &mut self,
             pn_space: PacketNumberSpace,
-            largest_acked: PacketNumber,
             acked_ranges: Vec<RangeInclusive<PacketNumber>>,
             ack_ecn: Option<EcnCount>,
             ack_delay: Duration,
             now: Instant,
         ) -> (Vec<SentPacket>, Vec<SentPacket>) {
-            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 +1136,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 +1289,6 @@ mod tests {
         ));
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            1,
             vec![1..=1],
             None,
             ACK_DELAY,
@@ -1323,7 +1312,6 @@ mod tests {
 
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            2,
             vec![2..=2],
             None,
             ACK_DELAY,
@@ -1353,7 +1341,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 +1362,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 +1421,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 +1473,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),

neqo-transport/tests/connection.rs

@@ -132,6 +132,70 @@ 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, 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 largest_acked in the ACK frame with 3 (a packet that was never sent).
+    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());
+    client.process_input(&server_hs.unwrap(), 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) =