Skip to content

Commit d7d4499

Browse files
committed
fix(sandbox): address review findings across the hardening changes
Fixes: - The blocking local-connect worker no longer toggles O_NONBLOCK on the open file it shares with the workload; it waits with a plain connect. - tgkill and rt_tgsigqueueinfo are notified only when they send SIGCONT, so ordinary thread signals such as Go preemption stay in the kernel. The frozen flag is re-read immediately before delivery. - A shell redirect to the caller's own thread comm file works again; O_CREAT and O_TRUNC are no-ops there and O_EXCL returns EEXIST. - The teardown scan is a linear walk and fails closed when /proc cannot be read. Simplifications: - Remove tests that need infrastructure outside the repo or prove nothing: the topology harness tests, the static-server benchmark, the direct-syscall accept test, the comm rename test without Landlock, a serde-default test, and the test-only panic hook in setsockopt. - Drop the unused Failed connect outcome, the redundant read-back after binding to loopback, redundant OPENSHELL_SANDBOX settings, and stale or duplicated comments; reuse the reserved environment prefix constant. - Tighten the support matrix and OpenShift wording. Signed-off-by: Drew Newberry <anewberry@nvidia.com>
1 parent 7a26c20 commit d7d4499

13 files changed

Lines changed: 177 additions & 570 deletions

File tree

‎crates/openshell-isolation-interface/src/linux/child_seccomp.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,9 @@ pub fn mark_inherited_descriptors_close_on_exec() -> io::Result<()> {
124124
/// `sandbox_tgid` is the sandbox PID as visible from its workload namespace.
125125
/// The filter blocks thread-targeting operations that name the trusted sandbox
126126
/// leader and blocks process-directed operations with the same target. The
127-
/// ordinary workload listener additionally mediates `kill`, `tkill`, and
128-
/// `rt_sigqueueinfo`: Linux accepts nonleader TIDs for these operations, so a
127+
/// ordinary workload listener additionally mediates `kill`, `tkill`,
128+
/// `rt_sigqueueinfo`, and `SIGCONT` sent with `tgkill` or
129+
/// `rt_tgsigqueueinfo`: Linux accepts nonleader TIDs for these operations, so a
129130
/// static TGID comparison alone cannot protect future sandbox worker threads.
130131
pub fn prepare(sandbox_tgid: u32) -> io::Result<ChildHardeningProgram> {
131132
if sandbox_tgid == 0 {

‎crates/openshell-isolation-interface/src/linux/process_signal.rs‎

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
use std::io;
1414
use std::os::fd::{AsRawFd, FromRawFd, OwnedFd};
15+
use std::sync::atomic::{AtomicBool, Ordering};
1516

1617
use crate::linux::seccomp_notify::{Notification, NotificationListener};
1718
use crate::linux::task_memory;
@@ -27,7 +28,7 @@ pub fn mediate_process_signal(
2728
listener: &NotificationListener,
2829
notification: Notification,
2930
sandbox_tgid: u32,
30-
workload_frozen: bool,
31+
workload_frozen: &AtomicBool,
3132
) -> io::Result<()> {
3233
listener.validate_id(notification.id)?;
3334
let target = scalar_int(notification.args[0]);
@@ -66,6 +67,7 @@ pub fn mediate_process_signal(
6667
_ => return Err(io::Error::from_raw_os_error(libc::ENOSYS)),
6768
};
6869
listener.validate_id(notification.id)?;
70+
refuse_resume_while_frozen(signal, workload_frozen)?;
6971
// SAFETY: retained owns a live pidfd; info is null or a complete trusted
7072
// copy. The kernel targets that process object, never a reused numeric PID.
7173
let result = unsafe {
@@ -83,59 +85,57 @@ pub fn mediate_process_signal(
8385
listener.respond_value(notification.id, 0)
8486
}
8587

86-
/// Continue a positive-target `tkill` only when the target thread belongs to
87-
/// an untrusted workload process rather than the sandbox runtime itself.
88+
/// Continue a thread-directed signal (`tkill`, `tgkill`,
89+
/// `rt_tgsigqueueinfo`) aimed at an untrusted workload thread.
8890
///
8991
/// Continuing preserves Linux's thread-directed signal semantics, including
90-
/// the cancellation signal used by musl. A target that exits between the
91-
/// ownership check and continuation can only be reused inside the same PID
92-
/// namespace; the static child filter still rejects the sandbox leader.
92+
/// the cancellation signal used by musl. The static child filter rejects the
93+
/// sandbox leader as a `tgkill`/`rt_tgsigqueueinfo` group, and the kernel
94+
/// rejects a thread outside the named group, so those two are notified only
95+
/// for `SIGCONT`. A `tkill` names a bare thread, so its group is resolved here;
96+
/// a target reused between this check and continuation stays inside the same
97+
/// PID namespace.
9398
pub fn mediate_thread_signal(
9499
listener: &NotificationListener,
95100
notification: Notification,
96101
sandbox_tgid: u32,
97-
workload_frozen: bool,
102+
workload_frozen: &AtomicBool,
98103
) -> io::Result<()> {
99104
listener.validate_id(notification.id)?;
100-
// tkill(tid, sig); tgkill(tgid, tid, sig); rt_tgsigqueueinfo(tgid, tid,
101-
// sig, info). The kernel itself validates a queued siginfo's code.
102-
let (claimed_group, target, signal) = match i64::from(notification.syscall) {
103-
libc::SYS_tkill => (
104-
None,
105-
scalar_int(notification.args[0]),
106-
scalar_int(notification.args[1]),
107-
),
108-
libc::SYS_tgkill | libc::SYS_rt_tgsigqueueinfo => (
109-
Some(scalar_int(notification.args[0])),
110-
scalar_int(notification.args[1]),
111-
scalar_int(notification.args[2]),
112-
),
105+
let signal = match i64::from(notification.syscall) {
106+
libc::SYS_tkill => {
107+
let target = scalar_int(notification.args[0]);
108+
let signal = scalar_int(notification.args[1]);
109+
if target <= 0 {
110+
return Err(io::Error::from_raw_os_error(libc::EPERM));
111+
}
112+
let target =
113+
u32::try_from(target).map_err(|_| io::Error::from_raw_os_error(libc::ESRCH))?;
114+
let target_group = thread_group_id(target)?;
115+
if target_group == sandbox_tgid || target_group == 0 {
116+
return Err(io::Error::from_raw_os_error(libc::EPERM));
117+
}
118+
signal
119+
}
120+
libc::SYS_tgkill | libc::SYS_rt_tgsigqueueinfo => scalar_int(notification.args[2]),
113121
_ => return Err(io::Error::from_raw_os_error(libc::ENOSYS)),
114122
};
115-
if target <= 0 || claimed_group.is_some_and(|group| group <= 0) {
116-
return Err(io::Error::from_raw_os_error(libc::EPERM));
117-
}
118123
if !(0..=64).contains(&signal) {
119124
return Err(io::Error::from_raw_os_error(libc::EINVAL));
120125
}
121-
refuse_resume_while_frozen(signal, workload_frozen)?;
122-
let target = u32::try_from(target).map_err(|_| io::Error::from_raw_os_error(libc::ESRCH))?;
123-
let target_group = thread_group_id(target)?;
124-
if target_group == sandbox_tgid || target_group == 0 {
125-
return Err(io::Error::from_raw_os_error(libc::EPERM));
126-
}
127-
if claimed_group.is_some_and(|group| u32::try_from(group).ok() != Some(target_group)) {
128-
// The kernel reports a thread outside the named group as missing.
129-
return Err(io::Error::from_raw_os_error(libc::ESRCH));
130-
}
131126
listener.validate_id(notification.id)?;
127+
refuse_resume_while_frozen(signal, workload_frozen)?;
132128
listener.respond_continue(notification.id)
133129
}
134130

135131
/// While the boundary has stopped the workload for supervisor recovery, a
136132
/// workload process that was not yet stopped must not resume the others.
137-
fn refuse_resume_while_frozen(signal: i32, workload_frozen: bool) -> io::Result<()> {
138-
if workload_frozen && signal == libc::SIGCONT {
133+
///
134+
/// The flag is read immediately before delivery. The freezer does not wait
135+
/// for in-flight notifications, so a signal already past this check when the
136+
/// freeze begins can still be delivered.
137+
fn refuse_resume_while_frozen(signal: i32, workload_frozen: &AtomicBool) -> io::Result<()> {
138+
if signal == libc::SIGCONT && workload_frozen.load(Ordering::Acquire) {
139139
return Err(io::Error::from_raw_os_error(libc::EPERM));
140140
}
141141
Ok(())
@@ -205,8 +205,13 @@ mod tests {
205205
.recv_timeout(std::time::Duration::from_secs(5))
206206
.unwrap();
207207
let notification = listener.receive().unwrap();
208-
let error =
209-
mediate_process_signal(&listener, notification, std::process::id(), false).unwrap_err();
208+
let error = mediate_process_signal(
209+
&listener,
210+
notification,
211+
std::process::id(),
212+
&AtomicBool::new(false),
213+
)
214+
.unwrap_err();
210215
assert_eq!(error.raw_os_error(), Some(libc::EPERM));
211216
listener
212217
.respond_errno(notification.id, libc::EPERM)

‎crates/openshell-isolation-interface/src/linux/seccomp_notify.rs‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,10 @@ impl NotificationProbeReport {
170170
/// Owned listener returned by `SECCOMP_FILTER_FLAG_NEW_LISTENER`.
171171
///
172172
/// The listener is installed without `WAIT_KILLABLE_RECV`, so mediation is the
173-
/// same on every kernel. A signal can interrupt a notified syscall and the
174-
/// kernel then restarts it; broker handlers check the notification is still
175-
/// live before acting and answer a repeated operation as the kernel would.
173+
/// same on every kernel. A signal can interrupt a notified syscall, which the
174+
/// kernel then restarts or fails with `EINTR`; broker handlers check the
175+
/// notification is still live before acting and answer a repeated operation
176+
/// as the kernel would.
176177
pub struct NotificationListener {
177178
fd: OwnedFd,
178179
}
@@ -705,6 +706,10 @@ fn build_filter(syscalls: &[i64]) -> io::Result<Vec<libc::sock_filter>> {
705706
append_sendto_filter(&mut program)?;
706707
continue;
707708
}
709+
if matches!(syscall, libc::SYS_tgkill | libc::SYS_rt_tgsigqueueinfo) {
710+
append_resume_signal_filter(&mut program, syscall)?;
711+
continue;
712+
}
708713
let syscall = u32::try_from(syscall)
709714
.map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "negative syscall number"))?;
710715
program.extend([
@@ -745,6 +750,27 @@ fn append_sendto_filter(program: &mut Vec<libc::sock_filter>) -> io::Result<()>
745750
Ok(())
746751
}
747752

753+
/// Notify a thread-group signal only when it sends `SIGCONT`, which the broker
754+
/// refuses while the workload is frozen. Every other signal (for example Go's
755+
/// preemption signal) stays in the kernel.
756+
fn append_resume_signal_filter(
757+
program: &mut Vec<libc::sock_filter>,
758+
syscall: i64,
759+
) -> io::Result<()> {
760+
let syscall = u32::try_from(syscall)
761+
.map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "negative syscall number"))?;
762+
let resume = u32::try_from(libc::SIGCONT)
763+
.map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "negative signal number"))?;
764+
program.extend([
765+
jump(BPF_JMP_JEQ_K, syscall, 0, 4),
766+
stmt(BPF_LD_W_ABS, argument_word_offset(2, 0)),
767+
jump(BPF_JMP_JEQ_K, resume, 0, 1),
768+
stmt(BPF_RET_K, SECCOMP_RET_USER_NOTIF),
769+
stmt(BPF_RET_K, SECCOMP_RET_ALLOW),
770+
]);
771+
Ok(())
772+
}
773+
748774
const fn argument_word_offset(argument: u32, word: u32) -> u32 {
749775
SECCOMP_DATA_ARGS_OFFSET + argument * 8 + word * 4
750776
}

0 commit comments

Comments
 (0)