Skip to content

Commit

Permalink
fix: Check that the largest_acked was sent (#2150)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@larseggert @mxinden
larseggert and mxinden authored Oct 8, 2024
1 parent bd02153 commit 9d12a1f
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 41 deletions.
45 changes: 33 additions & 12 deletions neqo-transport/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[..]);
Expand All @@ -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))?;
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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".
Expand Down
41 changes: 39 additions & 2 deletions neqo-transport/src/connection/tests/recovery.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@ 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,
},
rtt::GRANULARITY,
stats::MAX_PTO_COUNTS,
tparams::TransportParameter,
tracking::DEFAULT_ACK_DELAY,
Pmtud, StreamType,
CloseReason, Error, Pmtud, StreamType,
};

#[test]
Expand Down Expand Up @@ -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),
..
}
));
}
38 changes: 11 additions & 27 deletions neqo-transport/src/recovery/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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());
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1299,7 +1289,6 @@ mod tests {
));
let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
1,
vec![1..=1],
None,
ACK_DELAY,
Expand All @@ -1323,7 +1312,6 @@ mod tests {

let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
2,
vec![2..=2],
None,
ACK_DELAY,
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit 9d12a1f

Please sign in to comment.