Repository navigation
fix: preserve caller access checks and pin chroot targets - #274
jamesboyzj-design wants to merge 8 commits into
Conversation
congwang-mk
left a comment
There was a problem hiding this comment.
The chroot open and access fixes look right. The COW access rewrite regresses symlink lookups; see inline.
| }; | ||
| let open_flags = libc::O_PATH | libc::O_CLOEXEC | ||
| | if flags & libc::AT_SYMLINK_NOFOLLOW != 0 { libc::O_NOFOLLOW } else { 0 }; | ||
| match open_confined(cow.upper_dir(), cow.workdir(), &real, open_flags, 0) { |
There was a problem hiding this comment.
RESOLVE_IN_ROOT breaks symlinks that leave the workdir: .venv/bin/python -> /usr/bin/python3 now resolves to <workdir>/usr/bin/python3 and access(X_OK) returns ENOENT, so PATH lookup fails. Unmodified read opens still pass through (needs_read_intercept), so cat works but access does not. Please confine only paths the COW layer changed, and add a test for this.
| Err(a) => return a, | ||
| }; | ||
| let ret = unsafe { | ||
| libc::faccessat(libc::AT_FDCWD, path.as_ptr(), kernel_mode, flags as i32) |
There was a problem hiding this comment.
This follows host symlinks on real_path. If the child swaps the last component after resolution, it learns the permission bits of any host path. Pin with an O_PATH open first and query with AT_EMPTY_PATH, as the COW handler does.
| } | ||
| }; | ||
| // Query the pinned object, avoiding a second pathname resolution after | ||
| // confinement. The supervisor shares the sandbox's filesystem identity. |
There was a problem hiding this comment.
Not true with the uid option: the child is root in its user namespace, so for files owned by the mapped user its access results differ from the supervisor's. This applies to the chroot path too.
Review follow-up: validation report and remaining blockersThe current revision is not ready to be presented as a completed fix. This report records the local follow-up to the three review comments; it does not request that the threads be resolved. Versions: main Correction to the identity validationMy earlier local identity matrices used an ordinary supervised sandbox as the supposed native reference. That was incorrect: the generic The corrected references use Findings against the three comments
Matched validationEnvironment: Linux aarch64, kernel
All 22 workspace test binaries completed per version. Both runs also have the same HOME-dependent learner failure under the inherited root HOME; that exact case passes in separate root controls. The profile HOME case was explicitly excluded from the unprivileged run and passed separately as root. No review regression was ignored. The historical BrokenPipe unit failure did not recur. Doc tests executed zero examples, with 8 ignored. Build entrypoint: The three remaining local integration failures are external lower symlinks and the corrected COW/chroot identity matrices. The original open side-effect protections and the local confined-object/visible-target regressions pass. Native x86_64 legacy syscalls, the release CI matrix and checkpoint restore remain unverified for this revision; old-head CI is not evidence for the local changes. Remaining implementation boundaryA candidate solution is an isolated kernel permission executor matching the notification task's current IDs, namespace, groups and capability sets, with directory-search checks and retained, policy-bounded COW/virtual-mount targets. Entering the child's namespace alone is insufficient and can grant capabilities it has discarded. I have not substituted mode-bit emulation, a setfsuid-only workaround, supervisor credential mutation or blanket uid-mode denial. The generic metadata access path shares the credential issue. Related chroot stat/statx/readlink paths retain separate resolution/reopen concerns and are not claimed fixed here. No functionality from #273 or #275 was imported. #273 shares |
|
Thanks for the thorough follow-up. To keep this PR bounded:
|
Recheck virtual policy on the retained object and query it through faccessat2 AT_EMPTY_PATH. This local partial revision does not resolve COW external-link compatibility or prove caller credential equivalence.
|
Implemented the three requested scope changes:
Local validation: #274 has 537/537 core integration; #277 has 887/887 core unit and 525/525 core integration. #274's full core unit run was 880/881 due to the existing control BrokenPipe race; an exact matched 10-run check produced 9 passes and 1 identical BrokenPipe on each of reviewed head and revision. Both workspace runs completed, with the already documented HOME-environment learner case passing in explicit root controls. The PR bodies record these limits rather than claiming fully green local workspaces. Current-head CI is running for both PRs. Review threads remain open for your assessment; #278 is deferred work, not silently counted as resolved. |
|
Validation update: please keep this PR under review. The new replacement regression fails on the native ARM CI runner, although the local aarch64 integration suite (537 tests) and native x86_64 CI pass. The independent open fix #277 has all 17 checks passing and is unaffected. An isolated instrumented branch showed that some The credential mismatch remains separately tracked in #278, as requested; it is not being used to explain away this object-selection failure. |
|
The three requested changes are now published for review:
Current #274 head is The ARM investigation has an important boundary: a standalone C program without Sandlock also returned the parent directory during final-link replacement (2 wrong-parent results in 1,000,000 raw Expanded ARM confinement result: 80,000 queries passed, and the complete diagnostic release workspace run passed, on isolated branch The PR body contains the complete validation history and boundaries. I have left review threads unresolved and have not closed any issue; this is ready for your review of the requested bounded scope. |
congwang-mk
left a comment
There was a problem hiding this comment.
Thanks, the scope matches what I asked for. Two things before this can land:
-
Chroot+COW waives W_OK for unchanged files (
chroot/dispatch.rs:1863).copied_on_writecomes fromcow.matches(), which is true for every workdir path, not only copied-up ones. Soaccess(ro_lower_file, W_OK)returns 0 under chroot+COW but EACCES under plain COW. Please check for an actual copy-up (as plain COW does) so both modes agree. -
needs_read_interceptchange affectsopentoo (cow/seccomp.rs:981). Switching fromexists()toupper_has()changeshandle_cow_open's fast path: dangling upper symlinks are now intercepted, upper symlinks pointing outside the upper dir no longer are. Is that intended? If yes, please call it out in the commit message and add anopentest; otherwise move it out of this PR.
Follow-ups, not blocking: rename the test_review274_* tests after the behavior they check, and once #277 lands, rebase onto main and squash the superseded commits.
Access queries on unchanged COW paths were being answered by the supervisor against a workdir-confined target. That broke lower symlinks to external executables and used the wrong caller credentials. The revision follows the scope requested in #274 (comment).
The open creation/truncation side-effect fix
70e258bhas been extracted into #277, independently based on latest maina0eb434. This PR retains the original shared commit without rewriting history; once #277 lands, its change will disappear from this PR's effective diff.Caller-credential correctness in mediated metadata, chroot and modified COW paths is deferred to #278, with verified command-line reproductions for both false denials and false allows, including capability drops. The failing chroot credential acceptance cases from the earlier local experiment were not included in this bounded revision and are not claimed fixed. The prior local checkpoint remains preserved. The retained unchanged-COW identity tests use genuine kernel-only references and pass as both ordinary user and root.
Validation history and current revision
Local full-workspace results below are for
5749f7e; the finald3e647bintegration suite and current-head CI are reported separately below.Linux aarch64, Rust 1.96.1, kernel
7.0.14-orbstack-00380-ga7e0a2dc9535, Landlock ABI 8; Docker--init --security-opt seccomp=unconfined, without privileged mode. Serial unprivileged test execution uses uid/gid 65534 and cleared supplementary groups, native Linux/tmpoutput; C smoke and explicit HOME/root controls run as root.control::tests::server_closes_on_another_uid_without_answeringhit BrokenPipe. A bounded matched check of that exact test on reviewed49197ebcand this revision produced 9 passes / 1 BrokenPipe failure out of 10 on each. No control test or implementation was modified.git diff --checkpassed. Native x86_64 legacy open/access and release workspace coverage are delegated to CI for this actual pushed head, not to old-head results. Local aarch64 legacy fixtures use raw *at fallback. Checkpoint restore is unverified.Build:
cargo test --offline --locked --workspace --no-run --message-format=json; run the Cargo-reported executables with--test-threads=1, generally throughsetpriv --reuid=65534 --regid=65534 --clear-groups. Identity subset:access_matches_in the core integration executable. The previous broad identity matrices are documented in #278; no ignored credential test is counted as a pass here.The kernel re-reads mutable paths after Continue; this is not a snapshot guarantee or a resolution of #27. No credential-matching executor, mode-bit approximation, setfsuid-only workaround, supervisor credential mutation, or blanket uid-mode denial is introduced. #273/#275 functionality and future write-only semantics are not imported. This PR does not close #27, #143 or #150.
ARM replacement investigation
The initial stress fixture made only the target file unreadable, so any readable-parent answer also failed its assertion. On Linux 6.17.0-1022-azure aarch64, a standalone C program with no Sandlock, seccomp or supervisor reproduced the same wrong-parent selection: 1,000,000 raw confined openat2 calls produced 334,075 files, 2 directories and 665,923 errors. Local Linux 7.0.14 produced 337,119 files, 0 directories and 662,881 errors. This establishes a native lookup limitation on that runner; it is not a credential result or a claim that this PR fixes kernel object selection.
Reproduction: standalone source, ARM run. Compile with
cc -O2 -pthread diagnostics/openat2-race.c -o /tmp/openat2-raceand run it as an ordinary user. It uses synthetic temporary files only.f16e8f4pins the original virtual request rather than reopening the preliminary resolved name, but the expanded ARM run still reproduced native wrong-parent selection; that commit alone is not represented as resolving the failure.d3e647bmakes the fixture's in-root parents unreadable while retaining search/write permissions, so the bounded stress assertion specifically detects access to the readable outside sentinel. The deterministic dangling-host-link regression remains unchanged. The test does not establish exact object selection beyond the kernel's guarantees. Diagnostic instrumentation, 80,000-query expansion and the standalone reproducer are confined to the separate diagnostic branch.Final
d3e647b: all 17 official CI checks passed (run); native ARM core unit/integration counts are 887/537, x86 counts are 900/538, all with zero failures. Final local core integration: 537 passed. The isolated expanded ARM run (843a335) also passed the full release workspace, including 80,000 confinement queries (run). The expansion uses a unique fixture per iteration; its earlier directory-reuse setup failure is not counted as a pass.