From 9d32abc28c050fdbb780ce5ffedacb0bd42ce384 Mon Sep 17 00:00:00 2001 From: jamesboyzj-design Date: Sat, 3 Oct 2026 18:03:58 +0800 Subject: [PATCH] fix: require chroot write grants for open side effects --- crates/sandlock-core/src/chroot/dispatch.rs | 48 ++++++++++-- .../tests/integration/test_chroot.rs | 75 +++++++++++++++++++ tests/rootfs-helper.c | 26 +++++++ 3 files changed, 142 insertions(+), 7 deletions(-) diff --git a/crates/sandlock-core/src/chroot/dispatch.rs b/crates/sandlock-core/src/chroot/dispatch.rs index 55bef5b1..d08a170a 100644 --- a/crates/sandlock-core/src/chroot/dispatch.rs +++ b/crates/sandlock-core/src/chroot/dispatch.rs @@ -143,6 +143,25 @@ impl ChrootCtx<'_> { self.writable.iter().any(|p| virtual_path.starts_with(p)) } + /// The access mode governs the returned fd; creation and truncation are + /// separate effects, even when the requested fd is O_RDONLY. The + /// supervisor opens on behalf of the child, so Landlock cannot enforce + /// these checks on the supervisor's open. + fn can_open(&self, virtual_path: &Path, flags: i32) -> bool { + if flags & libc::O_PATH != 0 { + // O_PATH does not mutate or open file contents. Invalid flag + // combinations are still rejected by the openat2 path below. + return self.can_read(virtual_path); + } + let mode = flags & libc::O_ACCMODE; + let reads = mode != libc::O_WRONLY; + let writes = mode == libc::O_WRONLY + || mode == libc::O_RDWR + || flags & (libc::O_CREAT | libc::O_TRUNC) != 0 + || flags & libc::O_TMPFILE == libc::O_TMPFILE; + (!reads || self.can_read(virtual_path)) && (!writes || self.can_write(virtual_path)) + } + /// Check if a virtual path falls under any mount point. fn is_mounted(&self, virtual_path: &Path) -> bool { self.mounts.iter().any(|(vp, _)| virtual_path.starts_with(vp)) @@ -594,13 +613,8 @@ pub(crate) async fn handle_chroot_open( None => return NotifAction::Errno(libc::EACCES), }; - // Access check: writes need can_write, reads need can_read - let is_write = (flags as i32 & (libc::O_WRONLY | libc::O_RDWR)) != 0; - if is_write { - if !ctx.can_write(&virtual_path) { - return NotifAction::Errno(libc::EACCES); - } - } else if !ctx.can_read(&virtual_path) { + // Check both descriptor access and effects before any COW/open work. + if !ctx.can_open(&virtual_path, flags as i32) { return NotifAction::Errno(libc::EACCES); } @@ -2702,4 +2716,24 @@ mod mount_ro_tests { assert!(c.can_read(Path::new("/data/file"))); assert!(c.can_write(Path::new("/data/file"))); } + + #[test] + fn open_effects_require_write_permission_independently_of_fd_mode() { + let mounts = vec![(PathBuf::from("/ro"), PathBuf::from("/host"))]; + let read_only = vec![PathBuf::from("/ro")]; + let writable = vec![PathBuf::from("/")]; + let processes = Arc::new(ProcessIndex::new()); + let c = ctx(&mounts, &read_only, &writable, &processes); + for flags in [libc::O_RDONLY, libc::O_RDONLY | libc::O_DIRECTORY, libc::O_PATH] { + assert!(c.can_open(Path::new("/ro/file"), flags), "{flags:#x}"); + } + for flags in [ + libc::O_WRONLY, libc::O_RDWR, libc::O_RDONLY | libc::O_CREAT, + libc::O_RDONLY | libc::O_TRUNC, libc::O_RDONLY | libc::O_CREAT | libc::O_EXCL, + libc::O_RDWR | libc::O_TMPFILE, + ] { + assert!(!c.can_open(Path::new("/ro/file"), flags), "{flags:#x}"); + assert!(c.can_open(Path::new("/rw/file"), flags), "{flags:#x}"); + } + } } diff --git a/crates/sandlock-core/tests/integration/test_chroot.rs b/crates/sandlock-core/tests/integration/test_chroot.rs index bf6f2ddf..76984541 100644 --- a/crates/sandlock-core/tests/integration/test_chroot.rs +++ b/crates/sandlock-core/tests/integration/test_chroot.rs @@ -2465,3 +2465,78 @@ async fn test_chroot_shebang_loop_is_eloop() { ); cleanup_rootfs(&rootfs); } + +/// O_RDONLY describes the returned fd, not all effects of open(2): O_TRUNC +/// and O_CREAT still mutate the filesystem in the supervisor's context. +#[tokio::test] +async fn test_chroot_readonly_open_cannot_mutate_files() { + check_readonly_open_effects(false).await; +} + +#[tokio::test] +async fn test_chroot_cow_readonly_open_cannot_mutate_files() { + check_readonly_open_effects(true).await; +} + +async fn check_readonly_open_effects(cow: bool) { + let rootfs = build_test_rootfs(&format!("readonly-open-mutation-{cow}")); + let mounted = temp_dir(&format!("readonly-open-mutation-mount-{cow}")); + fs::create_dir_all(rootfs.join("ro")).unwrap(); + fs::create_dir_all(rootfs.join("rw")).unwrap(); + fs::create_dir_all(rootfs.join("mnt")).unwrap(); + let mut builder = minimal_exec_policy(&rootfs) + .fs_read("/ro") + .fs_write("/rw") + .fs_mount_ro("/mnt", &mounted); + if cow { builder = builder.workdir(&rootfs).on_exit(BranchAction::Commit); } + let policy = builder.build().unwrap(); + let mut failures = Vec::new(); + for spelling in ["open", "openat", "openat2"] { + for (virtual_dir, host_dir, allowed) in [ + ("/ro", rootfs.join("ro"), false), + ("/mnt", mounted.clone(), false), + ("/rw", rootfs.join("rw"), true), + ] { + for (name, flags, exists) in [ + ("read", libc::O_RDONLY, true), + ("truncate", libc::O_RDONLY | libc::O_TRUNC, true), + ("create", libc::O_RDONLY | libc::O_CREAT, false), + ("create-exclusive", libc::O_RDONLY | libc::O_CREAT | libc::O_EXCL, false), + ("path-only", libc::O_PATH | libc::O_TRUNC, true), + ] { + let basename = format!("{spelling}-{name}"); + let host = host_dir.join(&basename); + if exists { fs::write(&host, b"KEEP").unwrap(); } + let path = format!("{virtual_dir}/{basename}"); + let flags_text = flags.to_string(); + let result = policy.clone().run(&[ + "rootfs-helper", "open-flags", spelling, &path, &flags_text, + ]).await.expect("sandbox must run: no skip on setup errors"); + assert!(result.success(), "helper failed: {:?}", result.stderr_str()); + let mutates = name != "read" && name != "path-only"; + let denied = mutates && !allowed; + let expected = if name == "path-only" { + format!("ERR:{}", libc::EINVAL) + } else if denied { format!("ERR:{}", libc::EACCES) } else { "OPENED".into() }; + let output = result.stdout_str().unwrap().trim(); + if output != expected { + failures.push(format!("{path}: expected {expected}, got {output}")); + } + if denied || !mutates { + if exists { + if fs::read(&host).unwrap() != b"KEEP" { + failures.push(format!("{path}: original bytes changed")); + } + } else if host.exists() { + failures.push(format!("{path}: forbidden file was created")); + } + } else if !host.exists() || !fs::read(&host).unwrap().is_empty() { + failures.push(format!("{path}: allowed operation did not take effect")); + } + } + } + } + cleanup_rootfs(&rootfs); + fs::remove_dir_all(mounted).unwrap(); + assert!(failures.is_empty(), "{}", failures.join("\n")); +} diff --git a/tests/rootfs-helper.c b/tests/rootfs-helper.c index abe0659c..cc0afc92 100644 --- a/tests/rootfs-helper.c +++ b/tests/rootfs-helper.c @@ -731,6 +731,31 @@ struct helper_open_how { unsigned long long resolve; }; +/* Report the raw errno without hiding failed syscall execution behind a skip. */ +static int cmd_open_flags(int argc, char **argv) { + if (argc != 3) return 2; + int flags = (int)strtol(argv[2], NULL, 0); + unsigned int mode = (flags & O_CREAT) ? 0600 : 0; + long fd; + if (strcmp(argv[0], "openat2") == 0) { + struct helper_open_how how = { .flags = (unsigned int)flags, .mode = mode }; + fd = syscall(__NR_openat2, AT_FDCWD, argv[1], &how, sizeof(how)); + } else if (strcmp(argv[0], "openat") == 0) { + fd = syscall(SYS_openat, AT_FDCWD, argv[1], flags, mode); + } else if (strcmp(argv[0], "open") == 0) { +#ifdef SYS_open + fd = syscall(SYS_open, argv[1], flags, mode); +#else + fd = syscall(SYS_openat, AT_FDCWD, argv[1], flags, mode); +#endif + } else { + return 2; + } + if (fd < 0) printf("ERR:%d\n", errno); + else { close((int)fd); puts("OPENED"); } + return 0; +} + 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). */ @@ -880,6 +905,7 @@ static int cmd_fexecve(int argc, char **argv) { /* ── dispatch ───────────────────────────────────────────────── */ static int dispatch(const char *cmd, int argc, char **argv) { + if (strcmp(cmd, "open-flags") == 0) return cmd_open_flags(argc, 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);