Skip to content

Commit dc85fde

Browse files
committed
fix(sandbox): address review — legacy read-only listener mode + cancellation invariant
Follow-up to the PR review (GATOR-de00bfcc-01 / mrunalp): make the < 5.19 fallback cancellation-safe instead of racing broker writes. - Record an explicit ListenerMode (Killable vs LegacyReadOnly); add writes_disabled()/mode() and emit the selected mode in qualification output (seccomp_listener_mode). - Centralize task-memory output writes behind NotificationListener:: write_task_output; in LegacyReadOnly mode getpeername, accept/accept4 with a non-null address, and sendmmsg length write-backs fail closed with EOPNOTSUPP. accept with a null address, socket/connect/bind/listen/sendto/sendmsg keep working (copied inputs, scalar responses, atomic ADDFD_SEND). - Enforce the launch invariant `cancellation || task_memory_writes_disabled` in SandboxConfirmEvidence::validate() rather than dropping cancellation unconditionally; add task_memory_writes_disabled to SeccompEvidence. - Add per-path fail-closed tests (write_task_output, write_socket_addr, a real plain listener installed on a modern kernel) and confirmation-invariant tests. - Correct the flag-semantics comments and document both modes plus the reduced legacy syscall compatibility in architecture/sandbox.md. Signed-off-by: Akram <akram.benaissi@gmail.com>
1 parent de00bfc commit dc85fde

8 files changed

Lines changed: 260 additions & 21 deletions

File tree

‎architecture/sandbox.md‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,31 @@ The sandbox reserves `SIGUSR2` with a non-restarting no-op handler for these
169169
broker threads; startup rejects a conflicting handler. This signal disposition
170170
is process-global kernel state, while registrations and cancellation state are
171171
owned by the broker. Workload exec resets the caught handler to its default.
172-
This sandbox runtime requires Linux 6.2 or newer for Landlock ABI v3 and treats
173-
`SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV` as mandatory so cancelled
174-
notifications cannot race task-memory writes.
172+
This sandbox runtime requires Landlock ABI v3 (Linux 6.2, or an equivalent
173+
vendor backport). The seccomp listener is installed in one of two cancellation
174+
modes, and the launch confirmation enforces the invariant
175+
`cancellation || task_memory_writes_disabled`:
176+
177+
- **Killable** (`SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV`, Linux 5.19+): the
178+
notified workload thread waits kill-only, so a non-fatal signal cannot resume
179+
a mediated syscall between notification validation and the broker's result
180+
write. Full mediation, including task-memory output writes.
181+
- **LegacyReadOnly** (kernels < 5.19, e.g. RHEL 9.x / 5.14): the flag is
182+
unavailable (`EINVAL`), so the listener falls back to a plain notifier and the
183+
broker refuses every task-memory *output* write to stay cancellation-safe.
184+
Concretely, in this mode `getpeername`, `accept`/`accept4` **with a non-null
185+
peer-address argument**, and `sendmmsg` paths that write per-message lengths
186+
fail closed with `EOPNOTSUPP`. `accept` with a null address, and socket
187+
creation, `connect`, `bind`, `listen`, `sendto`, and `sendmsg` continue to
188+
work — they use copied inputs, scalar responses, or atomic `ADDFD_SEND`, none
189+
of which write into workload memory. Some server workloads whose accept
190+
wrappers request the peer address will therefore not run until the kernel
191+
provides `WAIT_KILLABLE_RECV` (a distribution backport); outbound-oriented
192+
workloads are unaffected.
193+
194+
Input mediation, DNS/TCP authorization, and outer-fence enforcement are
195+
identical in both modes. The selected mode is emitted in the sandbox
196+
qualification output (`seccomp_listener_mode`).
175197

176198
DNS uses an exact sandbox-local resolver at `127.0.0.53:53`. The driver sets the
177199
nameserver and permits an unprivileged bind to port 53. UDP and TCP DNS requests

‎crates/openshell-isolation-interface/src/contract.rs‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,10 @@ pub struct SeccompEvidence {
423423
pub task_memory_read: bool,
424424
pub task_memory_write: bool,
425425
pub cancellation: bool,
426+
/// The broker disables every task-memory *output* write (legacy plain
427+
/// listener on kernels < 5.19). Paired with `cancellation` in the
428+
/// launch-safety invariant: one of the two must hold.
429+
pub task_memory_writes_disabled: bool,
426430
}
427431

428432
/// Driver-owned evidence that the mandatory outer network fence is installed.
@@ -562,11 +566,15 @@ impl SandboxConfirmEvidence {
562566
&& self.seccomp.proc_fd_identity
563567
&& self.seccomp.task_memory_read
564568
&& self.seccomp.task_memory_write
565-
// `cancellation` (seccomp WAIT_KILLABLE_RECV, Linux 5.19+) is a
566-
// robustness property, not a containment prerequisite: on older
567-
// kernels (e.g. RHEL 9.x / 5.14) the sandbox falls back to a plain
568-
// listener and reports cancellation=false. Containment still holds
569-
// via the listener itself, so launch is not gated on cancellation.
569+
// Cancellation-safety invariant: `cancellation || writes_disabled`.
570+
// Either the listener keeps the notified workload thread kill-only
571+
// (WAIT_KILLABLE_RECV / cancellation, Linux 5.19+), or the broker
572+
// disables every task-memory output write (legacy plain listener on
573+
// kernels < 5.19). One of the two MUST hold so a signal-resumed
574+
// syscall can never race a privileged write into workload memory.
575+
// We do not remove cancellation unconditionally; we require the
576+
// legacy read-only guarantee in its place.
577+
&& (self.seccomp.cancellation || self.seccomp.task_memory_writes_disabled)
570578
&& self.landlock_abi >= 3
571579
&& self.landlock_allow_deny
572580
&& self.udp_dns_round_trip

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

Lines changed: 141 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,13 +177,52 @@ impl NotificationProbeReport {
177177
}
178178
}
179179

180+
/// Cancellation posture a listener was installed with.
181+
///
182+
/// `WAIT_KILLABLE_RECV` (Linux 5.19+) keeps the notified workload thread in a
183+
/// kill-only wait so a non-fatal signal cannot resume the mediated syscall
184+
/// after the broker has validated the notification. A plain listener has no
185+
/// such guarantee, so it runs read-only: the broker must refuse every
186+
/// task-memory *output* write to stay cancellation-safe.
187+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
188+
pub enum ListenerMode {
189+
/// Modern kernel: `WAIT_KILLABLE_RECV` active; full mediation including
190+
/// task-memory output writes.
191+
Killable,
192+
/// Legacy kernel (< 5.19): plain listener; task-memory output writes are
193+
/// disabled so a resumed syscall cannot race a broker write.
194+
LegacyReadOnly,
195+
}
196+
197+
impl ListenerMode {
198+
/// Stable identifier for qualification output and diagnostics.
199+
#[must_use]
200+
pub fn as_str(self) -> &'static str {
201+
match self {
202+
Self::Killable => "killable",
203+
Self::LegacyReadOnly => "legacy_read_only",
204+
}
205+
}
206+
}
207+
180208
/// Owned listener returned by `SECCOMP_FILTER_FLAG_NEW_LISTENER`.
181209
pub struct NotificationListener {
182210
fd: OwnedFd,
183211
wait_killable_recv: bool,
184212
}
185213

186214
impl NotificationListener {
215+
/// Construct a listener from an already-owned notification descriptor in a
216+
/// specific mode. Intended for tests that must exercise the legacy
217+
/// read-only fail-closed paths without a `< 5.19` kernel.
218+
#[must_use]
219+
pub fn from_fd_with_mode(fd: OwnedFd, mode: ListenerMode) -> Self {
220+
Self {
221+
fd,
222+
wait_killable_recv: matches!(mode, ListenerMode::Killable),
223+
}
224+
}
225+
187226
/// Raw listener descriptor for readiness integration and diagnostics.
188227
#[must_use]
189228
pub fn as_raw_fd(&self) -> RawFd {
@@ -196,6 +235,23 @@ impl NotificationListener {
196235
self.wait_killable_recv
197236
}
198237

238+
/// The cancellation mode this listener was installed with.
239+
#[must_use]
240+
pub fn mode(&self) -> ListenerMode {
241+
if self.wait_killable_recv {
242+
ListenerMode::Killable
243+
} else {
244+
ListenerMode::LegacyReadOnly
245+
}
246+
}
247+
248+
/// Whether broker task-memory output writes are disabled for this listener.
249+
/// True exactly in `LegacyReadOnly` mode (no `WAIT_KILLABLE_RECV`).
250+
#[must_use]
251+
pub fn writes_disabled(&self) -> bool {
252+
matches!(self.mode(), ListenerMode::LegacyReadOnly)
253+
}
254+
199255
/// Receive the next kernel notification.
200256
pub fn receive(&self) -> io::Result<Notification> {
201257
let mut raw = RawNotification::default();
@@ -223,6 +279,34 @@ impl NotificationListener {
223279
Ok(())
224280
}
225281

282+
/// Write broker-produced output into the notified task's memory, closing
283+
/// the validation-to-write race that a plain listener cannot.
284+
///
285+
/// In `Killable` mode `WAIT_KILLABLE_RECV` keeps the notified workload
286+
/// thread in a kill-only wait, so a non-fatal signal cannot resume the
287+
/// mediated syscall between `validate_id` and this write. In
288+
/// `LegacyReadOnly` mode (kernels < 5.19) there is no such guarantee: a
289+
/// resumed syscall could repurpose the target buffer while the privileged
290+
/// broker writes through the captured tid and pointer — via `/proc/<tid>/mem`
291+
/// even into pages the workload has since made read-only. There is no way to
292+
/// close that window without the flag, so this fails closed (`EOPNOTSUPP`)
293+
/// rather than racing. Callers must route every task-memory *output* write
294+
/// through this method; input reads never write workload memory and are
295+
/// unaffected.
296+
pub fn write_task_output(
297+
&self,
298+
id: u64,
299+
tid: u32,
300+
address: u64,
301+
data: &[u8],
302+
) -> io::Result<()> {
303+
if self.writes_disabled() {
304+
return Err(io::Error::from_raw_os_error(libc::EOPNOTSUPP));
305+
}
306+
self.validate_id(id)?;
307+
crate::linux::task_memory::write_exact(tid, address, data)
308+
}
309+
226310
/// Return a successful scalar result to the notifying syscall.
227311
pub fn respond_value(&self, id: u64, value: i64) -> io::Result<()> {
228312
self.validate_id(id)?;
@@ -330,13 +414,15 @@ pub fn install_listener(syscalls: &[i64]) -> io::Result<NotificationListener> {
330414
verify_notification_sizes()?;
331415
set_no_new_privileges()?;
332416

333-
// WAIT_KILLABLE_RECV (Linux 5.19+) makes the supervisor's notification
334-
// receive interruptible by a fatal signal. Kernels older than 5.19 (for
335-
// example RHEL 9.x / 5.14 nodes) reject the flag with EINVAL. Rather than
336-
// refusing to start there, fall back to a plain listener: the notification
337-
// receive is then uninterruptible, but the sandbox is otherwise fully
338-
// functional. The resulting listener records `wait_killable_recv = false`
339-
// so callers can observe the degraded cancellation semantics.
417+
// WAIT_KILLABLE_RECV (Linux 5.19+) keeps the *notified workload thread* in
418+
// a kill-only wait while the broker services its syscall, so a non-fatal
419+
// signal cannot resume the syscall and repurpose its buffers underneath a
420+
// pending broker write. Kernels older than 5.19 (for example RHEL 9.x /
421+
// 5.14 nodes) reject the flag with EINVAL. Rather than refusing to start
422+
// there, fall back to a plain listener so the sandbox boots; the resulting
423+
// listener records `wait_killable_recv = false`, and the broker then fails
424+
// closed on every task-memory output write (see `write_task_output`)
425+
// instead of racing them. Input mediation is unaffected.
340426
match install_listener_with_flags(syscalls, true) {
341427
Ok(listener) => Ok(listener),
342428
Err(error) if error.raw_os_error() == Some(libc::EINVAL) => {
@@ -939,4 +1025,52 @@ mod tests {
9391025
.expect_err("zero errno must fail");
9401026
assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
9411027
}
1028+
1029+
#[test]
1030+
fn plain_listener_fails_closed_on_output_write() {
1031+
// A LegacyReadOnly listener (kernels < 5.19) must refuse every
1032+
// task-memory output write rather than race a resumed syscall. The
1033+
// guard short-circuits before touching the descriptor or workload
1034+
// memory, so a dup of stderr is a sufficient stand-in.
1035+
// SAFETY: dup takes one valid descriptor and returns a new descriptor
1036+
// or a negative error without modifying memory.
1037+
let duplicated = unsafe { libc::dup(libc::STDERR_FILENO) };
1038+
assert!(duplicated >= 0, "duplicate stderr for validation test");
1039+
// SAFETY: successful dup returned a new owned descriptor.
1040+
let listener = NotificationListener::from_fd_with_mode(
1041+
unsafe { OwnedFd::from_raw_fd(duplicated) },
1042+
ListenerMode::LegacyReadOnly,
1043+
);
1044+
assert!(listener.writes_disabled());
1045+
assert_eq!(listener.mode(), ListenerMode::LegacyReadOnly);
1046+
let error = listener
1047+
.write_task_output(1, 0, 0, &[0_u8; 4])
1048+
.expect_err("plain listener must reject output writes");
1049+
assert_eq!(error.raw_os_error(), Some(libc::EOPNOTSUPP));
1050+
}
1051+
1052+
#[test]
1053+
fn real_plain_listener_disables_output_writes() {
1054+
// Deliberately install a plain NEW_LISTENER (no WAIT_KILLABLE_RECV)
1055+
// even on a modern CI kernel and prove the broker write path fails
1056+
// closed on the real listener object.
1057+
set_no_new_privileges().expect("no_new_privs for listener install");
1058+
let listener = install_listener_with_flags(&[libc::SYS_getppid], false)
1059+
.expect("install plain listener");
1060+
assert_eq!(listener.mode(), ListenerMode::LegacyReadOnly);
1061+
assert!(listener.writes_disabled());
1062+
let error = listener
1063+
.write_task_output(1, 0, 0, &[0_u8; 4])
1064+
.expect_err("plain listener must reject output writes");
1065+
assert_eq!(error.raw_os_error(), Some(libc::EOPNOTSUPP));
1066+
}
1067+
1068+
#[test]
1069+
fn killable_listener_enables_output_writes() {
1070+
set_no_new_privileges().expect("no_new_privs for listener install");
1071+
let listener = install_listener_with_flags(&[libc::SYS_getppid], true)
1072+
.expect("install killable listener");
1073+
assert_eq!(listener.mode(), ListenerMode::Killable);
1074+
assert!(!listener.writes_disabled());
1075+
}
9421076
}

‎crates/openshell-isolation-interface/tests/backend_conformance.rs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,7 @@ fn confirmation_evidence() -> SandboxConfirmEvidence {
395395
task_memory_read: true,
396396
task_memory_write: true,
397397
cancellation: true,
398+
task_memory_writes_disabled: false,
398399
},
399400
landlock_abi: 3,
400401
landlock_allow_deny: true,
@@ -551,6 +552,37 @@ fn confirmation_constructor_rejects_incomplete_evidence() {
551552
assert!(matches!(result, Err(BackendError::Confirm(_))));
552553
}
553554

555+
#[test]
556+
fn confirmation_accepts_legacy_read_only_listener() {
557+
// Cancellation-safety invariant: cancellation OR writes-disabled. A legacy
558+
// plain listener (< 5.19) has no cancellation but disables broker output
559+
// writes, so it satisfies the invariant and must be accepted.
560+
let mut evidence = confirmation_evidence();
561+
evidence.seccomp.cancellation = false;
562+
evidence.seccomp.task_memory_writes_disabled = true;
563+
let result = ConfirmedBoundary::try_new(
564+
Box::new(MockReady::<Primary> { _k: PhantomData }),
565+
evidence,
566+
&workload_identity(),
567+
);
568+
assert!(result.is_ok());
569+
}
570+
571+
#[test]
572+
fn confirmation_rejects_plain_listener_with_writes_enabled() {
573+
// The unsafe configuration this PR must never admit: no cancellation AND
574+
// task-memory output writes still enabled (the cancellation race).
575+
let mut evidence = confirmation_evidence();
576+
evidence.seccomp.cancellation = false;
577+
evidence.seccomp.task_memory_writes_disabled = false;
578+
let result = ConfirmedBoundary::try_new(
579+
Box::new(MockReady::<Primary> { _k: PhantomData }),
580+
evidence,
581+
&workload_identity(),
582+
);
583+
assert!(matches!(result, Err(BackendError::Confirm(_))));
584+
}
585+
554586
#[test]
555587
fn confirmation_constructor_rejects_another_workload_identity() {
556588
let expected = ResolvedWorkloadIdentity::new(

‎crates/openshell-sandbox-backend/src/runtime.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2386,6 +2386,7 @@ mod tests {
23862386
task_memory_read: true,
23872387
task_memory_write: true,
23882388
cancellation: true,
2389+
task_memory_writes_disabled: false,
23892390
},
23902391
landlock_abi: 3,
23912392
landlock_allow_deny: true,

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4211,6 +4211,7 @@ mod linux {
42114211
task_memory_read: true,
42124212
task_memory_write: true,
42134213
cancellation: true,
4214+
task_memory_writes_disabled: false,
42144215
},
42154216
landlock_abi: 6,
42164217
landlock_allow_deny: true,

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ struct QualificationReport {
146146
tcp_allow_round_trip: bool,
147147
tcp_deny_round_trip: bool,
148148
wait_killable_recv: bool,
149+
/// Selected seccomp listener cancellation mode: "killable" (>= 5.19) or
150+
/// "legacy_read_only" (< 5.19, broker output writes disabled).
151+
seccomp_listener_mode: &'static str,
152+
/// Whether the broker disables task-memory output writes (legacy mode).
153+
task_memory_writes_disabled: bool,
149154
}
150155

151156
#[cfg(target_os = "linux")]
@@ -237,6 +242,12 @@ fn qualify_runtime() -> Result<(openshell_sandbox::RuntimeQualification, Qualifi
237242
tcp_allow_round_trip: true,
238243
tcp_deny_round_trip: true,
239244
wait_killable_recv: notification.wait_killable_recv,
245+
seccomp_listener_mode: if notification.wait_killable_recv {
246+
"killable"
247+
} else {
248+
"legacy_read_only"
249+
},
250+
task_memory_writes_disabled: !notification.wait_killable_recv,
240251
};
241252
let qualification = openshell_sandbox::RuntimeQualification {
242253
seccomp: openshell_isolation_interface::contract::SeccompEvidence {
@@ -249,6 +260,9 @@ fn qualify_runtime() -> Result<(openshell_sandbox::RuntimeQualification, Qualifi
249260
task_memory_read: notification.task_memory_copy(),
250261
task_memory_write: notification.task_memory_copy(),
251262
cancellation: notification.wait_killable_recv,
263+
// Legacy plain listener (< 5.19) disables broker output writes;
264+
// satisfies the `cancellation || writes_disabled` launch invariant.
265+
task_memory_writes_disabled: !notification.wait_killable_recv,
252266
},
253267
landlock_abi,
254268
landlock_allow_deny: true,

0 commit comments

Comments
 (0)