fix(sandbox): preserve ownership for existing read_write paths (#827)

Closes #783

Only chown read_write paths that the supervisor creates at startup, and leave pre-existing image paths with their original ownership. Add sandbox tests for creation, symlink rejection, and existing-path ownership preservation, and update architecture docs to match the new behavior.

Co-authored-by: John Myers <johntmyers@users.noreply.github.com>

Author: johntmyers

architecture/sandbox.md

@@ -88,7 +88,7 @@ flowchart TD
 
 3. **Binary identity cache**: If OPA engine is active, create `Arc<BinaryIdentityCache::new()>` for SHA256 TOFU enforcement.
 
-4. **Filesystem preparation** (`prepare_filesystem()`): For each path in `filesystem.read_write`, create the directory if it does not exist and `chown` to the configured `run_as_user`/`run_as_group`. Runs as the supervisor (root) before forking.
+4. **Filesystem preparation** (`prepare_filesystem()`): For each path in `filesystem.read_write`, reject symlinks, create the directory if it does not exist, and `chown` only newly-created paths to the configured `run_as_user`/`run_as_group`. Pre-existing paths keep the image-defined ownership. Runs as the supervisor (root) before forking.
 
 5. **TLS state for L7 inspection** (proxy mode only):
    - Generate ephemeral CA via `SandboxCa::generate()` using `rcgen`

architecture/security-policy.md

@@ -322,7 +322,7 @@ Controls which filesystem paths the sandboxed process can access. Enforced via L
 
 **Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V2)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V2)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset.
 
-**Filesystem preparation**: Before the child process spawns, the supervisor creates any `read_write` directories that do not exist and sets their ownership to `process.run_as_user`:`process.run_as_group` via `chown()`. See `crates/openshell-sandbox/src/lib.rs` -- `prepare_filesystem()`.
+**Filesystem preparation**: Before the child process spawns, the supervisor rejects symlinked `read_write` paths, creates any missing `read_write` directories, and sets ownership via `chown()` only on paths it created. Pre-existing image paths keep their existing ownership. See `crates/openshell-sandbox/src/lib.rs` -- `prepare_filesystem()`.
 
 **Working directory**: When `include_workdir` is `true` and a `--workdir` is specified, the working directory path is appended to `read_write` if not already present. See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `apply()`.
 

crates/openshell-sandbox/src/lib.rs

@@ -1764,11 +1764,44 @@ fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> {
     Ok(())
 }
 
+/// Prepare a `read_write` path for the sandboxed process.
+///
+/// Returns `true` when the path was created by the supervisor and therefore
+/// still needs to be chowned to the sandbox user/group. Existing paths keep
+/// their image-defined ownership.
+#[cfg(unix)]
+fn prepare_read_write_path(path: &std::path::Path) -> Result<bool> {
+    // SECURITY: use symlink_metadata (lstat) to inspect each path *before*
+    // calling chown. chown follows symlinks, so a malicious container image
+    // could place a symlink (e.g. /sandbox -> /etc/shadow) to trick the
+    // root supervisor into transferring ownership of arbitrary files.
+    // The TOCTOU window between lstat and chown is not exploitable because
+    // no untrusted process is running yet (the child has not been forked).
+    if let Ok(meta) = std::fs::symlink_metadata(path) {
+        if meta.file_type().is_symlink() {
+            return Err(miette::miette!(
+                "read_write path '{}' is a symlink — refusing to chown (potential privilege escalation)",
+                path.display()
+            ));
+        }
+
+        debug!(
+            path = %path.display(),
+            "Preserving ownership for existing read_write path"
+        );
+        Ok(false)
+    } else {
+        debug!(path = %path.display(), "Creating read_write directory");
+        std::fs::create_dir_all(path).into_diagnostic()?;
+        Ok(true)
+    }
+}
+
 /// Prepare filesystem for the sandboxed process.
 ///
 /// Creates `read_write` directories if they don't exist and sets ownership
-/// to the configured sandbox user/group. This runs as the supervisor (root)
-/// before forking the child process.
+/// on newly-created paths to the configured sandbox user/group. This runs as
+/// the supervisor (root) before forking the child process.
 #[cfg(unix)]
 fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> {
     use nix::unistd::{Group, User, chown};
@@ -1810,31 +1843,17 @@ fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> {
         None
     };
 
-    // Create and chown each read_write path.
-    //
-    // SECURITY: use symlink_metadata (lstat) to inspect each path *before*
-    // calling chown.  chown follows symlinks, so a malicious container image
-    // could place a symlink (e.g. /sandbox -> /etc/shadow) to trick the
-    // root supervisor into transferring ownership of arbitrary files.
-    // The TOCTOU window between lstat and chown is not exploitable because
-    // no untrusted process is running yet (the child has not been forked).
+    // Create missing read_write paths and only chown the ones we created.
     for path in &policy.filesystem.read_write {
-        // Check for symlinks before touching the path.  Character/block devices
-        // (e.g. /dev/null) are legitimate read_write entries and must be allowed.
-        if let Ok(meta) = std::fs::symlink_metadata(path) {
-            if meta.file_type().is_symlink() {
-                return Err(miette::miette!(
-                    "read_write path '{}' is a symlink — refusing to chown (potential privilege escalation)",
-                    path.display()
-                ));
-            }
-        } else {
-            debug!(path = %path.display(), "Creating read_write directory");
-            std::fs::create_dir_all(path).into_diagnostic()?;
+        if prepare_read_write_path(path)? {
+            debug!(
+                path = %path.display(),
+                ?uid,
+                ?gid,
+                "Setting ownership on newly created read_write path"
+            );
+            chown(path, uid, gid).into_diagnostic()?;
         }
-
-        debug!(path = %path.display(), ?uid, ?gid, "Setting ownership on read_write directory");
-        chown(path, uid, gid).into_diagnostic()?;
     }
 
     Ok(())
@@ -2163,6 +2182,11 @@ fn format_setting_value(es: &openshell_core::proto::EffectiveSetting) -> String
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::policy::{FilesystemPolicy, LandlockPolicy, ProcessPolicy};
+    #[cfg(unix)]
+    use nix::unistd::{Group, User};
+    #[cfg(unix)]
+    use std::os::unix::fs::{MetadataExt, symlink};
     use temp_env::with_vars;
 
     static ENV_LOCK: std::sync::LazyLock<std::sync::Mutex<()>> =
@@ -2583,4 +2607,100 @@ filesystem_policy:
         assert_eq!(read.len(), 1);
         assert_eq!(read[0].model, "original-model");
     }
+
+    #[cfg(unix)]
+    fn sandbox_policy_with_read_write(
+        path: std::path::PathBuf,
+        run_as_user: Option<String>,
+        run_as_group: Option<String>,
+    ) -> SandboxPolicy {
+        SandboxPolicy {
+            version: 1,
+            filesystem: FilesystemPolicy {
+                read_only: vec![],
+                read_write: vec![path],
+                include_workdir: false,
+            },
+            network: NetworkPolicy::default(),
+            landlock: LandlockPolicy::default(),
+            process: ProcessPolicy {
+                run_as_user,
+                run_as_group,
+            },
+        }
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn prepare_read_write_path_creates_missing_directory() {
+        let dir = tempfile::tempdir().unwrap();
+        let missing = dir.path().join("missing").join("nested");
+
+        assert!(prepare_read_write_path(&missing).unwrap());
+        assert!(missing.is_dir());
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn prepare_read_write_path_preserves_existing_directory() {
+        let dir = tempfile::tempdir().unwrap();
+        let existing = dir.path().join("existing");
+        std::fs::create_dir(&existing).unwrap();
+
+        assert!(!prepare_read_write_path(&existing).unwrap());
+        assert!(existing.is_dir());
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn prepare_read_write_path_rejects_symlink() {
+        let dir = tempfile::tempdir().unwrap();
+        let target = dir.path().join("target");
+        let link = dir.path().join("link");
+        std::fs::create_dir(&target).unwrap();
+        symlink(&target, &link).unwrap();
+
+        let error = prepare_read_write_path(&link).unwrap_err();
+        assert!(
+            error
+                .to_string()
+                .contains("is a symlink — refusing to chown"),
+            "unexpected error: {error}"
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn prepare_filesystem_skips_chown_for_existing_read_write_paths() {
+        if nix::unistd::geteuid().is_root() {
+            return;
+        }
+
+        let current_user = User::from_uid(nix::unistd::geteuid())
+            .unwrap()
+            .expect("current user entry");
+        let restricted_group = Group::from_gid(nix::unistd::Gid::from_raw(0))
+            .unwrap()
+            .expect("gid 0 group entry");
+        if restricted_group.gid == nix::unistd::getegid() {
+            return;
+        }
+
+        let dir = tempfile::tempdir().unwrap();
+        let existing = dir.path().join("existing");
+        std::fs::create_dir(&existing).unwrap();
+        let before = std::fs::metadata(&existing).unwrap();
+
+        let policy = sandbox_policy_with_read_write(
+            existing.clone(),
+            Some(current_user.name),
+            Some(restricted_group.name),
+        );
+
+        prepare_filesystem(&policy).expect("existing path should not be re-owned");
+
+        let after = std::fs::metadata(&existing).unwrap();
+        assert_eq!(after.uid(), before.uid());
+        assert_eq!(after.gid(), before.gid());
+    }
 }