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/6] 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/6] 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/6] 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() From 888f91a47dfd8f14f86986356a385b24cd805184 Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Mon, 20 Jul 2026 11:11:34 -0700 Subject: [PATCH 4/6] fix(sandbox): allow similar_names clippy lint in chown test Signed-off-by: Varsha Prasad Narsing --- crates/openshell-supervisor-process/src/process.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index e06e62a244..77c6bea6f6 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -2182,6 +2182,7 @@ mod tests { #[cfg(unix)] #[test] + #[allow(clippy::similar_names)] fn chown_sandbox_home_does_not_chown_across_device_boundary() { use std::os::unix::fs::MetadataExt; From 08695910c048edf59643e547bef38b1af61bf38a Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Tue, 21 Jul 2026 11:35:48 -0700 Subject: [PATCH 5/6] fix(sandbox): remove cross-device skip and inject chown for EROFS tests Remove the unconditional st_dev boundary check that prevented chown on writable PVC mounts with a different device ID. Rely solely on EROFS errors to skip read-only paths while continuing traversal into children and siblings. Inject the chown operation into chown_recursive to enable unit testing the EROFS recovery path without requiring root or mount privileges. Signed-off-by: Varsha Prasad Narsing Signed-off-by: Varsha Prasad Narsing --- .../src/process.rs | 124 ++++++++---------- 1 file changed, 55 insertions(+), 69 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 77c6bea6f6..e719cd6788 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1169,14 +1169,11 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { /// 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 at debug level and the walk still -/// recurses into children (a read-only directory may contain writable submounts). +/// recurses into children (a read-only directory may contain writable submounts +/// or siblings that must be owned by the sandbox user). #[cfg(unix)] fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result<()> { - use std::os::unix::fs::MetadataExt; - let meta = std::fs::symlink_metadata(root).into_diagnostic()?; if meta.file_type().is_symlink() { return Err(miette::miette!( @@ -1185,15 +1182,16 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result )); } - let root_dev = meta.dev(); - chown_recursive(root, uid, gid, root_dev) + chown_recursive(root, uid, gid, &nix::unistd::chown) } #[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; - +fn chown_recursive( + path: &Path, + uid: Option, + gid: Option, + do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, +) -> Result<()> { let meta = std::fs::symlink_metadata(path).into_diagnostic()?; if meta.file_type().is_symlink() { @@ -1201,12 +1199,7 @@ fn chown_recursive(path: &Path, uid: Option, gid: Option, root_dev: u6 return Ok(()); } - 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 let Err(e) = do_chown(path, uid, gid) { if e == nix::errno::Errno::EROFS { debug!(path = %path.display(), "Skipping read-only path during sandbox home chown"); } else { @@ -1220,7 +1213,7 @@ 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(); - chown_recursive(&child, uid, gid, root_dev)?; + chown_recursive(&child, uid, gid, do_chown)?; } } @@ -2142,79 +2135,72 @@ mod tests { #[cfg(unix)] #[test] - fn chown_recursive_skips_different_device() { - use std::os::unix::fs::MetadataExt; + fn chown_recursive_skips_erofs_and_continues() { + use std::sync::{Arc, Mutex}; 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 readonly_dir = root.join("ro-mount"); + std::fs::create_dir(&readonly_dir).unwrap(); + std::fs::write(readonly_dir.join("writable-child.txt"), "data").unwrap(); + + std::fs::write(root.join("writable-sibling.txt"), "data").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 chowned: Arc>> = Arc::new(Mutex::new(Vec::new())); + let chowned_ref = Arc::clone(&chowned); - 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" + let readonly_dir_clone = readonly_dir.clone(); + let fake_chown = move |path: &Path, + _uid: Option, + _gid: Option| + -> nix::Result<()> { + if path == readonly_dir_clone { + return Err(nix::errno::Errno::EROFS); + } + chowned_ref.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; + + chown_recursive(&root, uid, gid, &fake_chown) + .expect("EROFS should be handled gracefully"); + + let chowned = chowned.lock().unwrap(); + assert!( + chowned.contains(&root), + "root should have been chowned" ); - assert_eq!( - before_child.uid(), - after_child.uid(), - "child file should not have been chowned" + assert!( + chowned.contains(&readonly_dir.join("writable-child.txt")), + "writable child under EROFS directory should still be chowned" + ); + assert!( + chowned.contains(&root.join("writable-sibling.txt")), + "writable sibling should still be chowned" ); } #[cfg(unix)] #[test] - #[allow(clippy::similar_names)] - fn chown_sandbox_home_does_not_chown_across_device_boundary() { - use std::os::unix::fs::MetadataExt; - + fn chown_recursive_propagates_non_erofs_errors() { 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 uid = Some(nix::unistd::geteuid()); + let gid = Some(nix::unistd::getegid()); - let owned = std::fs::metadata(root.join("owned.txt")).unwrap(); - assert_eq!( - owned.uid(), - expected_uid.as_raw(), - "same-device file should be chowned" - ); + let fake_chown = |_path: &Path, + _uid: Option, + _gid: Option| + -> nix::Result<()> { Err(nix::errno::Errno::EPERM) }; - // 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" - ); + let result = chown_recursive(&root, uid, gid, &fake_chown); + assert!(result.is_err(), "non-EROFS errors should propagate"); } #[cfg(unix)] From 2250cb23c012f9da545b81bfeb9d0b099406e115 Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Tue, 21 Jul 2026 11:55:23 -0700 Subject: [PATCH 6/6] style(sandbox): apply rustfmt to chown test closures Signed-off-by: Varsha Prasad Narsing Signed-off-by: Varsha Prasad Narsing --- .../src/process.rs | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index e719cd6788..3f291c6bb2 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -2155,25 +2155,19 @@ mod tests { let chowned_ref = Arc::clone(&chowned); let readonly_dir_clone = readonly_dir.clone(); - let fake_chown = move |path: &Path, - _uid: Option, - _gid: Option| - -> nix::Result<()> { - if path == readonly_dir_clone { - return Err(nix::errno::Errno::EROFS); - } - chowned_ref.lock().unwrap().push(path.to_path_buf()); - Ok(()) - }; + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + if path == readonly_dir_clone { + return Err(nix::errno::Errno::EROFS); + } + chowned_ref.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; - chown_recursive(&root, uid, gid, &fake_chown) - .expect("EROFS should be handled gracefully"); + chown_recursive(&root, uid, gid, &fake_chown).expect("EROFS should be handled gracefully"); let chowned = chowned.lock().unwrap(); - assert!( - chowned.contains(&root), - "root should have been chowned" - ); + assert!(chowned.contains(&root), "root should have been chowned"); assert!( chowned.contains(&readonly_dir.join("writable-child.txt")), "writable child under EROFS directory should still be chowned" @@ -2194,10 +2188,9 @@ mod tests { let uid = Some(nix::unistd::geteuid()); let gid = Some(nix::unistd::getegid()); - let fake_chown = |_path: &Path, - _uid: Option, - _gid: Option| - -> nix::Result<()> { Err(nix::errno::Errno::EPERM) }; + let fake_chown = |_path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + Err(nix::errno::Errno::EPERM) + }; let result = chown_recursive(&root, uid, gid, &fake_chown); assert!(result.is_err(), "non-EROFS errors should propagate");