From 04d24d80afdd597295d964b18952e7ec25a6a0e2 Mon Sep 17 00:00:00 2001 From: mertcano <35747700+mertcano@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:05:29 +0300 Subject: [PATCH] Fix: Harden TCP liveness, atomic IP assignment, and invalid network input handling ### Description This PR addresses High-severity networking and liveness vulnerabilities in the `deterministic-simulator` repository, as identified during the workspace-wide security audit. **Quality & Security Defects Remediated:** * **TCP Connection Liveness (`msim/src/sim/net/network.rs`, `msim/src/sim/net/mod.rs`):** The simulator previously passed only the local TCP ID when deregistering a connection. This caused remote peer reads to hang indefinitely because they waited on their own local ID. The teardown path now accurately carries and routes both IDs, instantly waking the remote mailbox. * **Network State Integrity (`msim/src/sim/net/network.rs`):** IP conflicts (`set_ip`) previously mutated the address map before throwing a panic while the `NetSim` mutex was held, poisoning the simulator. IP assignment now strictly validates the conflict before mutation and gracefully returns typed `io::ErrorKind::AddrInUse` or `io::ErrorKind::NotFound` errors. * **Invalid Network Input Handling:** Binding to unspecified addresses without a node IP previously reached a `todo!()` macro, and empty `ToSocketAddrs` iterators triggered panics via unwrapping. These paths now gracefully return `io::ErrorKind::AddrNotAvailable` and `io::ErrorKind::InvalidInput`, respectively. --- msim/src/sim/net/mod.rs | 74 ++++++++++++++++++++++++++++++------- msim/src/sim/net/network.rs | 63 ++++++++++++++++++++++--------- 2 files changed, 106 insertions(+), 31 deletions(-) diff --git a/msim/src/sim/net/mod.rs b/msim/src/sim/net/mod.rs index 988cfe7..bf30805 100644 --- a/msim/src/sim/net/mod.rs +++ b/msim/src/sim/net/mod.rs @@ -946,9 +946,9 @@ impl NetSim { } /// Set IP address of a node. - pub fn set_ip(&self, node: NodeId, ip: IpAddr) { + pub fn set_ip(&self, node: NodeId, ip: IpAddr) -> io::Result<()> { let mut network = self.network.lock().unwrap(); - network.set_ip(node, ip); + network.set_ip(node, ip) } /// Get IP address of a node. @@ -994,6 +994,18 @@ impl NetSim { } } +// SECURITY FIX: Explicitly handle empty ToSocketAddrs iterators by returning `InvalidInput` +// instead of unwrapping, preventing panics and denial of service from malformed addresses. +fn resolve_first_addr(addr: impl ToSocketAddrs) -> io::Result { + let mut addrs = addr.to_socket_addrs()?; + addrs.next().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "address resolution returned no addresses", + ) + }) +} + /// An endpoint. pub struct Endpoint { net: Arc, @@ -1019,7 +1031,7 @@ impl Endpoint { pub fn bind_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let addr = addr.to_socket_addrs()?.next().unwrap(); + let addr = resolve_first_addr(addr)?; let addr = net.network.lock().unwrap().bind(node, proto, addr)?; let ep = Endpoint { net, @@ -1052,7 +1064,7 @@ impl Endpoint { pub async fn bind(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let addr = addr.to_socket_addrs()?.next().unwrap(); + let addr = resolve_first_addr(addr)?; net.rand_delay().await; let addr = net.network.lock().unwrap().bind(node, proto, addr)?; Ok(Endpoint { @@ -1076,7 +1088,7 @@ impl Endpoint { pub fn connect_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let peer = addr.to_socket_addrs()?.next().unwrap(); + let peer = resolve_first_addr(addr)?; let addr = if peer.ip().is_loopback() { SocketAddr::from((Ipv4Addr::LOCALHOST, 0)) } else { @@ -1112,17 +1124,21 @@ impl Endpoint { } /// Remove a tcp id number from this node. - pub fn deregister_tcp_id(&self, remote_sock: &SocketAddr, id: u32) { + pub fn deregister_tcp_id(&self, remote_sock: &SocketAddr, id: u32, remote_tcp_id: u32) { assert!( self.live_tcp_ids.lock().unwrap().remove(&id), "unknown tcp id {}", id ); - self.net - .network - .lock() - .unwrap() - .deregister_tcp_id(self.node, self.proto, remote_sock, id); + // QUALITY FIX: Added `remote_tcp_id` propagation to ensure the peer mailbox + // is correctly woken up on connection close, preventing liveness issues. + self.net.network.lock().unwrap().deregister_tcp_id( + self.node, + self.proto, + remote_sock, + id, + remote_tcp_id, + ); } /// Returns the local socket address. @@ -1153,7 +1169,7 @@ impl Endpoint { tag: u64, payload: Payload, ) -> io::Result<()> { - let dst = dst.to_socket_addrs()?.next().unwrap(); + let dst = resolve_first_addr(dst)?; self.send_to_raw(dst, tag, payload).await } @@ -1502,6 +1518,38 @@ mod tests { runtime.block_on(f).unwrap(); } + #[test] + fn set_ip_conflict_preserves_existing_mappings() { + let runtime = Runtime::new(); + let ip1 = "10.0.0.1".parse::().unwrap(); + let ip2 = "10.0.0.2".parse::().unwrap(); + let node1 = runtime.create_node().ip(ip1).build(); + let node2 = runtime.create_node().ip(ip2).build(); + + runtime.block_on(async move { + let error = simulator::().set_ip(node1.id(), ip2).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::AddrInUse); + assert_eq!(simulator::().get_ip(node1.id()), Some(ip1)); + assert_eq!(simulator::().get_ip(node2.id()), Some(ip2)); + }); + } + + #[test] + fn bind_without_node_ip_returns_error() { + let runtime = Runtime::new(); + let node = runtime.create_node().build(); + + let task = node.spawn(async move { + let error = Endpoint::bind(libc::SOCK_STREAM, "0.0.0.0:0") + .await + .err() + .unwrap(); + assert_eq!(error.kind(), io::ErrorKind::AddrNotAvailable); + }); + + runtime.block_on(task).unwrap(); + } + #[test] #[ignore] fn localhost() { @@ -1617,4 +1665,4 @@ mod tests { f.await.unwrap(); }); } -} +} \ No newline at end of file diff --git a/msim/src/sim/net/network.rs b/msim/src/sim/net/network.rs index 1466718..a56e452 100644 --- a/msim/src/sim/net/network.rs +++ b/msim/src/sim/net/network.rs @@ -146,17 +146,37 @@ impl Network { } } - pub fn set_ip(&mut self, id: NodeId, ip: IpAddr) { + pub fn set_ip(&mut self, id: NodeId, ip: IpAddr) -> io::Result<()> { debug!("set-ip: {id}: {ip}"); - let node = self.nodes.get_mut(&id).expect("node not found"); - if let Some(old_ip) = node.ip.replace(ip) { - self.addr_to_node.remove(&old_ip); + // SECURITY FIX: Validate node existence and IP conflicts before mutating the `addr_to_node` map. + // Returns typed `AddrInUse` or `NotFound` errors instead of panicking, preserving deterministic simulator liveness. + let old_ip = self + .nodes + .get(&id) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::NotFound, format!("node not found: {id}")) + })? + .ip; + + if old_ip == Some(ip) { + return Ok(()); + } + + if let Some(existing_node) = self.addr_to_node.get(&ip).copied() { + return Err(io::Error::new( + io::ErrorKind::AddrInUse, + format!("IP address {ip} is already assigned to {existing_node}"), + )); } - let old_node = self.addr_to_node.insert(ip, id); - if let Some(old_node) = old_node { - panic!("IP conflict: {ip} {old_node}"); + + if let Some(old_ip) = old_ip { + self.addr_to_node.remove(&old_ip); } - // TODO: what if we change the IP when there are opening sockets? + self.addr_to_node.insert(ip, id); + self.nodes.get_mut(&id).expect("node was checked above").ip = Some(ip); + + // TODO: what if we change the IP when there are open sockets? + Ok(()) } pub fn get_ip(&self, id: NodeId) -> Option { @@ -197,16 +217,21 @@ impl Network { mut addr: SocketAddr, ) -> io::Result { debug!("binding ({}): {addr} -> {node_id}", proto_str(proto)); - let node = self.nodes.get_mut(&node_id).expect("node not found"); - // resolve IP if unspecified + let node = self.nodes.get_mut(&node_id).ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!("node not found: {node_id}"), + ) + })?; + // Resolve an unspecified address using the node's configured IP address. if addr.ip().is_unspecified() { - if let Some(ip) = node.ip { - addr.set_ip(ip); - } else { - todo!("try to bind 0.0.0.0, but the node IP is also unspecified"); - } + // SECURITY FIX: Return `AddrNotAvailable` instead of triggering `todo!()` or panicking when binding to an unspecified address without a configured node IP. + let ip = node.ip.ok_or_else(|| { + io::Error::new(io::ErrorKind::AddrNotAvailable, "node IP is not configured") + })?; + addr.set_ip(ip); } else if addr.ip().is_loopback() { - } else if addr.ip() != node.ip.expect("node IP is unset") { + } else if node.ip != Some(addr.ip()) { return Err(io::Error::new( io::ErrorKind::AddrNotAvailable, format!("invalid address: {addr}"), @@ -261,6 +286,7 @@ impl Network { proto: libc::c_int, remote_addr: &SocketAddr, tcp_id: u32, + remote_tcp_id: u32, ) { trace!("deregistering tcp id {} for node {}", tcp_id, node); @@ -281,6 +307,7 @@ impl Network { return; }; + // SECURITY/QUALITY FIX: Retrieve the remote socket using both node ID and remote TCP ID, and wake its mailbox correctly. if let Some(socket) = self .nodes .get_mut(node_id) @@ -288,7 +315,7 @@ impl Network { .tap_none(|| debug!("No node found for {node_id}")) .flatten() { - socket.lock().unwrap().wake_tcp_connection(tcp_id); + socket.lock().unwrap().wake_tcp_connection(remote_tcp_id); } } @@ -633,4 +660,4 @@ impl Mailbox { fn accept_connect(&mut self) -> Option { self.sync_connections.pop_front() } -} +} \ No newline at end of file