From 2a636c1800a45316bcf346d6b69fa6b20ebca1f1 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Wed, 10 Jan 2024 22:11:23 -0800 Subject: [PATCH] Send path validation responses to the correct remote --- quinn-proto/src/connection/mod.rs | 36 +++++++++++++++++-- quinn-proto/src/connection/paths.rs | 56 +++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 73660874c..d420b44d6 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -796,6 +796,38 @@ impl Connection { break; } + // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that path + // validation can occur while the link is saturated. + if space_id == SpaceId::Data && num_datagrams == 1 { + if let Some((token, remote)) = self.path_responses.pop_off_path(&self.path.remote) { + // `unwrap` guaranteed to succeed because `builder_storage` was populated just + // above. + let mut builder = builder_storage.take().unwrap(); + trace!("PATH_RESPONSE {:08x} (off-path)", token); + buf.write(frame::Type::PATH_RESPONSE); + buf.write(token); + self.stats.frame_tx.path_response += 1; + builder.pad_to(MIN_INITIAL_SIZE); + builder.finish_and_track( + now, + self, + Some(SentFrames { + non_retransmits: true, + ..SentFrames::default() + }), + buf, + ); + self.stats.udp_tx.on_sent(1, buf.len()); + return Some(Transmit { + destination: remote, + size: buf.len(), + ecn: None, + segment_size: None, + src_ip: self.local_ip, + }); + } + } + let sent = self.populate_packet( now, space_id, @@ -2583,7 +2615,7 @@ impl Connection { close = Some(reason); } Frame::PathChallenge(token) => { - self.path_responses.push(number, token); + self.path_responses.push(number, token, remote); if remote == self.path.remote { // PATH_CHALLENGE on active path, possible off-path packet forwarding // attack. Send a non-probing packet to recover the active path. @@ -3003,7 +3035,7 @@ impl Connection { // PATH_RESPONSE if buf.len() + 9 < max_size && space_id == SpaceId::Data { - if let Some(token) = self.path_responses.pop() { + if let Some(token) = self.path_responses.pop_on_path(&self.path.remote) { sent.non_retransmits = true; sent.requires_padding = true; trace!("PATH_RESPONSE {:08x}", token); diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 03cdffb10..22d8f1416 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -1,5 +1,7 @@ use std::{cmp, net::SocketAddr, time::Duration, time::Instant}; +use tracing::trace; + use super::{mtud::MtuDiscovery, pacing::Pacer}; use crate::{config::MtuDiscoveryConfig, congestion, packet::SpaceId, TIMER_GRANULARITY}; @@ -167,27 +169,67 @@ impl RttEstimator { #[derive(Default)] pub(crate) struct PathResponses { - pending: Option, + pending: Vec, } impl PathResponses { - pub(crate) fn push(&mut self, packet: u64, token: u64) { - if self.pending.as_ref().map_or(true, |x| x.packet <= packet) { - self.pending = Some(PathResponse { packet, token }); + pub(crate) fn push(&mut self, packet: u64, token: u64, remote: SocketAddr) { + /// Arbitrary permissive limit to prevent abuse + const MAX_PATH_RESPONSES: usize = 16; + let response = PathResponse { + packet, + token, + remote, + }; + let existing = self.pending.iter_mut().find(|x| x.remote == remote); + if let Some(existing) = existing { + // Update a queued response + if existing.packet <= packet { + *existing = response; + } + return; + } + if self.pending.len() < MAX_PATH_RESPONSES { + self.pending.push(response); + } else { + // We don't expect to ever hit this with well-behaved peers, so we don't bother dropping + // older challenges. + trace!("ignoring excessive PATH_CHALLENGE"); } } - pub(crate) fn pop(&mut self) -> Option { - Some(self.pending.take()?.token) + pub(crate) fn pop_off_path(&mut self, remote: &SocketAddr) -> Option<(u64, SocketAddr)> { + let response = *self.pending.last()?; + if response.remote == *remote { + // We don't bother searching further because we expect that the on-path response will + // get drained in the immediate future by a call to `pop_on_path` + return None; + } + self.pending.pop(); + Some((response.token, response.remote)) + } + + pub(crate) fn pop_on_path(&mut self, remote: &SocketAddr) -> Option { + let response = *self.pending.last()?; + if response.remote != *remote { + // We don't bother searching further because we expect that the off-path response will + // get drained in the immediate future by a call to `pop_off_path` + return None; + } + self.pending.pop(); + Some(response.token) } pub(crate) fn is_empty(&self) -> bool { - self.pending.is_none() + self.pending.is_empty() } } +#[derive(Copy, Clone)] struct PathResponse { /// The packet number the corresponding PATH_CHALLENGE was received in packet: u64, token: u64, + /// The address the corresponding PATH_CHALLENGE was received from + remote: SocketAddr, }