Skip to content

Commit 32c1957

Browse files
committed
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 <emacchi@redhat.com>
1 parent 9321d16 commit 32c1957

8 files changed

Lines changed: 418 additions & 34 deletions

File tree

‎crates/openshell-sandbox/src/bin/seccomp-perf.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33

44
//! Microbenchmark entry point for the production seccomp network broker.
55
6+
// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a
7+
// crate-specific lint table (see #5188).
8+
#![warn(clippy::undocumented_unsafe_blocks)]
9+
610
#[cfg(target_os = "linux")]
711
use std::net::SocketAddr;
812

‎crates/openshell-sandbox/src/boundary_server.rs‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4884,6 +4884,8 @@ mod linux {
48844884
.map_err(|_| {
48854885
io::Error::new(io::ErrorKind::InvalidInput, "sockaddr_vm exceeds socklen_t")
48864886
})?;
4887+
// SAFETY: socket() creates a fresh descriptor and takes only
4888+
// scalar arguments; the result is checked below before any use.
48874889
let raw_fd = unsafe {
48884890
libc::socket(
48894891
libc::AF_VSOCK,
@@ -4894,6 +4896,8 @@ mod linux {
48944896
if raw_fd < 0 {
48954897
return Err(io::Error::last_os_error());
48964898
}
4899+
// SAFETY: raw_fd was checked non-negative above and is uniquely
4900+
// owned; this is its single wrap into an OwnedFd.
48974901
let fd = unsafe { OwnedFd::from_raw_fd(raw_fd) };
48984902
let address = libc::sockaddr_vm {
48994903
svm_family: family,
@@ -4902,6 +4906,8 @@ mod linux {
49024906
svm_cid: libc::VMADDR_CID_ANY,
49034907
svm_zero: [0; 4],
49044908
};
4909+
// SAFETY: bind operates on the live OwnedFd with a fully
4910+
// initialized sockaddr_vm of the declared length.
49054911
let result = unsafe {
49064912
libc::bind(
49074913
fd.as_raw_fd(),
@@ -4912,6 +4918,7 @@ mod linux {
49124918
if result < 0 {
49134919
return Err(io::Error::last_os_error());
49144920
}
4921+
// SAFETY: listen operates on the live, bound OwnedFd.
49154922
if unsafe { libc::listen(fd.as_raw_fd(), 16) } < 0 {
49164923
return Err(io::Error::last_os_error());
49174924
}
@@ -4935,6 +4942,9 @@ mod linux {
49354942
listener,
49364943
server_config,
49374944
} => {
4945+
// SAFETY: accept4 takes the live listener descriptor with
4946+
// SOCK_CLOEXEC and a valid null address buffer; the
4947+
// returned descriptor is checked below before any use.
49384948
let raw_fd = unsafe {
49394949
libc::accept4(
49404950
listener.as_raw_fd(),
@@ -4946,8 +4956,11 @@ mod linux {
49464956
if raw_fd < 0 {
49474957
Err(io::Error::last_os_error())
49484958
} else {
4959+
// SAFETY: raw_fd was checked non-negative above and
4960+
// is uniquely owned; this is its single wrap.
4961+
let stream = unsafe { File::from_raw_fd(raw_fd) };
49494962
Ok(ControlStream::PendingTls {
4950-
stream: PlainControlStream::Vsock(unsafe { File::from_raw_fd(raw_fd) }),
4963+
stream: PlainControlStream::Vsock(stream),
49514964
server_config: server_config.clone(),
49524965
})
49534966
}
@@ -5142,8 +5155,26 @@ mod linux {
51425155
) -> io::Result<openshell_isolation_interface::contract::BoundaryDuplexStream> {
51435156
match self {
51445157
Self::Vsock(file) => {
5145-
let stream =
5146-
unsafe { std::os::unix::net::UnixStream::from_raw_fd(file.into_raw_fd()) };
5158+
let raw_fd = file.into_raw_fd();
5159+
// SAFETY: `into_raw_fd` handed over a valid, open,
5160+
// solely-owned descriptor exactly once, so this is its
5161+
// single re-wrap. The descriptor is an AF_VSOCK stream
5162+
// reinterpreted as a Unix stream; the stream only ever
5163+
// sees family-agnostic operations (fcntl, read/write,
5164+
// epoll registration via `from_std`, and shutdown), which
5165+
// behave the same on AF_VSOCK streams. `peer_addr`,
5166+
// `local_addr`, and `peer_cred` must never be called on
5167+
// it: they would fail at runtime on a vsock descriptor.
5168+
// The surface is fenced by this private enum and by the
5169+
// `Box<dyn DuplexStream>` erasure in `into_tokio`. The
5170+
// dial side performs the same reinterpretation in the
5171+
// `RuntimeInitiatedVsock` arm of `dial_supervisor`, but
5172+
// files its vsock descriptors under the `Unix` variant
5173+
// (not `Vsock`) and uses only `take_error()` (a
5174+
// SOL_SOCKET getsockopt) and `writable()`, both
5175+
// family-agnostic; the allowed-ops list above covers
5176+
// this accept-side stream only.
5177+
let stream = unsafe { std::os::unix::net::UnixStream::from_raw_fd(raw_fd) };
51475178
stream.set_nonblocking(true)?;
51485179
Ok(Box::new(tokio::net::UnixStream::from_std(stream)?))
51495180
}

‎crates/openshell-sandbox/src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33

44
//! Capability-free in-workload sandbox boundary.
55
6+
// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a
7+
// crate-specific lint table (see #5188).
8+
#![warn(clippy::undocumented_unsafe_blocks)]
9+
#![cfg_attr(test, allow(clippy::undocumented_unsafe_blocks))]
10+
611
#[cfg(target_os = "linux")]
712
mod accept_errors;
813
pub mod binary_pins;

‎crates/openshell-sandbox/src/linux/seccomp_notify.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -831,8 +831,6 @@ fn verify_notification_sizes() -> io::Result<()> {
831831
}
832832

833833
fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) -> io::Result<i64> {
834-
// SAFETY: every caller supplies the UAPI structure encoded into `request`,
835-
// alive and writable for the ioctl duration.
836834
// `libc::ioctl` models the request as `c_ulong` for glibc and `c_int`
837835
// for musl. Linux UAPI request values fit both representations.
838836
#[cfg(target_env = "musl")]
@@ -841,6 +839,8 @@ fn ioctl_ptr(fd: RawFd, request: libc::c_ulong, argument: *mut libc::c_void) ->
841839
})?;
842840
#[cfg(target_env = "musl")]
843841
let request = libc::c_int::from_ne_bytes(request.to_ne_bytes());
842+
// SAFETY: every caller supplies the UAPI structure encoded into `request`,
843+
// alive and writable for the ioctl duration.
844844
let result = unsafe { libc::ioctl(fd, request, argument) };
845845
if result < 0 {
846846
Err(io::Error::last_os_error())

‎crates/openshell-sandbox/src/main.rs‎

Lines changed: 185 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33

44
//! `OpenShell` capability-free in-workload sandbox boundary.
55
6+
// Crate-scoped: Cargo rejects mixing `[lints] workspace = true` with a
7+
// crate-specific lint table (see #5188).
8+
#![warn(clippy::undocumented_unsafe_blocks)]
9+
#![cfg_attr(test, allow(clippy::undocumented_unsafe_blocks))]
10+
611
#[cfg(target_os = "linux")]
712
use std::mem::size_of;
813
use std::path::Path;
@@ -685,6 +690,10 @@ fn probe_socket_virtualization() -> Result<()> {
685690
{
686691
return Err(std::io::Error::last_os_error()).into_diagnostic();
687692
}
693+
// SAFETY: the retained source is the exact injected OFD and
694+
// the payload buffer stays live for its declared length;
695+
// MSG_NOSIGNAL keeps a refused send from raising SIGPIPE in
696+
// the dispatcher thread.
688697
let sent = unsafe {
689698
libc::send(
690699
retained.as_raw_fd(),
@@ -796,6 +805,15 @@ fn set_child_core_limit() -> std::io::Result<()> {
796805
.map_err(std::io::Error::from)
797806
}
798807

808+
/// Reset every catchable signal disposition in the calling process to its
809+
/// default.
810+
///
811+
/// # Safety
812+
///
813+
/// Destroys all installed handlers for signals 1 through 64 (skipping the
814+
/// uncatchable SIGKILL and SIGSTOP) in the calling process. Must only run
815+
/// in the forked pre-exec child, never in the launcher parent, which still
816+
/// needs its own dispositions.
799817
#[cfg(target_os = "linux")]
800818
#[allow(unsafe_code)]
801819
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) -
12171235
)
12181236
};
12191237
require_probe_errno(read, libc::EPERM, "process_vm_readv sandbox")?;
1220-
// SAFETY: signal zero would only probe process existence if the filter did
1221-
// not reject the trusted sandbox target.
12221238
require_probe_errno(
1239+
// SAFETY: signal zero would only probe process existence if the
1240+
// filter did not reject the trusted sandbox target.
12231241
isize::try_from(unsafe { libc::kill(sandbox_tgid, 0) }).expect("kill result fits isize"),
12241242
libc::EPERM,
12251243
"kill sandbox",
12261244
)?;
12271245
// SAFETY: scalar syscall arguments request a read-only resource query;
12281246
// the child filter rejects non-self targets.
1247+
let prlimit_result = isize::try_from(unsafe {
1248+
libc::syscall(libc::SYS_prlimit64, sandbox_tgid, libc::RLIMIT_CORE, 0, 0)
1249+
})
1250+
.expect("prlimit result fits isize");
1251+
require_probe_errno(prlimit_result, libc::EPERM, "prlimit sandbox")?;
1252+
// SAFETY: signal zero on the process group would only probe group
1253+
// existence if the filter did not reject the trusted sandbox target.
1254+
let group_kill_result = unsafe { libc::kill(-sandbox_tgid, 0) };
12291255
require_probe_errno(
1230-
isize::try_from(unsafe {
1231-
libc::syscall(libc::SYS_prlimit64, sandbox_tgid, libc::RLIMIT_CORE, 0, 0)
1232-
})
1233-
.expect("prlimit result fits isize"),
1234-
libc::EPERM,
1235-
"prlimit sandbox",
1236-
)?;
1237-
require_probe_errno(
1238-
isize::try_from(unsafe { libc::kill(-sandbox_tgid, 0) }).expect("kill result fits isize"),
1256+
isize::try_from(group_kill_result).expect("kill result fits isize"),
12391257
libc::EPERM,
12401258
"process-group signal",
12411259
)?;
1260+
// SAFETY: F_SETOWN would only record the sandbox target as the owner of
1261+
// the child's own socket if the filter did not reject it first.
1262+
let fcntl_result = unsafe { libc::fcntl(socket, libc::F_SETOWN, sandbox_tgid) };
12421263
require_probe_errno(
1243-
isize::try_from(unsafe { libc::fcntl(socket, libc::F_SETOWN, sandbox_tgid) })
1244-
.expect("fcntl result fits isize"),
1264+
isize::try_from(fcntl_result).expect("fcntl result fits isize"),
12451265
libc::EPERM,
12461266
"fcntl F_SETOWN",
12471267
)?;
12481268
let mut owner = sandbox_tgid;
1249-
require_probe_errno(
1250-
isize::try_from(unsafe {
1251-
libc::syscall(libc::SYS_ioctl, socket, 0x8901_u32, &raw mut owner)
1252-
})
1253-
.expect("ioctl result fits isize"),
1254-
libc::EPERM,
1255-
"ioctl FIOSETOWN",
1256-
)?;
1269+
// SAFETY: FIOSETOWN would only record the sandbox target as the owner of
1270+
// the child's own socket if the filter did not reject it first.
1271+
let ioctl_result = isize::try_from(unsafe {
1272+
libc::syscall(libc::SYS_ioctl, socket, 0x8901_u32, &raw mut owner)
1273+
})
1274+
.expect("ioctl result fits isize");
1275+
require_probe_errno(ioctl_result, libc::EPERM, "ioctl FIOSETOWN")?;
12571276
Ok(())
12581277
}
12591278

@@ -2039,4 +2058,149 @@ mod tests {
20392058
let final_path = dest_dir.join("openshell-sandbox");
20402059
assert!(final_path.exists(), "binary should land inside dest dir");
20412060
}
2061+
2062+
/// Environment variable marking the re-executed probe child process.
2063+
const PROBE_CHILD_ENV: &str = "OPENSHELL_SANDBOX_PROBE_CHILD";
2064+
2065+
/// Re-executes `test_path` (the full libtest path, module prefix
2066+
/// included) in a fresh process so it can change its rlimits and load
2067+
/// seccomp filters without contaminating the test harness. The child
2068+
/// asserts on its own probe results; this parent only requires a
2069+
/// successful exit. A fresh executable process, unlike a raw `fork` child
2070+
/// of the multithreaded test harness, may allocate freely after the
2071+
/// filter is installed.
2072+
fn run_isolated_probe_child(test_path: &str) {
2073+
let output = std::process::Command::new(std::env::current_exe().expect("test executable"))
2074+
.args(["--exact", test_path, "--nocapture"])
2075+
.env(PROBE_CHILD_ENV, "1")
2076+
.output()
2077+
.expect("start isolated probe child process");
2078+
let stdout = String::from_utf8_lossy(&output.stdout);
2079+
assert!(
2080+
output.status.success(),
2081+
"isolated probe child failed:\nstdout: {stdout}\nstderr: {}",
2082+
String::from_utf8_lossy(&output.stderr)
2083+
);
2084+
// `--exact` matches full test paths; a wrong name would silently
2085+
// select zero tests and exit successfully.
2086+
assert!(
2087+
stdout.contains("1 passed"),
2088+
"isolated probe child matched no test:\nstdout: {stdout}"
2089+
);
2090+
}
2091+
2092+
/// Prepares a probe child: zeroes the core-dump limit the probe requires
2093+
/// and allows the unprivileged seccomp install.
2094+
#[allow(unsafe_code)]
2095+
fn prepare_probe_child() -> std::io::Result<()> {
2096+
nix::sys::resource::setrlimit(nix::sys::resource::Resource::RLIMIT_CORE, 0, 0)
2097+
.map_err(std::io::Error::from)?;
2098+
// SAFETY: PR_SET_NO_NEW_PRIVS is a one-way scalar transition required
2099+
// before loading an unprivileged seccomp filter.
2100+
if unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) } < 0 {
2101+
return Err(std::io::Error::last_os_error());
2102+
}
2103+
Ok(())
2104+
}
2105+
2106+
/// The production child filter rejects every self-protection probe with
2107+
/// EPERM. Running `probe_child_self_protection` against that exact filter
2108+
/// keeps each hoisted probe result wired to its own errno check. The test
2109+
/// re-executes itself in a fresh process so the rlimit and filter changes
2110+
/// never touch the test harness process.
2111+
#[test]
2112+
fn probe_child_self_protection_accepts_the_production_child_filter() {
2113+
if std::env::var_os(PROBE_CHILD_ENV).is_none() {
2114+
run_isolated_probe_child(
2115+
"tests::probe_child_self_protection_accepts_the_production_child_filter",
2116+
);
2117+
return;
2118+
}
2119+
prepare_probe_child().expect("prepare probe child");
2120+
let tgid = std::process::id();
2121+
let sandbox_tgid = libc::pid_t::try_from(tgid).expect("test pid fits pid_t");
2122+
let mut program = openshell_sandbox::linux::child_seccomp::prepare(tgid)
2123+
.expect("prepare production child filter");
2124+
program.install().expect("install production child filter");
2125+
probe_child_self_protection(sandbox_tgid, -1)
2126+
.expect("probes pass under the production child filter");
2127+
}
2128+
2129+
/// Denies every probe except the FIOSETOWN ioctl with EPERM; the kernel
2130+
/// then rejects the ioctl's invalid descriptor with EBADF. The
2131+
/// distinct errno only satisfies the check that belongs to the ioctl probe
2132+
/// when every check runs directly after its own syscall.
2133+
fn probe_adjacency_filter() -> seccompiler::BpfProgram {
2134+
use seccompiler::{
2135+
SeccompAction, SeccompCmpArgLen, SeccompCmpOp, SeccompCondition, SeccompFilter,
2136+
SeccompRule,
2137+
};
2138+
use std::collections::BTreeMap;
2139+
2140+
let mut rules: BTreeMap<i64, Vec<SeccompRule>> = BTreeMap::new();
2141+
rules.insert(libc::SYS_process_vm_readv, Vec::new());
2142+
rules.insert(libc::SYS_kill, Vec::new());
2143+
rules.insert(
2144+
libc::SYS_prlimit64,
2145+
vec![
2146+
SeccompRule::new(vec![
2147+
SeccompCondition::new(0, SeccompCmpArgLen::Dword, SeccompCmpOp::Ne, 0)
2148+
.expect("valid prlimit64 argument condition"),
2149+
])
2150+
.expect("valid prlimit64 rule"),
2151+
],
2152+
);
2153+
rules.insert(
2154+
libc::SYS_fcntl,
2155+
vec![
2156+
SeccompRule::new(vec![
2157+
SeccompCondition::new(
2158+
1,
2159+
SeccompCmpArgLen::Dword,
2160+
SeccompCmpOp::Eq,
2161+
u64::from(libc::F_SETOWN.cast_unsigned()),
2162+
)
2163+
.expect("valid fcntl command condition"),
2164+
])
2165+
.expect("valid fcntl rule"),
2166+
],
2167+
);
2168+
let architecture = std::env::consts::ARCH
2169+
.try_into()
2170+
.expect("supported target architecture");
2171+
SeccompFilter::new(
2172+
rules,
2173+
SeccompAction::Allow,
2174+
SeccompAction::Errno(libc::EPERM.cast_unsigned()),
2175+
architecture,
2176+
)
2177+
.expect("build probe filter")
2178+
.try_into()
2179+
.expect("compile probe filter")
2180+
}
2181+
2182+
#[test]
2183+
fn probe_child_self_protection_checks_each_syscall_errno_independently() {
2184+
if std::env::var_os(PROBE_CHILD_ENV).is_none() {
2185+
run_isolated_probe_child(
2186+
"tests::probe_child_self_protection_checks_each_syscall_errno_independently",
2187+
);
2188+
return;
2189+
}
2190+
prepare_probe_child().expect("prepare probe child");
2191+
let filter = probe_adjacency_filter();
2192+
seccompiler::apply_filter(&filter).expect("install adjacency filter");
2193+
let sandbox_tgid = libc::pid_t::try_from(std::process::id()).expect("test pid fits pid_t");
2194+
// The invalid descriptor is enough: the fcntl probe is rejected by
2195+
// the filter before the kernel validates it, and the unfiltered
2196+
// ioctl probe reports EBADF from the kernel.
2197+
let error = probe_child_self_protection(sandbox_tgid, -1)
2198+
.expect_err("adjacency filter must let the ioctl probe through");
2199+
assert!(
2200+
error
2201+
.to_string()
2202+
.contains("ioctl FIOSETOWN was not rejected with errno"),
2203+
"unexpected probe failure: {error}"
2204+
);
2205+
}
20422206
}

0 commit comments

Comments
 (0)