Repository navigation
fix(containment): retain Linux bind mount targets - #254
kevin-orellana wants to merge 1 commit into
Conversation
## Problem Linux mount setup can follow a changed path after source validation. The saved baseline fails nested_directory_targets_do_not_create_outside_after_source_pinning and nested_file_targets_do_not_mount_outside_after_source_pinning. ## Solution Retain mount targets through preparation, attachment, and placement checks. Refuse missing targets below exposed host trees, as checked by missing_nested_targets_are_not_recreated. Resolve permitted aliases inside the staged root. The test absolute_file_alias_remains_writable_inside_the_view checks this behavior. Preserve canonical permissions and dependent mount ordering, as checked by a_read_only_directory_alias_preserves_the_canonical_write_grant and an_alias_bind_does_not_replace_nested_metadata_mounts. No public interface or policy authority changes. ## Tests The focused nested_grants suite passed 30 tests across 39 fixture cases. Its uncaptured output contains no explicit skips. The workspace suite passed with EXPERIMENTAL_LINUX_SUPPORT=TRUE; 17 tests were ignored. Containment builds passed with all features and without default features. Formatting and workspace Clippy passed with the repository's justfile allowances. The build without default features emitted 66 warnings, without a baseline comparison. The separate deterministic suite was GREEN: 124 passed, zero failures or errors, and nine declared macOS-only skips. It used debug product binaries. No scripted examples remain. The live SDK tutorial was not run. Saved baseline logs show the reported failures, but the original test body is absent from the saved baseline patch. Fresh baseline and protection-removal checks were not completed. Recursive remounts still resolve pathnames. The tests do not cover every race after mount-placement verification or every outside metadata field. Native validation covers Linux AArch64 only. Hosted checks remain separate.
github-actions
Bot
added
Area - Agent Containment
bug
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Read-only and no-exec are a *second* operation. The bind above is `MS_REC`, so a | ||
| // granted directory containing a submount brings that submount in too. | ||
| let mountpoint = target_pin.location(); |
There was a problem hiding this comment.
Important (non-blocking, but cheap to close in this PR): The flag remount after placement verification still resolves location() as a pathname. If that resolution is redirected, the new bind keeps its inherited flags, so the residual fails open, not closed.
Example: The attacker is the one that symlink_replacement_after_mount_* models: a process that can write the exposed host tree, such as another box's workload with write = ["~/work"]. The grants are read = ["~/work"] and read_file = ["~/work/sub/x"]. After require_new_mount succeeds, the attacker renames sub to sub2, which moves the new mount with it. Then they create sub -> D with D/x -> /proc, or any other path that is a mountpoint in this pre-pivot namespace. remount_read_only_noexec("/stage/…/sub/x") follows the links and succeeds on that other mount. The submount scan matches nothing under the old prefix. The bind of x keeps the flags it copied from the host's /home mount: writable and executable. That breaks the premise in AGENTS.md, "the workload … writes only where a list grants a write", and also W^X. Nothing re-checks the flags after this step. The PR description says that remounts still resolve pathnames, but it does not say that this outcome is fail-open.
Suggestion: Use the descriptor that remount_protected(&mounted_path) already uses. Remount the top mount through mounted_path, and keep location() only for the submount scan. Alternatively, after the remount, call fstatvfs on mounted and refuse unless ST_RDONLY/ST_NOEXEC match (entry.writable, entry.executable). Either one adds a few lines and gives a fail-closed result. To pin it, add a hook between require_new_mount and the remount, like the existing after_bind_mount.
| let create = (!matches!(entry.origin, MountOrigin::Protection { .. })) | ||
| .then_some((view, bind.then_some(entry.target.as_path()))); | ||
| let aliases = bind | ||
| && !matches!(entry.origin, MountOrigin::Protection { .. }) |
There was a problem hiding this comment.
Important (blocking): A write protection under a writable alias now refuses startup. On d74f5dd the same configuration ran.
Example: This is the permitted_intermediate_alias fixture with a policy file added. It is the case protect_authorities produces when the policy file sits inside the workspace:
.allow(&original, Read, Root) // original/link -> child
.allow(original.join("link"), Write, Root)
.protect_write(&original.join("child/policy"), &opened)plan emits two Protection entries: original/child/policy and original/link/policy. The planner adds the second one because the alias entry is writable (entry.target.join(relative)). At establish time, the alias bind for original/link resolves to the canonical location and is skipped, so original/link in the staged view is still the host's symlink. The protection entry walks with aliases = false, so O_PATH|O_NOFOLLOW|O_DIRECTORY returns ENOTDIR:
opening write-protected mountpoint '…/original/link/policy': … opening mountpoint '…/original/link' without following links: Not a directory
I ran establish against a staged copy of that shape at both commits, without privilege. On d74f5dd, both protection entries reach mount(2); there, symlink_metadata follows the intermediate link. On this head, the alias entry is refused at the walk. Before this change, the protection branch did not use walk. This PR moved it onto walk without alias resolution.
Suggestion: Let a protection entry resolve aliases in the same way, so drop the Protection exclusion from aliases. Leave create as it is. The walk stays inside the staging root, and require_mounted_identity on the descriptor still pins the object. Two protection entries can then land at the same location, so deduplicate by location or accept the stacked mount. Then add a protect_write case to permitted_intermediate_alias.
|
Assessment: Request Changes The descriptor-retaining walk closes the two reported nested-target cases. One result changes for a configuration that used to run: a write protection under a skipped writable alias now refuses startup (see the inline comment). The remaining flag-remount window fails open, and the PR already holds a descriptor that would close it. Review details
Checked and held:
The test hooks around source pin, target open, and mount make the race claims checkable. |
Human readable section
Problem: Linux mount setup can use a changed path after validating a filesystem grant. A nested target can then reach an unintended location.
nested_directory_targets_do_not_create_outside_after_source_pinningandnested_file_targets_do_not_mount_outside_after_source_pinningcover the reported directory and file cases.Solution: Retain open references while preparing and attaching bind mounts. Refuse missing targets inside exposed host trees. Resolve permitted aliases inside the staged filesystem and check mount placement. Preserve enclosing-mount and canonical-source ordering.
Trade-off: Setup refuses when a nested target disappears.
missing_nested_targets_are_not_recreatedpins this refusal. Legitimate nested grants remain usable, as checked bylegitimate_nested_directory_and_file_grants_remain_usable.Description
The repair extends the existing mount-target mechanism behind the current filesystem grant contract. It changes no public interface or policy authority.
Alias resolution must preserve normal filesystem behavior.
directory_alias_resolves_a_link_before_its_parent_componentchecks link resolution before a parent component.absolute_file_alias_remains_writable_inside_the_viewchecks absolute aliases within the staged root.an_authored_alias_replaced_with_a_loop_refuses_startupchecks bounded traversal.An alias reuses its canonical mount when the object identity matches and the planned access flags cover the alias.
a_read_only_directory_alias_preserves_the_canonical_write_grantchecks permission preservation.an_alias_bind_does_not_replace_nested_metadata_mountschecks mount ordering.Codex assisted with implementation, tests, documentation, and review.
Related Issues
Related: #159, #160.
Type of Change
Bug fix
Testing
Review status: Changes are required before human review. The hosted reviewer identified a protected-file alias startup regression and a permission-remount race. Follow-up fixes are saved but not pushed. All 176 focused mount-view tests, formatting checks, and prescribed workspace Clippy passed. Development-desktop migration interrupted the full workspace test run; that run is neither a pass nor a test assertion failure. Remaining validation and a fresh independent review are pending until normal remote workspace access returns. The published head below has passed hosted checks, but both review threads remain unresolved.
Native Amazon Linux 2023, AArch64. Validation matches commit
25dc576096e2e9a55237f09d1e6df5b3a56e5788, whose parent isd74f5dd. The patch also applies cleanly to observed maine4ca8b8.just check, orcargo test --workspace --all-features)cargo fmt --alland the workspace Clippy command prescribed byjust clippy, including its documented allowances.CONTRIBUTING requires the lint allowances in
justfile. That exact recipe passed locally and in the hosted lint job. A separate run without allowances failed on six existing vendored Shell diagnostics, as CONTRIBUTING documents.Validation details:
nested_grantslibrary tests: 30 passed across 39 fixture cases; zero ignored and no explicit skips in uncaptured output.EXPERIMENTAL_LINUX_SUPPORT=TRUE cargo test --workspace --all-features: exit 0; 17 ignored. The containment library reports 421 passed.cargo fmt --alland its check passed. Workspace Clippy passed with the exactjustfileallowances:too_many_arguments,only_used_in_recursion,await_holding_refcell_ref,explicit_auto_deref,chunks_exact_to_as_chunks,dead_code, andnon_snake_case.bash test-integ/run.sh: GREEN; 124 passed, zero failures or errors, and nine declared macOS-only skips. This used debug product binaries and a release test harness. Its raw commit field isunknown; the source hashes match the reviewed patch.git diff --checkpassed.Evidence limits:
Checklist
Warning comparison:
cargo check -p strands-box-containment --no-default-features --message-format=jsonexited zero on upstreame4ca8b8and PR head25dc576. All 66 warning diagnostics matched by code, message, and source file. No test or workload ran in that compiler-only comparison. The all-feature workspace lint gate also passed with the prescribed allowances.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.