diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 83376372..b1ce43f0 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -420,9 +420,10 @@ pub(crate) async fn handle_chroot_open( } Ok(None) => { // Fall through to openat2_in_root below. This keeps - // directory opens and other non-COW cases confined to - // the chroot instead of executing the original host - // syscall. + // directory opens and other non-COW / non-whiteout + // cases confined to the chroot instead of executing the + // original host syscall. (Whiteouts are handled by the + // BranchError::Deleted arm below, not here.) } Err(crate::error::BranchError::QuotaExceeded) => { return NotifAction::Errno(libc::ENOSPC); @@ -430,6 +431,14 @@ pub(crate) async fn handle_chroot_open( Err(crate::error::BranchError::Exists) => { return NotifAction::Errno(libc::EEXIST); } + Err(crate::error::BranchError::Deleted) => { + // Whiteout read-open: the lower file still physically + // exists, but the branch deleted it. Return ENOENT + // rather than leaking pre-delete bytes — parity with the + // async cow open path (cow/dispatch.rs) and + // stat/statx/access/readlink. + return NotifAction::Errno(libc::ENOENT); + } Err(_) => return NotifAction::Errno(libc::EIO), } } diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index 3c7094ca..91931b68 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -256,6 +256,10 @@ pub(crate) async fn handle_cow_open( // Phase 2: execute I/O plan without holding the lock let real_path = match plan { CowOpenPlan::Skip => return NotifAction::Continue, + // Deleted in this branch (whiteout): the lower file still exists, so + // Continue would read its pre-delete content. Return ENOENT, matching + // the stat/access handlers. + CowOpenPlan::Deleted => return NotifAction::Errno(libc::ENOENT), CowOpenPlan::Resolved(p) | CowOpenPlan::UpperReady { upper: p } => p, CowOpenPlan::NeedsCopy { upper, lower: _lower, file_size, rel_path } => { // Do the potentially-expensive copy on a blocking thread diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index f6f71372..95d82b55 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -46,6 +46,10 @@ pub enum CowCopyPlan { pub enum CowOpenPlan { /// No interception needed — let the kernel handle it. Skip, + /// The path was deleted in this branch (a whiteout) and is opened without + /// `O_CREAT`. The caller must return `ENOENT` rather than letting the kernel + /// open the untouched lower file, which still holds the pre-delete bytes. + Deleted, /// File already resolved (upper or lower) — open this path directly. Resolved(PathBuf), /// Need to copy lower to upper, then open upper. @@ -382,7 +386,12 @@ impl SeccompCowBranch { if flags & O_CREAT != 0 { return self.ensure_cow_copy(&rel).map(Some); } - return Ok(None); + // Whiteout: the lower file still physically exists with its + // pre-delete bytes. Surface the deletion so the caller returns + // ENOENT rather than falling through to the lower file — matching + // the async prepare_open (CowOpenPlan::Deleted) and the stat/access + // handlers. + return Err(BranchError::Deleted); } // O_EXCL: fail if file already exists (in upper or lower) @@ -447,7 +456,11 @@ impl SeccompCowBranch { if flags & O_CREAT != 0 { return self.prepare_cow_copy(&rel); } - return Ok(CowOpenPlan::Skip); + // Whiteout: the file was deleted in this branch. Do NOT skip to the + // lower file (which still physically exists with its pre-delete + // content); report the deletion so the caller returns ENOENT, + // matching the stat/access path. + return Ok(CowOpenPlan::Deleted); } // O_EXCL: fail if file already exists @@ -1384,6 +1397,37 @@ mod tests { assert!(matches!(plan, CowOpenPlan::Resolved(_))); } + #[test] + fn test_prepare_open_read_deleted_reports_deleted() { + // A file deleted in this branch is a whiteout: a read-only open must NOT + // fall through to the untouched lower file (which still holds the + // pre-delete bytes). It must report the deletion so the caller returns + // ENOENT, matching the stat/access path. + let (workdir, storage) = setup_workdir(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + branch.mark_deleted("existing.txt"); + let path = abs(&branch, "existing.txt"); + // O_RDONLY + let plan = branch.prepare_open(&path, 0).unwrap(); + assert!(matches!(plan, CowOpenPlan::Deleted)); + } + + #[test] + fn test_handle_open_read_deleted_reports_deleted() { + // Sync mirror of test_prepare_open_read_deleted_reports_deleted: the + // chroot dispatcher calls the sync handle_open, so a read-only open of a + // whiteout must surface BranchError::Deleted (mapped to ENOENT at the + // chroot call site) instead of Ok(None), which fell through to the + // untouched lower file and leaked its pre-delete bytes. + let (workdir, storage) = setup_workdir(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + branch.mark_deleted("existing.txt"); + let path = abs(&branch, "existing.txt"); + // O_RDONLY + let err = branch.handle_open(&path, 0).unwrap_err(); + assert!(matches!(err, BranchError::Deleted)); + } + #[test] fn test_prepare_open_write_existing_needs_copy() { let (workdir, storage) = setup_workdir(); diff --git a/crates/sandlock-core/src/error.rs b/crates/sandlock-core/src/error.rs index c7d2c8d0..a5014d24 100644 --- a/crates/sandlock-core/src/error.rs +++ b/crates/sandlock-core/src/error.rs @@ -120,6 +120,13 @@ pub enum BranchError { #[error("file already exists")] Exists, + + /// The path was deleted in this branch (a whiteout). The lower file still + /// physically exists with its pre-delete bytes, so the open call site must + /// return `ENOENT` instead of falling through to it. Sync mirror of the + /// async `CowOpenPlan::Deleted`. + #[error("file was deleted in this branch")] + Deleted, } /// Convenience type alias. diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index c48458d5..1e064a67 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -507,6 +507,60 @@ async fn test_chroot_with_cow() { cleanup_rootfs(&rootfs); } +/// A file deleted in the COW branch (a whiteout) must return ENOENT on a +/// read-open under CHROOT mode — the sync `handle_open` path. Before the fix, +/// `handle_open` returned `Ok(None)` for the whiteout and `chroot/dispatch.rs` +/// fell through to `openat2_in_root` on the still-present lower file, leaking the +/// pre-delete bytes. This is the chroot-mode sibling of the async +/// `test_seccomp_cow_read_deleted_file_is_enoent`, and it exercises the real +/// dispatch arm (not just the branch object): `cat` issues a bare +/// `open(O_RDONLY)` with no preceding `stat` (rootfs-helper.c), so it drives the +/// open path rather than short-circuiting on the (correct) stat ENOENT. +#[tokio::test] +async fn test_chroot_cow_read_deleted_file_is_enoent() { + let rootfs = build_test_rootfs("cow-read-deleted"); + let tmp_dir = rootfs.join("tmp"); + // Seed the LOWER file with sentinel content that must never leak. + fs::write(tmp_dir.join("secret.txt"), "PREDELETE").unwrap(); + + let policy = Sandbox::builder() + .chroot(&rootfs) + .fs_read("/usr") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write("/tmp") + .workdir(&tmp_dir) + .on_exit(BranchAction::Abort) + .build() + .unwrap(); + + // `rm` marks the file deleted in the branch (whiteout); `cat` then + // bare-opens it in the same cage/branch. With the fix the open returns + // ENOENT and nothing is printed; without it the untouched lower "PREDELETE" + // bytes leak to stdout. (rootfs-helper's `sh -c` dispatches applets + // in-process, so `rm` and `cat` share one COW branch.) + let result = policy + .clone() + .with_name("test") + .run(&["rootfs-helper", "sh", "-c", "rm /tmp/secret.txt; cat /tmp/secret.txt"]) + .await; + match result { + Ok(r) => { + let stdout = r.stdout_str().unwrap_or("").to_string(); + assert!( + !stdout.contains("PREDELETE"), + "pre-delete lower bytes leaked through the chroot read path (stdout: {:?})", + stdout + ); + } + Err(e) => eprintln!("Chroot test skipped: {}", e), + } + + cleanup_rootfs(&rootfs); +} + /// readlink /proc/self/root returns / #[tokio::test] async fn test_chroot_proc_self_root() { diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index df72121c..dcacba6e 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -398,6 +398,65 @@ async fn test_seccomp_cow_read_existing() { let _ = fs::remove_dir_all(&workdir); } +/// Regression test: a file deleted inside the COW workdir must read back as +/// ENOENT, not its pre-delete content. The read/open path returned +/// `Skip -> Continue` for a whiteout, so the kernel opened the untouched lower +/// file and leaked the original bytes — while stat/access already returned +/// ENOENT, so the two paths disagreed and a deletion was invisible to a reader. +#[tokio::test] +async fn test_seccomp_cow_read_deleted_file_is_enoent() { + let workdir = temp_dir("seccomp-read-deleted"); + let out_file = std::env::temp_dir().join(format!( + "sandlock-test-read-deleted-{}", std::process::id() + )); + fs::write(workdir.join("secret.txt"), "PREDELETE").unwrap(); + + let 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(); + + // Delete the file, then read it back with `dd`, which issues a bare + // open(O_RDONLY) with no preceding path stat — unlike `cat FILE` or a shell + // redirect, which stat first and would short-circuit on the (correct) stat + // ENOENT without ever exercising the open path this fix targets. `dd` + // succeeding means the open was honored and the untouched lower bytes leaked; + // `dd` failing means the whiteout was honored (ENOENT). Both the copied bytes + // and the marker land in /tmp (not the COW workdir), so they are real writes. + let secret = workdir.join("secret.txt"); + let leak_file = std::env::temp_dir().join(format!( + "sandlock-test-read-deleted-leak-{}", std::process::id() + )); + let cmd = format!( + "rm -f {secret}; if dd if={secret} of={leak} status=none; then printf 'OPENED' > {marker}; else printf 'DENIED' > {marker}; fi", + secret = secret.display(), + leak = leak_file.display(), + marker = out_file.display(), + ); + + let result = policy.clone().with_name("test").run(&["sh", "-c", &cmd]).await.unwrap(); + assert!(result.success(), "exit={:?}, stderr={}", result.code(), result.stderr_str().unwrap_or("")); + let marker = fs::read_to_string(&out_file).unwrap_or_default(); + let leaked = fs::read_to_string(&leak_file).unwrap_or_default(); + assert_eq!( + marker, "DENIED", + "open of a deleted COW file must be denied (ENOENT), not read lower content (leaked: {:?})", leaked + ); + assert!( + leaked.is_empty(), + "no pre-delete bytes may leak through the read path, got: {:?}", leaked + ); + + let _ = fs::remove_dir_all(&workdir); + let _ = fs::remove_file(&out_file); + let _ = fs::remove_file(&leak_file); +} + /// Regression test: statx on a COW-created file must succeed. /// /// statx is what `ls`, `stat`, and most modern coreutils use. The COW