From aac73beb41f16e11cd4e0b2beea1ac2952248ace Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 09:50:15 -0700 Subject: [PATCH 01/10] chroot: drop the trailing slash getcwd reports at a mount point host_to_virtual mapped a host path back to its virtual path by joining the remainder of the mount source onto the mount's virtual base, but a child sitting exactly on the mount point leaves an empty remainder, and PathBuf::join("") appends a separator. getcwd then copied "/proc/" into the child instead of "/proc". Path comparison ignores a trailing separator, so nothing caught this internally; only the bytes handed to the child differ. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/resolve.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/crates/sandlock-core/src/chroot/resolve.rs b/crates/sandlock-core/src/chroot/resolve.rs index 8ac991c7..20427a0d 100644 --- a/crates/sandlock-core/src/chroot/resolve.rs +++ b/crates/sandlock-core/src/chroot/resolve.rs @@ -54,7 +54,14 @@ pub fn host_to_virtual( .max_by_key(|(_, source)| source.as_os_str().len()) .map(|(virtual_base, source)| { // strip_prefix cannot fail: the filter above already matched it. - virtual_base.join(host_path.strip_prefix(source).expect("prefix matched")) + let rest = host_path.strip_prefix(source).expect("prefix matched"); + // join("") appends a separator, so the mount point itself would + // render as "/proc/" and reach the child that way through getcwd. + if rest.as_os_str().is_empty() { + virtual_base.to_path_buf() + } else { + virtual_base.join(rest) + } }) } @@ -263,6 +270,17 @@ mod tests { ); } + #[test] + fn host_to_virtual_at_a_mount_point_renders_without_a_trailing_slash() { + // The rendered bytes matter, not just Path equality (which ignores a + // trailing separator): getcwd copies this string into the child, and a + // child sitting exactly on a mount point saw "/proc/". + let mounts = vec![(PathBuf::from("/proc"), PathBuf::from("/proc"))]; + let virtual_path = host_to_virtual(Path::new("/rootfs"), &mounts, Path::new("/proc")) + .expect("a mount point maps to its own virtual path"); + assert_eq!(virtual_path.to_string_lossy(), "/proc"); + } + #[test] fn test_confine_escape_attempt() { // Deeply nested .. should always clamp at / From 472b8b04ab8f87b217702b0e6e70669ac2121b21 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 10:00:14 -0700 Subject: [PATCH 02/10] core: track a per-task cwd that follows fs_struct sharing The supervisor cannot chdir a child on its behalf: only the kernel can update the calling task's fs_struct. The chroot handler works around that by rewriting the child's path argument to /proc/self/fd/N, which is what makes issue #178's short paths fail. Replacing that rewrite means the supervisor has to know where each task believes it is, so add the storage first. The cell hangs off ProcessIndex rather than PerProcessState because path resolution reads it from synchronous helpers, and it is shared behind an Arc the way the kernel shares fs_struct: a thread joins its leader's cell so a sibling's chdir is visible, while anything else copies its parent's value at registration the way fork(2) does. Seeding a child needs the parent pid, so notif.rs's private read_ppid moves in beside the other /proc identity readers rather than being duplicated. Signed-off-by: Cong Wang --- crates/sandlock-core/src/seccomp/notif.rs | 17 +-- crates/sandlock-core/src/seccomp/state.rs | 139 +++++++++++++++++++++- 2 files changed, 139 insertions(+), 17 deletions(-) diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index e77f890e..c4f34482 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -1580,18 +1580,6 @@ fn syscall_category(nr: i64) -> crate::policy_fn::SyscallCategory { } } -/// Read the parent PID from /proc/{pid}/stat. -fn read_ppid(pid: u32) -> Option { - let stat = std::fs::read_to_string(format!("/proc/{}/stat", pid)).ok()?; - // Format: "pid (comm) state ppid ..." - // Find the closing ')' then split the rest - let close_paren = stat.rfind(')')?; - let rest = &stat[close_paren + 2..]; // skip ") " - let fields: Vec<&str> = rest.split_whitespace().collect(); - // fields[0] = state, fields[1] = ppid - fields.get(1)?.parse().ok() -} - /// Read a NUL-terminated path from child memory (up to PATH_MAX bytes). fn read_path_for_event(notif: &SeccompNotif, addr: u64, notif_fd: RawFd) -> Option { if addr == 0 { return None; } @@ -1895,7 +1883,10 @@ async fn emit_policy_event( let denied = matches!(action, NotifAction::Errno(_)); let name = syscall_name(nr); let category = syscall_category(nr); - let parent_pid = read_ppid(notif.pid); + let parent_pid = i32::try_from(notif.pid) + .ok() + .and_then(crate::seccomp::state::read_ppid) + .and_then(|p| u32::try_from(p).ok()); // Extract metadata based on syscall type. // diff --git a/crates/sandlock-core/src/seccomp/state.rs b/crates/sandlock-core/src/seccomp/state.rs index 03bda601..40f8974c 100644 --- a/crates/sandlock-core/src/seccomp/state.rs +++ b/crates/sandlock-core/src/seccomp/state.rs @@ -4,6 +4,7 @@ // `ProcessIndex`; cleanup on exit is just dropping the entry's `Arc`. use std::collections::{HashMap, HashSet}; +use std::path::PathBuf; use std::sync::Arc; use tokio::sync::Mutex as AsyncMutex; @@ -98,6 +99,17 @@ pub(crate) fn read_tgid_of_tid(tid: i32) -> Option { None } +/// Read the parent pid (field 4 of `/proc//stat`) for `pid`. +/// `None` when the task is gone or /proc is unreadable. +pub(crate) fn read_ppid(pid: i32) -> Option { + let stat = std::fs::read_to_string(format!("/proc/{}/stat", pid)).ok()?; + // Skip past "pid (comm)": comm may contain spaces and parens, but the + // last ") " in the line ends it. The first token after it is the state, + // and the parent pid follows. + let rest = stat.rsplit_once(") ")?.1; + rest.split_whitespace().nth(1)?.parse().ok() +} + /// Read the process start time (field 22 of /proc//stat) for `pid`. /// Returns None if the process is gone or /proc is not readable. pub(crate) fn read_pid_start_time(pid: i32) -> Option { @@ -174,6 +186,17 @@ pub struct ProcessIndex { inner: std::sync::RwLock>, } +/// A task's current directory as the sandbox believes it to be: the +/// path `getcwd` should report, in whatever namespace the child sees +/// (the virtual path under chroot, the real path otherwise). +/// +/// `None` means the task has never moved, so the kernel's own cwd is +/// still authoritative. Shared behind an `Arc` the way the kernel +/// shares `fs_struct`, so a chdir in one thread is seen by its +/// siblings. Kept outside `PerProcessState` (and behind a std mutex) +/// because path resolution reads it from synchronous helpers. +pub type SharedCwd = Arc>>; + #[derive(Clone)] struct ProcessEntry { key: PidKey, @@ -183,6 +206,7 @@ struct ProcessEntry { /// index's read lock. tgid: i32, state: Arc>, + cwd: SharedCwd, } impl ProcessIndex { @@ -200,18 +224,67 @@ impl ProcessIndex { pub fn register(&self, pid: i32) -> Option { let start_time = read_pid_start_time(pid)?; let key = PidKey { pid, start_time }; + // Unreadable /proc means the task is its own address space as far + // as accounting is concerned: better local than misrouted. + let tgid = read_tgid_of_tid(pid).unwrap_or(pid); let entry = ProcessEntry { key, - // Unreadable /proc means the task is its own address space - // as far as accounting is concerned: better local than - // misrouted. - tgid: read_tgid_of_tid(pid).unwrap_or(pid), + tgid, state: Arc::new(AsyncMutex::new(PerProcessState::default())), + cwd: self.inherited_cwd(pid, tgid), }; self.inner.write().ok()?.insert(pid, entry); Some(key) } + /// The cwd cell a task starts life with. + /// + /// A thread joins its leader's cell, because the kernel hands + /// pthreads a shared `fs_struct` and one thread's chdir moves its + /// siblings. Anything else copies the parent's current value, which + /// is what `fork(2)` does. Thread-group membership stands in for + /// `CLONE_FS` here, the same approximation `addr_space_state` makes + /// for `CLONE_VM`: a bare `clone(CLONE_FS)` without `CLONE_THREAD` + /// gets a private copy instead of sharing. An untracked parent + /// leaves the child at None, which falls back to the kernel's cwd. + fn inherited_cwd(&self, pid: i32, tgid: i32) -> SharedCwd { + let ppid = if tgid == pid { read_ppid(pid) } else { None }; + let Ok(guard) = self.inner.read() else { + return SharedCwd::default(); + }; + if tgid != pid { + if let Some(leader) = guard.get(&tgid) { + return Arc::clone(&leader.cwd); + } + } + let parent_cwd = ppid + .and_then(|p| guard.get(&p)) + .and_then(|e| e.cwd.lock().ok().and_then(|c| c.clone())); + Arc::new(std::sync::Mutex::new(parent_cwd)) + } + + /// The cwd this task believes it is in, or None when the task is + /// untracked or has never moved. + pub fn virtual_cwd(&self, pid: i32) -> Option { + let guard = self.inner.read().ok()?; + let cwd = guard.get(&pid)?.cwd.lock().ok()?.clone(); + cwd + } + + /// Record where this task now believes it is. Silently does nothing + /// for an untracked pid: the fallback is the kernel's own cwd. + pub fn set_virtual_cwd(&self, pid: i32, cwd: PathBuf) { + let cell = match self.inner.read() { + Ok(guard) => guard.get(&pid).map(|e| Arc::clone(&e.cwd)), + Err(_) => None, + }; + if let Some(cell) = cell { + if let Ok(mut slot) = cell.lock() { + *slot = Some(cwd); + } + } + } + /// Look up the canonical PidKey for a notification's raw pid. /// Returns None if this pid was never registered (e.g. pidfd_open /// failed at fork) — callers should fall back to a no-op. @@ -661,6 +734,62 @@ mod tests { assert_eq!(idx.max_pid(), None); } + #[test] + fn threads_of_one_process_share_one_cwd() { + // The kernel gives pthreads a shared fs_struct, so a chdir in one + // thread moves its siblings. Registering a tid must join the leader's + // cwd rather than start a private one. + let leader = unsafe { libc::getpid() }; + let idx = ProcessIndex::new(); + idx.register(leader).expect("leader registers"); + + let (tid_tx, tid_rx) = std::sync::mpsc::channel(); + let (stop_tx, stop_rx) = std::sync::mpsc::channel::<()>(); + let thread = std::thread::spawn(move || { + let tid = unsafe { libc::syscall(libc::SYS_gettid) } as i32; + tid_tx.send(tid).unwrap(); + // Stay alive: register() reads /proc//stat. + let _ = stop_rx.recv(); + }); + let tid = tid_rx.recv().unwrap(); + idx.register(tid).expect("thread registers"); + + idx.set_virtual_cwd(tid, PathBuf::from("/workspace")); + assert_eq!(idx.virtual_cwd(leader), Some(PathBuf::from("/workspace"))); + + let _ = stop_tx.send(()); + thread.join().unwrap(); + } + + #[test] + fn a_child_copies_the_parent_cwd_instead_of_sharing_it() { + // fork(2) copies fs_struct: the child starts where the parent stood, + // and its later chdir must not move the parent. + let parent = unsafe { libc::getpid() }; + let idx = ProcessIndex::new(); + idx.register(parent).expect("parent registers"); + idx.set_virtual_cwd(parent, PathBuf::from("/workspace")); + + let child = unsafe { libc::fork() }; + assert!(child >= 0, "fork failed"); + if child == 0 { + // Async-signal-safe only: sleep, then leave without unwinding. + let ts = libc::timespec { tv_sec: 30, tv_nsec: 0 }; + unsafe { libc::nanosleep(&ts, std::ptr::null_mut()) }; + unsafe { libc::_exit(0) }; + } + + idx.register(child).expect("child registers"); + assert_eq!(idx.virtual_cwd(child), Some(PathBuf::from("/workspace"))); + + idx.set_virtual_cwd(child, PathBuf::from("/tmp")); + assert_eq!(idx.virtual_cwd(parent), Some(PathBuf::from("/workspace"))); + + unsafe { libc::kill(child, libc::SIGKILL) }; + let mut status = 0; + unsafe { libc::waitpid(child, &mut status, 0) }; + } + #[test] fn process_index_register_overwrites_stale_entry_for_recycled_pid() { let self_pid = unsafe { libc::getpid() }; @@ -672,6 +801,7 @@ mod tests { key: stale_key, tgid: self_pid, state: Arc::new(AsyncMutex::new(PerProcessState::default())), + cwd: SharedCwd::default(), }; idx.inner.write().unwrap().insert(self_pid, stale); } @@ -731,6 +861,7 @@ mod tests { key: stale_key, tgid: self_pid, state: Arc::new(AsyncMutex::new(PerProcessState::default())), + cwd: SharedCwd::default(), }; idx.inner.write().unwrap().insert(self_pid, stale); From 66e0f2c2e185ac46b1b25f1901d81f93d9ce9e93 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 10:18:23 -0700 Subject: [PATCH 03/10] chroot: service chdir by tracking the cwd instead of rewriting the path A chdir to a short absolute path failed with ENAMETOOLONG under chroot: "/proc/self/fd/N" is 16 bytes and the handler could only redirect the child through it by overwriting the path buffer the child had already passed, so anything shorter than 15 characters was refused. That hit every common mount point (cd /root, cd /tmp, cd /workspace) and the virtual root itself, while ls and open on the same paths worked, and it broke mkdir -p, which chdirs through each component. Reported as #178. The rewrite cannot be made to fit: seccomp-notify cannot change the syscall arguments, so the replacement has to live in the child's own buffer. Resolve the target on-behalf and record the result instead, and let every AT_FDCWD resolution, getcwd, and the /proc virtualization read that notion rather than /proc//cwd, which now stays where exec left it. fchdir moves the real cwd without passing a path, so it joins the notified set to keep the tracked value in step. Dropping the rewrite also drops what it cost: the TOCTOU window where the kernel re-read a path we had just written (issue #27), and a force-write through /proc//mem that punched past page protections and left a .rodata path literal permanently corrupted in the child. Signed-off-by: Cong Wang --- crates/sandlock-core/src/ca_inject.rs | 3 +- crates/sandlock-core/src/chroot/dispatch.rs | 263 +++++++++--------- crates/sandlock-core/src/procfs.rs | 54 ++-- crates/sandlock-core/src/random.rs | 3 +- crates/sandlock-core/src/seccomp/dispatch.rs | 54 ++-- crates/sandlock-core/src/seccomp/state.rs | 61 +++- crates/sandlock-core/src/seccomp_plan.rs | 4 + .../tests/integration/test_chroot.rs | 131 +++++++++ tests/rootfs-helper.c | 27 ++ 9 files changed, 399 insertions(+), 201 deletions(-) diff --git a/crates/sandlock-core/src/ca_inject.rs b/crates/sandlock-core/src/ca_inject.rs index 827ec29c..b6433f83 100644 --- a/crates/sandlock-core/src/ca_inject.rs +++ b/crates/sandlock-core/src/ca_inject.rs @@ -41,8 +41,9 @@ pub(crate) fn handle_ca_inject_open( notif_fd: RawFd, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(PathBuf, PathBuf)], + processes: &crate::seccomp::state::ProcessIndex, ) -> Option { - let resolved = crate::procfs::resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts)?; + let resolved = crate::procfs::resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts, processes)?; if !path_matches(&resolved, inject_paths) { return None; } diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index a49edd00..3565ae28 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -49,8 +49,8 @@ use tokio::sync::Mutex; use crate::chroot::resolve::{confine, resolve_existing_in_root, resolve_in_root}; use crate::sys::fs::openat2_in_root; -use crate::seccomp::notif::{read_child_mem, write_child_mem, write_child_mem_force, NotifAction}; -use crate::seccomp::state::{ChrootState, CowState}; +use crate::seccomp::notif::{read_child_mem, write_child_mem, NotifAction, NotifPolicy}; +use crate::seccomp::state::{ChrootState, CowState, ProcessIndex}; use crate::sys::structs::{SeccompNotif, SeccompNotifAddfd, SECCOMP_IOCTL_NOTIF_ADDFD}; // ============================================================ @@ -66,6 +66,27 @@ pub(crate) struct ChrootCtx<'a> { pub mounts: &'a [(PathBuf, PathBuf)], /// Virtual paths of read-only mounts: reads allowed, writes denied. pub mount_ro: &'a [PathBuf], + /// Per-process supervisor state, for handlers that track the caller's + /// filesystem context rather than just resolving one path. + pub processes: &'a Arc, +} + +impl<'a> ChrootCtx<'a> { + /// Borrow the chroot half of a notification policy. + /// + /// Only ever called from handlers registered when `chroot_root` is set, + /// which is what makes the unwrap sound. + pub(crate) fn new(policy: &'a NotifPolicy, processes: &'a Arc) -> Self { + ChrootCtx { + root: policy.chroot_root.as_ref().expect("chroot handlers are only registered with a chroot root"), + readable: &policy.chroot_readable, + writable: &policy.chroot_writable, + denied: &policy.chroot_denied, + mounts: &policy.chroot_mounts, + mount_ro: &policy.chroot_mount_ro, + processes, + } + } } impl ChrootCtx<'_> { @@ -217,6 +238,29 @@ fn canon_proc_self(virtual_path: &str, pid: u32) -> String { virtual_path.to_string() } +/// The virtual cwd of the calling task. +/// +/// The supervisor's own notion wins: since chdir is serviced without moving +/// the child's real cwd, `/proc//cwd` still points wherever exec left +/// it. That kernel value is the right answer only for a task that has never +/// moved, which is exactly when nothing is tracked. +fn virtual_cwd_of(notif: &SeccompNotif, ctx: &ChrootCtx<'_>) -> Option { + if let Ok(pid) = i32::try_from(notif.pid) { + if let Some(cwd) = ctx.processes.virtual_cwd(pid) { + return Some(cwd); + } + } + let host_cwd = std::fs::read_link(format!("/proc/{}/cwd", notif.pid)).ok()?; + ctx.host_to_virtual(&host_cwd) +} + +/// Record the calling task's new virtual cwd. +fn set_virtual_cwd(notif: &SeccompNotif, ctx: &ChrootCtx<'_>, cwd: PathBuf) { + if let Ok(pid) = i32::try_from(notif.pid) { + ctx.processes.set_virtual_cwd(pid, cwd); + } +} + /// Build the full virtual path from dirfd + relative path. fn build_virtual_path( notif: &SeccompNotif, @@ -228,12 +272,12 @@ fn build_virtual_path( path.to_string() } else { let dirfd32 = dirfd as i32; - let base_host = if dirfd32 == libc::AT_FDCWD { - std::fs::read_link(format!("/proc/{}/cwd", notif.pid)).ok()? + let base_virtual = if dirfd32 == libc::AT_FDCWD { + virtual_cwd_of(notif, ctx)? } else { - std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)).ok()? + let base_host = std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)).ok()?; + ctx.host_to_virtual(&base_host)? }; - let base_virtual = ctx.host_to_virtual(&base_host)?; let combined = base_virtual.join(path); combined.to_string_lossy().to_string() }; @@ -745,19 +789,13 @@ pub(crate) async fn handle_chroot_exec( let full_path = if Path::new(&rel_path).is_absolute() { rel_path } else { - let dirfd32 = dirfd as i32; - let base_host = if dirfd32 == libc::AT_FDCWD { - match std::fs::read_link(format!("/proc/{}/cwd", notif.pid)) { - Ok(p) => p, - Err(_) => return NotifAction::Errno(libc::EACCES), - } - } else { - match std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)) { - Ok(p) => p, - Err(_) => return NotifAction::Errno(libc::EACCES), - } + let base = match dirfd as i32 { + libc::AT_FDCWD => virtual_cwd_of(notif, ctx), + _ => std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)) + .ok() + .and_then(|host| ctx.host_to_virtual(&host)), }; - match ctx.host_to_virtual(&base_host) { + match base { Some(base) => base.join(&rel_path).to_string_lossy().to_string(), None => return NotifAction::Errno(libc::EACCES), } @@ -1357,19 +1395,13 @@ pub(crate) async fn handle_chroot_readlink( let full_path = if Path::new(&path).is_absolute() { path.clone() } else { - let dirfd32 = dirfd as i32; - let base_host = if dirfd32 == libc::AT_FDCWD { - match std::fs::read_link(format!("/proc/{}/cwd", notif.pid)) { - Ok(p) => p, - Err(_) => return NotifAction::Errno(libc::EACCES), - } - } else { - match std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)) { - Ok(p) => p, - Err(_) => return NotifAction::Errno(libc::EACCES), - } + let base = match dirfd as i32 { + libc::AT_FDCWD => virtual_cwd_of(notif, ctx), + _ => std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)) + .ok() + .and_then(|host| ctx.host_to_virtual(&host)), }; - let base_virtual = match ctx.host_to_virtual(&base_host) { + let base_virtual = match base { Some(p) => p, None => return NotifAction::Errno(libc::EACCES), }; @@ -1650,41 +1682,19 @@ pub(crate) async fn handle_chroot_chdir( notif_fd: RawFd, ctx: &ChrootCtx<'_>, ) -> NotifAction { - let path_ptr = notif.data.args[0]; - let path = match read_path(notif, path_ptr, notif_fd) { + let path = match read_path(notif, notif.data.args[0], notif_fd) { Some(p) => p, - None => return NotifAction::Continue, + None => return NotifAction::Errno(libc::EFAULT), }; - // Bytes the child mapped for the path argument (string + NUL): the upper - // bound for an in-place rewrite that must not overflow into adjacent memory. - let orig_path_buf_len = path.len() + 1; - - // Build the full virtual path from AT_FDCWD + path. - let was_absolute = Path::new(&path).is_absolute(); - let virtual_path = if was_absolute { - path - } else { - match std::fs::read_link(format!("/proc/{}/cwd", notif.pid)) { - Ok(cwd) => match ctx.host_to_virtual(&cwd) { - Some(base) => base.join(&path).to_string_lossy().to_string(), - None => return NotifAction::Errno(libc::EACCES), - }, - Err(_) => return NotifAction::Errno(libc::EACCES), - } + + let full_path = match build_virtual_path(notif, libc::AT_FDCWD as i64, &path, ctx) { + Some(p) => p, + None => return NotifAction::Errno(libc::EACCES), }; - // Canonicalize /proc/self and /proc/thread-self to the child's own PID, - // exactly as the open path does (build_virtual_path). The on-behalf - // openat2 below runs in the supervisor task, so the kernel would resolve a - // literal "self" against the supervisor and point the child's cwd at a - // non-sandbox process's /proc/ dir. Rewriting to the numeric child PID - // makes "self" resolve to the real caller and keeps every /proc spelling - // subject to the same numeric per-PID filter. For a same-path /proc mount - // this also lets the native-chdir fast path below fire: the resolved fd's - // host path then equals full_path, so the kernel re-runs the original - // "/proc/self" in the child's context (where it resolves correctly). - let full_path = canon_proc_self(&virtual_path, notif.pid); - - // Open directly via openat2(RESOLVE_IN_ROOT), routing to mount target if applicable. + + // Resolve on-behalf: this is what decides whether the directory exists, + // is reachable inside the root, and is a directory at all, and it gives + // the errno the child gets when it is not. let confined = confine(&full_path); let (chdir_root, chdir_path) = if let Some((mt, sub)) = ctx.mount_target(&confined) { (mt.to_path_buf(), sub) @@ -1700,78 +1710,53 @@ pub(crate) async fn handle_chroot_chdir( Ok(fd) => fd, Err(errno) => return NotifAction::Errno(errno), }; - - // Native-chdir fast path. The child runs in the host filesystem (sandlock - // does not chroot(2); it confines via on-behalf openat2 + Landlock), so an - // absolute chdir resolves against the real host root. When the on-behalf - // resolved directory's real host path is identical to the path the child - // asked for (the common case for same-path mounts like /proc and /sys), a - // raw chdir reaches the exact same directory. Let the kernel run the - // original syscall unchanged instead of rewriting the argument to the - // longer /proc/self/fd/N: that rewrite goes through process_vm_writev, - // which fails with EFAULT when the child passed a read-only string literal - // (e.g. busybox `top`'s chdir("/proc")), killing the process. Confinement - // is preserved: every subsequent open/stat/getdents is independently - // re-resolved on-behalf, and the equality check only holds when no symlink - // redirection occurred during the confined open. - if was_absolute - && std::fs::read_link(format!("/proc/self/fd/{}", src_fd)) - .ok() - .as_deref() - == Some(Path::new(&full_path)) - { - unsafe { libc::close(src_fd) }; - return NotifAction::Continue; - } - - // Inject fd into child and rewrite path to /proc/self/fd/N. - let addfd = SeccompNotifAddfd { - id: notif.id, - flags: 0, - srcfd: src_fd as u32, - newfd: 0, - newfd_flags: libc::O_CLOEXEC as u32, - }; - let child_fd = unsafe { - libc::ioctl( - notif_fd, - SECCOMP_IOCTL_NOTIF_ADDFD as libc::c_ulong, - &addfd as *const _, - ) - }; + // Record where the kernel actually landed, not what the child asked for: + // symlinks and .. are already collapsed in the resolved fd. + let resolved = std::fs::read_link(format!("/proc/self/fd/{}", src_fd)).ok(); unsafe { libc::close(src_fd) }; + let virtual_cwd = resolved + .as_deref() + .and_then(|host| ctx.host_to_virtual(host)) + .unwrap_or(confined); + + // The child's own cwd never moves. chdir cannot be run on-behalf (only + // the kernel can update the calling task's fs_struct) and the argument + // registers are not writable from seccomp-notify, so the handler used to + // rewrite the child's path buffer in place to /proc/self/fd/N and let the + // kernel run that. It could not: 16 bytes do not fit the buffer behind a + // path as short as "/tmp" or "/", which is issue #178. Tracking the cwd + // here instead serves every spelling, and it drops both a TOCTOU window + // (the kernel re-read the path we wrote) and a force-write through + // /proc//mem that permanently corrupted a .rodata path literal. + set_virtual_cwd(notif, ctx, virtual_cwd); + NotifAction::ReturnValue(0) +} - if child_fd < 0 { - return NotifAction::Errno(libc::EIO); - } +// ============================================================ +// fchdir handler +// ============================================================ - let fd_path = format!("/proc/self/fd/{}\0", child_fd); - // chdir leaves the process running, so the rewrite must not overflow the - // child's path buffer into adjacent memory. Only force-write when the - // redirect fits; otherwise the original (short) buffer can't be redirected. - // src_fd is already closed (above); the injected child fd is O_CLOEXEC and - // is reclaimed on the child's next exec/exit. - if orig_path_buf_len < fd_path.len() { - return NotifAction::Errno(libc::ENAMETOOLONG); - } - // Force-write past read-only page protections (a .rodata chdir literal that - // process_vm_writev can't overwrite). - if write_child_mem_force(notif_fd, notif.id, notif.pid, path_ptr, fd_path.as_bytes()).is_err() { - return NotifAction::Errno(libc::EFAULT); +/// Observe an fchdir so the tracked cwd cannot go stale. +/// +/// The child's real cwd does move here, since the fd is already open and the +/// kernel needs no path from us. But the supervisor's notion is what resolves +/// every later relative path, so it has to follow along. +pub(crate) async fn handle_chroot_fchdir( + notif: &SeccompNotif, + _chroot_state: &Arc>, + _cow_state: &Arc>, + _notif_fd: RawFd, + ctx: &ChrootCtx<'_>, +) -> NotifAction { + let fd = notif.data.args[0] as i32; + let target = std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, fd)).ok(); + // Only follow a target that is really a directory: anything else fails + // the kernel's fchdir, and recording it would desync the tracked cwd. + if let Some(host) = target.filter(|t| t.is_dir()) { + if let Some(virtual_cwd) = ctx.host_to_virtual(&host) { + set_virtual_cwd(notif, ctx, virtual_cwd); + } } - - // KNOWN TOCTOU LIMITATION (issue #27, same class as case 2): - // - // We've written "/proc/self/fd/N" into the child's path_ptr and - // returned Continue. The kernel will re-read path_ptr to perform - // the actual chdir. A multi-threaded child can race this read - // and substitute a different path. - // - // chdir cannot be on-behalf'd: the kernel must update the calling - // task's fs_struct (per-task cwd), which the supervisor cannot do - // for the child. The race window is bounded by Landlock — a - // racing path is still subject to landlock_restrict_self. The - // planned mitigation is opt-in CLONE_THREAD deny. NotifAction::Continue } @@ -1789,12 +1774,7 @@ pub(crate) async fn handle_chroot_getcwd( let buf_addr = notif.data.args[0]; let buf_size = (notif.data.args[1] & 0xFFFFFFFF) as usize; - let cwd = match std::fs::read_link(format!("/proc/{}/cwd", notif.pid)) { - Ok(c) => c, - Err(_) => return NotifAction::Continue, - }; - - let virtual_cwd = ctx.host_to_virtual(&cwd).unwrap_or_else(|| PathBuf::from("/")); + let virtual_cwd = virtual_cwd_of(notif, ctx).unwrap_or_else(|| PathBuf::from("/")); let cwd_str = virtual_cwd.to_string_lossy(); let cwd_bytes = cwd_str.as_bytes(); @@ -2203,13 +2183,15 @@ mod self_rewrite_tests { #[cfg(test)] mod mount_ro_tests { - use super::ChrootCtx; + use super::{ChrootCtx, ProcessIndex}; use std::path::{Path, PathBuf}; + use std::sync::Arc; fn ctx<'a>( mounts: &'a [(PathBuf, PathBuf)], mount_ro: &'a [PathBuf], writable: &'a [PathBuf], + processes: &'a Arc, ) -> ChrootCtx<'a> { ChrootCtx { root: Path::new("/rootfs"), @@ -2218,6 +2200,7 @@ mod mount_ro_tests { denied: &[], mounts, mount_ro, + processes, } } @@ -2228,7 +2211,8 @@ mod mount_ro_tests { // Even with a writable rootfs ("/" granted), the read-only /proc mount // must still deny writes — this is the host-escape guard. let writable = vec![PathBuf::from("/")]; - let c = ctx(&mounts, &ro, &writable); + let processes = Arc::new(ProcessIndex::new()); + let c = ctx(&mounts, &ro, &writable, &processes); assert!(c.can_read(Path::new("/proc/version"))); assert!(!c.can_write(Path::new("/proc/sys/kernel/core_pattern"))); assert!(!c.can_write(Path::new("/proc/self/oom_score_adj"))); @@ -2239,7 +2223,8 @@ mod mount_ro_tests { let mounts = vec![(PathBuf::from("/data"), PathBuf::from("/host/data"))]; let ro: Vec = vec![]; let writable = vec![PathBuf::from("/data")]; - let c = ctx(&mounts, &ro, &writable); + let processes = Arc::new(ProcessIndex::new()); + let c = ctx(&mounts, &ro, &writable, &processes); assert!(c.can_read(Path::new("/data/file"))); assert!(c.can_write(Path::new("/data/file"))); } diff --git a/crates/sandlock-core/src/procfs.rs b/crates/sandlock-core/src/procfs.rs index 6c7c2064..d2c04d84 100644 --- a/crates/sandlock-core/src/procfs.rs +++ b/crates/sandlock-core/src/procfs.rs @@ -417,6 +417,7 @@ pub(crate) async fn handle_proc_open( notif_fd, policy.chroot_root.as_deref(), &policy.chroot_mounts, + processes, ) { Some(p) => p, None => return NotifAction::Continue, @@ -607,8 +608,9 @@ pub(crate) fn handle_hostname_open( notif_fd: RawFd, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(std::path::PathBuf, std::path::PathBuf)], + processes: &ProcessIndex, ) -> Option { - let resolved = resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts)?; + let resolved = resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts, processes)?; if resolved != std::path::Path::new("/etc/hostname") { return None; } @@ -629,8 +631,9 @@ pub(crate) fn handle_etc_hosts_open( notif_fd: RawFd, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(std::path::PathBuf, std::path::PathBuf)], + processes: &ProcessIndex, ) -> Option { - let resolved = resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts)?; + let resolved = resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts, processes)?; if resolved != std::path::Path::new("/etc/hosts") { return None; } @@ -657,6 +660,7 @@ pub(crate) fn resolve_open_target( notif_fd: RawFd, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(std::path::PathBuf, std::path::PathBuf)], + processes: &ProcessIndex, ) -> Option { let nr = notif.data.nr as i64; let (dirfd, path_ptr): (i64, u64) = if Some(nr) == crate::arch::sys_open() { @@ -670,7 +674,7 @@ pub(crate) fn resolve_open_target( return None; }; let path = read_path(notif, path_ptr, notif_fd)?; - resolve_to_normalized_absolute(notif.pid, dirfd, &path, chroot_root, chroot_mounts) + resolve_to_normalized_absolute(notif.pid, dirfd, &path, chroot_root, chroot_mounts, processes) } /// Lexical normalization of `(pid, dirfd, path)`: @@ -690,30 +694,40 @@ fn resolve_to_normalized_absolute( path: &str, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(std::path::PathBuf, std::path::PathBuf)], + processes: &ProcessIndex, ) -> Option { use std::path::{Component, Path, PathBuf}; + // The dirfd/cwd symlink target is the *real* host directory. Under + // chroot, sandlock services /proc, /etc and /dev via on-behalf opens, + // so that target is e.g. `/proc` while the child's absolute + // spelling of the same file is `/proc/...`. Map the base back into the + // sandbox's virtual namespace so relative and absolute spellings + // resolve identically and the open-family shims (proc synthesis, + // /etc/hosts, /etc/hostname, random seed, CA inject) match either way. + let to_virtual = |host: PathBuf| match chroot_root { + Some(root) => { + crate::chroot::resolve::host_to_virtual(root, chroot_mounts, &host).unwrap_or(host) + } + None => host, + }; + let joined: PathBuf = if Path::new(path).is_absolute() { PathBuf::from(path) - } else { - let base = if dirfd as i32 == libc::AT_FDCWD { - std::fs::read_link(format!("/proc/{}/cwd", pid)).ok()? - } else { - std::fs::read_link(format!("/proc/{}/fd/{}", pid, dirfd as i32)).ok()? - }; - // The dirfd/cwd symlink target is the *real* host directory. Under - // chroot, sandlock services /proc, /etc and /dev via on-behalf opens, - // so that target is e.g. `/proc` while the child's absolute - // spelling of the same file is `/proc/...`. Map the base back into the - // sandbox's virtual namespace so relative and absolute spellings - // resolve identically and the open-family shims (proc synthesis, - // /etc/hosts, /etc/hostname, random seed, CA inject) match either way. - let base = match chroot_root { - Some(root) => crate::chroot::resolve::host_to_virtual(root, chroot_mounts, &base) - .unwrap_or(base), - None => base, + } else if dirfd as i32 == libc::AT_FDCWD { + // Under chroot the supervisor services chdir itself and the child's + // real cwd never moves, so its own notion is the only current one, + // and it is already virtual. Falling back to the kernel's is right + // only for a task that has never moved, which is when nothing is + // tracked. + let base = match i32::try_from(pid).ok().and_then(|p| processes.virtual_cwd(p)) { + Some(tracked) => tracked, + None => to_virtual(std::fs::read_link(format!("/proc/{}/cwd", pid)).ok()?), }; base.join(path) + } else { + let base = std::fs::read_link(format!("/proc/{}/fd/{}", pid, dirfd as i32)).ok()?; + to_virtual(base).join(path) }; let mut out = PathBuf::new(); diff --git a/crates/sandlock-core/src/random.rs b/crates/sandlock-core/src/random.rs index 9deef97a..f7d40234 100644 --- a/crates/sandlock-core/src/random.rs +++ b/crates/sandlock-core/src/random.rs @@ -58,11 +58,12 @@ pub(crate) fn handle_random_open( notif_fd: RawFd, chroot_root: Option<&std::path::Path>, chroot_mounts: &[(std::path::PathBuf, std::path::PathBuf)], + processes: &crate::seccomp::state::ProcessIndex, ) -> Option { // Resolve the open path so dirfd-relative or non-canonical spellings // (`/dev/../dev/urandom`, `openat(open("/dev"), "urandom", ...)`) // can't sidestep the seed and read real kernel entropy. - let resolved = crate::procfs::resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts)?; + let resolved = crate::procfs::resolve_open_target(notif, notif_fd, chroot_root, chroot_mounts, processes)?; let path = resolved.to_str()?; if path != "/dev/urandom" && path != "/dev/random" { return None; diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 8ff2ca38..5ec931fb 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -403,6 +403,7 @@ pub(crate) fn build_dispatch_table( if let Some(action) = crate::random::handle_random_open( ¬if, rng, notif_fd, policy.chroot_root.as_deref(), &policy.chroot_mounts, + &sup.processes, ) { return action; } @@ -451,15 +452,18 @@ pub(crate) fn build_dispatch_table( for nr in open_family_syscalls() { let etc_hosts = etc_hosts.clone(); let policy_hosts = Arc::clone(policy); + let processes_for_open = Arc::clone(&ctx.processes); table.register(nr, move |cx: &HandlerCtx| { let notif = cx.notif; let notif_fd = cx.notif_fd; let etc_hosts = etc_hosts.clone(); let policy = Arc::clone(&policy_hosts); + let processes = Arc::clone(&processes_for_open); async move { if let Some(action) = crate::procfs::handle_etc_hosts_open( ¬if, &etc_hosts, notif_fd, policy.chroot_root.as_deref(), &policy.chroot_mounts, + &processes, ) { action } else { @@ -483,16 +487,19 @@ pub(crate) fn build_dispatch_table( let ca_pem = std::sync::Arc::clone(&ca_pem); let inject_paths = std::sync::Arc::clone(&inject_paths); let policy_ca = Arc::clone(policy); + let processes_for_open = Arc::clone(&ctx.processes); table.register(nr, move |cx: &HandlerCtx| { let notif = cx.notif; let notif_fd = cx.notif_fd; let ca_pem = std::sync::Arc::clone(&ca_pem); let inject_paths = std::sync::Arc::clone(&inject_paths); let policy = Arc::clone(&policy_ca); + let processes = Arc::clone(&processes_for_open); async move { crate::ca_inject::handle_ca_inject_open( ¬if, &inject_paths, &ca_pem, notif_fd, policy.chroot_root.as_deref(), &policy.chroot_mounts, + &processes, ) .unwrap_or(NotifAction::Continue) } @@ -600,15 +607,18 @@ pub(crate) fn build_dispatch_table( for nr in open_family_syscalls() { let hostname = hostname_for_open.clone(); let policy_hostname = Arc::clone(policy); + let processes_for_open = Arc::clone(&ctx.processes); table.register(nr, move |cx: &HandlerCtx| { let notif = cx.notif; let notif_fd = cx.notif_fd; let hostname = hostname.clone(); let policy = Arc::clone(&policy_hostname); + let processes = Arc::clone(&processes_for_open); async move { if let Some(action) = crate::procfs::handle_hostname_open( ¬if, &hostname, notif_fd, policy.chroot_root.as_deref(), &policy.chroot_mounts, + &processes, ) { action } else { @@ -773,21 +783,16 @@ fn register_chroot_handlers( let policy = Arc::clone($policy); let chroot_state = Arc::clone(&ctx.chroot); let cow_state = Arc::clone(&ctx.cow); + let processes = Arc::clone(&ctx.processes); move |cx: &HandlerCtx| { let notif = cx.notif; let chroot_state = Arc::clone(&chroot_state); let cow_state = Arc::clone(&cow_state); + let processes = Arc::clone(&processes); let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy); async move { - let chroot_ctx = ChrootCtx { - root: policy.chroot_root.as_ref().unwrap(), - readable: &policy.chroot_readable, - writable: &policy.chroot_writable, - denied: &policy.chroot_denied, - mounts: &policy.chroot_mounts, - mount_ro: &policy.chroot_mount_ro, - }; + let chroot_ctx = ChrootCtx::new(&policy, &processes); $handler(¬if, &chroot_state, &cow_state, notif_fd, &chroot_ctx).await } } @@ -801,21 +806,16 @@ fn register_chroot_handlers( let policy = Arc::clone($policy); let chroot_state = Arc::clone(&ctx.chroot); let cow_state = Arc::clone(&ctx.cow); + let processes = Arc::clone(&ctx.processes); move |cx: &HandlerCtx| { let notif = cx.notif; let chroot_state = Arc::clone(&chroot_state); let cow_state = Arc::clone(&cow_state); + let processes = Arc::clone(&processes); let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy); async move { - let chroot_ctx = ChrootCtx { - root: policy.chroot_root.as_ref().unwrap(), - readable: &policy.chroot_readable, - writable: &policy.chroot_writable, - denied: &policy.chroot_denied, - mounts: &policy.chroot_mounts, - mount_ro: &policy.chroot_mount_ro, - }; + let chroot_ctx = ChrootCtx::new(&policy, &processes); $handler(¬if, &chroot_state, &cow_state, notif_fd, &chroot_ctx).await } } @@ -888,14 +888,7 @@ fn register_chroot_handlers( let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy_for_chown); async move { - let chroot_ctx = ChrootCtx { - root: policy.chroot_root.as_ref().unwrap(), - readable: &policy.chroot_readable, - writable: &policy.chroot_writable, - denied: &policy.chroot_denied, - mounts: &policy.chroot_mounts, - mount_ro: &policy.chroot_mount_ro, - }; + let chroot_ctx = ChrootCtx::new(&policy, &sup.processes); crate::chroot::dispatch::handle_chroot_legacy_chown(¬if, &sup.chroot, &sup.cow, notif_fd, &chroot_ctx, false).await } }); @@ -911,14 +904,7 @@ fn register_chroot_handlers( let notif_fd = cx.notif_fd; let policy = Arc::clone(&policy_for_lchown); async move { - let chroot_ctx = ChrootCtx { - root: policy.chroot_root.as_ref().unwrap(), - readable: &policy.chroot_readable, - writable: &policy.chroot_writable, - denied: &policy.chroot_denied, - mounts: &policy.chroot_mounts, - mount_ro: &policy.chroot_mount_ro, - }; + let chroot_ctx = ChrootCtx::new(&policy, &sup.processes); crate::chroot::dispatch::handle_chroot_legacy_chown(¬if, &sup.chroot, &sup.cow, notif_fd, &chroot_ctx, true).await } }); @@ -970,9 +956,11 @@ fn register_chroot_handlers( crate::chroot::dispatch::handle_chroot_getdents)); } - // chdir, getcwd, statfs, utimensat + // chdir, fchdir, getcwd, statfs, utimensat table.register(libc::SYS_chdir as i64, chroot_handler!(policy, crate::chroot::dispatch::handle_chroot_chdir)); + table.register(libc::SYS_fchdir as i64, chroot_handler!(policy, + crate::chroot::dispatch::handle_chroot_fchdir)); table.register(libc::SYS_getcwd as i64, chroot_handler!(policy, crate::chroot::dispatch::handle_chroot_getcwd)); table.register(libc::SYS_statfs as i64, chroot_handler!(policy, diff --git a/crates/sandlock-core/src/seccomp/state.rs b/crates/sandlock-core/src/seccomp/state.rs index 40f8974c..3dd8e682 100644 --- a/crates/sandlock-core/src/seccomp/state.rs +++ b/crates/sandlock-core/src/seccomp/state.rs @@ -263,22 +263,40 @@ impl ProcessIndex { Arc::new(std::sync::Mutex::new(parent_cwd)) } + /// The cwd cell to read or write for `pid`. + /// + /// A task without an entry of its own falls back to its + /// thread-group leader: `pidfd_open` on a non-leader tid needs + /// `PIDFD_THREAD` (Linux 6.9), so `register_pid_if_new` can leave a + /// thread unregistered. Since threads share one `fs_struct`, the + /// leader's cell is the correct answer for them, not an + /// approximation. Only that miss pays for the extra /proc read. + fn cwd_cell(&self, pid: i32) -> Option { + if let Ok(guard) = self.inner.read() { + if let Some(entry) = guard.get(&pid) { + return Some(Arc::clone(&entry.cwd)); + } + } + let tgid = read_tgid_of_tid(pid)?; + if tgid == pid { + return None; + } + let guard = self.inner.read().ok()?; + guard.get(&tgid).map(|e| Arc::clone(&e.cwd)) + } + /// The cwd this task believes it is in, or None when the task is /// untracked or has never moved. pub fn virtual_cwd(&self, pid: i32) -> Option { - let guard = self.inner.read().ok()?; - let cwd = guard.get(&pid)?.cwd.lock().ok()?.clone(); + let cell = self.cwd_cell(pid)?; + let cwd = cell.lock().ok()?.clone(); cwd } /// Record where this task now believes it is. Silently does nothing /// for an untracked pid: the fallback is the kernel's own cwd. pub fn set_virtual_cwd(&self, pid: i32, cwd: PathBuf) { - let cell = match self.inner.read() { - Ok(guard) => guard.get(&pid).map(|e| Arc::clone(&e.cwd)), - Err(_) => None, - }; - if let Some(cell) = cell { + if let Some(cell) = self.cwd_cell(pid) { if let Ok(mut slot) = cell.lock() { *slot = Some(cwd); } @@ -761,6 +779,35 @@ mod tests { thread.join().unwrap(); } + #[test] + fn an_unregistered_thread_uses_its_leader_cwd() { + // pidfd_open on a non-leader tid needs PIDFD_THREAD (Linux 6.9), so + // register_pid_if_new can leave a thread without an entry of its own. + // It still shares the leader's fs_struct, so its chdir must land in + // the leader's cell rather than vanish. + let leader = unsafe { libc::getpid() }; + let idx = ProcessIndex::new(); + idx.register(leader).expect("leader registers"); + + let (tid_tx, tid_rx) = std::sync::mpsc::channel(); + let (stop_tx, stop_rx) = std::sync::mpsc::channel::<()>(); + let thread = std::thread::spawn(move || { + let tid = unsafe { libc::syscall(libc::SYS_gettid) } as i32; + tid_tx.send(tid).unwrap(); + let _ = stop_rx.recv(); + }); + let tid = tid_rx.recv().unwrap(); + // Deliberately not registered. + assert!(!idx.contains(tid)); + + idx.set_virtual_cwd(tid, PathBuf::from("/workspace")); + assert_eq!(idx.virtual_cwd(tid), Some(PathBuf::from("/workspace"))); + assert_eq!(idx.virtual_cwd(leader), Some(PathBuf::from("/workspace"))); + + let _ = stop_tx.send(()); + thread.join().unwrap(); + } + #[test] fn a_child_copies_the_parent_cwd_instead_of_sharing_it() { // fork(2) copies fs_struct: the child starts where the parent stood, diff --git a/crates/sandlock-core/src/seccomp_plan.rs b/crates/sandlock-core/src/seccomp_plan.rs index ac44b764..9aa782df 100644 --- a/crates/sandlock-core/src/seccomp_plan.rs +++ b/crates/sandlock-core/src/seccomp_plan.rs @@ -199,6 +199,10 @@ fn chroot_path_syscalls() -> Vec { libc::SYS_readlinkat, libc::SYS_getdents64, libc::SYS_chdir, + // fchdir carries no path, but it still moves the cwd that every + // later relative path resolves against, so the supervisor has to + // see it to keep its own notion in step. + libc::SYS_fchdir, libc::SYS_getcwd, libc::SYS_statfs, libc::SYS_utimensat, diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index d1479095..8b4f5d4e 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -205,6 +205,137 @@ async fn test_chroot_getcwd() { cleanup_rootfs(&rootfs); } +/// A short absolute path must be reachable (issue #178). The old handler +/// redirected the child through "/proc/self/fd/N", 16 bytes that cannot fit +/// the buffer behind a path as short as "/tmp", so every short mount point +/// failed with ENAMETOOLONG while ls and open on the same path worked. +#[tokio::test] +async fn test_chroot_chdir_short_path() { + let rootfs = build_test_rootfs("chdir-short"); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy.clone().run(&["rootfs-helper", "chdir", "/tmp"]).await { + Ok(r) => { + assert!( + r.success(), + "chdir(/tmp) should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert_eq!(r.stdout_str().unwrap_or("").trim(), "OK /tmp"); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// The virtual root is the shortest path there is, and no redirect can ever +/// fit its two-byte buffer. `cd /` has to work without one. +#[tokio::test] +async fn test_chroot_chdir_virtual_root() { + let rootfs = build_test_rootfs("chdir-root"); + + let policy = minimal_exec_policy(&rootfs).build().unwrap(); + + match policy.clone().run(&["rootfs-helper", "chdir", "/"]).await { + Ok(r) => { + assert!( + r.success(), + "chdir(/) should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert_eq!(r.stdout_str().unwrap_or("").trim(), "OK /"); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// A relative path opened after a chdir must resolve against the directory +/// the child moved to. The supervisor resolves the path itself, so this is +/// what proves its notion of the cwd actually followed the chdir. +#[tokio::test] +async fn test_chroot_relative_open_follows_chdir() { + let rootfs = build_test_rootfs("chdir-relative"); + fs::write(rootfs.join("tmp/marker.txt"), "marker-body\n").unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "sh", "-c", "chdir /tmp && cat marker.txt"]) + .await + { + Ok(r) => { + assert!( + r.success(), + "relative cat after chdir should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert!( + r.stdout_str().unwrap_or("").contains("marker-body"), + "relative open should have read /tmp/marker.txt, got: {}", + r.stdout_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// fchdir carries a dirfd instead of a path, so a supervisor tracking the cwd +/// has to observe this spelling too. Chained after a chdir, which is where the +/// supervisor's own notion takes over: miss the fchdir and that notion goes +/// stale, sending the following relative open back to the chdir's directory. +#[tokio::test] +async fn test_chroot_relative_open_follows_fchdir() { + let rootfs = build_test_rootfs("fchdir-relative"); + fs::write(rootfs.join("tmp/marker.txt"), "from-tmp\n").unwrap(); + fs::write(rootfs.join("etc/marker.txt"), "from-etc\n").unwrap(); + + let policy = minimal_exec_policy(&rootfs) + .fs_read("/etc") + .fs_write("/tmp") + .build() + .unwrap(); + + match policy + .clone() + .run(&[ + "rootfs-helper", + "sh", + "-c", + "chdir /tmp && fchdir /etc && cat marker.txt", + ]) + .await + { + Ok(r) => { + assert!( + r.success(), + "relative cat after fchdir should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + let stdout = r.stdout_str().unwrap_or("").to_string(); + assert!( + stdout.contains("from-etc"), + "relative open should have read /etc/marker.txt, got: {}", + stdout + ); + assert!( + !stdout.contains("from-tmp"), + "relative open resolved against the earlier chdir, got: {}", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// chdir into a same-path mount (/proc) from a READ-ONLY path buffer must /// succeed. Regression for the busybox-`top` EFAULT: rewriting the child's /// path argument to /proc/self/fd/N faults when the path lives in read-only diff --git a/tests/rootfs-helper.c b/tests/rootfs-helper.c index 2fdf1a90..ff016be0 100644 --- a/tests/rootfs-helper.c +++ b/tests/rootfs-helper.c @@ -662,6 +662,32 @@ static int cmd_chdir(int argc, char **argv) { return 0; } +/* ── fchdir (change directory through an already-open dirfd) ──── */ +/* + * fchdir() carries no path for the supervisor to inspect, so a supervisor + * that tracks the cwd itself has to notice this spelling too or its notion + * goes stale and later relative paths resolve somewhere else. Prints the + * resulting cwd like `chdir` does. + */ +static int cmd_fchdir(int argc, char **argv) { + if (argc < 1) { fprintf(stderr, "fchdir: missing operand\n"); return 1; } + int fd = open(argv[0], O_RDONLY | O_DIRECTORY); + if (fd < 0) { + fprintf(stderr, "fchdir: open %s: %s\n", argv[0], strerror(errno)); + return 1; + } + if (fchdir(fd) != 0) { + fprintf(stderr, "fchdir: %s: %s\n", argv[0], strerror(errno)); + close(fd); + return 1; + } + close(fd); + char buf[4096]; + if (!getcwd(buf, sizeof(buf))) { perror("fchdir: getcwd"); return 1; } + printf("OK %s\n", buf); + return 0; +} + /* ── chdir-self (chdir into a /proc/self path, confirm it is OUR dir) ─ */ /* * chdir(argv[0]) (e.g. "/proc/self"), then getcwd() and check its final @@ -742,6 +768,7 @@ static int cmd_write_fd_link(int argc, char **argv) { static int dispatch(const char *cmd, int argc, char **argv) { if (strcmp(cmd, "chdir") == 0) return cmd_chdir(argc, argv); + if (strcmp(cmd, "fchdir") == 0) return cmd_fchdir(argc, argv); if (strcmp(cmd, "chdir-self") == 0) return cmd_chdir_self(argc, argv); if (strcmp(cmd, "proc-dirfd") == 0) return cmd_proc_dirfd(argc, argv); if (strcmp(cmd, "write-fd-link") == 0) return cmd_write_fd_link(argc, argv); From 2b9522c3c467df438386743727f65e14c8368207 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 10:22:09 -0700 Subject: [PATCH 04/10] chroot: state the Continue rule the frozen cwd now depends on The module's Continue-safety notes still described a chdir that rewrote the child's path argument, which no longer exists, and the surviving categories were justified only against the TOCTOU race. They now rest on a second fact worth writing down: because the supervisor services chdir without moving the child's own cwd, that cwd is frozen at exec and diverges from the sandbox's view as soon as anything chdirs, so handing a healthy path syscall back to the kernel resolves it against the wrong root or the wrong directory. Every existing Continue was checked against that rule and none breaks it: each is either fd-based, where no path is resolved (AT_EMPTY_PATH stat, getdents, fchdir), or reached only after a fault that makes the kernel fail identically. This is the note that keeps the next handler from adding a third kind. Drop the claim that the execve rewrite's race would be closed by an opt-in CLONE_THREAD deny. No such plan exists, and it could not work: threads are ordinary and a sandbox that refuses them is unusable, so Landlock is the bound and the note should not promise otherwise. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 30 +++++++++++++-------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 3565ae28..50ce9ae3 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -27,17 +27,25 @@ //! EFAULT/-style error to the child. No security decision was made //! on contents we couldn't read, so this is safe. //! -//! 4. **Path-rewrite-then-Continue** (handle_chroot_exec, handle_chroot_chdir): -//! the supervisor rewrites `path_ptr` to `/proc/self/fd/N` and returns -//! `Continue` because the kernel must execute the syscall (execve -//! replaces the address space; chdir requires the kernel's per-task -//! fs_struct update). The TOCTOU window is real here — a racing -//! sibling thread can substitute a different path string between our -//! write and the kernel's read. The bound is Landlock: a racing path -//! is still subject to `landlock_restrict_self`. See per-site comments -//! in `handle_chroot_exec` and `handle_chroot_chdir`. The planned -//! mitigation is opt-in `CLONE_THREAD` deny in the BPF filter, which -//! eliminates the racer entirely. +//! 4. **Path-rewrite-then-Continue** (handle_chroot_exec): the supervisor +//! rewrites `path_ptr` to `/proc/self/fd/N` and returns `Continue` +//! because the kernel must run the syscall itself — execve replaces +//! the address space. The TOCTOU window is real here: a racing sibling +//! thread can substitute a different path string between our write and +//! the kernel's read. The bound is Landlock, since a racing path is +//! still subject to `landlock_restrict_self`. +//! +//! A `Continue` on a *healthy* path syscall would be a bug in this module, +//! not merely a race: the kernel resolves the path it is given against the +//! real root and the real cwd, so an absolute path would escape the virtual +//! root and a relative one would resolve against wherever exec left the +//! child. Since `handle_chroot_chdir` services chdir by recording the cwd +//! rather than moving the child's own (see there for why), that real cwd is +//! frozen for the process's whole life and diverges from the sandbox's view +//! the moment anything chdirs. Every `Continue` above is therefore either +//! fd-based, where no path is resolved at all (AT_EMPTY_PATH stat, getdents, +//! fchdir), or reached only after a fault that makes the kernel fail the +//! same syscall the same way. A new handler must keep to one of those two. use std::ffi::CString; use std::io::{Read, Seek, SeekFrom, Write}; From e6015f01367a174a4c07a9e9bd5eed9b06c24542 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 10:49:20 -0700 Subject: [PATCH 05/10] chroot: mediate openat2 instead of letting it reach the kernel openat2 was trapped so it could not bypass the fs deny check, but it was never registered with the chroot handlers, so under a virtual root it ran as written: an absolute path resolved against the host root rather than the rootfs, which is why openat2("/etc/passwd") read the host's file or, more often, failed with ENOENT for a file that plainly exists in the image. Relative paths were resolved against the child's real cwd, which no longer moves now that the supervisor services chdir itself. The handler could not simply be registered for it: openat2 keeps flags, mode and resolve in a struct open_how in child memory, so args[2] is a pointer where openat has flags. Decode through decode_open_args, which already knows all three spellings for the deny path, and drop the synthesized notification the legacy open handler used to build, since that shape now conflicts with decoding by syscall number. A caller's RESOLVE_* request has to survive being serviced on its behalf: NO_SYMLINKS and NO_MAGICLINKS constrain the path itself, so they are re-checked against the path as written (resolution for the policy check follows symlinks and would otherwise satisfy them silently) and passed to the on-behalf open. BENEATH, IN_ROOT and NO_XDEV are relative to the child's own dirfd, which the supervisor cannot replay from the sandbox root, so they are dropped rather than misapplied; RESOLVE_IN_ROOT still bounds the walk. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 88 +++++++++++--- crates/sandlock-core/src/seccomp/dispatch.rs | 8 +- crates/sandlock-core/src/seccomp/notif.rs | 15 +-- crates/sandlock-core/src/seccomp_plan.rs | 4 + crates/sandlock-core/src/sys/fs.rs | 19 +++- .../tests/integration/test_chroot.rs | 107 ++++++++++++++++++ tests/rootfs-helper.c | 36 ++++++ 7 files changed, 251 insertions(+), 26 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 50ce9ae3..7a5b363f 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -56,8 +56,8 @@ use std::sync::Arc; use tokio::sync::Mutex; use crate::chroot::resolve::{confine, resolve_existing_in_root, resolve_in_root}; -use crate::sys::fs::openat2_in_root; -use crate::seccomp::notif::{read_child_mem, write_child_mem, NotifAction, NotifPolicy}; +use crate::sys::fs::{openat2_in_root, openat2_in_root_with_resolve}; +use crate::seccomp::notif::{decode_open_args, read_child_mem, write_child_mem, NotifAction, NotifPolicy}; use crate::seccomp::state::{ChrootState, CowState, ProcessIndex}; use crate::sys::structs::{SeccompNotif, SeccompNotifAddfd, SECCOMP_IOCTL_NOTIF_ADDFD}; @@ -269,6 +269,58 @@ fn set_virtual_cwd(notif: &SeccompNotif, ctx: &ChrootCtx<'_>, cwd: PathBuf) { } } +/// `RESOLVE_NO_MAGICLINKS`: refuse traversal through a /proc magic link. +const RESOLVE_NO_MAGICLINKS: u64 = 0x02; +/// `RESOLVE_NO_SYMLINKS`: refuse traversal through any symlink. +const RESOLVE_NO_SYMLINKS: u64 = 0x04; + +/// The subset of an `openat2` caller's `RESOLVE_*` request the supervisor can +/// reproduce when it services the open itself. +/// +/// NO_SYMLINKS and NO_MAGICLINKS constrain the shape of the path, so they +/// hold whatever directory the walk starts from. RESOLVE_BENEATH, IN_ROOT and +/// NO_XDEV are all relative to the child's own starting dirfd, and the +/// supervisor walks from the sandbox root instead, so replaying them there +/// would refuse paths the child never asked to refuse. They are dropped, and +/// the sandbox's own RESOLVE_IN_ROOT is what bounds the walk in their place. +fn honorable_resolve_flags(resolve: u64) -> u64 { + resolve & (RESOLVE_NO_SYMLINKS | RESOLVE_NO_MAGICLINKS) +} + +/// Refuse an open whose `RESOLVE_*` request the path as written violates. +/// +/// Resolution for the policy check deliberately follows symlinks to find the +/// file the child would reach, which would quietly satisfy a NO_SYMLINKS +/// request the kernel was asked to refuse. Re-walk the original path under +/// the child's flags first and hand back the kernel's own ELOOP. Any other +/// failure (a missing O_CREAT target, most of all) belongs to the normal path +/// below, which knows how to create and how to phrase the error. +fn enforce_resolve_flags( + notif: &SeccompNotif, + dirfd: i64, + rel_path: &str, + ctx: &ChrootCtx<'_>, + resolve: u64, +) -> Option { + if resolve == 0 { + return None; + } + let full_path = build_virtual_path(notif, dirfd, rel_path, ctx)?; + let confined = confine(&full_path); + let (root, sub) = match ctx.mount_target(&confined) { + Some((mt, sub)) => (mt.to_path_buf(), sub), + None => (ctx.root.to_path_buf(), full_path), + }; + match openat2_in_root_with_resolve(&root, &sub, libc::O_PATH | libc::O_CLOEXEC, 0, resolve) { + Ok(fd) => { + unsafe { libc::close(fd) }; + None + } + Err(libc::ELOOP) => Some(NotifAction::Errno(libc::ELOOP)), + Err(_) => None, + } +} + /// Build the full virtual path from dirfd + relative path. fn build_virtual_path( notif: &SeccompNotif, @@ -420,15 +472,24 @@ pub(crate) async fn handle_chroot_open( notif_fd: RawFd, ctx: &ChrootCtx<'_>, ) -> NotifAction { - let dirfd = notif.data.args[0] as i64; - let path_ptr = notif.data.args[1]; - let flags = notif.data.args[2]; + // Every open spelling lands here, and they do not share an argument + // layout: openat2 keeps flags, mode and resolve in a struct open_how in + // child memory, where args[2] is the pointer to it rather than the flags. + let (dirfd, path_ptr, flags, resolve) = match decode_open_args(notif, notif_fd) { + Some(a) => (a.dirfd, a.path_ptr, a.flags, a.resolve), + None => return NotifAction::Continue, + }; let rel_path = match read_path(notif, path_ptr, notif_fd) { Some(p) => p, None => return NotifAction::Continue, }; + let honored = honorable_resolve_flags(resolve); + if let Some(refusal) = enforce_resolve_flags(notif, dirfd, &rel_path, ctx, honored) { + return refusal; + } + // Resolve to get the virtual path for access control. let (host_path, virtual_path) = match resolve_chroot_path(notif, dirfd, &rel_path, ctx) { Some(r) => r, @@ -513,7 +574,7 @@ pub(crate) async fn handle_chroot_open( } else { 0 }; - match open_in_namespace(ctx, notif.pid, &virtual_path, flags as i32, mode) { + match open_in_namespace(ctx, notif.pid, &virtual_path, flags as i32, mode, honored) { Ok(srcfd) => NotifAction::InjectFdSend { srcfd, newfd_flags }, Err(errno) => NotifAction::Errno(errno), } @@ -549,6 +610,7 @@ fn open_in_namespace( virtual_path: &Path, flags: i32, mode: u32, + resolve: u64, ) -> Result { let vp_str = virtual_path.to_string_lossy(); @@ -562,7 +624,7 @@ fn open_in_namespace( Some((mt, sub)) => (mt.to_path_buf(), sub), None => (ctx.root.to_path_buf(), vp_str.to_string()), }; - match openat2_in_root(&root, &sub, flags, mode) { + match openat2_in_root_with_resolve(&root, &sub, flags, mode, resolve) { // Category 1. Ok(fd) => Ok(unsafe { OwnedFd::from_raw_fd(fd) }), // Category 2, reached through symlinks. @@ -1929,15 +1991,9 @@ pub(crate) async fn handle_chroot_legacy_open( notif_fd: RawFd, ctx: &ChrootCtx<'_>, ) -> NotifAction { - // open(path, flags, mode) → openat(AT_FDCWD, path, flags, mode) - let synth = notif_with_args(notif, [ - libc::AT_FDCWD as u64, - notif.data.args[0], // path - notif.data.args[1], // flags - notif.data.args[2], // mode - 0, 0, - ]); - handle_chroot_open(&synth, chroot_state, cow_state, notif_fd, ctx).await + // open(path, flags, mode) needs no reshaping: decode_open_args reads the + // legacy layout from the syscall number and supplies the implied AT_FDCWD. + handle_chroot_open(notif, chroot_state, cow_state, notif_fd, ctx).await } /// SYS_stat(path, statbuf) → handle_chroot_stat via newfstatat(AT_FDCWD, path, statbuf, 0) diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 5ec931fb..3354cc43 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -823,8 +823,12 @@ fn register_chroot_handlers( } // openat — fallthrough if Continue - table.register(libc::SYS_openat, chroot_handler_fallthrough!(policy, - crate::chroot::dispatch::handle_chroot_open)); + // openat, openat2 — fallthrough if Continue. Both carry (dirfd, path) in + // the same slots; the handler decodes the rest per spelling. + for &nr in &[libc::SYS_openat, arch::SYS_OPENAT2] { + table.register(nr, chroot_handler_fallthrough!(policy, + crate::chroot::dispatch::handle_chroot_open)); + } // open (legacy) — fallthrough if Continue if let Some(open) = arch::sys_open() { diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index c4f34482..8664273c 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -492,20 +492,21 @@ fn deny_open_verdict( } /// open/openat/openat2 argument layout, normalized across the spellings. -struct OpenArgs { - dirfd: i64, - path_ptr: u64, - flags: u64, - mode: u64, +pub(crate) struct OpenArgs { + pub(crate) dirfd: i64, + pub(crate) path_ptr: u64, + pub(crate) flags: u64, + #[allow(dead_code)] + pub(crate) mode: u64, /// `openat2` `resolve` flags (`RESOLVE_*`); 0 for `open`/`openat`. - resolve: u64, + pub(crate) resolve: u64, } /// Decode the open arguments. `openat2` carries flags/mode/resolve inside a /// `struct open_how` in child memory, so its decode reads child memory and /// can fail; `None` means "could not decode" and the caller soft-falls-through /// (the kernel's own re-read fails the same way). -fn decode_open_args(notif: &SeccompNotif, notif_fd: RawFd) -> Option { +pub(crate) fn decode_open_args(notif: &SeccompNotif, notif_fd: RawFd) -> Option { let a = ¬if.data.args; let nr = notif.data.nr as i64; if nr == libc::SYS_openat { diff --git a/crates/sandlock-core/src/seccomp_plan.rs b/crates/sandlock-core/src/seccomp_plan.rs index 9aa782df..31baf2f9 100644 --- a/crates/sandlock-core/src/seccomp_plan.rs +++ b/crates/sandlock-core/src/seccomp_plan.rs @@ -182,6 +182,10 @@ fn cow_path_syscalls() -> Vec { fn chroot_path_syscalls() -> Vec { let mut v = vec![ libc::SYS_openat, + // openat2 resolves paths like openat and must be mediated the same + // way: left to the kernel, an absolute path resolves against the + // host root rather than the rootfs. + arch::SYS_OPENAT2, libc::SYS_execve, libc::SYS_execveat, libc::SYS_unlinkat, diff --git a/crates/sandlock-core/src/sys/fs.rs b/crates/sandlock-core/src/sys/fs.rs index a2351302..dd610677 100644 --- a/crates/sandlock-core/src/sys/fs.rs +++ b/crates/sandlock-core/src/sys/fs.rs @@ -39,6 +39,23 @@ pub(crate) fn openat2_in_root( path: &str, flags: i32, mode: u32, +) -> Result { + openat2_in_root_with_resolve(root, path, flags, mode, 0) +} + +/// As [`openat2_in_root`], plus the caller's own `RESOLVE_*` flags. +/// +/// Used when servicing an `openat2` on behalf of a child: the child chose +/// those flags to make the kernel refuse something (a symlink, a mount +/// crossing), and performing the open for it must not quietly grant what it +/// declined. The union with `RESOLVE_IN_ROOT` can only be stricter, never +/// looser, so the sandbox's own confinement still holds whatever is added. +pub(crate) fn openat2_in_root_with_resolve( + root: &Path, + path: &str, + flags: i32, + mode: u32, + extra_resolve: u64, ) -> Result { let c_root = CString::new(root.to_str().unwrap_or("")).map_err(|_| libc::EINVAL)?; let root_fd = unsafe { @@ -61,7 +78,7 @@ pub(crate) fn openat2_in_root( let how = OpenHow { flags: flags as u64, mode: mode as u64, - resolve: RESOLVE_IN_ROOT, + resolve: RESOLVE_IN_ROOT | extra_resolve, }; let fd = unsafe { diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index 8b4f5d4e..e3fe419a 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -336,6 +336,113 @@ async fn test_chroot_relative_open_follows_fchdir() { cleanup_rootfs(&rootfs); } +/// openat2 must be mediated like every other open spelling. It was trapped +/// for the deny check but never routed to the chroot handler, so an absolute +/// path reached the kernel as written and resolved against the host root +/// instead of the rootfs. +#[tokio::test] +async fn test_chroot_openat2_resolves_inside_the_rootfs() { + let rootfs = build_test_rootfs("openat2-absolute"); + fs::write(rootfs.join("etc/marker.txt"), "from-rootfs\n").unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_read("/etc").build().unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "openat2", "/etc/marker.txt"]) + .await + { + Ok(r) => { + assert!( + r.success(), + "openat2 of a rootfs path should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert!( + r.stdout_str().unwrap_or("").contains("from-rootfs"), + "openat2 should have read the rootfs file, got: {}", + r.stdout_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// The same for a relative openat2 after a chdir: it resolves against the +/// supervisor's notion of the cwd, like every other relative path does. +#[tokio::test] +async fn test_chroot_openat2_relative_follows_chdir() { + let rootfs = build_test_rootfs("openat2-relative"); + fs::write(rootfs.join("tmp/marker.txt"), "from-tmp\n").unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "sh", "-c", "chdir /tmp && openat2 marker.txt"]) + .await + { + Ok(r) => { + assert!( + r.success(), + "relative openat2 after chdir should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert!( + r.stdout_str().unwrap_or("").contains("from-tmp"), + "openat2 should have read /tmp/marker.txt, got: {}", + r.stdout_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// A RESOLVE_NO_SYMLINKS openat2 must still refuse a symlink once the +/// supervisor performs the open on its behalf. The child asked the kernel to +/// refuse it; servicing the open must not quietly grant what it declined. +#[tokio::test] +async fn test_chroot_openat2_honors_resolve_no_symlinks() { + const RESOLVE_NO_SYMLINKS: u64 = 0x04; + + let rootfs = build_test_rootfs("openat2-nosymlinks"); + fs::write(rootfs.join("etc/marker.txt"), "from-rootfs\n").unwrap(); + std::os::unix::fs::symlink("marker.txt", rootfs.join("etc/link.txt")).unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_read("/etc").build().unwrap(); + + match policy + .clone() + .run(&[ + "rootfs-helper", + "openat2", + "/etc/link.txt", + &RESOLVE_NO_SYMLINKS.to_string(), + ]) + .await + { + Ok(r) => { + assert!( + !r.success(), + "openat2 through a symlink with RESOLVE_NO_SYMLINKS should fail, stdout: {}", + r.stdout_str().unwrap_or("") + ); + assert!( + r.stderr_str().unwrap_or("").contains("openat2"), + "expected the helper's openat2 error, got: {}", + r.stderr_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// chdir into a same-path mount (/proc) from a READ-ONLY path buffer must /// succeed. Regression for the busybox-`top` EFAULT: rewriting the child's /// path argument to /proc/self/fd/N faults when the path lives in read-only diff --git a/tests/rootfs-helper.c b/tests/rootfs-helper.c index ff016be0..15eb326d 100644 --- a/tests/rootfs-helper.c +++ b/tests/rootfs-helper.c @@ -662,6 +662,41 @@ static int cmd_chdir(int argc, char **argv) { return 0; } +/* ── openat2 (the newest open spelling, via raw syscall) ──────── */ +/* + * openat2(2) carries flags, mode and resolve in a struct open_how in user + * memory instead of in registers, so a supervisor that reads open arguments + * positionally misreads its third argument as flags when it is really a + * pointer. Opens read-only and dumps it, so a test can tell which + * file the path actually resolved to. musl has no wrapper for it. + */ +#ifndef __NR_openat2 +#define __NR_openat2 437 +#endif +struct helper_open_how { + unsigned long long flags; + unsigned long long mode; + unsigned long long resolve; +}; + +static int cmd_openat2(int argc, char **argv) { + if (argc < 1) { fprintf(stderr, "openat2: missing operand\n"); return 1; } + /* Second operand, when present, is a RESOLVE_* mask (decimal). */ + struct helper_open_how how = { .flags = O_RDONLY, .mode = 0, .resolve = 0 }; + if (argc >= 2) how.resolve = strtoull(argv[1], NULL, 0); + long fd = syscall(__NR_openat2, AT_FDCWD, argv[0], &how, sizeof(how)); + if (fd < 0) { + fprintf(stderr, "openat2: %s: %s\n", argv[0], strerror(errno)); + return 1; + } + char buf[4096]; + ssize_t n; + while ((n = read((int)fd, buf, sizeof(buf))) > 0) + write(STDOUT_FILENO, buf, n); + close((int)fd); + return 0; +} + /* ── fchdir (change directory through an already-open dirfd) ──── */ /* * fchdir() carries no path for the supervisor to inspect, so a supervisor @@ -769,6 +804,7 @@ static int cmd_write_fd_link(int argc, char **argv) { static int dispatch(const char *cmd, int argc, char **argv) { if (strcmp(cmd, "chdir") == 0) return cmd_chdir(argc, argv); if (strcmp(cmd, "fchdir") == 0) return cmd_fchdir(argc, argv); + if (strcmp(cmd, "openat2") == 0) return cmd_openat2(argc, argv); if (strcmp(cmd, "chdir-self") == 0) return cmd_chdir_self(argc, argv); if (strcmp(cmd, "proc-dirfd") == 0) return cmd_proc_dirfd(argc, argv); if (strcmp(cmd, "write-fd-link") == 0) return cmd_write_fd_link(argc, argv); From cd71dce069eaefe73f68038e18288d19560702ca Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 10:59:50 -0700 Subject: [PATCH 06/10] chroot: answer /proc//cwd from the tracked cwd /proc/self/cwd is the kernel's own view of a cwd the supervisor stopped moving, so it reported wherever exec left the child: readlink returned the launch directory, a host path outside the virtual root, and opening through the link failed with EACCES because that path resolves under no mount the sandbox knows. The readlink handler was worse off than the rest. It never canonicalized "self" to the caller's pid, and since sandlock services /proc through an on-behalf openat2, /proc/self there is the *supervisor's* own directory, not the child's. That predates the cwd work and would have leaked the supervisor's cwd whenever the two diverged. Canonicalize first, like build_virtual_path already did for every other handler. Reads of the link now come from the tracked cwd, and paths that resolve through it are rewritten before resolution so open, stat and getdents land in the same directory the link reports. Where nothing is tracked and the real cwd maps to nothing the sandbox can name, the answer is "/" rather than the host path behind it, matching what getcwd already does. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 70 ++++++++++- .../tests/integration/test_chroot.rs | 115 ++++++++++++++++++ 2 files changed, 180 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 7a5b363f..a1d01b66 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -321,6 +321,41 @@ fn enforce_resolve_flags( } } +/// The pid whose cwd a `/proc//cwd[/...]` path names, if any. +/// +/// Callers must canonicalize `/proc/self` first; this only matches the +/// numeric spelling. +fn proc_cwd_link_pid(virtual_path: &str) -> Option<(i32, &str)> { + let rest = virtual_path.strip_prefix("/proc/")?; + let (pid, rest) = rest.split_once('/')?; + let pid: i32 = pid.parse().ok()?; + let tail = rest.strip_prefix("cwd")?; + if tail.is_empty() || tail.starts_with('/') { + Some((pid, tail)) + } else { + None + } +} + +/// Rewrite a `/proc//cwd` prefix to the cwd the sandbox believes that +/// task is in. +/// +/// The kernel's magic link points at the task's real cwd, which the +/// supervisor deliberately stopped moving when it took over chdir, so +/// resolving through the link would land wherever exec left the child (and +/// under the host root at that, since the launch directory is usually +/// outside the virtual root entirely). Left alone for an untracked pid, +/// where the kernel's link is still the only answer there is. +fn canon_proc_cwd(virtual_path: &str, ctx: &ChrootCtx<'_>) -> String { + let Some((pid, tail)) = proc_cwd_link_pid(virtual_path) else { + return virtual_path.to_string(); + }; + match ctx.processes.virtual_cwd(pid) { + Some(cwd) => format!("{}{}", cwd.to_string_lossy(), tail), + None => virtual_path.to_string(), + } +} + /// Build the full virtual path from dirfd + relative path. fn build_virtual_path( notif: &SeccompNotif, @@ -341,7 +376,7 @@ fn build_virtual_path( let combined = base_virtual.join(path); combined.to_string_lossy().to_string() }; - Some(canon_proc_self(&vpath, notif.pid)) + Some(canon_proc_cwd(&canon_proc_self(&vpath, notif.pid), ctx)) } /// Resolve a child path to (host_path, virtual_path) within the chroot. @@ -1436,14 +1471,39 @@ pub(crate) async fn handle_chroot_readlink( NotifAction::ReturnValue(len as i64) }; - // Special case: /proc/self/root -> "/" - if path == "/proc/self/root" { + // "self" here would be the SUPERVISOR: it services /proc through an + // on-behalf openat2, so the magic links below resolve in its own + // process unless the caller's pid is substituted first, exactly as + // build_virtual_path does for every other handler. + let path = canon_proc_self(&path, notif.pid); + let own_proc = format!("/proc/{}", notif.pid); + + // Special case: the caller's own /proc//root -> "/" + if path == format!("{}/root", own_proc) { return write_target(b"/"); } - // Special case: /proc/self/exe -> return the virtual path recorded during exec + // Special case: /proc//cwd is a magic link to the task's real cwd, + // which the supervisor no longer moves. Answer from what it tracks, and + // never fall back to the host path the link actually points at. + if let Some((pid, tail)) = proc_cwd_link_pid(&path) { + if tail.is_empty() { + let cwd = ctx + .processes + .virtual_cwd(pid) + .or_else(|| { + std::fs::read_link(format!("/proc/{}/cwd", pid)) + .ok() + .and_then(|host| ctx.host_to_virtual(&host)) + }) + .unwrap_or_else(|| PathBuf::from("/")); + return write_target(cwd.to_string_lossy().as_bytes()); + } + } + + // Special case: /proc//exe -> return the virtual path recorded during exec // (needed because memfd-backed binaries would show "/memfd:sandlock-exec" otherwise). - if path == "/proc/self/exe" { + if path == format!("{}/exe", own_proc) { let cs = chroot_state.lock().await; if let Some(ref exe) = cs.chroot_exe { let s = exe.to_string_lossy(); diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index e3fe419a..5f129e21 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -336,6 +336,121 @@ async fn test_chroot_relative_open_follows_fchdir() { cleanup_rootfs(&rootfs); } +/// /proc/self/cwd is the kernel's own view of the cwd, and the kernel's view +/// is the one the supervisor stopped moving. It has to be answered from the +/// tracked cwd or it reports wherever exec left the child. +#[tokio::test] +async fn test_chroot_proc_self_cwd_link_follows_chdir() { + let rootfs = build_test_rootfs("proc-self-cwd-link"); + + let policy = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .fs_write("/tmp") + .build() + .unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "sh", "-c", "chdir /tmp && readlink /proc/self/cwd"]) + .await + { + Ok(r) => { + // Exact match on the readlink line (the chdir prints its own): + // the test rootfs itself lives under a host path containing + // "/tmp", so a substring check would pass on a leak. + let stdout = r.stdout_str().unwrap_or("").to_string(); + assert_eq!( + stdout.lines().last().unwrap_or("").trim(), + "/tmp", + "readlink /proc/self/cwd should report the sandbox cwd, full stdout: {}", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// Opening *through* /proc/self/cwd has to land in the same directory the +/// link reports, so the magic link needs rewriting on the resolution path +/// too, not just when it is read. +#[tokio::test] +async fn test_chroot_open_through_proc_self_cwd() { + let rootfs = build_test_rootfs("proc-self-cwd-open"); + fs::write(rootfs.join("tmp/marker.txt"), "from-tmp\n").unwrap(); + + let policy = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .fs_write("/tmp") + .build() + .unwrap(); + + match policy + .clone() + .run(&[ + "rootfs-helper", + "sh", + "-c", + "chdir /tmp && cat /proc/self/cwd/marker.txt", + ]) + .await + { + Ok(r) => { + assert!( + r.success(), + "open through /proc/self/cwd should succeed, stderr: {}", + r.stderr_str().unwrap_or("") + ); + assert!( + r.stdout_str().unwrap_or("").contains("from-tmp"), + "should have read /tmp/marker.txt, got: {}", + r.stdout_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// A cwd the sandbox cannot name must never be answered with the host path +/// it happens to sit at. Without a `cwd` the child starts wherever sandlock +/// was launched, which is outside the virtual root entirely. +#[tokio::test] +async fn test_chroot_proc_self_cwd_never_leaks_a_host_path() { + let rootfs = build_test_rootfs("proc-self-cwd-leak"); + + let policy = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .build() + .unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "readlink", "/proc/self/cwd"]) + .await + { + Ok(r) => { + let stdout = r.stdout_str().unwrap_or("").trim().to_string(); + assert!( + !stdout.contains(rootfs.to_str().unwrap()), + "cwd link leaked the rootfs's host path: {}", + stdout + ); + let launch_dir = std::env::current_dir().unwrap(); + assert!( + !stdout.contains(launch_dir.to_str().unwrap()), + "cwd link leaked the launch directory: {}", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// openat2 must be mediated like every other open spelling. It was trapped /// for the deny check but never routed to the chroot handler, so an absolute /// path reached the kernel as written and resolved against the host root From 10dd920552bb518b54eeffecaf3039677e2621e2 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 11:15:30 -0700 Subject: [PATCH 07/10] chroot: stop naming an unreachable fd with its host path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /proc//fd/N is a magic link, so what readlink hands back is a real host path the kernel synthesized rather than link text. The handler ran that through host_to_virtual and, when the file lay outside the virtual root and every mount, fell back to printing it verbatim. A child could read the host location of anything it had inherited: redirect sandlock's output to a file and the sandbox learns exactly where that file lives. Ordinary symlinks must keep that same fallback, since read_link returns their stored text and a link to /etc/foo genuinely reads "/etc/foo", so the fix is scoped to the magic link rather than applied to the tail. Targets the sandbox can name still resolve to their virtual path, the kernel's own pipe:[…] and socket:[…] spellings pass through untouched, and a file with no name in the sandbox is reported as file:[inode], in the same shape, so callers telling one stream from another still work. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 42 +++++++++++++++++ .../tests/integration/test_chroot.rs | 46 +++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index a1d01b66..c49e0fec 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -337,6 +337,27 @@ fn proc_cwd_link_pid(virtual_path: &str) -> Option<(i32, &str)> { } } +/// The `(pid, fd)` a `/proc//fd/` path names, if any. Anything +/// deeper (`/proc//fd/3/x`) is a path *through* the link, not the link. +fn proc_fd_link(virtual_path: &str) -> Option<(i32, i32)> { + let rest = virtual_path.strip_prefix("/proc/")?; + let (pid, rest) = rest.split_once('/')?; + let fd = rest.strip_prefix("fd/")?; + Some((pid.parse().ok()?, fd.parse().ok()?)) +} + +/// Name an open file the sandbox has no path for. +/// +/// Modelled on the kernel's own `pipe:[inode]` spelling for fds that are not +/// reachable by name. A caller that reads an fd link to tell one stream from +/// another still gets a stable answer, without being handed a host path the +/// sandbox exists to keep out of reach. +fn unnameable_fd_name(link: &str) -> String { + use std::os::unix::fs::MetadataExt; + let ino = std::fs::metadata(link).map(|m| m.ino()).unwrap_or(0); + format!("file:[{}]", ino) +} + /// Rewrite a `/proc//cwd` prefix to the cwd the sandbox believes that /// task is in. /// @@ -1501,6 +1522,27 @@ pub(crate) async fn handle_chroot_readlink( } } + // Special case: /proc//fd/N is a magic link, so what the kernel + // returns is a real host path it synthesized rather than link text that + // the generic tail below could pass through untouched. + if let Some((pid, fd)) = proc_fd_link(&path) { + let link = format!("/proc/{}/fd/{}", pid, fd); + let target = match std::fs::read_link(&link) { + Ok(t) => t, + Err(_) => return NotifAction::Errno(libc::EBADF), + }; + // pipe:[…], socket:[…], anon_inode:… — the kernel's own synthetic + // names for fds with no path. Nothing to map, nothing to hide. + if !target.is_absolute() { + return write_target(target.to_string_lossy().as_bytes()); + } + let named = match ctx.host_to_virtual(&target) { + Some(virtual_target) => virtual_target.to_string_lossy().into_owned(), + None => unnameable_fd_name(&link), + }; + return write_target(named.as_bytes()); + } + // Special case: /proc//exe -> return the virtual path recorded during exec // (needed because memfd-backed binaries would show "/memfd:sandlock-exec" otherwise). if path == format!("{}/exe", own_proc) { diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index 5f129e21..df4eec25 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -451,6 +451,52 @@ async fn test_chroot_proc_self_cwd_never_leaks_a_host_path() { cleanup_rootfs(&rootfs); } +/// An fd whose file the sandbox cannot name must not be described with the +/// host path behind it. /proc//fd/N is a magic link, so the "target" +/// readlink hands back is a real host path the kernel synthesized, not link +/// text: an inherited stdio fd, or here a supervisor-opened /dev/null with +/// no /dev mount to map it into, would spell out where it lives on the host. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn test_chroot_fd_link_does_not_leak_a_host_path() { + use sandlock_core::StdioMode; + use std::fs::File; + use std::io::Read; + + let rootfs = build_test_rootfs("fd-link-leak"); + + let mut sb = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .build() + .unwrap(); + + match sb + .popen( + &["rootfs-helper", "readlink", "/proc/self/fd/0"], + StdioMode::Null, + StdioMode::Piped, + StdioMode::Piped, + ) + .await + { + Ok(mut child) => { + let mut out = String::new(); + if let Some(stdout) = child.take_stdout() { + let _ = File::from(stdout).read_to_string(&mut out); + } + let _ = child.wait().await; + let link = out.trim().to_string(); + assert!( + !link.starts_with('/'), + "fd link named a path the sandbox cannot reach: {}", + link + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// openat2 must be mediated like every other open spelling. It was trapped /// for the deny check but never routed to the chroot handler, so an absolute /// path reached the kernel as written and resolved against the host root From c8cc424855dc193e855a18ae29fcf3908aa89768 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 11:36:31 -0700 Subject: [PATCH 08/10] chroot: stop resolving through a final symlink for the no-follow family The chroot resolver walks a path with the kernel following symlinks, which is what open wants and the opposite of what several other syscalls need. Every one of them was handed the already-resolved target and acted on it: rm of a symlink deleted the file it pointed at and left the dangling link behind, mv moved the target, lstat described the target's type and size, lchown owned the target, and the l-prefixed xattr calls read the target's attributes. Only readlink was right, and only because it open-coded a parent walk of its own. Resolve the parent and append the final component instead, which is that open-coded walk lifted into resolve_in_root_nofollow and shared. It was already sitting inside resolve_in_root as the fallback for an O_CREAT target that does not exist yet: a name that cannot be walked to and a name that must not be walked through want the same answer. unlinkat and renameat2 take it unconditionally, linkat unless the caller passes AT_SYMLINK_FOLLOW, and the stat, statx, utimensat, fchownat and xattr paths when the caller asked not to follow. fchownat also has to call lchown rather than chown once it gets there, or the resolution stops at the link and the call walks past it anyway. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 185 +++++++++++------- crates/sandlock-core/src/chroot/resolve.rs | 29 ++- .../tests/integration/test_chroot.rs | 124 ++++++++++++ 3 files changed, 267 insertions(+), 71 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index c49e0fec..5e24194a 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -55,7 +55,9 @@ use std::sync::Arc; use tokio::sync::Mutex; -use crate::chroot::resolve::{confine, resolve_existing_in_root, resolve_in_root}; +use crate::chroot::resolve::{ + confine, resolve_existing_in_root, resolve_in_root, resolve_in_root_nofollow, +}; use crate::sys::fs::{openat2_in_root, openat2_in_root_with_resolve}; use crate::seccomp::notif::{decode_open_args, read_child_mem, write_child_mem, NotifAction, NotifPolicy}; use crate::seccomp::state::{ChrootState, CowState, ProcessIndex}; @@ -167,28 +169,33 @@ impl ChrootCtx<'_> { Some((mount_hp, sub_str)) } - /// Resolve a virtual path against mounts for paths that may not exist yet (O_CREAT). - /// Returns (host_path, virtual_path). - fn resolve_mount(&self, virtual_path: &str) -> Option<(PathBuf, PathBuf)> { + /// Resolve a virtual path against mounts, using `resolver` for the part + /// below the mount point. Returns (host_path, virtual_path); the virtual + /// path is the confined form of what the child asked for, since that is + /// what the policy check reads. + fn resolve_mount_with( + &self, + virtual_path: &str, + resolver: fn(&Path, &str) -> Option<(PathBuf, PathBuf)>, + ) -> Option<(PathBuf, PathBuf)> { let confined = confine(virtual_path); let (mount_target, sub_path) = self.mount_target(&confined)?; - if let Some(result) = resolve_in_root(mount_target, &sub_path) { - let vp = confined; - return Some((result.0, vp)); - } - None + resolver(mount_target, &sub_path).map(|(host, _)| (host, confined)) } - /// Resolve a virtual path against mounts for paths that must exist. - /// Returns (host_path, virtual_path). + /// Resolve against mounts for paths that may not exist yet (O_CREAT). + fn resolve_mount(&self, virtual_path: &str) -> Option<(PathBuf, PathBuf)> { + self.resolve_mount_with(virtual_path, resolve_in_root) + } + + /// Resolve against mounts for paths that must exist. fn resolve_mount_existing(&self, virtual_path: &str) -> Option<(PathBuf, PathBuf)> { - let confined = confine(virtual_path); - let (mount_target, sub_path) = self.mount_target(&confined)?; - if let Some(result) = resolve_existing_in_root(mount_target, &sub_path) { - let vp = confined; - return Some((result.0, vp)); - } - None + self.resolve_mount_with(virtual_path, resolve_existing_in_root) + } + + /// Resolve against mounts without following a final symlink. + fn resolve_mount_nofollow(&self, virtual_path: &str) -> Option<(PathBuf, PathBuf)> { + self.resolve_mount_with(virtual_path, resolve_in_root_nofollow) } /// Inverse: given a host path, return the virtual path. @@ -419,6 +426,24 @@ fn resolve_chroot_path( resolve_in_root(ctx.root, &full_path) } +/// Resolve a child path without following a final symlink. +/// +/// For the no-follow family: lstat describes the link, unlink removes it, +/// rename moves it, lchown owns it. Following the last component would point +/// every one of them at the target instead. +fn resolve_chroot_path_nofollow( + notif: &SeccompNotif, + dirfd: i64, + path: &str, + ctx: &ChrootCtx<'_>, +) -> Option<(PathBuf, PathBuf)> { + let full_path = build_virtual_path(notif, dirfd, path, ctx)?; + if let Some(result) = ctx.resolve_mount_nofollow(&full_path) { + return Some(result); + } + resolve_in_root_nofollow(ctx.root, &full_path) +} + /// Resolve a child path that must already exist within the chroot. /// /// Unlike [`resolve_chroot_path`], this does NOT fall back to parent @@ -486,6 +511,23 @@ fn read_and_resolve( Ok((path, host_path, virtual_path)) } +/// Like [`read_and_resolve`] but stops at a final symlink, for the callers +/// that must act on the link rather than on what it points at. +fn read_and_resolve_nofollow( + notif: &SeccompNotif, + notif_fd: RawFd, + ctx: &ChrootCtx<'_>, + dirfd_idx: usize, + path_idx: usize, +) -> Result<(String, PathBuf, PathBuf), NotifAction> { + let path = read_path(notif, notif.data.args[path_idx], notif_fd) + .ok_or(NotifAction::Continue)?; + let dirfd = notif.data.args[dirfd_idx] as i64; + let (host_path, virtual_path) = resolve_chroot_path_nofollow(notif, dirfd, &path, ctx) + .ok_or(NotifAction::Errno(libc::EACCES))?; + Ok((path, host_path, virtual_path)) +} + /// Like [`read_and_resolve`] but requires the path to already exist. /// Returns a fully kernel-resolved host path with no unresolved symlinks. fn read_and_resolve_existing( @@ -1072,7 +1114,8 @@ pub(crate) async fn handle_chroot_write( let nr = notif.data.nr as i64; if nr == libc::SYS_unlinkat { - let (_, host_path, vp) = match read_and_resolve(notif, notif_fd, ctx, 0, 1) { + // unlink(2) removes the link, never what it points at. + let (_, host_path, vp) = match read_and_resolve_nofollow(notif, notif_fd, ctx, 0, 1) { Ok(r) => r, Err(a) => return a, }; @@ -1131,11 +1174,13 @@ pub(crate) async fn handle_chroot_write( Some(p) => p, None => return NotifAction::Continue, }; - let (old_host, old_vp) = match resolve_chroot_path(notif, notif.data.args[0] as i64, &old_path, ctx) { + // rename(2) moves the names themselves: a symlink on either side is + // renamed, not chased. + let (old_host, old_vp) = match resolve_chroot_path_nofollow(notif, notif.data.args[0] as i64, &old_path, ctx) { Some(r) => r, None => return NotifAction::Errno(libc::EACCES), }; - let (new_host, new_vp) = match resolve_chroot_path(notif, notif.data.args[2] as i64, &new_path, ctx) { + let (new_host, new_vp) = match resolve_chroot_path_nofollow(notif, notif.data.args[2] as i64, &new_path, ctx) { Some(r) => r, None => return NotifAction::Errno(libc::EACCES), }; @@ -1215,11 +1260,20 @@ pub(crate) async fn handle_chroot_write( Some(p) => p, None => return NotifAction::Continue, }; - let (old_host, _) = match resolve_chroot_path(notif, notif.data.args[0] as i64, &old_path, ctx) { + // link(2) hardlinks the source name itself; only AT_SYMLINK_FOLLOW + // asks for the target. The destination is a name being created, so it + // never follows either. + let follow_old = (notif.data.args[4] & libc::AT_SYMLINK_FOLLOW as u64) != 0; + let old_resolved = if follow_old { + resolve_chroot_path(notif, notif.data.args[0] as i64, &old_path, ctx) + } else { + resolve_chroot_path_nofollow(notif, notif.data.args[0] as i64, &old_path, ctx) + }; + let (old_host, _) = match old_resolved { Some(r) => r, None => return NotifAction::Errno(libc::EACCES), }; - let (new_host, new_vp) = match resolve_chroot_path(notif, notif.data.args[2] as i64, &new_path, ctx) { + let (new_host, new_vp) = match resolve_chroot_path_nofollow(notif, notif.data.args[2] as i64, &new_path, ctx) { Some(r) => r, None => return NotifAction::Errno(libc::EACCES), }; @@ -1274,7 +1328,13 @@ pub(crate) async fn handle_chroot_write( } if nr == libc::SYS_fchownat { - let (_, host_path, vp) = match read_and_resolve(notif, notif_fd, ctx, 0, 1) { + let nofollow = (notif.data.args[4] & libc::AT_SYMLINK_NOFOLLOW as u64) != 0; + let resolved = if nofollow { + read_and_resolve_nofollow(notif, notif_fd, ctx, 0, 1) + } else { + read_and_resolve(notif, notif_fd, ctx, 0, 1) + }; + let (_, host_path, vp) = match resolved { Ok(r) => r, Err(a) => return a, }; @@ -1295,7 +1355,12 @@ pub(crate) async fn handle_chroot_write( } } } - return exec_on_host(|p| unsafe { libc::chown(p, uid, gid) }, &host_path); + return exec_on_host( + |p| unsafe { + if nofollow { libc::lchown(p, uid, gid) } else { libc::chown(p, uid, gid) } + }, + &host_path, + ); } if nr == libc::SYS_truncate { @@ -1386,7 +1451,12 @@ pub(crate) async fn handle_chroot_stat( return NotifAction::Continue; } - let (_, host_path, vp) = match read_and_resolve_existing(notif, notif_fd, ctx, 0, 1) { + let resolved = if (flags & libc::AT_SYMLINK_NOFOLLOW as u64) != 0 { + read_and_resolve_nofollow(notif, notif_fd, ctx, 0, 1) + } else { + read_and_resolve_existing(notif, notif_fd, ctx, 0, 1) + }; + let (_, host_path, vp) = match resolved { Ok(r) => r, Err(a) => return a, }; @@ -1435,7 +1505,12 @@ pub(crate) async fn handle_chroot_statx( _ => return NotifAction::Continue, }; - let (host_path, vp) = match resolve_chroot_path_existing(notif, dirfd, &path, ctx) { + let resolved = if (flags & libc::AT_SYMLINK_NOFOLLOW) != 0 { + resolve_chroot_path_nofollow(notif, dirfd, &path, ctx) + } else { + resolve_chroot_path_existing(notif, dirfd, &path, ctx) + }; + let (host_path, vp) = match resolved { Some(r) => r, None => return NotifAction::Errno(libc::ENOENT), }; @@ -1561,45 +1636,12 @@ pub(crate) async fn handle_chroot_readlink( return NotifAction::Continue; } - // Resolve the path WITHOUT following the final symlink. readlink - // must read the link itself, not its target. We resolve the parent - // directory (following intermediate symlinks) and append the filename. - let full_path = if Path::new(&path).is_absolute() { - path.clone() - } else { - let base = match dirfd as i32 { - libc::AT_FDCWD => virtual_cwd_of(notif, ctx), - _ => std::fs::read_link(format!("/proc/{}/fd/{}", notif.pid, dirfd)) - .ok() - .and_then(|host| ctx.host_to_virtual(&host)), - }; - let base_virtual = match base { - Some(p) => p, - None => return NotifAction::Errno(libc::EACCES), - }; - base_virtual.join(&path).to_string_lossy().to_string() - }; - let confined = crate::chroot::resolve::confine(&full_path); - let file_name = match confined.file_name() { - Some(f) => f.to_os_string(), - None => return NotifAction::Errno(libc::EINVAL), - }; - let parent = confined.parent().unwrap_or(Path::new("/")); - - // Check mount first for parent resolution - let parent_str = parent.to_str().unwrap_or("/"); - let parent_host = if let Some((mt, sub)) = ctx.mount_target(parent) { - match resolve_in_root(mt, &sub) { - Some((hp, _)) => hp, - None => return NotifAction::Errno(libc::EACCES), - } - } else { - match resolve_in_root(ctx.root, parent_str) { - Some((hp, _)) => hp, - None => return NotifAction::Errno(libc::EACCES), - } + // readlink must read the link itself, never what it points at, which is + // exactly what the no-follow resolver gives. + let (host_path, _) = match resolve_chroot_path_nofollow(notif, dirfd, &path, ctx) { + Some(r) => r, + None => return NotifAction::Errno(libc::EACCES), }; - let host_path = parent_host.join(&file_name); // COW { @@ -1744,7 +1786,11 @@ pub(crate) async fn handle_chroot_xattr( _ => return NotifAction::Continue, }; let (host_path, vp) = - match resolve_chroot_path_existing(notif, libc::AT_FDCWD as i64, &path, ctx) { + match if follow { + resolve_chroot_path_existing(notif, libc::AT_FDCWD as i64, &path, ctx) + } else { + resolve_chroot_path_nofollow(notif, libc::AT_FDCWD as i64, &path, ctx) + } { Some(r) => r, None => return NotifAction::Errno(libc::ENOENT), }; @@ -2032,7 +2078,12 @@ pub(crate) async fn handle_chroot_utimensat( None => return NotifAction::Continue, }; - let (host_path, vp) = match resolve_chroot_path(notif, dirfd, &path, ctx) { + let resolved = if (flags & libc::AT_SYMLINK_NOFOLLOW) != 0 { + resolve_chroot_path_nofollow(notif, dirfd, &path, ctx) + } else { + resolve_chroot_path(notif, dirfd, &path, ctx) + }; + let (host_path, vp) = match resolved { Some(r) => r, None => return NotifAction::Errno(libc::EACCES), }; diff --git a/crates/sandlock-core/src/chroot/resolve.rs b/crates/sandlock-core/src/chroot/resolve.rs index 20427a0d..f796767d 100644 --- a/crates/sandlock-core/src/chroot/resolve.rs +++ b/crates/sandlock-core/src/chroot/resolve.rs @@ -80,11 +80,32 @@ pub fn resolve_in_root(chroot_root: &Path, child_path: &str) -> Option<(PathBuf, return Some(result); } - // Full path doesn't exist — resolve parent directory and append the - // missing filename. This is needed for O_CREAT targets where the - // final component will be created. + // Full path doesn't exist — resolve the parent and append the missing + // filename. This is needed for O_CREAT targets where the final + // component will be created. + resolve_in_root_nofollow(chroot_root, child_path) +} + +/// Resolve a virtual path *without* following a final symlink. +/// +/// The parent is resolved by the kernel (following intermediate symlinks, +/// confined to `chroot_root`) and the final component is appended verbatim, +/// so the caller acts on the last component itself. +/// +/// This is what the no-follow family needs. `lstat` must describe the link, +/// `unlink` and `rename` must remove and move the link, and `lchown` must own +/// it: resolving through the final component would silently redirect every +/// one of them onto the target. It is also how an `O_CREAT` target resolves, +/// since a name that does not exist yet cannot be walked to. +pub fn resolve_in_root_nofollow( + chroot_root: &Path, + child_path: &str, +) -> Option<(PathBuf, PathBuf)> { let confined = confine(child_path); - let file_name = confined.file_name()?; + // "/" has no final component to leave unresolved. + let Some(file_name) = confined.file_name() else { + return resolve_existing_in_root(chroot_root, child_path); + }; let parent = confined.parent().unwrap_or(Path::new("/")); match openat2_in_root( diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index df4eec25..96ba485b 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -451,6 +451,130 @@ async fn test_chroot_proc_self_cwd_never_leaks_a_host_path() { cleanup_rootfs(&rootfs); } +/// Removing a symlink must remove the link, not what it points at. The +/// chroot resolver follows the final component to find the file a path names, +/// which is right for open and wrong for unlink: it deleted the target and +/// left the dangling link behind. +#[tokio::test] +async fn test_chroot_unlink_removes_the_symlink_not_its_target() { + let rootfs = build_test_rootfs("unlink-symlink"); + fs::write(rootfs.join("tmp/target.txt"), "target-body\n").unwrap(); + std::os::unix::fs::symlink("target.txt", rootfs.join("tmp/link.txt")).unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy.clone().run(&["rootfs-helper", "rm", "/tmp/link.txt"]).await { + Ok(r) => { + assert!(r.success(), "rm should succeed, stderr: {}", r.stderr_str().unwrap_or("")); + assert!( + rootfs.join("tmp/target.txt").exists(), + "rm of a symlink deleted its target" + ); + assert!( + fs::symlink_metadata(rootfs.join("tmp/link.txt")).is_err(), + "rm of a symlink left the link in place" + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// lstat must describe the link itself. Resolution for the policy check +/// follows the final component, so the no-follow spellings were being handed +/// an already-resolved path and reported the target's type and size. +#[tokio::test] +async fn test_chroot_lstat_describes_the_symlink() { + let rootfs = build_test_rootfs("lstat-symlink"); + fs::write(rootfs.join("tmp/target.txt"), "target-body\n").unwrap(); + std::os::unix::fs::symlink("target.txt", rootfs.join("tmp/link.txt")).unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "legacy-lstat", "/tmp/link.txt"]) + .await + { + Ok(r) => { + let stdout = r.stdout_str().unwrap_or("").to_string(); + assert!( + stdout.contains("type=link"), + "lstat should describe the link itself, got: {}", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// Renaming a symlink moves the link. Following the final component first +/// would rename whatever it points at, leaving the old name dangling. +#[tokio::test] +async fn test_chroot_rename_moves_the_symlink_not_its_target() { + let rootfs = build_test_rootfs("rename-symlink"); + fs::write(rootfs.join("tmp/target.txt"), "target-body\n").unwrap(); + std::os::unix::fs::symlink("target.txt", rootfs.join("tmp/link.txt")).unwrap(); + + let policy = minimal_exec_policy(&rootfs).fs_write("/tmp").build().unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "mv", "/tmp/link.txt", "/tmp/moved.txt"]) + .await + { + Ok(r) => { + assert!(r.success(), "mv should succeed, stderr: {}", r.stderr_str().unwrap_or("")); + assert!( + rootfs.join("tmp/target.txt").exists(), + "rename of a symlink moved its target" + ); + let moved = fs::symlink_metadata(rootfs.join("tmp/moved.txt")); + assert!( + moved.map(|m| m.file_type().is_symlink()).unwrap_or(false), + "the moved entry should still be a symlink" + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + +/// The /proc magic links are symlinks, and lstat has to say so even though +/// paths *through* them are rewritten to the directory they stand for. +#[tokio::test] +async fn test_chroot_lstat_of_proc_self_cwd_is_a_link() { + let rootfs = build_test_rootfs("lstat-proc-cwd"); + + let policy = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .fs_write("/tmp") + .build() + .unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "legacy-lstat", "/proc/self/cwd"]) + .await + { + Ok(r) => { + let stdout = r.stdout_str().unwrap_or("").to_string(); + assert!( + stdout.contains("type=link"), + "lstat of /proc/self/cwd should report a symlink, got: {}", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// An fd whose file the sandbox cannot name must not be described with the /// host path behind it. /proc//fd/N is a magic link, so the "target" /// readlink hands back is a real host path the kernel synthesized, not link From 100332113e3d69c6a8b9e8ba9b1bed36bf71f3fe Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 11:38:45 -0700 Subject: [PATCH 09/10] chroot: gate readlink of a /proc/ link on sandbox membership The /proc open path refuses a pid outside the sandbox, so a child cannot open /proc//cwd, but readlink of the same path went through the chroot handler, which had no such check. The supervisor answers on the child's behalf and sees the whole host process table, so the magic links reported real cwd, exe and fd targets for processes the sandbox is not supposed to know exist. Apply the same extract_proc_pid + membership test the open path uses, rather than a second notion of which pids are visible. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 11 ++++++ crates/sandlock-core/src/procfs.rs | 2 +- .../tests/integration/test_chroot.rs | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 5e24194a..a9ddaaa9 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -1574,6 +1574,17 @@ pub(crate) async fn handle_chroot_readlink( let path = canon_proc_self(&path, notif.pid); let own_proc = format!("/proc/{}", notif.pid); + // Reading a /proc/ link reads another process's state, so it takes + // the same per-PID gate the /proc open path applies. Without it a child + // could not open /proc//cwd but could still readlink it, and + // the supervisor answering on its behalf sees the whole host process + // table. + if let Some(pid) = crate::procfs::extract_proc_pid(&path) { + if !ctx.processes.contains(pid) { + return NotifAction::Errno(libc::EACCES); + } + } + // Special case: the caller's own /proc//root -> "/" if path == format!("{}/root", own_proc) { return write_target(b"/"); diff --git a/crates/sandlock-core/src/procfs.rs b/crates/sandlock-core/src/procfs.rs index d2c04d84..6e9e018c 100644 --- a/crates/sandlock-core/src/procfs.rs +++ b/crates/sandlock-core/src/procfs.rs @@ -56,7 +56,7 @@ pub(crate) fn is_sensitive_proc(path: &str) -> bool { /// /// Returns `None` for non-numeric components like `/proc/self/...`, /// `/proc/cpuinfo`, etc. Those are handled elsewhere or are safe. -fn extract_proc_pid(path: &str) -> Option { +pub(crate) fn extract_proc_pid(path: &str) -> Option { let rest = path.strip_prefix("/proc/")?; // Take the next path component (up to '/' or end of string). let component = rest.split('/').next()?; diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index 96ba485b..dad8d483 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -575,6 +575,42 @@ async fn test_chroot_lstat_of_proc_self_cwd_is_a_link() { cleanup_rootfs(&rootfs); } +/// Reading a magic link is a read of another process's state, so it needs the +/// same per-PID gate the /proc open path applies. Opening /proc/1/cwd was +/// already refused; readlinking it went straight to the supervisor's own view +/// of the host's process table. +#[tokio::test] +async fn test_chroot_readlink_of_a_foreign_pid_is_refused() { + let rootfs = build_test_rootfs("readlink-foreign-pid"); + + let policy = minimal_exec_policy(&rootfs) + .fs_mount("/proc", "/proc") + .build() + .unwrap(); + + match policy + .clone() + .run(&["rootfs-helper", "readlink", "/proc/1/cwd"]) + .await + { + Ok(r) => { + assert!( + !r.success(), + "readlink of a non-sandbox pid should fail, stdout: {}", + r.stdout_str().unwrap_or("") + ); + assert!( + !r.stdout_str().unwrap_or("").contains('/'), + "readlink of a non-sandbox pid returned a path: {}", + r.stdout_str().unwrap_or("") + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// An fd whose file the sandbox cannot name must not be described with the /// host path behind it. /proc//fd/N is a magic link, so the "target" /// readlink hands back is a real host path the kernel synthesized, not link From c8a9e4bcdef3b1b89d0f54847e2f1a42449fc468 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 7 Aug 2026 12:22:07 -0700 Subject: [PATCH 10/10] chroot: fix stat's struct layout and rename's syscall on non-x86 Two failures that only show up off x86_64, both older than this branch and both uncovered by tests added here. stat_and_write hand-packed the reply in x86_64 field order: st_nlink as a 64-bit field ahead of st_mode. aarch64 and riscv64 put st_mode and st_nlink first as 32-bit fields, so a child there read st_nlink's low half as its mode and got 1, with the file type bits missing entirely. st_size lands at the same offset on all three, which is why the existing stat tests, which assert on size, stayed green. Let libc lay the struct out and copy it whole, the way the statfs handler already does. rename went unmediated on aarch64 for a simpler reason: the chroot notif list carries renameat2 and the legacy rename, but not renameat, and libc's rename() compiles to renameat wherever the ABI dropped the plain call. An absolute path therefore reached the kernel as written and resolved against the host root, so mv inside a rootfs failed with ENOENT. renameat carries renameat2's argument slots minus the flags this handler never reads, so it routes to the same handler. Signed-off-by: Cong Wang --- crates/sandlock-core/src/chroot/dispatch.rs | 53 +++++++++++--------- crates/sandlock-core/src/seccomp/dispatch.rs | 8 ++- crates/sandlock-core/src/seccomp_plan.rs | 5 ++ 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index a9ddaaa9..bb649d85 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -1165,7 +1165,10 @@ pub(crate) async fn handle_chroot_write( return exec_on_host(|p| unsafe { libc::mkdir(p, mode) }, &host_path); } - if nr == libc::SYS_renameat2 { + // renameat carries the same (olddirfd, oldpath, newdirfd, newpath) slots + // as renameat2 and differs only by the flags argument, which this handler + // does not read. + if nr == libc::SYS_renameat2 || Some(nr) == crate::arch::sys_renameat() { let old_path = match read_path(notif, notif.data.args[1], notif_fd) { Some(p) => p, None => return NotifAction::Continue, @@ -1404,31 +1407,35 @@ fn stat_and_write(notif: &SeccompNotif, notif_fd: RawFd, path: &Path) -> NotifAc let flags = notif.data.args[3]; let follow = (flags & libc::AT_SYMLINK_NOFOLLOW as u64) == 0; - let meta = if follow { - std::fs::metadata(path) - } else { - std::fs::symlink_metadata(path) + // Let libc lay the struct out. Hand-packing it in field order is an + // x86_64 assumption: aarch64 and riscv64 put st_mode and st_nlink before + // st_uid in 32-bit slots where x86_64 has a 64-bit st_nlink first, so the + // child read st_nlink's low half as its mode. st_size happens to land at + // the same offset on all three, which is why only a test that looks at + // the mode ever noticed. + let c_path = match path_cstr(path, libc::ENOENT) { + Ok(c) => c, + Err(a) => return a, }; - let meta = match meta { - Ok(m) => m, - Err(_) => return NotifAction::Errno(libc::ENOENT), + let mut st: libc::stat = unsafe { std::mem::zeroed() }; + let rc = unsafe { + if follow { + libc::stat(c_path.as_ptr(), &mut st) + } else { + libc::lstat(c_path.as_ptr(), &mut st) + } }; + if rc < 0 { + return NotifAction::Errno(last_errno(libc::ENOENT)); + } - use std::os::unix::fs::MetadataExt; - let mut buf = vec![0u8; std::mem::size_of::()]; - let mut off = 0; - macro_rules! pack_u64 { ($v:expr) => { buf[off..off+8].copy_from_slice(&($v as u64).to_ne_bytes()); off += 8; }; } - macro_rules! pack_u32 { ($v:expr) => { buf[off..off+4].copy_from_slice(&($v as u32).to_ne_bytes()); off += 4; }; } - pack_u64!(meta.dev()); pack_u64!(meta.ino()); pack_u64!(meta.nlink()); - pack_u32!(meta.mode()); pack_u32!(meta.uid()); pack_u32!(meta.gid()); pack_u32!(0u32); - pack_u64!(meta.rdev()); pack_u64!(meta.size() as u64); - pack_u64!(meta.blksize()); pack_u64!(meta.blocks() as u64); - pack_u64!(meta.atime() as u64); pack_u64!(meta.atime_nsec() as u64); - pack_u64!(meta.mtime() as u64); pack_u64!(meta.mtime_nsec() as u64); - pack_u64!(meta.ctime() as u64); pack_u64!(meta.ctime_nsec() as u64); - let _ = off; - - if write_child_mem(notif_fd, notif.id, notif.pid, statbuf_addr, &buf).is_err() { + let bytes = unsafe { + std::slice::from_raw_parts( + &st as *const libc::stat as *const u8, + std::mem::size_of::(), + ) + }; + if write_child_mem(notif_fd, notif.id, notif.pid, statbuf_addr, bytes).is_err() { return NotifAction::Continue; } NotifAction::ReturnValue(0) diff --git a/crates/sandlock-core/src/seccomp/dispatch.rs b/crates/sandlock-core/src/seccomp/dispatch.rs index 3354cc43..3673d84b 100644 --- a/crates/sandlock-core/src/seccomp/dispatch.rs +++ b/crates/sandlock-core/src/seccomp/dispatch.rs @@ -843,11 +843,15 @@ fn register_chroot_handlers( } // Modern write syscalls - for &nr in &[ + let mut write_nrs = vec![ libc::SYS_unlinkat, libc::SYS_mkdirat, libc::SYS_renameat2, libc::SYS_symlinkat, libc::SYS_linkat, libc::SYS_fchmodat, libc::SYS_fchownat, libc::SYS_truncate, - ] { + ]; + // renameat only exists where the ABI kept it, and libc's rename() lands + // there on the arches without a plain rename(2). + write_nrs.extend(arch::sys_renameat()); + for nr in write_nrs { table.register(nr, chroot_handler!(policy, crate::chroot::dispatch::handle_chroot_write)); } diff --git a/crates/sandlock-core/src/seccomp_plan.rs b/crates/sandlock-core/src/seccomp_plan.rs index 31baf2f9..94bf0e7c 100644 --- a/crates/sandlock-core/src/seccomp_plan.rs +++ b/crates/sandlock-core/src/seccomp_plan.rs @@ -235,6 +235,11 @@ fn chroot_path_syscalls() -> Vec { arch::sys_rmdir(), arch::sys_mkdir(), arch::sys_rename(), + // Where the ABI has no plain rename(2), libc's rename() compiles + // to renameat, so leaving it out left rename unmediated on + // aarch64: an absolute path went to the kernel and resolved + // against the host root instead of the rootfs. + arch::sys_renameat(), arch::sys_symlink(), arch::sys_link(), arch::sys_chmod(),