From 587963284a133b6e478944d2bd5d001594a13462 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Tue, 13 Oct 2026 10:11:03 -0700 Subject: [PATCH 1/2] fix(sandbox): distinguish descriptor lookup outcomes Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- crates/openshell-sandbox/src/linux/proc_fd.rs | 51 +++++-- .../src/linux/socket_registry.rs | 136 ++++++++++++++++++ .../openshell-sandbox/src/network_broker.rs | 92 ++++++++++-- 3 files changed, 261 insertions(+), 18 deletions(-) diff --git a/crates/openshell-sandbox/src/linux/proc_fd.rs b/crates/openshell-sandbox/src/linux/proc_fd.rs index 4306f6d79a..dd5624f6bd 100644 --- a/crates/openshell-sandbox/src/linux/proc_fd.rs +++ b/crates/openshell-sandbox/src/linux/proc_fd.rs @@ -59,6 +59,19 @@ pub fn installed_socket_inodes_excluding( /// notification, task generation, and any retained socket cookie before a /// state-changing operation. pub fn socket_inode(tid: u32, fd: RawFd) -> io::Result { + socket_inode_if_socket(tid, fd)?.ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "procfs descriptor is not a socket", + ) + }) +} + +/// Return `None` only after procfs confirms that the descriptor is not a socket. +/// +/// An unreadable or missing descriptor remains an error so callers cannot +/// mistake failed inspection for a safe native socket operation. +pub fn socket_inode_if_socket(tid: u32, fd: RawFd) -> io::Result> { if tid == 0 || fd < 0 { return Err(io::Error::new( io::ErrorKind::InvalidInput, @@ -74,20 +87,24 @@ pub fn socket_inode(tid: u32, fd: RawFd) -> io::Result { })?; let digits = target .strip_prefix("socket:[") - .and_then(|value| value.strip_suffix(']')) - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidInput, - "procfs descriptor is not a socket", - ) - })?; + .and_then(|value| value.strip_suffix(']')); + let Some(digits) = digits else { + return if target.starts_with("socket:") { + Err(io::Error::new( + io::ErrorKind::InvalidData, + "procfs socket inode has an invalid representation", + )) + } else { + Ok(None) + }; + }; if digits.is_empty() || !digits.bytes().all(|byte| byte.is_ascii_digit()) { return Err(io::Error::new( io::ErrorKind::InvalidData, "procfs socket inode has an invalid representation", )); } - digits.parse::().map_err(|error| { + digits.parse::().map(Some).map_err(|error| { io::Error::new( io::ErrorKind::InvalidData, format!("procfs socket inode does not fit u64: {error}"), @@ -107,8 +124,16 @@ mod tests { fn identifies_socket_and_rejects_regular_file() { let (left, _right) = UnixStream::pair().expect("socketpair"); assert!(socket_inode(std::process::id(), left.as_raw_fd()).unwrap() > 0); + assert_eq!( + socket_inode_if_socket(std::process::id(), left.as_raw_fd()).unwrap(), + Some(socket_inode(std::process::id(), left.as_raw_fd()).unwrap()) + ); let file = File::open("/dev/null").expect("open regular descriptor"); + assert_eq!( + socket_inode_if_socket(std::process::id(), file.as_raw_fd()).unwrap(), + None + ); assert_eq!( socket_inode(std::process::id(), file.as_raw_fd()) .expect_err("regular descriptor") @@ -117,6 +142,16 @@ mod tests { ); } + #[test] + fn missing_descriptor_is_not_classified_as_non_socket() { + assert_eq!( + socket_inode_if_socket(std::process::id(), i32::MAX) + .expect_err("closed descriptor cannot be inspected") + .raw_os_error(), + Some(libc::ENOENT) + ); + } + #[test] fn installed_socket_snapshot_can_exclude_the_broker() { let (left, _right) = UnixStream::pair().expect("socketpair"); diff --git a/crates/openshell-sandbox/src/linux/socket_registry.rs b/crates/openshell-sandbox/src/linux/socket_registry.rs index 8a10759320..b7e2dbac27 100644 --- a/crates/openshell-sandbox/src/linux/socket_registry.rs +++ b/crates/openshell-sandbox/src/linux/socket_registry.rs @@ -301,6 +301,25 @@ impl SocketRegistry { Ok(entry) } + /// Return `None` only when procfs positively identifies an unregistered + /// descriptor. Inspection and retained-identity failures stay errors. + pub fn resolve_for_native_fallback( + &self, + tid: u32, + fd: RawFd, + ) -> io::Result> { + let Some(inode) = proc_fd::socket_inode_if_socket(tid, fd)? else { + return Ok(None); + }; + let Some(entry) = self.entries.get(&inode) else { + return Ok(None); + }; + if entry.retained_preconnect.is_some() { + entry.validate_retained_identity()?; + } + Ok(Some(entry)) + } + /// Mutable form of [`Self::resolve`]. pub fn resolve_mut(&mut self, tid: u32, fd: RawFd) -> io::Result<&mut SocketEntry> { let inode = proc_fd::socket_inode(tid, fd)?; @@ -316,6 +335,24 @@ impl SocketRegistry { Ok(entry) } + /// Mutable form of [`Self::resolve_for_native_fallback`]. + pub fn resolve_mut_for_native_fallback( + &mut self, + tid: u32, + fd: RawFd, + ) -> io::Result> { + let Some(inode) = proc_fd::socket_inode_if_socket(tid, fd)? else { + return Ok(None); + }; + let Some(entry) = self.entries.get_mut(&inode) else { + return Ok(None); + }; + if entry.retained_preconnect.is_some() { + entry.validate_retained_identity()?; + } + Ok(Some(entry)) + } + /// Remove metadata after descendant-FD collection proves no installed /// alias remains. pub fn remove_inode(&mut self, inode: u64) -> bool { @@ -347,6 +384,9 @@ fn socket_identity(fd: BorrowedFd<'_>, listener_generation: u64) -> io::Result() { + let registry = SocketRegistry::new(7, 1).unwrap(); + assert_eq!( + registry + .resolve_for_native_fallback(child.id(), fd) + .expect_err("unreadable descriptor must fail closed") + .kind(), + io::ErrorKind::PermissionDenied + ); + child.stdin.take().unwrap().write_all(&[1]).unwrap(); + break; + } + } + assert!(child.wait().unwrap().success()); + } + #[test] fn dup_alias_resolves_to_same_open_file_description() { let mut registry = SocketRegistry::new(9, 4).unwrap(); diff --git a/crates/openshell-sandbox/src/network_broker.rs b/crates/openshell-sandbox/src/network_broker.rs index dd93ed95f1..e7781a5135 100644 --- a/crates/openshell-sandbox/src/network_broker.rs +++ b/crates/openshell-sandbox/src/network_broker.rs @@ -845,7 +845,10 @@ fn connect_socket( return listener.respond_value(notification.id, 0); } } - if lock(®istry).resolve(notification.tid, fd).is_ok() { + if lock(®istry) + .resolve_for_native_fallback(notification.tid, fd)? + .is_some() + { // Every registered descriptor is an injected INET socket. Never // CONTINUE based on a mutable workload sockaddr for such an FD. return Err(io::Error::from_raw_os_error(libc::EAFNOSUPPORT)); @@ -1262,7 +1265,10 @@ fn bind_socket( ) -> io::Result<()> { let fd = raw_fd(notification.args[0])?; if !socket_address_is_inet(notification.tid, notification.args[1], notification.args[2])? { - if lock(registry).resolve(notification.tid, fd).is_ok() { + if lock(registry) + .resolve_for_native_fallback(notification.tid, fd)? + .is_some() + { return Err(io::Error::from_raw_os_error(libc::EAFNOSUPPORT)); } return listener.respond_continue(notification.id); @@ -1322,7 +1328,7 @@ fn listen_socket( let fd = raw_fd(notification.args[0])?; let backlog = i32::try_from(notification.args[1]).unwrap_or(i32::MAX); let mut registry = lock(registry); - let Ok(entry) = registry.resolve_mut(notification.tid, fd) else { + let Some(entry) = registry.resolve_mut_for_native_fallback(notification.tid, fd)? else { return listener.respond_continue(notification.id); }; if matches!(entry.state(), SocketState::Listening { .. }) { @@ -1357,7 +1363,7 @@ fn classify_send( } let (state, metadata) = { let registry = lock(registry); - let Ok(entry) = registry.resolve(notification.tid, fd) else { + let Some(entry) = registry.resolve_for_native_fallback(notification.tid, fd)? else { // Non-INET sockets are never injected into the registry. Leave // their native sendmsg/control-message semantics to the kernel. return listener.respond_continue(notification.id); @@ -1380,9 +1386,9 @@ fn classify_send( }; let mut registry = lock(registry); - let resolution = registry.resolve(notification.tid, fd); + let resolution = registry.resolve_for_native_fallback(notification.tid, fd)?; match resolution { - Ok(entry) + Some(entry) if entry.metadata().kind == InetKind::DnsUdp && matches!(entry.state(), SocketState::Local { .. }) => { @@ -1392,7 +1398,7 @@ fn classify_send( Err(io::Error::from_raw_os_error(libc::EACCES)) } } - Ok(entry) if matches!(entry.state(), SocketState::DnsUdp { .. }) => { + Some(entry) if matches!(entry.state(), SocketState::DnsUdp { .. }) => { let SocketState::DnsUdp { relay } = entry.state() else { unreachable!("guard requires DNS UDP state"); }; @@ -1413,7 +1419,7 @@ fn classify_send( Err(io::Error::from_raw_os_error(libc::EACCES)) } } - Ok(entry) + Some(entry) if entry.metadata().kind == InetKind::DnsUdp && matches!( entry.state(), @@ -1444,12 +1450,12 @@ fn classify_send( entry.release_preconnect(); listener.respond_continue(notification.id) } - Ok(_) => Err(io::Error::from_raw_os_error(libc::EDESTADDRREQ)), + Some(_) => Err(io::Error::from_raw_os_error(libc::EDESTADDRREQ)), // Non-INET sockets and natively accepted sockets were never // registered. Accepted sockets inherit their listener's loopback // binding, and the mandatory outer fence remains an independent // backstop against an external kernel route. - Err(_) => listener.respond_continue(notification.id), + None => listener.respond_continue(notification.id), } } @@ -2112,6 +2118,59 @@ mod tests { ))); } + #[test] + #[allow(unsafe_code)] + fn unreadable_workload_socket_cannot_use_native_fallback() { + const CHILD: &str = "OPENSHELL_UNREADABLE_WORKLOAD_TEST_CHILD"; + const TARGET: &str = "OPENSHELL_UNREADABLE_WORKLOAD_TEST_TARGET"; + if std::env::var_os(CHILD).is_some() { + let destination: SocketAddr = std::env::var(TARGET).unwrap().parse().unwrap(); + let udp = UdpSocket::bind("127.0.0.1:0").unwrap(); + let tcp = + socket2::Socket::new(socket2::Domain::IPV4, socket2::Type::STREAM, None).unwrap(); + // SAFETY: this disposable subprocess changes only its dumpability. + assert_eq!(unsafe { libc::prctl(libc::PR_SET_DUMPABLE, 0, 0, 0, 0) }, 0); + assert_eq!( + udp.send_to(b"test", destination) + .unwrap_err() + .raw_os_error(), + Some(libc::EACCES) + ); + assert_eq!( + tcp.listen(1).unwrap_err().raw_os_error(), + Some(libc::EACCES) + ); + return; + } + if rustix::process::geteuid().is_root() { + return; + } + + let receiver = UdpSocket::bind("127.0.0.1:0").unwrap(); + receiver.set_nonblocking(true).unwrap(); + let destination = receiver.local_addr().unwrap(); + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let _broker = NetworkBroker::start_for_test(listener).unwrap(); + let executable = std::env::current_exe().unwrap(); + let mut child = launcher + .execute(move || { + std::process::Command::new(executable) + .arg("--exact") + .arg("network_broker::tests::unreadable_workload_socket_cannot_use_native_fallback") + .arg("--nocapture") + .env(CHILD, "1") + .env(TARGET, destination.to_string()) + .spawn() + }) + .unwrap() + .unwrap(); + assert!(child.wait().unwrap().success()); + assert_eq!( + receiver.recv_from(&mut [0_u8; 4]).unwrap_err().kind(), + io::ErrorKind::WouldBlock + ); + } + #[test] fn relay_rejects_descriptor_replaced_after_policy_decision() { let metadata = SocketMetadata { @@ -2444,6 +2503,19 @@ mod tests { client.join().expect("join client").expect("Unix client"); } + #[test] + fn unix_listen_remains_kernel_driven() { + let directory = tempfile::tempdir().expect("temporary Unix socket directory"); + let path = directory.path().join("service.sock"); + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let _broker = NetworkBroker::start_for_test(listener).unwrap(); + let service = launcher + .execute(move || UnixListener::bind(path)) + .unwrap() + .unwrap(); + assert!(service.local_addr().unwrap().as_pathname().is_some()); + } + #[test] fn native_accept_inherits_loopback_confinement() { let (launcher, listener) = From 724ea2e42c8c7217080acf51ebad18088691ebd4 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Wed, 14 Oct 2026 10:26:51 -0700 Subject: [PATCH 2/2] fix(sandbox): preserve bad-descriptor errors in native fallback Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- crates/openshell-sandbox/src/linux/proc_fd.rs | 18 ++++++++++++++++- .../src/linux/socket_registry.rs | 9 ++++++++- .../openshell-sandbox/src/network_broker.rs | 20 +++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/crates/openshell-sandbox/src/linux/proc_fd.rs b/crates/openshell-sandbox/src/linux/proc_fd.rs index dd5624f6bd..729b60e03f 100644 --- a/crates/openshell-sandbox/src/linux/proc_fd.rs +++ b/crates/openshell-sandbox/src/linux/proc_fd.rs @@ -78,7 +78,17 @@ pub fn socket_inode_if_socket(tid: u32, fd: RawFd) -> io::Result> { "TID must be nonzero and FD must be nonnegative", )); } - let target = fs::read_link(format!("/proc/{tid}/fd/{fd}"))?; + let target = match fs::read_link(format!("/proc/{tid}/fd/{fd}")) { + Ok(target) => target, + Err(error) if error.raw_os_error() == Some(libc::ENOENT) => { + // A missing descriptor should keep the syscall's EBADF result. + // Check the parent so a vanished task or inaccessible procfs does + // not get mistaken for a closed descriptor. + fs::metadata(format!("/proc/{tid}/fd"))?; + return Err(io::Error::from_raw_os_error(libc::EBADF)); + } + Err(error) => return Err(error), + }; let target = target.to_str().ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, @@ -148,6 +158,12 @@ mod tests { socket_inode_if_socket(std::process::id(), i32::MAX) .expect_err("closed descriptor cannot be inspected") .raw_os_error(), + Some(libc::EBADF) + ); + assert_eq!( + socket_inode_if_socket(u32::MAX, 3) + .expect_err("missing task must remain an inspection error") + .raw_os_error(), Some(libc::ENOENT) ); } diff --git a/crates/openshell-sandbox/src/linux/socket_registry.rs b/crates/openshell-sandbox/src/linux/socket_registry.rs index b7e2dbac27..1983b038d9 100644 --- a/crates/openshell-sandbox/src/linux/socket_registry.rs +++ b/crates/openshell-sandbox/src/linux/socket_registry.rs @@ -499,7 +499,14 @@ mod tests { .resolve_for_native_fallback(std::process::id(), i32::MAX) .expect_err("vanished descriptor must fail closed") .raw_os_error(), - Some(libc::ENOENT) + Some(libc::EBADF) + ); + assert_eq!( + registry + .resolve_mut_for_native_fallback(std::process::id(), i32::MAX) + .expect_err("vanished descriptor must fail closed") + .raw_os_error(), + Some(libc::EBADF) ); } diff --git a/crates/openshell-sandbox/src/network_broker.rs b/crates/openshell-sandbox/src/network_broker.rs index e7781a5135..faaff9250d 100644 --- a/crates/openshell-sandbox/src/network_broker.rs +++ b/crates/openshell-sandbox/src/network_broker.rs @@ -2171,6 +2171,26 @@ mod tests { ); } + #[test] + #[allow(unsafe_code)] + fn closed_workload_socket_reports_bad_descriptor() { + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let _broker = NetworkBroker::start_for_test(listener).unwrap(); + let error = launcher + .execute(|| { + let socket = + socket2::Socket::new(socket2::Domain::IPV4, socket2::Type::STREAM, None) + .unwrap(); + let fd = socket.as_raw_fd(); + drop(socket); + // SAFETY: listen on a closed descriptor is a valid error probe. + assert_eq!(unsafe { libc::listen(fd, 1) }, -1); + io::Error::last_os_error().raw_os_error() + }) + .unwrap(); + assert_eq!(error, Some(libc::EBADF)); + } + #[test] fn relay_rejects_descriptor_replaced_after_policy_decision() { let metadata = SocketMetadata {