Repository navigation
prepare_filesystem() should not chown existing directories in read_write #783
Description
Problem
prepare_filesystem() in crates/openshell-sandbox/src/lib.rs chowns every path in the policy's read_write list to run_as_user:run_as_group before spawning the child process. This happens unconditionally — even for directories that already exist in the container image with intentional ownership.
This means an image author who sets root:root on a read_write directory (e.g., to protect immutable subdirectories via DAC while allowing writes to others) has that ownership silently overridden at sandbox start.
Example (NemoClaw)
Our Dockerfile sets /sandbox/.nemoclaw to root:root 755 to prevent the agent from renaming the root-owned blueprints/ subdirectory. The policy lists it as read_write because sandbox-owned subdirs (state/, migration/) inside it need to be writable.
At sandbox start, prepare_filesystem() chowns /sandbox/.nemoclaw to sandbox:sandbox, removing the DAC protection. QA confirmed the ownership flip at runtime in NVIDIA/NemoClaw#1607 / nvbug 6059437.
We've worked around this with the sticky bit (chmod 1755) in NVIDIA/NemoClaw#1121, which prevents the sandbox user from renaming or deleting root-owned entries even after the ownership flip. But the root cause is the unconditional chown.
Context
This came up during security hardening review for NemoClaw PR #1121 (read-only sandbox filesystem). The NemoClaw team discussed this with our tech lead and agreed the sticky bit is a sufficient short-term workaround, but that prepare_filesystem() respecting existing ownership would be the cleaner long-term fix — both for us and for any other OpenShell image author who intentionally hardens read_write paths.
Proposed Change
Only chown directories that prepare_filesystem() creates. If a directory already exists in the image, respect the image author's ownership:
for path in &policy.filesystem.read_write {
if let Ok(meta) = std::fs::symlink_metadata(path) {
if meta.file_type().is_symlink() {
return Err(...); // existing symlink check
}
// Directory exists — image author set ownership intentionally, leave it
} else {
// Directory doesn't exist — create and chown
std::fs::create_dir_all(path).into_diagnostic()?;
chown(path, uid, gid).into_diagnostic()?;
}
}This is backwards-compatible — directories that don't exist in the image still get created and chowned. Only pre-existing directories are left alone.
Activity
fix: preserve existing directory ownership in prepare_filesystem()
prekshivyas commentedon Apr 8, 2026
Reference implementation available at
prekshivyas/OpenShell:fix/prepare-filesystem-respect-existing-ownership— single commit, moves thechown()call into theelsebranch of the existingsymlink_metadatacheck. Happy to open a PR if given collaborator access, or the OpenShell team can pick it up directly.mjamiv commentedon Apr 8, 2026
+1 on this. We run 4 OpenClaw agents in separate OpenShell sandboxes with
read_writepaths for/sandbox/.openclaw/and subpaths. We've setchmod 444on specific files (e.g., persona files) as a soft integrity guard, but the unconditionalchowninprepare_filesystem()could interact unexpectedly if we tighten ownership in the future.The proposed fix (only chowning newly-created directories) seems clean and low-risk. Would be happy to test on our fleet if a PR lands.
johntmyers commentedon Apr 14, 2026
Implementation Plan
Issue type:
fixComplexity: Low
Confidence: High — clear path
Summary
prepare_filesystem()currently applieschown()to everyfilesystem.read_writepath, even when the path already exists in the image with intentional ownership. The fix is to preserve the existing symlink guard, but only callcreate_dir_all()andchown()for paths the supervisor creates at sandbox startup.Scope
crates/openshell-sandbox/src/lib.rs: move the ownership change into the directory-creation branch so pre-existing paths keep image-defined ownership while newly-created paths still become writable for the sandbox user.crates/openshell-sandbox/src/lib.rstests: add unit coverage for the create-vs-preserve behavior and keep the existing symlink safety behavior covered.Implementation Steps
prepare_filesystem()so an existing non-symlinkread_writepath is left unchanged, while a missing path is created and then chowned.prepare_filesystem()using temporary directories to verify newly-created paths are writable by the configured identity path, and pre-existing paths are not re-owned.Test Plan
crates/openshell-sandbox/src/lib.rstests to cover existing path preservation, new path creation, and symlink rejection.Risks & Open Questions
sandboxaccount exists in every dev environment.read_writeshould continue to behave as they do today; the fix should not broaden the function’s responsibilities beyond avoiding unnecessarychown()calls.Documentation Impact
None expected.
Revision 1 — initial plan
fix(sandbox): preserve ownership for existing read_write paths
johntmyers commentedon Apr 14, 2026
Implementation Complete
PR: #827
What was built
prepare_filesystem()now preserves ownership on pre-existingread_writepaths and onlychowns paths the supervisor creates at sandbox startup. The change keeps the symlink safety check in place, adds focused regression tests, and updates the architecture docs that described the old behavior.Tests
Docs updated
architecture/sandbox.mdarchitecture/security-policy.mdThe issue will auto-close when the PR is merged.
johntmyers commentedon Apr 14, 2026
@mjamiv thank you (as usual!). PR opened.
fix(sandbox): preserve ownership for existing read_write paths (#827)