Skip to content

Commit

Permalink
only raise PeerMigrated when the connection actually migrated
Browse files Browse the repository at this point in the history 
As a server, we need to check that the new path on which the client
(seems to) migrate is actually usable before reporting to the
application that the connection migrated. This lifts the need of
the application from handling connection migration attacks.
  • Loading branch information
@qdeconinck
qdeconinck committed May 25, 2022
1 parent 6756beb commit 21ce1c7
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 68 deletions.
51 changes: 8 additions & 43 deletions quiche/src/lib.rs
View file
Expand Up @@ -1607,6 +1607,7 @@ impl Connection {
let path_mgr = path::PathManager::new(
path,
config.local_transport_params.active_conn_id_limit as usize,
is_server,
config.events,
);

Expand Down Expand Up @@ -2750,40 +2751,8 @@ impl Connection {
recv_pid != active_path_id &&
self.pkt_num_spaces[epoch].largest_rx_non_probing_pkt_num == pn
{
self.path_mgr.set_active_path(recv_pid)?;
let recv_path = self.path_mgr.get_mut(recv_pid)?;
// Requests path validation if needed.
if !recv_path.validated() && !recv_path.under_validation() {
recv_path.request_validation();
}

let local_addr = recv_path.local_addr();
let peer_addr = recv_path.peer_addr();
let no_spare_dcid = recv_path.active_dcid_seq.is_none();

// Notify the application.
self.path_mgr.notify_event(path::PathEvent::PeerMigrated(
local_addr, peer_addr,
));

if no_spare_dcid {
if self.disable_dcid_reuse {
warn!(
"Connection migrated on a new path but no spare \
DCID; waiting for a new one"
);
} else {
self.path_mgr.get_mut(recv_pid)?.active_dcid_seq = self
.path_mgr
.get_mut(active_path_id)?
.active_dcid_seq
.take();
warn!(
"Connection migrated on a new path but no spare \
DCID; using the previous active path's one"
);
}
}
self.path_mgr
.on_peer_migrated(recv_pid, self.disable_dcid_reuse)?;
}
}

Expand Down Expand Up @@ -6486,7 +6455,7 @@ impl Connection {
},

frame::Frame::PathResponse { data } => {
self.path_mgr.on_response_received(data);
self.path_mgr.on_response_received(data)?;
},

frame::Frame::ConnectionClose {
Expand Down Expand Up @@ -14040,11 +14009,11 @@ mod tests {
);
assert_eq!(
pipe.server.poll_path(),
Ok(PathEvent::PeerMigrated(server_addr, client_addr_3))
Ok(PathEvent::Validated(server_addr, client_addr_3))
);
assert_eq!(
pipe.server.poll_path(),
Ok(PathEvent::Validated(server_addr, client_addr_3))
Ok(PathEvent::PeerMigrated(server_addr, client_addr_3))
);
assert_eq!(pipe.server.poll_path(), Err(Error::Done));
assert_eq!(
Expand Down Expand Up @@ -14181,11 +14150,11 @@ mod tests {
);
assert_eq!(
pipe.server.poll_path(),
Ok(PathEvent::PeerMigrated(server_addr, client_addr_2))
Ok(PathEvent::Validated(server_addr, client_addr_2))
);
assert_eq!(
pipe.server.poll_path(),
Ok(PathEvent::Validated(server_addr, client_addr_2))
Ok(PathEvent::PeerMigrated(server_addr, client_addr_2))
);
assert_eq!(pipe.server.poll_path(), Err(Error::Done));
assert_eq!(
Expand Down Expand Up @@ -14336,10 +14305,6 @@ mod tests {
pipe.server.poll_path(),
Ok(PathEvent::New(server_addr, spoofed_client_addr))
);
assert_eq!(
pipe.server.poll_path(),
Ok(PathEvent::PeerMigrated(server_addr, spoofed_client_addr))
);

assert_eq!(
pipe.server.is_path_validated(server_addr, client_addr),
Expand Down
116 changes: 91 additions & 25 deletions quiche/src/path.rs
View file
Expand Up @@ -96,17 +96,7 @@ pub enum PathEvent {
/// denoted by the pair of `SocketAddr`, i.e., non-probing packets have been
/// received on this network path. This is a server side only event.
///
/// Note that this does not imply that this network path has been validated,
/// but `quiche` automatically triggers a path validation on this new path
/// if needed. The connection has fully migrated if either
///
/// * it later receives a `Validated` event with the same addresses, or
///
/// * the path was already validated before.
///
/// If the path validation fails, it receives a `FailedValidation` event
/// containing the path addresses and the host migrates back to the
/// previously active working path.
/// Note that this event is only raised if the path has been validated.
PeerMigrated(SocketAddr, SocketAddr),
}

Expand Down Expand Up @@ -167,6 +157,9 @@ pub struct Path {
challenge_requested: bool,
/// Whether the failure of this path was notified.
failure_notified: bool,
/// Whether the connection tries to migrate to this path, but it still needs
/// to be validated.
migrating: bool,
}

impl Path {
Expand Down Expand Up @@ -205,6 +198,7 @@ impl Path {
peer_verified_local_address: false,
challenge_requested: false,
failure_notified: false,
migrating: false,
}
}

Expand Down Expand Up @@ -457,13 +451,17 @@ pub struct PathManager {

/// Path-specific events to be notified to the application.
events: Option<VecDeque<PathEvent>>,

/// Whether this manager serves a connection as a server.
is_server: bool,
}

impl PathManager {
/// Creates a new `PathManager` with the initial provided `path` and a
/// capacity limit.
pub fn new(
mut initial_path: Path, max_concurrent_paths: usize, enable_events: bool,
mut initial_path: Path, max_concurrent_paths: usize, is_server: bool,
enable_events: bool,
) -> Self {
let local_addr = initial_path.local_addr;
let peer_addr = initial_path.peer_addr;
Expand All @@ -485,6 +483,7 @@ impl PathManager {
addrs_to_paths,

events,
is_server,
}
}

Expand Down Expand Up @@ -698,32 +697,94 @@ impl PathManager {
}

/// Handles incoming PATH_RESPONSE data.
pub fn on_response_received(&mut self, data: [u8; 8]) {
if let Some((_, p)) = self
pub fn on_response_received(&mut self, data: [u8; 8]) -> Result<()> {
let active_pid = self.get_active_path_id()?;
if let Some((pid, p)) = self
.paths_mut()
.find(|(_, p)| p.has_pending_challenge(data))
{
if p.on_response_received(data) {
let local_addr = p.local_addr;
let peer_addr = p.peer_addr;
let was_migrating = p.migrating;

p.migrating = false;

// Notifies the application.
self.notify_event(PathEvent::Validated(local_addr, peer_addr));

// If this path was the candidate for migration, notifies the
// application.
if pid == active_pid && was_migrating {
self.notify_event(PathEvent::PeerMigrated(
local_addr, peer_addr,
));
}
}
}
Ok(())
}

/// Sets the path with identifier 'path_id' to be active.
///
/// There can be exactly one active path on which non-probing packets can be
/// sent. If another path is marked as active, it will be superseeded by the
/// one having `path_id` as identfier.
/// one having `path_id` as identifier.
///
/// A server should always ensure that the active path is validated. If it
/// is already the case, it notifies the application that the connection
/// migrated. Otherwise, it triggers a path validation and defers the
/// notification once it is actually validated.
pub fn set_active_path(&mut self, path_id: usize) -> Result<()> {
if let Ok(old_active_path) = self.get_active_mut() {
old_active_path.active = false;
}
let is_server = self.is_server;
let new_active_path = self.get_mut(path_id)?;
new_active_path.active = true;
if is_server {
if new_active_path.validated() {
let local_addr = new_active_path.local_addr();
let peer_addr = new_active_path.peer_addr();
self.notify_event(PathEvent::PeerMigrated(local_addr, peer_addr));
} else {
new_active_path.migrating = true;
// Requests path validation if needed.
if !new_active_path.under_validation() {
new_active_path.request_validation();
}
}
}
Ok(())
}

/// Handles potential connection migration.
pub fn on_peer_migrated(
&mut self, new_pid: usize, disable_dcid_reuse: bool,
) -> Result<()> {
let active_path_id = self.get_active_path_id()?;
if active_path_id == new_pid {
return Ok(());
}
self.set_active_path(new_pid)?;

let no_spare_dcid = self.get_mut(new_pid)?.active_dcid_seq.is_none();
if no_spare_dcid {
if disable_dcid_reuse {
trace!(
"Connection migrated on a new path but no spare \
DCID; waiting for a new one"
);
} else {
self.get_mut(new_pid)?.active_dcid_seq =
self.get_mut(active_path_id)?.active_dcid_seq;
trace!(
"Connection migrated on a new path but no spare \
DCID; using the previous active path's one"
);
}
}

Ok(())
}
}
Expand Down Expand Up @@ -837,16 +898,21 @@ mod tests {
#[test]
fn path_validation_limited_mtu() {
let client_addr = "127.0.0.1:1234".parse().unwrap();
let client_addr_2 = "127.0.0.1:5678".parse().unwrap();
let server_addr = "127.0.0.1:4321".parse().unwrap();

let config = Config::new(crate::PROTOCOL_VERSION).unwrap();
let recovery_config = RecoveryConfig::from_config(&config);

let path = Path::new(client_addr, server_addr, &recovery_config, false);
let mut path_mgr = PathManager::new(path, 2, true);
let path = Path::new(client_addr, server_addr, &recovery_config, true);
let mut path_mgr = PathManager::new(path, 2, false, true);

let probed_path =
Path::new(client_addr_2, server_addr, &recovery_config, false);
path_mgr.insert_path(probed_path, false).unwrap();

let pid = path_mgr
.path_id_from_addrs(&(client_addr, server_addr))
.path_id_from_addrs(&(client_addr_2, server_addr))
.unwrap();
path_mgr.get_mut(pid).unwrap().request_validation();
assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), true);
Expand All @@ -873,7 +939,7 @@ mod tests {

// Receives the response. The path is reachable, but the MTU is not
// validated yet.
path_mgr.on_response_received(data);
path_mgr.on_response_received(data).unwrap();

assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), true);
assert_eq!(path_mgr.get_mut(pid).unwrap().probing_required(), true);
Expand All @@ -897,7 +963,7 @@ mod tests {
)
.unwrap();

path_mgr.on_response_received(data);
path_mgr.on_response_received(data).unwrap();

assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), false);
assert_eq!(path_mgr.get_mut(pid).unwrap().probing_required(), false);
Expand All @@ -906,7 +972,7 @@ mod tests {
assert_eq!(path_mgr.get_mut(pid).unwrap().state, PathState::Validated);
assert_eq!(
path_mgr.pop_event(),
Some(PathEvent::Validated(client_addr, server_addr))
Some(PathEvent::Validated(client_addr_2, server_addr))
);
}

Expand All @@ -918,8 +984,8 @@ mod tests {
let config = Config::new(crate::PROTOCOL_VERSION).unwrap();
let recovery_config = RecoveryConfig::from_config(&config);

let path = Path::new(client_addr, server_addr, &recovery_config, false);
let mut client_path_mgr = PathManager::new(path, 2, true);
let path = Path::new(client_addr, server_addr, &recovery_config, true);
let mut client_path_mgr = PathManager::new(path, 2, false, true);
let mut server_path =
Path::new(server_addr, client_addr, &recovery_config, false);

Expand Down Expand Up @@ -965,7 +1031,7 @@ mod tests {
assert_eq!(server_path.received_challenges.len(), 2);

// Response for first probe.
client_path_mgr.on_response_received(data);
client_path_mgr.on_response_received(data).unwrap();
assert_eq!(
client_path_mgr
.get(client_pid)
Expand All @@ -976,7 +1042,7 @@ mod tests {
);

// Response for second probe.
client_path_mgr.on_response_received(data_2);
client_path_mgr.on_response_received(data_2).unwrap();
assert_eq!(
client_path_mgr
.get(client_pid)
Expand Down

0 comments on commit 21ce1c7

Please sign in to comment.