diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 72effa98f8..3f291c6bb2 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1168,10 +1168,12 @@ 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. +/// +/// `EROFS` errors from `chown` are logged at debug level and the walk still +/// 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 nix::unistd::chown; - let meta = std::fs::symlink_metadata(root).into_diagnostic()?; if meta.file_type().is_symlink() { return Err(miette::miette!( @@ -1180,22 +1182,38 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result )); } - chown(root, uid, gid).into_diagnostic()?; + chown_recursive(root, uid, gid, &nix::unistd::chown) +} + +#[cfg(unix)] +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() { + debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); + return Ok(()); + } + + 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 { + 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 - .symlink_metadata() - .is_ok_and(|m| m.file_type().is_symlink()) - { - debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); - continue; - } - chown_sandbox_home(&path, uid, gid)?; + let child = entry.path(); + chown_recursive(&child, uid, gid, do_chown)?; } } @@ -2115,6 +2133,69 @@ mod tests { .expect("should skip symlink children without error"); } + #[cfg(unix)] + #[test] + 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 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()); + + let chowned: Arc>> = Arc::new(Mutex::new(Vec::new())); + 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(()) + }; + + 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(&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] + fn chown_recursive_propagates_non_erofs_errors() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + + 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 result = chown_recursive(&root, uid, gid, &fake_chown); + assert!(result.is_err(), "non-EROFS errors should propagate"); + } + #[cfg(unix)] #[test] fn rewrite_passwd_modifies_existing_sandbox_entry() {