Use O_NOFOLLOW on file writes: flusher, credential, settings#132
Merged
Conversation
`os.OpenFile` with `O_CREATE|O_WRONLY|O_TRUNC` and `os.WriteFile`
silently follow a pre-existing symlink at the destination path. This
let three write sites be redirected to attacker-chosen targets:
- Flusher (internal/infra/review/flusher.go): a symlink at the flush
destination — smuggled past the differ's non-regular-file filter
or planted in a TOCTOU race between diff and flush — would cause
bbox to write agent-authored content through to the symlink's
target. (The common case of "user has a symlink in their workspace
that the agent edits" is unaffected: the snapshot cloner preserves
symlinks, writes inside the VM resolve through them to regular
target files, and the differ attributes the change to the target
path.)
- Credential store (internal/infra/credential/store.go): a malicious
or buggy guest base image with pre-existing symlinks at the
credential destination paths could redirect credential writes out
of the rootfs.
- Settings injector (internal/infra/settings/injector.go): same
concern as the credential store for per-agent settings (e.g.
~/.claude/*, ~/.config/opencode/*).
Add a shared `internal/infra/safeio` package with `OpenForWrite` and
`WriteFileNoFollow`. Both use `O_NOFOLLOW` on the final path
component; if the destination is a symlink, open fails with ELOOP
and is wrapped as:
/path/to/file: refusing to follow symlink to "target" —
edit that file directly or add the path to .broodboxignore
The message names the symlink target so users hitting the rejection
can see what bbox would have overwritten, and points at the two
recovery paths (edit the target, or exclude via .broodboxignore).
Wire all three write sites through the new helper. ValidateInBounds
remains the first line of defense (catches paths escaping the base
directory via symlink resolution); safeio.OpenForWrite is
defense-in-depth for the in-bounds-but-symlinked case.
Follow-ups (not in this PR):
- Additional write sites in internal/infra/vm/* (knownhosts.go,
gitconfig.go, mcpconfig.go, hooks.go) have the same exposure class
and should migrate to safeio. They operate on the bbox-built guest
rootfs where a malicious base image could plant symlinks.
- Flusher's src-side open (copyFilePreserveMode) still follows
symlinks; the narrow TOCTOU window between HashFile and os.Open
on snapPath is unchanged by this patch.
- Flusher's partial-flush semantics (returns on first error) could
be replaced with all-or-nothing pre-validation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Closes a MEDIUM security finding about destination symlink-follow on three write sites.
os.OpenFile/os.WriteFilewithoutO_NOFOLLOWlets a pre-existing symlink at the destination silently redirect the write to an attacker-chosen target.New
internal/infra/safeiopackage withOpenForWriteandWriteFileNoFollow. Three call sites migrated:internal/infra/review/flusher.go) — snapshot → host workspace.internal/infra/credential/store.go) — host → VM rootfs.internal/infra/settings/injector.go) — both directions, three sites.Clarifying the flusher threat model
The common case of "user has a symlink in their workspace that the agent edits" is unaffected. The snapshot cloner preserves symlinks, writes inside the VM resolve through them to regular target files, and the differ attributes the change to the target path (not the symlink path). The O_NOFOLLOW in the flusher is defense-in-depth for narrow scenarios — a symlink smuggled past the differ's non-regular-file filter, or a TOCTOU race planting a symlink between diff and flush.
Verified by end-to-end smoke: a workspace with
alias.txt -> real-target.txthas the agent's edit toalias.txtcorrectly flushed toreal-target.txt(the resolved target), no rejection.Error message (when O_NOFOLLOW does trigger)
Names the target (so users know what bbox would have overwritten) and both recovery paths.
Test plan
task fmt && task lint && task test— all greensafeiounit tests: creates new, truncates existing, rejects symlink (with target name in error), rejects dangling symlink, rawOpenForWriteexercises the happy pathTestFSFlusher_RefusesToWriteThroughSymlink: in-workspace symlink redirect is caught by O_NOFOLLOW afterValidateInBoundspasses, and the target file is verified untouchedFollow-ups (not in this PR)
internal/infra/vm/*(knownhosts, gitconfig, mcpconfig, hooks) have the same exposure class and should migrate tosafeio.HashFileandos.OpenonsnapPathis unchanged by this patch.🤖 Generated with Claude Code