From d041242e22bca5411909ff671d4cf49f9ab59d8d Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 5 Nov 2026 09:22:06 +0400 Subject: [PATCH 1/3] [fix] cow: read legacy readlink's own arguments readlink(path, buf, size) and readlinkat(dirfd, path, buf, size) do not share an argument layout, but the handler read both as readlinkat. For a legacy readlink that took the output buffer as the path, so a symlink created during the run could not be read back. Check the syscall number first: legacy readlink takes path, buf and size from the first three arguments with AT_FDCWD as the directory, readlinkat keeps its four. Signed-off-by: Vahagn --- crates/sandlock-core/src/cow/dispatch.rs | 16 ++++-- .../tests/integration/test_cow.rs | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index d8f71fec..edb7b98e 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -1381,15 +1381,21 @@ pub(crate) async fn handle_cow_readlink( processes: &Arc, notif_fd: RawFd, ) -> NotifAction { - // readlinkat(dirfd, pathname, buf, bufsiz) - let dirfd = notif.data.args[0] as i64; + // readlinkat(dirfd, pathname, buf, bufsiz), and legacy + // readlink(pathname, buf, bufsiz), which shifts every argument by one. + let a = notif.data.args; + let (dirfd, path_ptr, buf_addr, bufsiz) = if arch::sys_readlink() == Some(notif.data.nr as i64) + { + (libc::AT_FDCWD as i64, a[0], a[1], a[2]) + } else { + (a[0] as i64, a[1], a[2], a[3]) + }; + let bufsiz = (bufsiz & 0xFFFFFFFF) as usize; let virtual_cwd = current_virtual_cwd(processes, notif.pid).await; - let path = match read_path(notif, notif.data.args[1], notif_fd) { + let path = match read_path(notif, path_ptr, notif_fd) { Some(p) => resolve_at_path_with_virtual(notif, dirfd, &p, virtual_cwd.as_deref()), None => return NotifAction::Continue, }; - let buf_addr = notif.data.args[2]; - let bufsiz = (notif.data.args[3] & 0xFFFFFFFF) as usize; let st = cow_state.lock().await; let cow = match st.branch.as_ref() { diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index fb32ac24..4d8ea001 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -2105,3 +2105,52 @@ async fn test_seccomp_cow_fchmodat2_is_virtualized() { assert!(!workdir.join("made").exists()); let _ = fs::remove_dir_all(&workdir); } + +/// readlink(path, buf, size) and readlinkat(dirfd, path, buf, size) do not +/// share an argument layout. Read as readlinkat, a legacy readlink has its +/// output buffer taken as the path, so the branch never recognises the call +/// and a symlink created during the run cannot be read back. +#[tokio::test] +async fn test_seccomp_cow_legacy_readlink_reads_its_own_arguments() { + let workdir = temp_dir("seccomp-cow-legacy-readlink"); + + let script = concat!( + "import os, sys\n", + "os.symlink('target', 'lnk')\n", // exists only in the upper layer + "try:\n", + " sys.stdout.write(os.readlink('lnk'))\n", + "except OSError as e:\n", + " sys.stdout.write('errno=%d' % e.errno)\n", + ); + + let mut policy = Sandbox::builder() + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write(&workdir) + .fs_write("/tmp") + .workdir(&workdir) + .cwd(&workdir) + .on_exit(BranchAction::Abort) + .build() + .unwrap(); + + let result = policy.run(&["python3", "-c", script]).await.unwrap(); + assert!( + result.success(), + "exit={:?}, stderr={}", + result.code(), + result.stderr_str().unwrap_or("") + ); + assert_eq!( + result.stdout_str().unwrap_or("").trim(), + "target", + "a symlink created during the run must read back through legacy readlink" + ); + + let _ = fs::remove_dir_all(&workdir); +} From 4430dd1e77b9ceca7af1ffa1a333f3f62a0078d5 Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 5 Nov 2026 09:27:23 +0400 Subject: [PATCH 2/3] [fix] cow: gate stat, readlink and exec on ownership, not just has_changes() With the workdir at "/", matches() is true for every path. stat, statx, exec's upper-redirect, and readlink only gated on has_changes() || !matches(), so once anything changed anywhere in the branch, they'd serve any matching path from the supervisor, even one the branch never touched. For a self-referential path (/proc/self, /dev/stdin), the supervisor answered for itself instead of the caller: /proc/self/exe came back as the sandlock binary, /proc/self/fd/N as the supervisor's fd. Gate these the same way handle_cow_open already gates reads: also require needs_read_intercept(), which checks whether the upper actually owns the path (staged, or covered by a whiteout). It had a latent bug of its own: Path::exists() follows symlinks and misses a dangling one, so a symlink staged in upper could read back as ENOENT. Fixed by also checking is_symlink(), matching resolve_read()'s existing fallback. Signed-off-by: Vahagn --- crates/sandlock-core/src/cow/dispatch.rs | 8 +-- crates/sandlock-core/src/cow/seccomp.rs | 17 ++++- .../tests/integration/test_cow.rs | 68 +++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index edb7b98e..9c6989ec 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -1122,7 +1122,7 @@ pub(crate) async fn handle_cow_stat( let upper_root = cow.upper_dir().to_path_buf(); let workdir_root = cow.workdir().to_path_buf(); let path = map_cow_upper_path(cow, &path); - if !cow.has_changes() || !cow.matches(&path) { + if !cow.has_changes() || !cow.matches(&path) || !cow.needs_read_intercept(&path) { return NotifAction::Continue; } let real = match cow.handle_stat(&path) { @@ -1226,7 +1226,7 @@ pub(crate) async fn handle_cow_statx( let upper_root = cow.upper_dir().to_path_buf(); let workdir_root = cow.workdir().to_path_buf(); let path = map_cow_upper_path(cow, &path); - if !cow.has_changes() || !cow.matches(&path) { + if !cow.has_changes() || !cow.matches(&path) || !cow.needs_read_intercept(&path) { return NotifAction::Continue; } let real = match cow.handle_stat(&path) { @@ -1311,7 +1311,7 @@ pub(crate) async fn handle_cow_exec( let upper_root = cow.upper_dir().to_path_buf(); let workdir_root = cow.workdir().to_path_buf(); let path = map_cow_upper_path(cow, &resolved); - if !cow.has_changes() || !cow.matches(&path) { + if !cow.has_changes() || !cow.matches(&path) || !cow.needs_read_intercept(&path) { return NotifAction::Continue; } let real = match cow.handle_stat(&path) { @@ -1404,7 +1404,7 @@ pub(crate) async fn handle_cow_readlink( }; let path = map_cow_upper_path(cow, &path); - if !cow.has_changes() || !cow.matches(&path) { + if !cow.has_changes() || !cow.matches(&path) || !cow.needs_read_intercept(&path) { return NotifAction::Continue; } diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index 9e720289..e602a829 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -1099,7 +1099,8 @@ impl SeccompCowBranch { /// read must be redirected there, never fall through to the lower file. pub fn needs_read_intercept(&self, path: &str) -> bool { if let Some(rel) = self.safe_rel(path) { - self.deleted.covers(&rel) || self.upper.join(&rel).exists() + let upper_entry = self.upper.join(&rel); + self.deleted.covers(&rel) || upper_entry.exists() || upper_entry.is_symlink() } else { false } @@ -3971,6 +3972,20 @@ mod tests { ); } + /// A dangling symlink staged in upper (target does not exist) must still + /// report as intercepted: `Path::exists()` follows symlinks, so a naive + /// check would miss it. + #[test] + fn needs_read_intercept_sees_a_dangling_symlink_in_upper() { + let (workdir, storage) = setup_workdir(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + let lnk = abs(&branch, "lnk"); + + assert!(!branch.needs_read_intercept(&lnk)); + branch.handle_symlink("missing-target", &lnk).unwrap(); + assert!(branch.needs_read_intercept(&lnk)); + } + /// Nothing outside the workdir may be mapped into the upper. `safe_rel` is /// the only thing standing between a host path and a COW copy of it, so it /// must reject both an ordinary escape and the string-prefix trap — a diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index 4d8ea001..4ec8de58 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -2106,6 +2106,74 @@ async fn test_seccomp_cow_fchmodat2_is_virtualized() { let _ = fs::remove_dir_all(&workdir); } +/// A read is intercepted only for a path the branch actually owns; anything +/// else must reach the kernel, not the supervisor. /proc/self and a pipe +/// made in the child (not inherited, so the supervisor has no fd of the +/// same number pointing at it) make the unowned case observable, since +/// intercepted they'd answer for the supervisor instead of the caller. +#[tokio::test] +async fn test_seccomp_cow_read_intercept_follows_ownership() { + let staged = std::env::temp_dir().join(format!("sandlock-proc-self-{}", std::process::id())); + let _ = fs::remove_dir_all(&staged); + + let script = format!( + concat!( + "import os, sys\n", + "os.mkdir('{staged}')\n", // staged in upper, this one must be intercepted + "dfd = os.open('/etc', os.O_RDONLY | os.O_DIRECTORY)\n", + "exe = os.readlink('/proc/self/exe', dir_fd=dfd)\n", + "r, w = os.pipe()\n", + "own_pipe = os.stat('/proc/self/fd/%d' % r).st_ino == os.fstat(r).st_ino\n", + "staged_ok = os.path.isdir('{staged}')\n", + "sys.stdout.write(exe + '\\n' + str(own_pipe) + '\\n' + str(staged_ok))\n", + ), + staged = staged.display() + ); + + let mut policy = Sandbox::builder() + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write("/tmp") + .workdir("/") + .cwd("/tmp") + .on_exit(BranchAction::Abort) + .build() + .unwrap(); + + let result = policy.run(&["python3", "-c", &script]).await.unwrap(); + assert!( + result.success(), + "exit={:?}, stderr={}", + result.code(), + result.stderr_str().unwrap_or("") + ); + + let out = result.stdout_str().unwrap_or("").to_string(); + let mut lines = out.lines(); + let exe = lines.next().unwrap_or(""); + let own_pipe = lines.next().unwrap_or(""); + let staged_ok = lines.next().unwrap_or(""); + assert!( + exe.contains("python"), + "not intercepted: /proc/self/exe must name the child's interpreter, not the supervisor: {exe}" + ); + assert_eq!( + own_pipe, "True", + "not intercepted: /proc/self/fd/N must resolve to the child's own fd, not the supervisor's" + ); + assert_eq!( + staged_ok, "True", + "intercepted: the staged directory is owned by the branch and must still be visible" + ); + + let _ = fs::remove_dir_all(&staged); +} + /// readlink(path, buf, size) and readlinkat(dirfd, path, buf, size) do not /// share an argument layout. Read as readlinkat, a legacy readlink has its /// output buffer taken as the path, so the branch never recognises the call From 836b7e5c1dc2a9a4d9c6aa0590aa8d10c682d8b2 Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 5 Nov 2026 09:31:51 +0400 Subject: [PATCH 3/3] [fix] cow: serve O_PATH opens of branch entries ADDFD resolves the source descriptor with fget(), which refuses an O_PATH one (fixed in Linux by 54971c806194), so an O_PATH open of an entry that exists only in the upper layer could not be answered and the child got EOPNOTSUPP. fstat the descriptor and reopen a regular file or a directory as O_RDONLY. The child's fd then reports O_RDONLY via F_GETFL, not O_PATH, and read() on it succeeds, which grants nothing a plain O_RDONLY would not under COW. Anything else (symlink, FIFO, socket, device) is rejected by type before any reopen is attempted, so it keeps the original O_PATH descriptor and ADDFD refuses it as before. An entry the reopen cannot read for lack of permission ends up the same way, since open() itself fails. Signed-off-by: Vahagn --- crates/sandlock-core/src/cow/dispatch.rs | 39 +++++++++ .../tests/integration/test_cow.rs | 83 +++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index 9c6989ec..ade2ab6f 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -198,6 +198,29 @@ pub(crate) fn open_confined( crate::sys::fs::openat2_in_root(root, &rel, flags, mode) } +/// Reopen an `O_PATH` descriptor as `O_RDONLY`, for a regular file or a +/// directory. `None` for anything else. TOCTOU-safe: the stat and the +/// reopen both go through the held descriptor. +fn reopen_o_path_readable(fd: RawFd) -> Option { + let mut st: libc::stat = unsafe { std::mem::zeroed() }; + let empty = c"".as_ptr(); + if unsafe { libc::fstatat(fd, empty, &mut st, libc::AT_EMPTY_PATH) } < 0 { + return None; + } + let flags = match st.st_mode & libc::S_IFMT { + libc::S_IFDIR => libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC, + libc::S_IFREG => libc::O_RDONLY | libc::O_CLOEXEC, + _ => return None, // symlink, FIFO, socket, device + }; + let link = std::ffi::CString::new(format!("/proc/self/fd/{}", fd)).ok()?; + let reopened = unsafe { libc::open(link.as_ptr(), flags) }; + if reopened < 0 { + return None; + } + unsafe { libc::close(fd) }; + Some(reopened) +} + /// The branch's directories, taken under the lock for work done without it. #[derive(Clone)] pub(crate) struct Layers { @@ -433,6 +456,22 @@ impl CowOpen { // The kernel cannot see the upper layer, so its answer would be wrong. Err(errno) => return NotifAction::Errno(errno), }; + + // ADDFD refuses an O_PATH source fd, so hand over a readable handle to + // the same entry. Under COW the workdir governs access to upper-layer + // entries, so this grants nothing a plain O_RDONLY would not. The + // child's fd reports O_RDONLY via F_GETFL, not O_PATH, and read() on + // it succeeds. Entries without read permission still return + // EOPNOTSUPP. + let fd = if self.open_flags & libc::O_PATH != 0 { + match reopen_o_path_readable(fd) { + Some(reopened) => reopened, + None => fd, + } + } else { + fd + }; + // Preserve O_CLOEXEC from the original openat flags. let newfd_flags = if self.flags & libc::O_CLOEXEC as u64 != 0 { libc::O_CLOEXEC as u32 diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index 4ec8de58..0270fce3 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -2222,3 +2222,86 @@ async fn test_seccomp_cow_legacy_readlink_reads_its_own_arguments() { let _ = fs::remove_dir_all(&workdir); } + +/// An O_PATH open of an entry that lives only in the upper layer must be +/// served. ADDFD refuses an O_PATH source descriptor, so the supervisor hands +/// over a read-only handle to the same entry instead, which grants nothing a +/// plain O_RDONLY would not under COW. +/// +/// Callers do not reliably pass O_DIRECTORY even when opening a directory, so +/// the decision is made on what the entry is, not on the flags. +#[tokio::test] +async fn test_seccomp_cow_o_path_open_of_created_entries() { + let workdir = temp_dir("seccomp-cow-o-path-kinds"); + + let script = format!( + concat!( + "import os, sys, fcntl\n", + "os.mkdir('{wd}/d')\n", + "open('{wd}/f', 'w').write('x')\n", + "open('{wd}/noperm', 'w').write('x')\n", + "os.chmod('{wd}/noperm', 0)\n", + "def probe_dir(p):\n", + " try:\n", + " fd = os.open(p, os.O_PATH)\n", + " os.close(fd)\n", + " return 'ok'\n", + " except OSError as e:\n", + " return 'errno=%d' % e.errno\n", + "def probe_file(p):\n", + " try:\n", + " fd = os.open(p, os.O_PATH)\n", + " except OSError as e:\n", + " return 'errno=%d' % e.errno\n", + " try:\n", + " rdonly = (fcntl.fcntl(fd, fcntl.F_GETFL) & os.O_ACCMODE) == os.O_RDONLY\n", + " return 'ok rdonly=%s data=%s' % (rdonly, os.read(fd, 1) == b'x')\n", + " finally:\n", + " os.close(fd)\n", + // no O_DIRECTORY on the directory: that is what callers actually do + "sys.stdout.write('dir=%s|file=%s|noperm=%s' % (\n", + " probe_dir('{wd}/d'), probe_file('{wd}/f'), probe_file('{wd}/noperm')))\n", + ), + wd = workdir.display() + ); + + let mut policy = Sandbox::builder() + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write(&workdir) + .fs_write("/tmp") + .workdir(&workdir) + .cwd(&workdir) + .on_exit(BranchAction::Abort) + .build() + .unwrap(); + let result = policy.run(&["python3", "-c", &script]).await.unwrap(); + assert!( + result.success(), + "exit={:?}, stderr={}", + result.code(), + result.stderr_str().unwrap_or("") + ); + + let got = result.stdout_str().unwrap_or("").trim().to_string(); + let file_ok = "file=ok rdonly=True data=True"; + let noperm = if unsafe { libc::geteuid() } == 0 { + // A root supervisor's reopen bypasses the permission bit too. + "noperm=ok rdonly=True data=True".to_string() + } else { + format!("noperm=errno={}", libc::EOPNOTSUPP) + }; + assert_eq!( + got, + format!("dir=ok|{file_ok}|{noperm}"), + "O_PATH of an upper-layer entry: served with a real O_RDONLY fd, \ + not a bare O_PATH one" + ); + + let _ = fs::remove_dir_all(&workdir); +}