From fce4e5aea5abf7b431f3375c8f77fbbee6349b2a Mon Sep 17 00:00:00 2001 From: "John T. Myers" <9696606+johntmyers@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:13:43 +0000 Subject: [PATCH] fix(sandbox): release broker-owned listening sockets (#4444) Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- crates/openshell-sandbox/README.md | 8 + .../src/linux/socket_registry.rs | 30 ++- .../openshell-sandbox/src/network_broker.rs | 176 +++++++++++++++++- e2e/rust/tests/listener_lifetime.rs | 154 +++++++++++++++ 4 files changed, 360 insertions(+), 8 deletions(-) create mode 100644 e2e/rust/tests/listener_lifetime.rs diff --git a/crates/openshell-sandbox/README.md b/crates/openshell-sandbox/README.md index b43b21c56..3ee57c085 100644 --- a/crates/openshell-sandbox/README.md +++ b/crates/openshell-sandbox/README.md @@ -9,3 +9,11 @@ socket identity, Landlock qualification, and the workload launch thread. the lower-level mechanisms used by that orchestration and the boundary server. Unsafe kernel ABI, process-launch, and descriptor-ownership operations stay in this runtime crate behind safe APIs where their contracts can be enforced. + +The network broker retains source socket descriptors only while preparing a +workload endpoint. A successful connection or `listen` releases that reference; +the kernel then tracks the lifetime through workload-owned descriptors. Native +`accept` and repeated `listen` preserve the socket's loopback confinement. Closing +the last workload reference to a listener removes it without waiting for registry +collection. Broker source descriptors always use close-on-exec independently of +the workload's requested descriptor flags. diff --git a/crates/openshell-sandbox/src/linux/socket_registry.rs b/crates/openshell-sandbox/src/linux/socket_registry.rs index 984e7545a..8a1075932 100644 --- a/crates/openshell-sandbox/src/linux/socket_registry.rs +++ b/crates/openshell-sandbox/src/linux/socket_registry.rs @@ -119,10 +119,15 @@ impl SocketEntry { /// Replace the stable state. Callers perform policy and notification /// revalidation before invoking this commit primitive. pub fn set_state(&mut self, state: SocketState) { + if matches!(state, SocketState::Listening { .. }) { + // Native accept needs no broker reference. Only workload aliases + // should keep a listener alive after this transition. + self.release_preconnect(); + } self.state = state; } - /// Close the temporary source descriptor after a connection commits. + /// Close the temporary source descriptor after a connection or listen commits. pub fn release_preconnect(&mut self) { self.retained_preconnect = None; } @@ -268,10 +273,7 @@ impl SocketRegistry { )); } let identity = tentative.identity; - let retain_source = matches!( - state, - SocketState::Created | SocketState::Bound { .. } | SocketState::Listening { .. } - ); + let retain_source = matches!(state, SocketState::Created | SocketState::Bound { .. }); self.entries.insert( identity.inode, SocketEntry { @@ -369,6 +371,24 @@ mod tests { } } + #[test] + fn listening_commit_does_not_retain_source() { + let mut registry = SocketRegistry::new(7, 1).unwrap(); + let source = tcp_socket(); + let installed = rustix::io::fcntl_dupfd_cloexec(&source, 3).unwrap(); + let tentative = registry.stage(source, metadata()).unwrap(); + let local = "127.0.0.1:12345".parse().unwrap(); + registry + .commit_with_state(tentative, SocketState::Listening { local }) + .unwrap(); + let entry = registry + .resolve(std::process::id(), installed.as_raw_fd()) + .unwrap(); + assert_eq!(entry.state(), &SocketState::Listening { local }); + assert!(entry.retained_preconnect().is_err()); + assert_eq!(registry.retained_preconnect_count(), 0); + } + #[test] fn tentative_entry_is_invisible_until_commit() { let mut registry = SocketRegistry::new(7, 1).unwrap(); diff --git a/crates/openshell-sandbox/src/network_broker.rs b/crates/openshell-sandbox/src/network_broker.rs index 3f7d45381..dd93ed95f 100644 --- a/crates/openshell-sandbox/src/network_broker.rs +++ b/crates/openshell-sandbox/src/network_broker.rs @@ -677,14 +677,17 @@ fn create_socket( if let Err(error) = prepare_registry_for_socket(registry, descriptor_soft_limit) { return listener.respond_errno(notification.id, error_to_errno(&error)); } + // The broker's copy must never survive workload exec, even when the + // injected descriptor is intentionally inheritable in the workload. + let source_kind = raw_kind | libc::SOCK_CLOEXEC; // SAFETY: arguments were reduced to the supported native INET matrix. A // successful call returns one newly owned descriptor. - let mut source = unsafe { libc::socket(domain, raw_kind, protocol) }; + let mut source = unsafe { libc::socket(domain, source_kind, protocol) }; if source < 0 && io::Error::last_os_error().raw_os_error() == Some(libc::EMFILE) { collect_closed_socket_entries(registry)?; // SAFETY: same validated native INET socket creation after reclaiming // broker-held descriptors for closed workload sockets. - source = unsafe { libc::socket(domain, raw_kind, protocol) }; + source = unsafe { libc::socket(domain, source_kind, protocol) }; } if source < 0 { return Err(io::Error::last_os_error()); @@ -1150,7 +1153,10 @@ fn repeated_connect_outcome( destination: SocketAddr, ) -> Option { match state { - SocketState::Created | SocketState::Bound { .. } | SocketState::Listening { .. } => None, + SocketState::Created | SocketState::Bound { .. } => None, + // A listening TCP socket cannot be connected, and no longer has a + // broker-owned descriptor on which to attempt it. + SocketState::Listening { .. } => Some(libc::EISCONN), // UDP connect replaces the association; repeating the same one succeeds. SocketState::DnsUdp { relay } if *relay == destination => Some(0), SocketState::Local { peer } if kind == InetKind::DnsUdp && *peer == destination => Some(0), @@ -1268,6 +1274,9 @@ fn bind_socket( let bind_result = { let mut registry = lock(registry); let entry = registry.resolve_mut(notification.tid, fd)?; + if matches!(entry.state(), SocketState::Listening { .. }) { + return Err(io::Error::from_raw_os_error(libc::EINVAL)); + } // A native bind is never restarted, but a notified one can be after a // signal. Report a repeat of the bind the broker completed as success. if entry.state() == &(SocketState::Bound { local }) { @@ -1316,6 +1325,11 @@ fn listen_socket( let Ok(entry) = registry.resolve_mut(notification.tid, fd) else { return listener.respond_continue(notification.id); }; + if matches!(entry.state(), SocketState::Listening { .. }) { + // Backlog changes and restarted notifications operate on the workload + // descriptor. Confinement is standing kernel state, as for accept. + return listener.respond_continue(notification.id); + } // listen(2) may be repeated natively, so a restart needs no special case. listener.validate_id(notification.id)?; // SAFETY: retained descriptor is the exact registered socket OFD. @@ -1724,6 +1738,162 @@ mod tests { use super::*; use crate::linux::socket_confinement; + fn assert_listener_closed(address: SocketAddr, inode: u64) { + let table = if address.is_ipv4() { + "/proc/net/tcp" + } else { + "/proc/net/tcp6" + }; + let inode = inode.to_string(); + assert!( + !std::fs::read_to_string(table).unwrap().lines().any(|line| { + let fields: Vec<_> = line.split_whitespace().collect(); + fields.get(3) == Some(&"0A") && fields.get(9) == Some(&inode.as_str()) + }), + "closed listener remains in {table}" + ); + assert_eq!( + TcpStream::connect_timeout(&address, Duration::from_secs(1)) + .expect_err("closed listener accepted a connection") + .raw_os_error(), + Some(libc::ECONNREFUSED) + ); + // Check disappearance and refusal before bind can trigger collection. + let _replacement = TcpListener::bind(address).expect("reuse listener port"); + } + + #[test] + fn listening_socket_lives_only_as_long_as_workload_aliases() { + for address in ["127.0.0.1:0", "[::1]:0"] { + let address: SocketAddr = address.parse().unwrap(); + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let broker = NetworkBroker::start_for_test(listener).unwrap(); + let (socket, alias, address, inode) = launcher + .execute(move || -> io::Result<_> { + let socket = socket2::Socket::new( + socket2::Domain::for_address(address), + socket2::Type::STREAM, + None, + )?; + socket.bind(&address.into())?; + socket.listen(1)?; + socket.listen(8)?; + let address = socket.local_addr()?.as_socket().unwrap(); + assert_eq!( + socket.bind(&address.into()).unwrap_err().raw_os_error(), + Some(libc::EINVAL) + ); + assert_eq!( + socket.connect(&address.into()).unwrap_err().raw_os_error(), + Some(libc::EISCONN) + ); + let inode = crate::linux::proc_fd::socket_inode( + std::process::id(), + socket.as_raw_fd(), + )?; + Ok((socket.try_clone()?, socket, address, inode)) + }) + .unwrap() + .unwrap(); + let registry = lock(&broker.registry); + let entry = registry + .resolve(std::process::id(), socket.as_raw_fd()) + .unwrap(); + assert!(matches!(entry.state(), SocketState::Listening { .. })); + assert!(entry.retained_preconnect().is_err()); + drop(registry); + drop(socket); + let _client = TcpStream::connect_timeout(&address, Duration::from_secs(1)).unwrap(); + drop(alias); + assert_listener_closed(address, inode); + } + } + + #[test] + fn implicit_listen_releases_source_and_failed_listen_retains_it() { + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let broker = NetworkBroker::start_for_test(listener).unwrap(); + let (address, inode, udp) = launcher + .execute(|| -> io::Result<_> { + let socket = + socket2::Socket::new(socket2::Domain::IPV4, socket2::Type::STREAM, None)?; + socket.listen(1)?; + let address = socket.local_addr()?.as_socket().unwrap(); + let inode = + crate::linux::proc_fd::socket_inode(std::process::id(), socket.as_raw_fd())?; + drop(socket); + let udp = socket2::Socket::new(socket2::Domain::IPV4, socket2::Type::DGRAM, None)?; + assert_eq!( + udp.listen(1).unwrap_err().raw_os_error(), + Some(libc::EOPNOTSUPP) + ); + Ok((address, inode, udp)) + }) + .unwrap() + .unwrap(); + let registry = lock(&broker.registry); + let entry = registry + .resolve(std::process::id(), udp.as_raw_fd()) + .unwrap(); + assert_eq!(entry.state(), &SocketState::Created); + assert!(entry.retained_preconnect().is_ok()); + drop(registry); + assert_listener_closed(address, inode); + } + + #[test] + fn source_cloexec_is_independent_of_workload_inheritance() { + use rustix::io::{FdFlags, fcntl_getfd}; + for close_on_exec in [false, true] { + let (launcher, listener) = crate::linux::workload_launcher::start().unwrap(); + let broker = NetworkBroker::start_for_test(listener).unwrap(); + let socket = launcher + .execute(move || -> io::Result { + let kind = + libc::SOCK_STREAM | if close_on_exec { libc::SOCK_CLOEXEC } else { 0 }; + // SAFETY: supported socket arguments; a successful result + // transfers one injected descriptor into OwnedFd. + let fd = unsafe { libc::socket(libc::AF_INET, kind, 0) }; + if fd < 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: the successful call returned one owned descriptor. + Ok(unsafe { OwnedFd::from_raw_fd(fd) }) + }) + .unwrap() + .unwrap(); + assert_eq!( + fcntl_getfd(&socket).unwrap().contains(FdFlags::CLOEXEC), + close_on_exec + ); + let registry = lock(&broker.registry); + let entry = registry + .resolve(std::process::id(), socket.as_raw_fd()) + .unwrap(); + assert!( + fcntl_getfd(entry.retained_preconnect().unwrap()) + .unwrap() + .contains(FdFlags::CLOEXEC) + ); + let inode = entry.identity().inode; + drop(registry); + let output = std::process::Command::new("/bin/sh") + .args(["-c", "for fd in /proc/$$/fd/*; do readlink \"$fd\"; done"]) + .output() + .unwrap(); + let inherited = String::from_utf8(output.stdout) + .unwrap() + .lines() + .filter(|line| *line == format!("socket:[{inode}]")) + .count(); + assert_eq!( + inherited, + usize::from(!close_on_exec), + "broker source leaked across exec" + ); + } + } + #[test] fn provider_files_are_opened_on_demand_and_replaced() { let (launcher, listener) = diff --git a/e2e/rust/tests/listener_lifetime.rs b/e2e/rust/tests/listener_lifetime.rs new file mode 100644 index 000000000..ec58a0dcd --- /dev/null +++ b/e2e/rust/tests/listener_lifetime.rs @@ -0,0 +1,154 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#![cfg(feature = "e2e")] + +use openshell_e2e::harness::sandbox::SandboxGuard; + +const LIFETIME_PROBE: &str = r" +import errno, json, os, socket, subprocess, sys + +def assert_closed(address, inode): + # Check before bind can trigger the broker's stale-descriptor collection. + for table in ('/proc/net/tcp', '/proc/net/tcp6'): + for line in open(table): + fields = line.split() + assert not (len(fields) > 9 and fields[3] == '0A' and fields[9] == str(inode)), line + with socket.socket() as client: + client.settimeout(2) + assert client.connect_ex(tuple(address)) == errno.ECONNREFUSED, address + with socket.socket() as replacement: + replacement.bind(tuple(address)) + +def inode(sock): + return int(os.readlink('/proc/self/fd/' + str(sock.fileno()))[8:-1]) + +with socket.socket() as bound: + bound.bind(('127.0.0.1', 0)) + address = bound.getsockname() +with socket.socket() as client: + client.settimeout(2) + assert client.connect_ex(address) == errno.ECONNREFUSED + +for host in ('127.0.0.1', '::1'): + family = socket.AF_INET if host == '127.0.0.1' else socket.AF_INET6 + listener = socket.socket(family) + listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + listener.bind((host, 0)) + listener.listen(1) + listener.listen(8) + address, identity = listener.getsockname(), inode(listener) + for operation, expected in ((lambda: listener.bind(address), errno.EINVAL), + (lambda: listener.connect(address), errno.EISCONN)): + try: + operation() + raise AssertionError('operation on listener unexpectedly succeeded') + except OSError as error: + assert error.errno == expected, error + alias = listener.dup() + alias.settimeout(2) + listener.close() + with socket.socket(family) as client: + client.settimeout(2) + client.connect(address) + accepted, _ = alias.accept() + with accepted: + accepted.sendall(b'ok') + assert client.recv(2) == b'ok' + alias.close() + table = '/proc/net/tcp' if family == socket.AF_INET else '/proc/net/tcp6' + for line in open(table): + fields = line.split() + assert not (len(fields) > 9 and fields[3] == '0A' and fields[9] == str(identity)), line + with socket.socket(family) as client: + client.settimeout(2) + assert client.connect_ex(address) == errno.ECONNREFUSED + with socket.socket(family) as replacement: + replacement.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + replacement.bind(address) + +# The child exits while still owning its listener. Its broker must not keep it alive. +child = r''' +import json, os, socket +s = socket.socket() +s.bind(('127.0.0.1', 0)) +s.listen() +inode = int(os.readlink('/proc/self/fd/' + str(s.fileno()))[8:-1]) +print(json.dumps([s.getsockname(), inode]), flush=True) +os._exit(0) +''' +result = subprocess.run([sys.executable, '-c', child], check=True, capture_output=True, text=True, timeout=10) +address, identity = json.loads(result.stdout) +assert_closed(address, identity) +print(json.dumps([address, identity])) +"; + +#[tokio::test] +async fn closing_or_exiting_listener_refuses_connections_and_releases_port() { + let mut sandbox = SandboxGuard::create(&[]).await.expect("create sandbox"); + let output = sandbox + .exec(&["python3", "-c", LIFETIME_PROBE]) + .await + .expect("listener lifetime probe"); + let observer = r" +import json, os, sys +_, inode = json.loads(sys.argv[1]) +for fd in os.listdir('/proc/self/fd'): + try: + target = os.readlink('/proc/self/fd/' + fd) + except FileNotFoundError: + continue + assert target != 'socket:[' + str(inode) + ']', (fd, target) +"; + sandbox + .exec(&["python3", "-c", observer, output.trim()]) + .await + .expect("subsequent exec must not inherit closed listener"); + sandbox.cleanup().await; +} + +#[tokio::test] +async fn unrelated_exec_does_not_inherit_broker_preconnect_socket() { + // Request a non-CLOEXEC socket explicitly; Python's socket constructor + // otherwise masks broker inheritance bugs by requesting SOCK_CLOEXEC. + let owner = r" +import ctypes, json, os, socket, time +libc = ctypes.CDLL(None, use_errno=True) +fd = libc.socket(socket.AF_INET, socket.SOCK_STREAM, 0) +assert fd >= 0, ctypes.get_errno() +assert os.get_inheritable(fd) +inode = int(os.readlink('/proc/self/fd/' + str(fd))[8:-1]) +with open('/sandbox/preconnect-inode.json', 'w') as f: + json.dump(inode, f) +while True: + time.sleep(60) +"; + let mut sandbox = SandboxGuard::create_detached_main(&["python3", "-c", owner]) + .await + .expect("create socket owner"); + let observer = r" +import json, os, time +path = '/sandbox/preconnect-inode.json' +for _ in range(100): + try: + with open(path) as f: + inode = json.load(f) + break + except (FileNotFoundError, json.JSONDecodeError): + time.sleep(0.1) +else: + raise AssertionError('socket owner did not publish its inode') +for fd in os.listdir('/proc/self/fd'): + try: + target = os.readlink('/proc/self/fd/' + fd) + except FileNotFoundError: + continue + assert target != 'socket:[' + str(inode) + ']', (fd, target) +print('broker socket was not inherited') +"; + sandbox + .exec(&["python3", "-c", observer]) + .await + .expect("unrelated exec inherited broker descriptor"); + sandbox.cleanup().await; +}