From 21ce1c7126605d4c70de34941003efb9041e4b83 Mon Sep 17 00:00:00 2001 From: Quentin De Coninck Date: Wed, 18 May 2022 09:54:37 +0200 Subject: [PATCH] only raise `PeerMigrated` when the connection actually migrated 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. --- quiche/src/lib.rs | 51 ++++---------------- quiche/src/path.rs | 116 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 99 insertions(+), 68 deletions(-) diff --git a/quiche/src/lib.rs b/quiche/src/lib.rs index d42d53d9d9..1372d1e694 100644 --- a/quiche/src/lib.rs +++ b/quiche/src/lib.rs @@ -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, ); @@ -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)?; } } @@ -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 { @@ -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!( @@ -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!( @@ -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), diff --git a/quiche/src/path.rs b/quiche/src/path.rs index 57100842f1..fa401fbb7b 100644 --- a/quiche/src/path.rs +++ b/quiche/src/path.rs @@ -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), } @@ -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 { @@ -205,6 +198,7 @@ impl Path { peer_verified_local_address: false, challenge_requested: false, failure_notified: false, + migrating: false, } } @@ -457,13 +451,17 @@ pub struct PathManager { /// Path-specific events to be notified to the application. events: Option>, + + /// 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; @@ -485,6 +483,7 @@ impl PathManager { addrs_to_paths, events, + is_server, } } @@ -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(()) } } @@ -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); @@ -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); @@ -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); @@ -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)) ); } @@ -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); @@ -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) @@ -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)