Skip to content

Fix: Harden TCP liveness, atomic IP assignment, and invalid network input handling #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mertcano wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Harden TCP liveness, atomic IP assignment, and invalid network input handling #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 61 additions & 13 deletions msim/src/sim/net/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<SocketAddr> {
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<NetSim>,
Expand All @@ -1019,7 +1031,7 @@ impl Endpoint {
pub fn bind_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result<Self> {
let net = plugin::simulator::<NetSim>();
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,
Expand Down Expand Up @@ -1052,7 +1064,7 @@ impl Endpoint {
pub async fn bind(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result<Self> {
let net = plugin::simulator::<NetSim>();
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 {
Expand All @@ -1076,7 +1088,7 @@ impl Endpoint {
pub fn connect_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result<Self> {
let net = plugin::simulator::<NetSim>();
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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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::<IpAddr>().unwrap();
let ip2 = "10.0.0.2".parse::<IpAddr>().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::<NetSim>().set_ip(node1.id(), ip2).unwrap_err();
assert_eq!(error.kind(), io::ErrorKind::AddrInUse);
assert_eq!(simulator::<NetSim>().get_ip(node1.id()), Some(ip1));
assert_eq!(simulator::<NetSim>().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() {
Expand Down Expand Up @@ -1617,4 +1665,4 @@ mod tests {
f.await.unwrap();
});
}
}
}
63 changes: 45 additions & 18 deletions msim/src/sim/net/network.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<IpAddr> {
Expand Down Expand Up @@ -197,16 +217,21 @@ impl Network {
mut addr: SocketAddr,
) -> io::Result<SocketAddr> {
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}"),
Expand Down Expand Up @@ -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);

Expand All @@ -281,14 +307,15 @@ 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)
.map(|node| node.sockets.get(&SocketKey(remote_addr.port(), proto)))
.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);
}
}

Expand Down Expand Up @@ -633,4 +660,4 @@ impl Mailbox {
fn accept_connect(&mut self) -> Option<SocketAddr> {
self.sync_connections.pop_front()
}
}
}