fix(sandbox): skip read-only mounts during recursive chown of /sandbox

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<Uid>, gid: Option<Gid>) -> 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<Uid>, gid: Option<Gid>) -> Result
         ));
     }
 
-    chown(root, uid, gid).into_diagnostic()?;
+    chown_recursive(root, uid, gid, &nix::unistd::chown)
+}
+
+#[cfg(unix)]
+fn chown_recursive(
+    path: &Path,
+    uid: Option<Uid>,
+    gid: Option<Gid>,
+    do_chown: &impl Fn(&Path, Option<Uid>, Option<Gid>) -> 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<Mutex<Vec<PathBuf>>> = 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<Uid>, _gid: Option<Gid>| -> 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<Uid>, _gid: Option<Gid>| -> 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() {