files: follow in-root symlinks via a kernel-faithful confined resolver#153
Merged
Conversation
upload/download walked every path component with O_NOFOLLOW and refused ANY symlink — far stronger than the jail's real invariant (the resolved target stays under UPLOAD_ROOT) and broken for legitimate filesystems: merged-usr images make /bin, /sbin, /lib symlinks to usr/*, and real repos contain symlinked sources. The resolver now follows symlinks the way openat2(RESOLVE_IN_ROOT) does, so escape is structurally impossible rather than lexically checked: - the root dir is opened ONCE with O_NOFOLLOW and retained for the whole walk — never reopened by path — so a concurrent rename/replace of the root cannot redirect a later step outside the jail (a TOCTOU an earlier reopen-per-restart draft was vulnerable to); - symlink targets are pushed onto the walk queue as raw components, never lexically normalized: `..` is applied by popping the open-dir stack (clamped at the root) against actually-resolved directories, so a target like `inner/../victim.txt` where `inner -> dest/sub` resolves to dest/victim.txt exactly as the kernel would, not to a lexically-wrong root file; - absolute symlink targets re-anchor at the root (chroot semantics), so a link to /etc/x reads root/etc/x and can never touch the host; - a 40-hop cap (the kernel's own limit) turns cycles into PermissionError. macOS reports ENOTDIR instead of ELOOP for the O_DIRECTORY case, so symlink detection arbitrates via readlink. Tests: monkeypatch files.UPLOAD_ROOT (reloading the module breaks UploadResult identity for the safepickle allowlist); coverage includes merged-usr relative links, nested link-to-link chains, `..`-bearing targets, absolute-target re-anchoring, a link resolving exactly to root, and cycle refusal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
90db1df to
f629b98
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
agentix.filesupload/downloadwalked every path component withO_NOFOLLOWand refused any symlink. That is far stronger than the jail's actual invariant (the resolved target stays under$AGENTIX_UPLOAD_ROOT) and breaks legitimate filesystems: merged-usr images make/bin,/sbin,/libsymlinks tousr/*, and real repos contain symlinked sources. With the root widened to/there is nothing to escape, yet the refusal still fired.What
The resolver now follows symlinks the way
openat2(RESOLVE_IN_ROOT)does — escape is structurally impossible, not lexically checked:O_NOFOLLOWand retained for the whole walk (never reopened by path), so a concurrent rename/replace of the root cannot redirect a later step outside the jail — a TOCTOU that an earlier reopen-per-restart draft was vulnerable to;..is applied by popping the open-dir stack (clamped at the root) against actually-resolved directories, so a target likeinner/../victim.txtwhereinner -> dest/subresolves todest/victim.txtexactly as the kernel would, not to a lexically-wrong root file;/etc/xreads<root>/etc/xand can never touch the host;PermissionError.macOS reports
ENOTDIRinstead ofELOOPfor theO_DIRECTORYcase, so symlink detection arbitrates viareadlink.Tests
merged-usr relative links, nested link-to-link chains,
..-bearing targets (contained + escaping), absolute-target re-anchoring, a link resolving exactly to the root, and cycle refusal. Tests monkeypatchfiles.UPLOAD_ROOTrather than reloading the module — reload re-createsUploadResultand breaks class identity for the safepickle allowlist.Reviewed by a second co-author (Codex, security lens) + adversarial pass. The HIGH findings — root-fd TOCTOU on restart and lexical-
..resolving the wrong file — drove the rewrite from the initial reopen-per-restart version to this retained-fd-stack resolver.🤖 Generated with Claude Code