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 · 169da12 · 169da12
1 parent 80db3a0
commit 169da12
Show file tree

Hide file tree

Showing 32 changed files with 106 additions and 70 deletions.
diff --git a/neqo-client/src/main.rs b/neqo-client/src/main.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use qlog::QlogStreamer;

diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml
@@ -8,11 +8,18 @@ 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"
+# error: there is no argument named `arch`
+clang-sys = "1.7.0"
 
 [features]
 default = ["deny-warnings"]

diff --git a/neqo-common/src/lib.rs b/neqo-common/src/lib.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 mod codec;

diff --git a/neqo-common/tests/log.rs b/neqo-common/tests/log.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use neqo_common::{qdebug, qerror, qinfo, qtrace, qwarn};

diff --git a/neqo-crypto/build.rs b/neqo-crypto/build.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 use bindgen::Builder;

diff --git a/neqo-crypto/src/lib.rs b/neqo-crypto/src/lib.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 // Bindgen auto generated code
 // won't adhere to the clippy rules below

diff --git a/neqo-crypto/tests/aead.rs b/neqo-crypto/tests/aead.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 #![cfg(not(feature = "fuzzing"))]
 

diff --git a/neqo-crypto/tests/agent.rs b/neqo-crypto/tests/agent.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 use neqo_crypto::{

diff --git a/neqo-crypto/tests/ext.rs b/neqo-crypto/tests/ext.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 use neqo_crypto::constants::{HandshakeMessage, TLS_HS_CLIENT_HELLO, TLS_HS_ENCRYPTED_EXTENSIONS};

diff --git a/neqo-crypto/tests/hkdf.rs b/neqo-crypto/tests/hkdf.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 use neqo_crypto::constants::{

diff --git a/neqo-crypto/tests/hp.rs b/neqo-crypto/tests/hp.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 use neqo_crypto::constants::{

diff --git a/neqo-crypto/tests/init.rs b/neqo-crypto/tests/init.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 // This uses external interfaces to neqo_crypto rather than being a module

diff --git a/neqo-crypto/tests/selfencrypt.rs b/neqo-crypto/tests/selfencrypt.rs
@@ -1,4 +1,3 @@
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 #![cfg(not(feature = "fuzzing"))]
 

diff --git a/neqo-http3/src/lib.rs b/neqo-http3/src/lib.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 
 /*!
@@ -397,7 +396,7 @@ impl From<AppError> for Error {
 }
 
 impl ::std::error::Error for Error {
-    fn source(&self) -> Option<&(dyn ::std::error::Error + 'static)> {
+    fn source(&self) -> Option<&(dyn::std::error::Error + 'static)> {
         match self {
             Self::TransportError(e) => Some(e),
             Self::QpackError(e) => Some(e),

diff --git a/neqo-interop/src/main.rs b/neqo-interop/src/main.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use neqo_common::{event::Provider, hex, Datagram};

diff --git a/neqo-qpack/src/lib.rs b/neqo-qpack/src/lib.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 // This is because of Encoder and Decoder structs. TODO: think about a better namings for crate and structs.
 #![allow(clippy::module_name_repetitions)]

diff --git a/neqo-server/src/main.rs b/neqo-server/src/main.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use std::cell::RefCell;

diff --git a/neqo-server/src/old_https.rs b/neqo-server/src/old_https.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use std::cell::RefCell;

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/fuzzing.rs b/neqo-transport/src/connection/tests/fuzzing.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::pedantic)]
 #![cfg(feature = "fuzzing")]
 

diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs
@@ -11,6 +11,9 @@ use super::{
     send_something, AT_LEAST_PTO, DEFAULT_RTT, DEFAULT_STREAM_DATA, POST_HANDSHAKE_CWND,
 };
 use crate::cc::CWND_MIN;
+use crate::connection::test_internal::FrameWriter;
+use crate::frame::FRAME_TYPE_ACK;
+use crate::packet::PacketBuilder;
 use crate::path::PATH_MTU_V6;
 use crate::recovery::{
     FAST_PTO_SCALE, MAX_OUTSTANDING_UNACK, MIN_OUTSTANDING_UNACK, PTO_PACKET_COUNT,
@@ -19,6 +22,8 @@ use crate::rtt::GRANULARITY;
 use crate::stats::MAX_PTO_COUNTS;
 use crate::tparams::TransportParameter;
 use crate::tracking::DEFAULT_ACK_DELAY;
+use crate::ConnectionError;
+use crate::Error;
 use crate::StreamType;
 
 use neqo_common::qdebug;
@@ -808,3 +813,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: ConnectionError::Transport(Error::AckedUnsentPacket),
+            ..
+        }
+    ));
+}
diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs
@@ -4,7 +4,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-#![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(clippy::use_self)]
 
 use neqo_common::qinfo;
@@ -176,7 +175,7 @@ impl From<std::num::TryFromIntError> for Error {
 }
 
 impl ::std::error::Error for Error {
-    fn source(&self) -> Option<&(dyn ::std::error::Error + 'static)> {
+    fn source(&self) -> Option<&(dyn::std::error::Error + 'static)> {
         match self {
             Self::CryptoError(e) => Some(e),
             _ => None,