Skip to content

Commit

Permalink
fix: Check that the largest_acked was sent (mozilla#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 committed Oct 14, 2024
1 parent 80db3a0 commit 4018f2a
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 40 deletions.
2 changes: 1 addition & 1 deletion neqo-common/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ 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"
Expand Down
38 changes: 33 additions & 5 deletions neqo-transport/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[..]);
Expand All @@ -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))?;
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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.
Expand Down
35 changes: 35 additions & 0 deletions neqo-transport/src/connection/tests/recovery.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
..
}
));
}
50 changes: 16 additions & 34 deletions neqo-transport/src/recovery.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -1361,7 +1347,6 @@ mod tests {

let (_, lost) = lr.on_ack_received(
PacketNumberSpace::ApplicationData,
2,
vec![2..=2],
ACK_DELAY,
pn2_ack_time,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions neqo-transport/src/tracking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4018f2a

Please sign in to comment.