From 32c195752a9336ccdf9fa78959c6ca1b7ea63ee5 Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Tue, 3 Nov 2026 00:05:29 -0500 Subject: [PATCH] fix(sandbox): document unsafe blocks and enforce SAFETY comments with clippy Every non-test unsafe block in openshell-sandbox now carries a // SAFETY: comment stating its invariant, and clippy::undocumented_unsafe_blocks is enabled at warn level in the crate roots (lib, the sandbox bin, and the seccomp-perf bin), so the CI clippy lanes with -D warnings fail the build on new undocumented blocks. Test-only blocks are exempt through cfg_attr(test, allow) to keep this change free of test-comment churn; the lint fires 89 times on main and zero times after this change. reset_child_signal_dispositions gains a # Safety section documenting its caller contract (forked pre-exec child only), and the vsock control stream reinterpreted as a UnixStream is documented in place: the family-agnostic operations it relies on, the AF_UNIX members that must never be called on it, and the dial-side twin of the same reinterpretation. Small mechanical edits were required so each comment sits on the line directly above its unsafe block (the lint is line-anchored): nine expressions hoisted into let bindings or nested ifs (five syscall probes in main.rs, two F_SETFL guards in network_broker.rs, two in boundary_server.rs), plus two reorderings (a timeout binding in network_broker.rs, the SAFETY comment placement in seccomp_notify.rs). Every movement preserves the original short-circuits and the errno adjacency the probe helpers rely on, and four new tests pin those properties: two run the self-protection probes against seccomp filters (one lets the FIOSETOWN ioctl through to the kernel, so a probe that reads another syscall's errno fails), and two cover the F_SETFL error arms in connect_exact (an O_PATH descriptor for the transition failure, a deny filter for the restore failure). multiple_unsafe_ops_per_block is deliberately not enabled: it is restriction-group and needs block splits at production sites owned by Closes #5188 Signed-off-by: Emilien Macchi --- .../openshell-sandbox/src/bin/seccomp-perf.rs | 4 + .../openshell-sandbox/src/boundary_server.rs | 37 +++- crates/openshell-sandbox/src/lib.rs | 5 + .../src/linux/seccomp_notify.rs | 4 +- crates/openshell-sandbox/src/main.rs | 206 ++++++++++++++++-- .../openshell-sandbox/src/network_broker.rs | 187 +++++++++++++++- crates/openshell-sandbox/src/process.rs | 6 + .../src/sandbox/linux/seccomp.rs | 3 + 8 files changed, 418 insertions(+), 34 deletions(-) diff --git a/crates/openshell-sandbox/src/bin/seccomp-perf.rs b/crates/openshell-sandbox/src/bin/seccomp-perf.rs index 4114d9a7a9..09bda3449f 100644 --- a/crates/openshell-sandbox/src/bin/seccomp-perf.rs +++ b/crates/openshell-sandbox/src/bin/seccomp-perf.rs @@ -3,6 +3,10 @@ //! Microbenchmark entry point for the production seccomp network broker. +// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a +// crate-specific lint table (see #5188). +#![warn(clippy::undocumented_unsafe_blocks)] + #[cfg(target_os = "linux")] use std::net::SocketAddr; diff --git a/crates/openshell-sandbox/src/boundary_server.rs b/crates/openshell-sandbox/src/boundary_server.rs index a01f3b797e..87f3f83354 100644 --- a/crates/openshell-sandbox/src/boundary_server.rs +++ b/crates/openshell-sandbox/src/boundary_server.rs @@ -4884,6 +4884,8 @@ mod linux { .map_err(|_| { io::Error::new(io::ErrorKind::InvalidInput, "sockaddr_vm exceeds socklen_t") })?; + // SAFETY: socket() creates a fresh descriptor and takes only + // scalar arguments; the result is checked below before any use. let raw_fd = unsafe { libc::socket( libc::AF_VSOCK, @@ -4894,6 +4896,8 @@ mod linux { if raw_fd < 0 { return Err(io::Error::last_os_error()); } + // SAFETY: raw_fd was checked non-negative above and is uniquely + // owned; this is its single wrap into an OwnedFd. let fd = unsafe { OwnedFd::from_raw_fd(raw_fd) }; let address = libc::sockaddr_vm { svm_family: family, @@ -4902,6 +4906,8 @@ mod linux { svm_cid: libc::VMADDR_CID_ANY, svm_zero: [0; 4], }; + // SAFETY: bind operates on the live OwnedFd with a fully + // initialized sockaddr_vm of the declared length. let result = unsafe { libc::bind( fd.as_raw_fd(), @@ -4912,6 +4918,7 @@ mod linux { if result < 0 { return Err(io::Error::last_os_error()); } + // SAFETY: listen operates on the live, bound OwnedFd. if unsafe { libc::listen(fd.as_raw_fd(), 16) } < 0 { return Err(io::Error::last_os_error()); } @@ -4935,6 +4942,9 @@ mod linux { listener, server_config, } => { + // SAFETY: accept4 takes the live listener descriptor with + // SOCK_CLOEXEC and a valid null address buffer; the + // returned descriptor is checked below before any use. let raw_fd = unsafe { libc::accept4( listener.as_raw_fd(), @@ -4946,8 +4956,11 @@ mod linux { if raw_fd < 0 { Err(io::Error::last_os_error()) } else { + // SAFETY: raw_fd was checked non-negative above and + // is uniquely owned; this is its single wrap. + let stream = unsafe { File::from_raw_fd(raw_fd) }; Ok(ControlStream::PendingTls { - stream: PlainControlStream::Vsock(unsafe { File::from_raw_fd(raw_fd) }), + stream: PlainControlStream::Vsock(stream), server_config: server_config.clone(), }) } @@ -5142,8 +5155,26 @@ mod linux { ) -> io::Result { match self { Self::Vsock(file) => { - let stream = - unsafe { std::os::unix::net::UnixStream::from_raw_fd(file.into_raw_fd()) }; + let raw_fd = file.into_raw_fd(); + // SAFETY: `into_raw_fd` handed over a valid, open, + // solely-owned descriptor exactly once, so this is its + // single re-wrap. The descriptor is an AF_VSOCK stream + // reinterpreted as a Unix stream; the stream only ever + // sees family-agnostic operations (fcntl, read/write, + // epoll registration via `from_std`, and shutdown), which + // behave the same on AF_VSOCK streams. `peer_addr`, + // `local_addr`, and `peer_cred` must never be called on + // it: they would fail at runtime on a vsock descriptor. + // The surface is fenced by this private enum and by the + // `Box` erasure in `into_tokio`. The + // dial side performs the same reinterpretation in the + // `RuntimeInitiatedVsock` arm of `dial_supervisor`, but + // files its vsock descriptors under the `Unix` variant + // (not `Vsock`) and uses only `take_error()` (a + // SOL_SOCKET getsockopt) and `writable()`, both + // family-agnostic; the allowed-ops list above covers + // this accept-side stream only. + let stream = unsafe { std::os::unix::net::UnixStream::from_raw_fd(raw_fd) }; stream.set_nonblocking(true)?; Ok(Box::new(tokio::net::UnixStream::from_std(stream)?)) } diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 7b1f4400c1..3c280cc977 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -3,6 +3,11 @@ //! Capability-free in-workload sandbox boundary. +// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a +// crate-specific lint table (see #5188). +#![warn(clippy::undocumented_unsafe_blocks)] +#![cfg_attr(test, allow(clippy::undocumented_unsafe_blocks))] + #[cfg(target_os = "linux")] mod accept_errors; pub mod binary_pins; diff --git a/crates/openshell-sandbox/src/linux/seccomp_notify.rs b/crates/openshell-sandbox/src/linux/seccomp_notify.rs index bc98813b00..665cadcefe 100644 --- a/crates/openshell-sandbox/src/linux/seccomp_notify.rs +++ b/crates/openshell-sandbox/src/linux/seccomp_notify.rs @@ -831,8 +831,6 @@ fn verify_notification_sizes() -> io::Result<()> { } fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) -> io::Result { - // SAFETY: every caller supplies the UAPI structure encoded into `request`, - // alive and writable for the ioctl duration. // `libc::ioctl` models the request as `c_ulong` for glibc and `c_int` // for musl. Linux UAPI request values fit both representations. #[cfg(target_env = "musl")] @@ -841,6 +839,8 @@ fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) -> })?; #[cfg(target_env = "musl")] let request = libc::c_int::from_ne_bytes(request.to_ne_bytes()); + // SAFETY: every caller supplies the UAPI structure encoded into `request`, + // alive and writable for the ioctl duration. let result = unsafe { libc::ioctl(fd, request, argument) }; if result < 0 { Err(io::Error::last_os_error()) diff --git a/crates/openshell-sandbox/src/main.rs b/crates/openshell-sandbox/src/main.rs index 5830d79dfe..d91a82a40f 100644 --- a/crates/openshell-sandbox/src/main.rs +++ b/crates/openshell-sandbox/src/main.rs @@ -3,6 +3,11 @@ //! `OpenShell` capability-free in-workload sandbox boundary. +// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a +// crate-specific lint table (see #5188). +#![warn(clippy::undocumented_unsafe_blocks)] +#![cfg_attr(test, allow(clippy::undocumented_unsafe_blocks))] + #[cfg(target_os = "linux")] use std::mem::size_of; use std::path::Path; @@ -685,6 +690,10 @@ fn probe_socket_virtualization() -> Result<()> { { return Err(std::io::Error::last_os_error()).into_diagnostic(); } + // SAFETY: the retained source is the exact injected OFD and + // the payload buffer stays live for its declared length; + // MSG_NOSIGNAL keeps a refused send from raising SIGPIPE in + // the dispatcher thread. let sent = unsafe { libc::send( retained.as_raw_fd(), @@ -796,6 +805,15 @@ fn set_child_core_limit() -> std::io::Result<()> { .map_err(std::io::Error::from) } +/// Reset every catchable signal disposition in the calling process to its +/// default. +/// +/// # Safety +/// +/// Destroys all installed handlers for signals 1 through 64 (skipping the +/// uncatchable SIGKILL and SIGSTOP) in the calling process. Must only run +/// in the forked pre-exec child, never in the launcher parent, which still +/// needs its own dispositions. #[cfg(target_os = "linux")] #[allow(unsafe_code)] unsafe fn reset_child_signal_dispositions() -> std::io::Result<()> { @@ -1217,43 +1235,44 @@ fn probe_child_self_protection(sandbox_tgid: libc::pid_t, socket: libc::c_int) - ) }; require_probe_errno(read, libc::EPERM, "process_vm_readv sandbox")?; - // SAFETY: signal zero would only probe process existence if the filter did - // not reject the trusted sandbox target. require_probe_errno( + // SAFETY: signal zero would only probe process existence if the + // filter did not reject the trusted sandbox target. isize::try_from(unsafe { libc::kill(sandbox_tgid, 0) }).expect("kill result fits isize"), libc::EPERM, "kill sandbox", )?; // SAFETY: scalar syscall arguments request a read-only resource query; // the child filter rejects non-self targets. + let prlimit_result = isize::try_from(unsafe { + libc::syscall(libc::SYS_prlimit64, sandbox_tgid, libc::RLIMIT_CORE, 0, 0) + }) + .expect("prlimit result fits isize"); + require_probe_errno(prlimit_result, libc::EPERM, "prlimit sandbox")?; + // SAFETY: signal zero on the process group would only probe group + // existence if the filter did not reject the trusted sandbox target. + let group_kill_result = unsafe { libc::kill(-sandbox_tgid, 0) }; require_probe_errno( - isize::try_from(unsafe { - libc::syscall(libc::SYS_prlimit64, sandbox_tgid, libc::RLIMIT_CORE, 0, 0) - }) - .expect("prlimit result fits isize"), - libc::EPERM, - "prlimit sandbox", - )?; - require_probe_errno( - isize::try_from(unsafe { libc::kill(-sandbox_tgid, 0) }).expect("kill result fits isize"), + isize::try_from(group_kill_result).expect("kill result fits isize"), libc::EPERM, "process-group signal", )?; + // SAFETY: F_SETOWN would only record the sandbox target as the owner of + // the child's own socket if the filter did not reject it first. + let fcntl_result = unsafe { libc::fcntl(socket, libc::F_SETOWN, sandbox_tgid) }; require_probe_errno( - isize::try_from(unsafe { libc::fcntl(socket, libc::F_SETOWN, sandbox_tgid) }) - .expect("fcntl result fits isize"), + isize::try_from(fcntl_result).expect("fcntl result fits isize"), libc::EPERM, "fcntl F_SETOWN", )?; let mut owner = sandbox_tgid; - require_probe_errno( - isize::try_from(unsafe { - libc::syscall(libc::SYS_ioctl, socket, 0x8901_u32, &raw mut owner) - }) - .expect("ioctl result fits isize"), - libc::EPERM, - "ioctl FIOSETOWN", - )?; + // SAFETY: FIOSETOWN would only record the sandbox target as the owner of + // the child's own socket if the filter did not reject it first. + let ioctl_result = isize::try_from(unsafe { + libc::syscall(libc::SYS_ioctl, socket, 0x8901_u32, &raw mut owner) + }) + .expect("ioctl result fits isize"); + require_probe_errno(ioctl_result, libc::EPERM, "ioctl FIOSETOWN")?; Ok(()) } @@ -2039,4 +2058,149 @@ mod tests { let final_path = dest_dir.join("openshell-sandbox"); assert!(final_path.exists(), "binary should land inside dest dir"); } + + /// Environment variable marking the re-executed probe child process. + const PROBE_CHILD_ENV: &str = "OPENSHELL_SANDBOX_PROBE_CHILD"; + + /// Re-executes `test_path` (the full libtest path, module prefix + /// included) in a fresh process so it can change its rlimits and load + /// seccomp filters without contaminating the test harness. The child + /// asserts on its own probe results; this parent only requires a + /// successful exit. A fresh executable process, unlike a raw `fork` child + /// of the multithreaded test harness, may allocate freely after the + /// filter is installed. + fn run_isolated_probe_child(test_path: &str) { + let output = std::process::Command::new(std::env::current_exe().expect("test executable")) + .args(["--exact", test_path, "--nocapture"]) + .env(PROBE_CHILD_ENV, "1") + .output() + .expect("start isolated probe child process"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success(), + "isolated probe child failed:\nstdout: {stdout}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + // `--exact` matches full test paths; a wrong name would silently + // select zero tests and exit successfully. + assert!( + stdout.contains("1 passed"), + "isolated probe child matched no test:\nstdout: {stdout}" + ); + } + + /// Prepares a probe child: zeroes the core-dump limit the probe requires + /// and allows the unprivileged seccomp install. + #[allow(unsafe_code)] + fn prepare_probe_child() -> std::io::Result<()> { + nix::sys::resource::setrlimit(nix::sys::resource::Resource::RLIMIT_CORE, 0, 0) + .map_err(std::io::Error::from)?; + // SAFETY: PR_SET_NO_NEW_PRIVS is a one-way scalar transition required + // before loading an unprivileged seccomp filter. + if unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) } < 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + } + + /// The production child filter rejects every self-protection probe with + /// EPERM. Running `probe_child_self_protection` against that exact filter + /// keeps each hoisted probe result wired to its own errno check. The test + /// re-executes itself in a fresh process so the rlimit and filter changes + /// never touch the test harness process. + #[test] + fn probe_child_self_protection_accepts_the_production_child_filter() { + if std::env::var_os(PROBE_CHILD_ENV).is_none() { + run_isolated_probe_child( + "tests::probe_child_self_protection_accepts_the_production_child_filter", + ); + return; + } + prepare_probe_child().expect("prepare probe child"); + let tgid = std::process::id(); + let sandbox_tgid = libc::pid_t::try_from(tgid).expect("test pid fits pid_t"); + let mut program = openshell_sandbox::linux::child_seccomp::prepare(tgid) + .expect("prepare production child filter"); + program.install().expect("install production child filter"); + probe_child_self_protection(sandbox_tgid, -1) + .expect("probes pass under the production child filter"); + } + + /// Denies every probe except the FIOSETOWN ioctl with EPERM; the kernel + /// then rejects the ioctl's invalid descriptor with EBADF. The + /// distinct errno only satisfies the check that belongs to the ioctl probe + /// when every check runs directly after its own syscall. + fn probe_adjacency_filter() -> seccompiler::BpfProgram { + use seccompiler::{ + SeccompAction, SeccompCmpArgLen, SeccompCmpOp, SeccompCondition, SeccompFilter, + SeccompRule, + }; + use std::collections::BTreeMap; + + let mut rules: BTreeMap> = BTreeMap::new(); + rules.insert(libc::SYS_process_vm_readv, Vec::new()); + rules.insert(libc::SYS_kill, Vec::new()); + rules.insert( + libc::SYS_prlimit64, + vec![ + SeccompRule::new(vec![ + SeccompCondition::new(0, SeccompCmpArgLen::Dword, SeccompCmpOp::Ne, 0) + .expect("valid prlimit64 argument condition"), + ]) + .expect("valid prlimit64 rule"), + ], + ); + rules.insert( + libc::SYS_fcntl, + vec![ + SeccompRule::new(vec![ + SeccompCondition::new( + 1, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Eq, + u64::from(libc::F_SETOWN.cast_unsigned()), + ) + .expect("valid fcntl command condition"), + ]) + .expect("valid fcntl rule"), + ], + ); + let architecture = std::env::consts::ARCH + .try_into() + .expect("supported target architecture"); + SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Errno(libc::EPERM.cast_unsigned()), + architecture, + ) + .expect("build probe filter") + .try_into() + .expect("compile probe filter") + } + + #[test] + fn probe_child_self_protection_checks_each_syscall_errno_independently() { + if std::env::var_os(PROBE_CHILD_ENV).is_none() { + run_isolated_probe_child( + "tests::probe_child_self_protection_checks_each_syscall_errno_independently", + ); + return; + } + prepare_probe_child().expect("prepare probe child"); + let filter = probe_adjacency_filter(); + seccompiler::apply_filter(&filter).expect("install adjacency filter"); + let sandbox_tgid = libc::pid_t::try_from(std::process::id()).expect("test pid fits pid_t"); + // The invalid descriptor is enough: the fcntl probe is rejected by + // the filter before the kernel validates it, and the unfiltered + // ioctl probe reports EBADF from the kernel. + let error = probe_child_self_protection(sandbox_tgid, -1) + .expect_err("adjacency filter must let the ioctl probe through"); + assert!( + error + .to_string() + .contains("ioctl FIOSETOWN was not rejected with errno"), + "unexpected probe failure: {error}" + ); + } } diff --git a/crates/openshell-sandbox/src/network_broker.rs b/crates/openshell-sandbox/src/network_broker.rs index 4d84c8734c..f86d090b61 100644 --- a/crates/openshell-sandbox/src/network_broker.rs +++ b/crates/openshell-sandbox/src/network_broker.rs @@ -1691,10 +1691,12 @@ fn connect_exact(fd: RawFd, address: SocketAddr) -> io::Result<()> { return Err(io::Error::last_os_error()); } let changed_flags = original_flags & libc::O_NONBLOCK == 0; - if changed_flags - && unsafe { libc::fcntl(fd, libc::F_SETFL, original_flags | libc::O_NONBLOCK) } < 0 - { - return Err(io::Error::last_os_error()); + if changed_flags { + // SAFETY: F_SETFL operates on the live retained socket descriptor + // with the flag word read by the documented F_GETFL above. + if unsafe { libc::fcntl(fd, libc::F_SETFL, original_flags | libc::O_NONBLOCK) } < 0 { + return Err(io::Error::last_os_error()); + } } let result = with_sockaddr(address, |pointer, length| { // SAFETY: pointer/length describe a live native sockaddr and `fd` is @@ -1712,8 +1714,8 @@ fn connect_exact(fd: RawFd, address: SocketAddr) -> io::Result<()> { events: libc::POLLOUT, revents: 0, }; - // SAFETY: poll points to one live pollfd. let timeout = i32::try_from(RELAY_CONNECT_TIMEOUT.as_millis()).map_err(io::Error::other)?; + // SAFETY: poll points to one live pollfd. if unsafe { libc::poll(&raw mut poll, 1, timeout) } <= 0 { return Err(io::Error::from_raw_os_error(libc::ETIMEDOUT)); } @@ -1738,9 +1740,16 @@ fn connect_exact(fd: RawFd, address: SocketAddr) -> io::Result<()> { Err(io::Error::from_raw_os_error(socket_error)) } }); - let restore = if changed_flags && unsafe { libc::fcntl(fd, libc::F_SETFL, original_flags) } < 0 - { - Err(io::Error::last_os_error()) + let restore = if changed_flags { + // SAFETY: F_SETFL restores the original flag word read by the + // documented F_GETFL; the restore runs even when the connect + // attempt failed. + let restore_result = unsafe { libc::fcntl(fd, libc::F_SETFL, original_flags) }; + if restore_result < 0 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } } else { Ok(()) }; @@ -2483,6 +2492,168 @@ mod tests { ); } + /// Runs `child` in a forked process and returns the bytes it writes to a + /// pipe. Assertions belong in the parent: a panic in the child would + /// unwind into a copy of the test harness. + #[allow(unsafe_code)] + fn fork_capture(child: impl FnOnce() -> Vec) -> Vec { + let (read_fd, write_fd) = nix::unistd::pipe().expect("create probe pipe"); + // SAFETY: fork isolates the rlimit and seccomp changes the callers + // make. The child writes one result and exits without running + // inherited destructors or the test harness. + match unsafe { nix::unistd::fork() }.expect("fork should succeed") { + nix::unistd::ForkResult::Child => { + drop(read_fd); + let result = child(); + let _ = rustix::io::write(&write_fd, &result); + drop(write_fd); + // SAFETY: terminate the fork child without unwinding through + // the parent's test harness state. + unsafe { libc::_exit(0) }; + } + nix::unistd::ForkResult::Parent { child } => { + drop(write_fd); + let mut result = Vec::new(); + let mut buffer = [0_u8; 512]; + loop { + match rustix::io::read(&read_fd, &mut buffer) { + Ok(0) => break, + Ok(read) => result.extend_from_slice(&buffer[..read]), + Err(error) if error == rustix::io::Errno::INTR => {} + Err(error) => panic!("read probe result: {error}"), + } + } + drop(read_fd); + match nix::sys::wait::waitpid(child, None).expect("waitpid should succeed") { + nix::sys::wait::WaitStatus::Exited(_, 0) => {} + status => panic!("probe child exited unexpectedly: {status:?}"), + } + result + } + } + } + + #[test] + fn connect_exact_returns_a_failed_nonblocking_transition() { + // O_PATH descriptors support F_GETFL but reject F_SETFL with EBADF. + // That reaches the set-failure return without invalidating a + // descriptor that is also being connected. + let path = std::ffi::CString::new("/").expect("static path is NUL-free"); + // SAFETY: path is a live NUL-terminated directory path; a successful + // open returns one newly owned descriptor. + let descriptor = unsafe { libc::open(path.as_ptr(), libc::O_PATH | libc::O_CLOEXEC) }; + assert!( + descriptor >= 0, + "open O_PATH: {}", + io::Error::last_os_error() + ); + // SAFETY: the successful open transferred ownership of the descriptor. + let descriptor = unsafe { OwnedFd::from_raw_fd(descriptor) }; + + let error = connect_exact(descriptor.as_raw_fd(), "127.0.0.1:9".parse().unwrap()) + .expect_err("an O_PATH descriptor rejects F_SETFL"); + assert_eq!(error.raw_os_error(), Some(libc::EBADF)); + } + + /// Denies only the `F_SETFL` restore that `connect_exact` performs after a + /// bounded connect: the initial transition carries `O_NONBLOCK`, the restore + /// does not. The connect itself still succeeds, so the restore error is + /// the one the caller observes. + fn flag_restore_failure_filter() -> seccompiler::BpfProgram { + use seccompiler::{ + SeccompAction, SeccompCmpArgLen, SeccompCmpOp, SeccompCondition, SeccompFilter, + SeccompRule, + }; + use std::collections::BTreeMap; + + let mut rules: BTreeMap> = BTreeMap::new(); + rules.insert( + libc::SYS_fcntl, + vec![ + SeccompRule::new(vec![ + SeccompCondition::new( + 1, + SeccompCmpArgLen::Dword, + SeccompCmpOp::Eq, + u64::from(libc::F_SETFL.cast_unsigned()), + ) + .expect("valid fcntl command condition"), + SeccompCondition::new( + 2, + SeccompCmpArgLen::Dword, + SeccompCmpOp::MaskedEq(u64::from(libc::O_NONBLOCK.cast_unsigned())), + 0, + ) + .expect("valid fcntl flag condition"), + ]) + .expect("valid fcntl rule"), + ], + ); + let architecture = std::env::consts::ARCH + .try_into() + .expect("supported target architecture"); + SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Errno(libc::EPERM.cast_unsigned()), + architecture, + ) + .expect("build restore filter") + .try_into() + .expect("compile restore filter") + } + + #[test] + fn connect_exact_returns_the_flag_restore_failure() { + // Nothing can invalidate the retained descriptor between the two fcntl + // calls, so a filter that denies only the restore transition is the + // way to reach the restore-failure arm. + let listener = TcpListener::bind("127.0.0.1:0").expect("bind listener"); + let address = listener.local_addr().expect("listener address"); + let filter = flag_restore_failure_filter(); + let result = fork_capture(move || { + // SAFETY: PR_SET_NO_NEW_PRIVS is a one-way scalar transition + // required before loading an unprivileged seccomp filter. + if unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) } < 0 { + return i32::MIN.to_ne_bytes().to_vec(); + } + if seccompiler::apply_filter(&filter).is_err() { + return i32::MIN.to_ne_bytes().to_vec(); + } + // SAFETY: socket returns one newly owned descriptor on success. + let descriptor = unsafe { + libc::socket( + libc::AF_INET, + libc::SOCK_STREAM | libc::SOCK_CLOEXEC, + libc::IPPROTO_TCP, + ) + }; + if descriptor < 0 { + return i32::MIN.to_ne_bytes().to_vec(); + } + // SAFETY: the successful socket call returned one owned descriptor. + let descriptor = unsafe { OwnedFd::from_raw_fd(descriptor) }; + match connect_exact(descriptor.as_raw_fd(), address) { + Ok(()) => 0_i32.to_ne_bytes().to_vec(), + Err(error) => error + .raw_os_error() + .unwrap_or(i32::MAX) + .to_ne_bytes() + .to_vec(), + } + }); + let errno = i32::from_ne_bytes( + result + .try_into() + .expect("probe child writes one i32 status"), + ); + assert_eq!( + errno, + libc::EPERM, + "restore failure must be returned to the caller" + ); + } + #[test] fn dns_admissions_reclaim_closed_sockets_at_the_bound() { let admissions = Mutex::new(HashMap::new()); diff --git a/crates/openshell-sandbox/src/process.rs b/crates/openshell-sandbox/src/process.rs index a8995bc7da..4967d20086 100644 --- a/crates/openshell-sandbox/src/process.rs +++ b/crates/openshell-sandbox/src/process.rs @@ -31,6 +31,9 @@ use tracing::debug; #[cfg(unix)] #[allow(unsafe_code, clippy::useless_conversion)] fn set_controlling_tty(fd: libc::c_int) -> std::io::Result<()> { + // SAFETY: fd is the slave PTY of a pre-exec child that already ran + // setsid; argument zero requests ownership without stealing the + // terminal from another session. if unsafe { libc::ioctl(fd, libc::TIOCSCTTY.into(), 0) } < 0 { return Err(std::io::Error::last_os_error()); } @@ -626,6 +629,9 @@ impl ProcessHandle { #[cfg(target_os = "linux")] let mut prepared_sandbox = prepared_sandbox; #[allow(unsafe_code)] + // SAFETY: pre_exec only registers the hook; the closure runs + // post-fork and pre-exec in the child process, never in the + // launcher parent. unsafe { cmd.pre_exec(move || { if let Some(slave_fd) = terminal_slave_fd { diff --git a/crates/openshell-sandbox/src/sandbox/linux/seccomp.rs b/crates/openshell-sandbox/src/sandbox/linux/seccomp.rs index d180920325..2dbdb53428 100644 --- a/crates/openshell-sandbox/src/sandbox/linux/seccomp.rs +++ b/crates/openshell-sandbox/src/sandbox/linux/seccomp.rs @@ -126,6 +126,9 @@ fn build_supervisor_prelude_rules() -> BTreeMap> { fn set_no_new_privs() -> Result<()> { // libc/syscall FFI requires unsafe #[allow(unsafe_code)] + // SAFETY: prctl receives only scalar arguments; PR_SET_NO_NEW_PRIVS is + // required before an unprivileged seccomp filter load and is + // irreversible. let rc = unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) }; if rc != 0 { return Err(miette::miette!(