Skip to content

hardening(file_write): refuse a symlink target - #91

Open
SaulBuilds wants to merge 3 commits into
mainfrom
hardening/file-write-nofollow
Open

SaulBuilds wants to merge 3 commits into
mainfrom
hardening/file-write-nofollow

Conversation

@SaulBuilds

Copy link
Copy Markdown
Contributor

Summary

file_write and file_edit (agent-code) now open their target through a new agent-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.

  • The workspace root is canonicalized once (it can still be reached through a symlinked path, e.g. /tmp on macOS).
  • Every component below the root is opened with openat + O_NOFOLLOW, relative to the descriptor of the directory before it. Missing directories are created with mkdirat, never create_dir_all. Because no step re-resolves a path, nothing can be swapped between a check and the open.
  • The target must be a regular file with exactly one hard link. A file this call creates uses O_CREAT|O_EXCL, and a file that already exists is truncated only after every check has passed.
  • On Linux the opened descriptor's /proc/self/fd path 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 /proc is missing.
  • Non-Unix builds refuse every write, because they have no O_NOFOLLOW.
  • file_edit reads and rewrites through the same descriptor.

Policy decisions (pinned by tests):

  • A symlink in the final or any intermediate component is refused, including one that points elsewhere inside the workspace. The tool writes only to the name it was given.
  • A hard-linked file (link count > 1) is refused, since another name for it may be outside the workspace.

Dead code removed: the canonicalize-the-ancestor loop and the re-check after create_dir_all in file_write, and resolve_existing_path in file_edit. The descriptor walk replaces all three.

rustix (already in Cargo.lock) becomes a direct dependency of agent-code, unix-only, with the fs feature. The lockfile diff is one line.

Standard: met

  • Red first: 5b76c95 adds agent-code/tests/file_write_symlink.rs, which drives the real FileWrite / FileEdit execute entry points. 11 of its 16 tests fail on an assertion against main (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.
  • Fix: dd6a708.
  • Revert check: with git revert --no-commit dd6a708, the same 11 tests fail again (5 passed, 11 failed). After restoring, all 16 pass.
  • Adversarial cases: a symlink to an outside file (by relative path and by absolute path), a symlink to an outside directory (as the target and as a parent), a dangling symlink (its target is not created), a symlink inside the workspace (final and intermediate, both refused), a hard link to an outside file (refused, outside file unchanged), and a deep intermediate symlink. Three swap races use atomic renames: the final component, an intermediate directory, and 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).
  • Test hygiene: tests write only under tempfile dirs with the dgx-fw- prefix. The protected "outside" file is a sibling inside the same temp tree. There are no host paths and no glob deletes.
  • Tripwire: d49f9f7 adds scripts/check_tool_write_paths.py and a tool-write-paths CI job. The script flags path-based writes (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 carries // write-path-ok: <reason>. --self-test holds the negative controls. Run against main, it flags the old file_write.rs:169,191 and file_edit.rs:116.
  • Commands (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 in shell_exec.rs:188 and git_operations.rs:53. -D warnings stops earlier on 5 errors already in citrate-agent-legacy, the same as on main.
    • rustfmt --check passes on the new files. file_write.rs and file_edit.rs were not rustfmt-clean on main and were not reformatted.
    • check_tool_write_paths.py --self-test and a plain run both pass, and so does check_hic_terminology.py.

Other write paths found (tripwire sweep)

  • agent-code/src/tools/git_operations.rs:37 and shell_exec.rs:292 create a tool-owned scratch HOME under the system temp dir with create_dir_all, using a name built from the pid and a counter. The model does not choose this path. It is marked write-path-ok here and left for a separate follow-up, which would create it with create_dir mode 0700 and fail if it already exists.
  • agent-code/src/tools/file_read.rs and search_code.rs only read, and they still resolve by canonicalize and then open by path. That is the same pattern on the read side and is not changed here.
  • agent-sidecar/src/files.rs (the sidecar file_write / sheet_write) already opens with O_NOFOLLOW / O_EXCL and is unchanged.
  • agent-legacy/src/adapters/sandbox.rs: SandboxPolicy::check_write has no production caller that writes. It is used by tests only.
  • git_ops writes through git itself 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

SaulBuilds and others added 3 commits October 22, 2026 18:50
…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
@SaulBuilds
SaulBuilds requested a review from a team as a code owner October 23, 2026 01:52

@pysol-dev pysol-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-fast at 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-test and 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 in file_edit.rs: the use super::safe_write order, and the line-wrapped tuple at :76. cargo clippy -p citrate-agent-code --no-deps --all-targets -- -D warnings reports 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(), expect or unsafe in non-test code. No mocks: the tests use real temp trees, symlinks, hard links and swapper threads.
  • Dependency: rustix 1.1 (fs feature, 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-paths job.

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

  1. The regular-file check has no test. file_edit opens the leaf O_RDWR|O_NONBLOCK, and that open succeeds on a FIFO on Linux. A test that edits a mkfifo in the workspace and asserts the "not a regular file" refusal would pin it.
  2. The tripwire matches std::fs/tokio::fs write calls in agent-code/src/tools/*.rs only. It does not match File::options/OpenOptions, create_dir(, remove_file/rename, or files in subdirectories.
  3. The leaf is opened before fstat. O_NONBLOCK covers 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.
  4. is_link_refusal maps ENOTDIR to the symlink message. A regular file used as an intermediate component is reported as a symlink.
  5. 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.
  6. The two write-path-ok exemptions create a scratch HOME at a predictable temp_dir()/citrate-*-home-<pid>-<seq> with create_dir_all, which accepts an existing directory. The config pins mitigate this. A tempfile::TempDir (mkdtemp) would remove the question. This was there before this PR.
  7. Commits: hardening(file_write) is not a Conventional Commits type (fix or security would fit main's history). No Signed-off-by on 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 Matr0xshka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 in tests/file_write_symlink.rs), 0 failed. Matches the body (55 → 71).
  • python3 scripts/check_tool_write_paths.py --self-test and a plain run: both clean. Run against main's agent-code/src/tools, it flags file_edit.rs:116, file_write.rs:169, file_write.rs:191, git_operations.rs:37 and shell_exec.rs:292, as the body says.
  • #90's tool_entry_pins.rs copied onto this head (not committed): 6 of 6 pass, so #90 and #91 compose.
  • rustfmt --check on safe_write.rs and tests/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 both file_write and file_edit: refused, victim unchanged;
  • a relative link as a parent (a -> ../outside), with a/b/c.txt, a/victim.txt and an edit of a/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.txt for edit: refused; an absolute path under the canonical root: written; . and "": refused;
  • a FIFO at the target: file_write and file_edit return 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_write calls creating the same new file: both succeed and the content is intact (the O_EXCL loser 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 with openat(O_NOFOLLOW|O_DIRECTORY) relative to the parent descriptor; missing directories use mkdirat and are re-opened with O_NOFOLLOW; the leaf uses O_CREAT|O_EXCL|O_NOFOLLOW, then a plain O_NOFOLLOW open for an existing file; fstat checks regular file and nlink == 1 on 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 loses file_write on Windows.
  • One security-relevant residual item (defence-in-depth, not a blocker) was reported privately.

Findings (none blocking)

  1. 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.
  2. P2, filed #95: the two items the body itself defers (read tools still resolve-then-open by path; the scratch HOME is created with create_dir_all and the result ignored).
  3. Nit: on macOS, cargo clippy -p citrate-agent-code reports unused import: AsFd at agent-code/src/tools/safe_write.rs:103 (it is used only by the Linux fd_inside). Linux CI does not see it. The other two clippy warnings (shell_exec.rs:188, git_operations.rs:53) predate this PR.
  4. Note: file_write.rs/file_edit.rs keep the pre-existing rustfmt drift (pysol-dev noted 2 diffs in file_edit.rs); fmt is advisory in ci.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 BerryManifold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 in tests/file_write_symlink.rs), 0 failed. Matches the body (55 → 71).
  • python3 scripts/check_tool_write_paths.py --self-test and a plain run: both clean. Run against main's agent-code/src/tools, it flags file_edit.rs:116, file_write.rs:169, file_write.rs:191, git_operations.rs:37 and shell_exec.rs:292, as the body says.
  • #90's tool_entry_pins.rs copied onto this head (not committed): 6 of 6 pass, so #90 and #91 compose.
  • rustfmt --check on safe_write.rs and tests/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 both file_write and file_edit: refused, victim unchanged;
  • a relative link as a parent (a -> ../outside), with a/b/c.txt, a/victim.txt and an edit of a/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.txt for edit: refused; an absolute path under the canonical root: written; . and "": refused;
  • a FIFO at the target: file_write and file_edit return 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_write calls creating the same new file: both succeed and the content is intact (the O_EXCL loser 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 with openat(O_NOFOLLOW|O_DIRECTORY) relative to the parent descriptor; missing directories use mkdirat and are re-opened with O_NOFOLLOW; the leaf uses O_CREAT|O_EXCL|O_NOFOLLOW, then a plain O_NOFOLLOW open for an existing file; fstat checks regular file and nlink == 1 on 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 loses file_write on Windows.
  • One security-relevant residual item (defence-in-depth, not a blocker) was reported privately.

Findings (none blocking)

  1. 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.
  2. P2, filed #95: the two items the body itself defers (read tools still resolve-then-open by path; the scratch HOME is created with create_dir_all and the result ignored).
  3. Nit: on macOS, cargo clippy -p citrate-agent-code reports unused import: AsFd at agent-code/src/tools/safe_write.rs:103 (it is used only by the Linux fd_inside). Linux CI does not see it. The other two clippy warnings (shell_exec.rs:188, git_operations.rs:53) predate this PR.
  4. Note: file_write.rs/file_edit.rs keep the pre-existing rustfmt drift (pysol-dev noted 2 diffs in file_edit.rs); fmt is advisory in ci.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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants