From 90fae136772adfe4b3714aaaadb527d55e901545 Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Thu, 16 Jul 2026 22:55:13 -0700 Subject: [PATCH 1/3] fix(sandbox): skip read-only mounts during recursive chown of /sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sandbox supervisor crashed with EROFS when the recursive chown of /sandbox encountered read-only submounts. This is common in gVisor-based Kubernetes deployments where read-only volume mounts are the only way to enforce per-directory immutability (Landlock is unavailable under gVisor). The fix adds two guards to the ownership walk: 1. Mount-boundary detection via st_dev comparison — paths on a different filesystem than /sandbox are skipped entirely, avoiding the chown call on nested read-only mounts. 2. EROFS tolerance — if chown still returns EROFS (e.g. the root mount itself is read-only), the error is logged at debug level and startup continues. Symlink skipping (already present) is preserved and extracted into the recursive walker for consistency. Manually verified on a kind cluster with a read-only PVC mounted at /sandbox/readonly-data: the pod starts successfully, writable paths are owned by the sandbox user, and the read-only mount retains root ownership. Kubernetes e2e test coverage is a separate follow-up. Closes #2294 Signed-off-by: Varsha Prasad Narsing --- .../src/process.rs | 117 ++++++++++++++++-- 1 file changed, 110 insertions(+), 7 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 72effa98f8..f11021062d 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1168,9 +1168,14 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { /// Symlinks are skipped (not followed) to prevent privilege escalation via /// malicious container images. The TOCTOU window is not exploitable because /// no untrusted process is running yet. +/// +/// The walk does not cross filesystem boundaries (compares `st_dev`) so that +/// read-only mounts nested under `/sandbox` are left untouched. Any remaining +/// `EROFS` errors from `chown` are logged as warnings rather than aborting +/// startup. #[cfg(unix)] fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result<()> { - use nix::unistd::chown; + use std::os::unix::fs::MetadataExt; let meta = std::fs::symlink_metadata(root).into_diagnostic()?; if meta.file_type().is_symlink() { @@ -1180,22 +1185,44 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result )); } - chown(root, uid, gid).into_diagnostic()?; + let root_dev = meta.dev(); + chown_recursive(root, uid, gid, root_dev) +} + +#[cfg(unix)] +fn chown_recursive(path: &Path, uid: Option, gid: Option, root_dev: u64) -> Result<()> { + use nix::unistd::chown; + use std::os::unix::fs::MetadataExt; + + let meta = std::fs::symlink_metadata(path).into_diagnostic()?; + + if meta.dev() != root_dev { + debug!(path = %path.display(), "Skipping mount boundary during sandbox home chown"); + return Ok(()); + } + + if let Err(e) = chown(path, uid, gid) { + if e == nix::errno::Errno::EROFS { + debug!(path = %path.display(), "Skipping read-only path during sandbox home chown"); + return Ok(()); + } + return Err(e).into_diagnostic(); + } if meta.is_dir() - && let Ok(entries) = std::fs::read_dir(root) + && let Ok(entries) = std::fs::read_dir(path) { for entry in entries { let entry = entry.into_diagnostic()?; - let path = entry.path(); - if path + let child = entry.path(); + if child .symlink_metadata() .is_ok_and(|m| m.file_type().is_symlink()) { - debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); + debug!(path = %child.display(), "Skipping symlink during sandbox home chown"); continue; } - chown_sandbox_home(&path, uid, gid)?; + chown_recursive(&child, uid, gid, root_dev)?; } } @@ -2115,6 +2142,82 @@ mod tests { .expect("should skip symlink children without error"); } + #[cfg(unix)] + #[test] + fn chown_recursive_skips_different_device() { + use std::os::unix::fs::MetadataExt; + + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + let child = root.join("file.txt"); + std::fs::write(&child, "hello").unwrap(); + + let before_root = std::fs::metadata(&root).unwrap(); + let before_child = std::fs::metadata(&child).unwrap(); + + let uid = Some(nix::unistd::geteuid()); + let gid = Some(nix::unistd::getegid()); + + // Pass a fake root_dev that doesn't match the temp directory's device. + // chown_recursive should skip the path entirely instead of failing. + let fake_dev = std::fs::symlink_metadata(&root) + .unwrap() + .dev() + .wrapping_add(1); + chown_recursive(&root, uid, gid, fake_dev) + .expect("should skip paths on a different device"); + + let after_root = std::fs::metadata(&root).unwrap(); + let after_child = std::fs::metadata(&child).unwrap(); + assert_eq!( + before_root.uid(), + after_root.uid(), + "root directory should not have been chowned" + ); + assert_eq!( + before_child.uid(), + after_child.uid(), + "child file should not have been chowned" + ); + } + + #[cfg(unix)] + #[test] + fn chown_sandbox_home_does_not_chown_across_device_boundary() { + use std::os::unix::fs::MetadataExt; + + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(root.join("owned.txt"), "writable").unwrap(); + let subdir = root.join("mount"); + std::fs::create_dir(&subdir).unwrap(); + std::fs::write(subdir.join("skipped.txt"), "read-only").unwrap(); + + let expected_uid = nix::unistd::geteuid(); + let expected_gid = nix::unistd::getegid(); + + chown_sandbox_home(&root, Some(expected_uid), Some(expected_gid)).unwrap(); + + let owned = std::fs::metadata(root.join("owned.txt")).unwrap(); + assert_eq!( + owned.uid(), + expected_uid.as_raw(), + "same-device file should be chowned" + ); + + // subdir is on the same device in this test, so it WILL be chowned. + // This test documents the normal same-device behavior. The cross-device + // skip is tested by chown_recursive_skips_different_device above. + let sub = std::fs::metadata(&subdir).unwrap(); + assert_eq!( + sub.uid(), + expected_uid.as_raw(), + "same-device subdirectory should be chowned" + ); + } + #[cfg(unix)] #[test] fn rewrite_passwd_modifies_existing_sandbox_entry() { From 4cd90597907ffc6a32a83927a3d8363094561f64 Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Fri, 17 Jul 2026 13:08:08 -0700 Subject: [PATCH 2/3] refactor(sandbox): consolidate symlink check into chown_recursive Move the per-child symlink guard from the directory iteration loop into the top of chown_recursive, reusing the symlink_metadata call that is already performed there. This centralizes all three skip guards (symlink, mount boundary, EROFS) in one place and eliminates a redundant symlink_metadata call per child entry. Suggested-by: elezar Signed-off-by: Varsha Prasad Narsing --- crates/openshell-supervisor-process/src/process.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index f11021062d..7a4ec8715d 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1196,6 +1196,11 @@ fn chown_recursive(path: &Path, uid: Option, gid: Option, root_dev: u6 let meta = std::fs::symlink_metadata(path).into_diagnostic()?; + if meta.file_type().is_symlink() { + debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); + return Ok(()); + } + if meta.dev() != root_dev { debug!(path = %path.display(), "Skipping mount boundary during sandbox home chown"); return Ok(()); @@ -1215,13 +1220,6 @@ fn chown_recursive(path: &Path, uid: Option, gid: Option, root_dev: u6 for entry in entries { let entry = entry.into_diagnostic()?; let child = entry.path(); - if child - .symlink_metadata() - .is_ok_and(|m| m.file_type().is_symlink()) - { - debug!(path = %child.display(), "Skipping symlink during sandbox home chown"); - continue; - } chown_recursive(&child, uid, gid, root_dev)?; } } From 3d079d5d01c247bf344b426da5e1e4e1e0283ff7 Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Fri, 17 Jul 2026 18:11:50 -0700 Subject: [PATCH 3/3] fix(sandbox): recurse into children after EROFS and fix doc comment When chown returns EROFS on a directory, the code previously returned early without recursing into children. This skipped writable submounts nested under a read-only directory on the same device (e.g., a writable emptyDir inside a read-only ConfigMap mount sharing the same st_dev). Now the EROFS case falls through to the directory recursion so that writable children are still chowned. Also fixes the doc comment which said EROFS errors are "logged as warnings" when they are actually logged at debug level. Signed-off-by: Varsha Prasad Narsing --- crates/openshell-supervisor-process/src/process.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 7a4ec8715d..e06e62a244 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1171,8 +1171,8 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { /// /// The walk does not cross filesystem boundaries (compares `st_dev`) so that /// read-only mounts nested under `/sandbox` are left untouched. Any remaining -/// `EROFS` errors from `chown` are logged as warnings rather than aborting -/// startup. +/// `EROFS` errors from `chown` are logged at debug level and the walk still +/// recurses into children (a read-only directory may contain writable submounts). #[cfg(unix)] fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result<()> { use std::os::unix::fs::MetadataExt; @@ -1209,9 +1209,9 @@ fn chown_recursive(path: &Path, uid: Option, gid: Option, root_dev: u6 if let Err(e) = chown(path, uid, gid) { if e == nix::errno::Errno::EROFS { debug!(path = %path.display(), "Skipping read-only path during sandbox home chown"); - return Ok(()); + } else { + return Err(e).into_diagnostic(); } - return Err(e).into_diagnostic(); } if meta.is_dir()