Repository navigation
hardening(file_write): refuse a symlink target - #91
SaulBuilds wants to merge 3 commits into
Conversation
…nked targets (red) Drives the real FileWrite/FileEdit entry points against a private dgx-fw- temp tree: final and intermediate symlinks (outside file, outside dir, dangling, inside the workspace), hard links, and bounded swap races on the final and an intermediate component. 11 of 16 fail on an assertion against main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4
file_write and file_edit now open through tools/safe_write.rs: the workspace root is canonicalized once, then every component below it is opened with openat + O_NOFOLLOW relative to the previous directory descriptor (missing directories via mkdirat), so no component may be a symbolic link and nothing can be swapped between a check and the open. The target must be a regular file with one hard link; on Linux the opened descriptor's /proc path is re-checked against the workspace before any byte is written (fails closed without /proc). Non-Unix platforms refuse every write. file_edit reads and rewrites through the same descriptor. Removes the old canonicalize-the-ancestor check and the post create_dir_all re-check, which the descriptor walk replaces. rustix (already in Cargo.lock) gains a direct unix-only dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4
scripts/check_tool_write_paths.py flags fs::write / File::create / OpenOptions / create_dir_all / fs::copy / fs::rename / symlink / hard_link in agent-code/src/tools outside safe_write.rs unless the line (or the one above) carries '// write-path-ok: <reason>'. --self-test holds the negative controls; against main it flags the old file_write and file_edit write sites. The two tool-owned scratch-HOME dirs in git_operations and shell_exec are marked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4
pysol-dev
left a comment
There was a problem hiding this comment.
Verdict: approve at d49f9f740297ddb66cccbaf2fccd4c1907f3b0d8. file_write and file_edit now open every component relative to a descriptor for the canonical workspace root with O_NOFOLLOW. The leaf is created with O_EXCL, then fstat-checked (regular file, nlink == 1) and checked against /proc/self/fd on Linux. A symlinked target, a hard-linked target, or a swapped component is refused. The checks fail closed.
What I measured (local, rustc 1.96.0, CARGO_BUILD_JOBS=2)
cargo test -p citrate-agent-code --locked: base ce34d3e has 55 passed. Head has 71 passed / 0 failed (+16, matching the body).- Red first: at 5b76c95 (red), 60 passed and 11 failed, all on assertions. The fix commit turns them green.
cargo test --workspace --locked --no-fail-fastat head: 2327 passed / 0 failed / 11 ignored. Only agent-code tests were added, so the count only goes up.scripts/check_tool_write_paths.py --self-testand the full scan are clean.check_hic_terminology.py(self-test and scan) is clean.- fmt and clippy are advisory in ci.yml (
continue-on-error), and the workspace has fmt drift at main. This PR adds 2 fmt diffs infile_edit.rs: theuse super::safe_writeorder, and the line-wrapped tuple at :76.cargo clippy -p citrate-agent-code --no-deps --all-targets -- -D warningsreports 2 errors, both in lines this PR did not touch (shell_exec.rs:188 and git_operations.rs:53, both from 1ad3fe0 or e76a067). - No new
.unwrap(),expectorunsafein non-test code. No mocks: the tests use real temp trees, symlinks, hard links and swapper threads. - Dependency:
rustix 1.1(fsfeature, unix only). It was already in the lockfile, and only the edge to agent-code was added. - CI at head: all checks green, including the new
tool-write-pathsjob.
Mutation checks (hand-applied to safe_write.rs, then restored)
| Mutant | Result |
|---|---|
drop NOFOLLOW in open_dir |
killed (2: intermediate symlink, inside and deep-outside) |
drop NOFOLLOW on the leaf |
killed (2: final symlink, write and edit) |
drop O_EXCL |
killed (ordinary_writes_still_work, because the existing file is not truncated) |
drop the nlink != 1 check |
killed (2: hard-linked target, write and edit) |
| drop the regular-file check | survives |
drop fd_inside |
survives (the descriptor chain already guarantees confinement, so this is defence in depth) |
Non-blocking
- The regular-file check has no test.
file_editopens the leafO_RDWR|O_NONBLOCK, and that open succeeds on a FIFO on Linux. A test that edits amkfifoin the workspace and asserts the "not a regular file" refusal would pin it. - The tripwire matches
std::fs/tokio::fswrite calls inagent-code/src/tools/*.rsonly. It does not matchFile::options/OpenOptions,create_dir(,remove_file/rename, or files in subdirectories. - The leaf is opened before fstat.
O_NONBLOCKcovers a FIFO, but opening a device node can still have side effects before the refusal. This is minor, because planting a device node needs privileges. is_link_refusalmapsENOTDIRto the symlink message. A regular file used as an intermediate component is reported as a symlink.- Non-Unix builds refuse every write. That is fine today, since the Windows sidecar build does not use agent-code. Worth a line in the README.
- The two
write-path-okexemptions create a scratch HOME at a predictabletemp_dir()/citrate-*-home-<pid>-<seq>withcreate_dir_all, which accepts an existing directory. The config pins mitigate this. Atempfile::TempDir(mkdtemp) would remove the question. This was there before this PR. - Commits:
hardening(file_write)is not a Conventional Commits type (fixorsecuritywould fit main's history). NoSigned-off-byon any of the 3 commits (DCO).
Merge gate (fed#331)
No [MAC]/[DGX] lane review and no Codex review at this SHA yet. The PR still needs a lane APPROVED at the SHA, a clean Codex run, and this maintainer approval.
Matr0xshka
left a comment
There was a problem hiding this comment.
[MAC independent review] APPROVED at d49f9f7
Reviewer: MAC-lane independent reviewer agent (did not author this PR). Not Codex, not DGX. Per owner decision 2026-10-23 this review stands in for the Codex and DGX gates.
Head d49f9f740297ddb66cccbaf2fccd4c1907f3b0d8, checked live over REST. Base main at merge-base ce34d3e9. Commits: red 5b76c95, fix dd6a708, tripwire d49f9f7. Reviewed in my own worktree with a private target dir, macOS arm64, cargo +1.96.0. Earlier reviews: pysol-dev APPROVED at this head. No Codex result exists for this PR.
What I ran
cargo test -p citrate-agent-code --locked: 71 passed (55 lib + 16 intests/file_write_symlink.rs), 0 failed. Matches the body (55 → 71).python3 scripts/check_tool_write_paths.py --self-testand a plain run: both clean. Run againstmain'sagent-code/src/tools, it flagsfile_edit.rs:116,file_write.rs:169,file_write.rs:191,git_operations.rs:37andshell_exec.rs:292, as the body says.- #90's
tool_entry_pins.rscopied onto this head (not committed): 6 of 6 pass, so #90 and #91 compose. rustfmt --checkonsafe_write.rsandtests/file_write_symlink.rs: clean.
Revert check (reproduced)
I copied the fix files aside (safe_write.rs, file_write.rs, file_edit.rs, tools/mod.rs, agent-code/Cargo.toml, Cargo.lock), restored their versions from the red commit 5b76c95, and ran the new suite: 5 passed, 11 failed, the same 11 the body lists (final-component links outside/inside/dangling/absolute request, hard link, intermediate link inside, both write races, the edit race, and the two file_edit refusals). Restored: 16 of 16 pass. No git stash was used.
Adversarial probes (throwaway test file, deleted afterwards)
All passed against the head:
- a relative final link (
t -> ../outside/victim.txt) for bothfile_writeandfile_edit: refused, victim unchanged; - a relative link as a parent (
a -> ../outside), witha/b/c.txt,a/victim.txtand an edit ofa/victim.txt: all refused, nothing created in the link target; - a link three levels down (
x/y/z -> outside), existing and to-be-created children: refused, nothing created; - a dangling link as a parent (
d -> ../outside/notyet): refused, the target is not created; a self-referencing link loop: refused; a/../../outside/victim.txt, an absolute path outside, and../outside/victim.txtfor edit: refused; an absolute path under the canonical root: written;.and"": refused;- a FIFO at the target:
file_writeandfile_editreturn at once (no hang) and refuse; a directory at the target: refused; - an existing longer file is truncated to the new content; an edit rewrites through the same descriptor with the right length;
- a refused hard-linked target is not truncated (checks run before
set_len); - 200 rounds of two concurrent
file_writecalls creating the same new file: both succeed and the content is intact (theO_EXCLloser falls back to the checked open).
The swap races in the suite (final component, intermediate directory, file_edit) are real races: each asserts the swapper ran, and they are red on the red commit.
Code reading
safe_write.rs: the root is canonicalized once; every component below it is opened withopenat(O_NOFOLLOW|O_DIRECTORY)relative to the parent descriptor; missing directories usemkdiratand are re-opened withO_NOFOLLOW; the leaf usesO_CREAT|O_EXCL|O_NOFOLLOW, then a plainO_NOFOLLOWopen for an existing file;fstatchecks regular file andnlink == 1on the descriptor; truncation happens only after all checks...never reaches the filesystem (lexical normalization first). Non-Unix refuses every write; agent-code has no in-tree consumer outside this crate, so nothing else losesfile_writeon Windows.- One security-relevant residual item (defence-in-depth, not a blocker) was reported privately.
Findings (none blocking)
- P2, filed #94: the tripwire's pattern misses
File::options(),fs::create_dir,remove_file/remove_dir_all,set_permissions, aliased imports, and a//inside a string literal ends the scanned code early. No file uses these forms today. - P2, filed #95: the two items the body itself defers (read tools still resolve-then-open by path; the scratch
HOMEis created withcreate_dir_alland the result ignored). - Nit: on macOS,
cargo clippy -p citrate-agent-codereportsunused import: AsFdatagent-code/src/tools/safe_write.rs:103(it is used only by the Linuxfd_inside). Linux CI does not see it. The other two clippy warnings (shell_exec.rs:188,git_operations.rs:53) predate this PR. - Note:
file_write.rs/file_edit.rskeep the pre-existing rustfmt drift (pysol-dev noted 2 diffs infile_edit.rs); fmt is advisory inci.yml.
CI at the head
All check runs succeeded: ci, rust, audit, secret-scan, HIC terminology guard, the new "Tool-layer write paths" job, Windows (agent-shell tests + agent-sidecar release build), CodeQL (actions, python, rust). The PR is behind main (#86 merged since) and merges cleanly.
Standard: met
Red commit fails on assertions (11 of 16), the revert check goes red and restores green, adversarial and race paths are covered, the tripwire guards the class, and the replaced code (ancestor loop, post-create_dir_all re-check, resolve_existing_path) is removed.
verification cleared: yes (security hardening; symlink, hard-link, dangling, parent-link and swap-race cases refuse and leave outside files unchanged).
BerryManifold
left a comment
There was a problem hiding this comment.
[MAC independent review] APPROVED at d49f9f7
Reviewer: MAC-lane independent reviewer agent (did not author this PR). Not Codex, not DGX. Per owner decision 2026-10-23 this review stands in for the Codex and DGX gates.
Head d49f9f740297ddb66cccbaf2fccd4c1907f3b0d8, checked live over REST. Base main at merge-base ce34d3e9. Commits: red 5b76c95, fix dd6a708, tripwire d49f9f7. Reviewed in my own worktree with a private target dir, macOS arm64, cargo +1.96.0. Earlier reviews: pysol-dev APPROVED at this head. No Codex result exists for this PR.
What I ran
cargo test -p citrate-agent-code --locked: 71 passed (55 lib + 16 intests/file_write_symlink.rs), 0 failed. Matches the body (55 → 71).python3 scripts/check_tool_write_paths.py --self-testand a plain run: both clean. Run againstmain'sagent-code/src/tools, it flagsfile_edit.rs:116,file_write.rs:169,file_write.rs:191,git_operations.rs:37andshell_exec.rs:292, as the body says.- #90's
tool_entry_pins.rscopied onto this head (not committed): 6 of 6 pass, so #90 and #91 compose. rustfmt --checkonsafe_write.rsandtests/file_write_symlink.rs: clean.
Revert check (reproduced)
I copied the fix files aside (safe_write.rs, file_write.rs, file_edit.rs, tools/mod.rs, agent-code/Cargo.toml, Cargo.lock), restored their versions from the red commit 5b76c95, and ran the new suite: 5 passed, 11 failed, the same 11 the body lists (final-component links outside/inside/dangling/absolute request, hard link, intermediate link inside, both write races, the edit race, and the two file_edit refusals). Restored: 16 of 16 pass. No git stash was used.
Adversarial probes (throwaway test file, deleted afterwards)
All passed against the head:
- a relative final link (
t -> ../outside/victim.txt) for bothfile_writeandfile_edit: refused, victim unchanged; - a relative link as a parent (
a -> ../outside), witha/b/c.txt,a/victim.txtand an edit ofa/victim.txt: all refused, nothing created in the link target; - a link three levels down (
x/y/z -> outside), existing and to-be-created children: refused, nothing created; - a dangling link as a parent (
d -> ../outside/notyet): refused, the target is not created; a self-referencing link loop: refused; a/../../outside/victim.txt, an absolute path outside, and../outside/victim.txtfor edit: refused; an absolute path under the canonical root: written;.and"": refused;- a FIFO at the target:
file_writeandfile_editreturn at once (no hang) and refuse; a directory at the target: refused; - an existing longer file is truncated to the new content; an edit rewrites through the same descriptor with the right length;
- a refused hard-linked target is not truncated (checks run before
set_len); - 200 rounds of two concurrent
file_writecalls creating the same new file: both succeed and the content is intact (theO_EXCLloser falls back to the checked open).
The swap races in the suite (final component, intermediate directory, file_edit) are real races: each asserts the swapper ran, and they are red on the red commit.
Code reading
safe_write.rs: the root is canonicalized once; every component below it is opened withopenat(O_NOFOLLOW|O_DIRECTORY)relative to the parent descriptor; missing directories usemkdiratand are re-opened withO_NOFOLLOW; the leaf usesO_CREAT|O_EXCL|O_NOFOLLOW, then a plainO_NOFOLLOWopen for an existing file;fstatchecks regular file andnlink == 1on the descriptor; truncation happens only after all checks...never reaches the filesystem (lexical normalization first). Non-Unix refuses every write; agent-code has no in-tree consumer outside this crate, so nothing else losesfile_writeon Windows.- One security-relevant residual item (defence-in-depth, not a blocker) was reported privately.
Findings (none blocking)
- P2, filed #94: the tripwire's pattern misses
File::options(),fs::create_dir,remove_file/remove_dir_all,set_permissions, aliased imports, and a//inside a string literal ends the scanned code early. No file uses these forms today. - P2, filed #95: the two items the body itself defers (read tools still resolve-then-open by path; the scratch
HOMEis created withcreate_dir_alland the result ignored). - Nit: on macOS,
cargo clippy -p citrate-agent-codereportsunused import: AsFdatagent-code/src/tools/safe_write.rs:103(it is used only by the Linuxfd_inside). Linux CI does not see it. The other two clippy warnings (shell_exec.rs:188,git_operations.rs:53) predate this PR. - Note:
file_write.rs/file_edit.rskeep the pre-existing rustfmt drift (pysol-dev noted 2 diffs infile_edit.rs); fmt is advisory inci.yml.
CI at the head
All check runs succeeded: ci, rust, audit, secret-scan, HIC terminology guard, the new "Tool-layer write paths" job, Windows (agent-shell tests + agent-sidecar release build), CodeQL (actions, python, rust). The PR is behind main (#86 merged since) and merges cleanly.
Standard: met
Red commit fails on assertions (11 of 16), the revert check goes red and restores green, adversarial and race paths are covered, the tripwire guards the class, and the replaced code (ancestor loop, post-create_dir_all re-check, resolve_existing_path) is removed.
verification cleared: yes (security hardening; symlink, hard-link, dangling, parent-link and swap-race cases refuse and leave outside files unchanged).
Summary
file_writeandfile_edit(agent-code) now open their target through a newagent-code/src/tools/safe_write.rs, so a write can no longer follow a symbolic link or a second hard link out of the workspace./tmpon macOS).openat+O_NOFOLLOW, relative to the descriptor of the directory before it. Missing directories are created withmkdirat, nevercreate_dir_all. Because no step re-resolves a path, nothing can be swapped between a check and the open.O_CREAT|O_EXCL, and a file that already exists is truncated only after every check has passed./proc/self/fdpath is re-checked against the workspace before any byte is written. A file this call created is removed again if that check fails. The check fails closed if/procis missing.O_NOFOLLOW.file_editreads and rewrites through the same descriptor.Policy decisions (pinned by tests):
Dead code removed: the canonicalize-the-ancestor loop and the re-check after
create_dir_allinfile_write, andresolve_existing_pathinfile_edit. The descriptor walk replaces all three.rustix(already inCargo.lock) becomes a direct dependency of agent-code, unix-only, with thefsfeature. The lockfile diff is one line.Standard: met
5b76c95addsagent-code/tests/file_write_symlink.rs, which drives the realFileWrite/FileEditexecuteentry points. 11 of its 16 tests fail on an assertion againstmain(ce34d3e). The other five (outside-dir symlink, deep intermediate symlink to outside, ordinary writes, symlinked workspace root, plain edit) already pass on main and stay as guards.dd6a708.git revert --no-commit dd6a708, the same 11 tests fail again (5 passed, 11 failed). After restoring, all 16 pass.file_edit. Each race runs at most 3,000 writes and asserts that the swapper actually ran. All three races are red on main and green with the fix (I re-ran them 3 times).tempfiledirs with thedgx-fw-prefix. The protected "outside" file is a sibling inside the same temp tree. There are no host paths and no glob deletes.d49f9f7addsscripts/check_tool_write_paths.pyand atool-write-pathsCI job. The script flags path-based writes (fs::write,File::create,OpenOptions,create_dir_all,fs::copy,fs::rename,symlink,hard_link) inagent-code/src/toolsoutsidesafe_write.rs, unless the line carries// write-path-ok: <reason>.--self-testholds the negative controls. Run againstmain, it flags the oldfile_write.rs:169,191andfile_edit.rs:116.cargo +1.96.0,RUST_TEST_THREADS=2):cargo test -p citrate-agent-code --locked: 55 lib + 16 new = 71 passed (was 55).cargo clippy -p citrate-agent-code --all-targets --locked: no new warnings in agent-code. The two existing ones are inshell_exec.rs:188andgit_operations.rs:53.-D warningsstops earlier on 5 errors already incitrate-agent-legacy, the same as on main.rustfmt --checkpasses on the new files.file_write.rsandfile_edit.rswere not rustfmt-clean on main and were not reformatted.check_tool_write_paths.py --self-testand a plain run both pass, and so doescheck_hic_terminology.py.Other write paths found (tripwire sweep)
agent-code/src/tools/git_operations.rs:37andshell_exec.rs:292create a tool-owned scratchHOMEunder the system temp dir withcreate_dir_all, using a name built from the pid and a counter. The model does not choose this path. It is markedwrite-path-okhere and left for a separate follow-up, which would create it withcreate_dirmode 0700 and fail if it already exists.agent-code/src/tools/file_read.rsandsearch_code.rsonly read, and they still resolve bycanonicalizeand then open by path. That is the same pattern on the read side and is not changed here.agent-sidecar/src/files.rs(the sidecarfile_write/sheet_write) already opens withO_NOFOLLOW/O_EXCLand is unchanged.agent-legacy/src/adapters/sandbox.rs:SandboxPolicy::check_writehas no production caller that writes. It is used by tests only.git_opswrites throughgititself and has no direct file writes.This PR does not touch any file in #86, #87, #89 or #90. It is based on
main.🤖 Generated with Claude Code
https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4