security(compiler): refuse symlink destinations for generated artifacts - #1000
Merged
Merged
Conversation
Closes #888. Every output path is derived from the SOURCE FILENAME, so a checked-out tree someone else controls — an untrusted pull request, a shared build directory — chooses them. A symlink named `main.s`, `main.map`, `libmain.h` or `main.key` next to `main.php` made the compiler truncate and overwrite that symlink's target with the compiling user's permissions. One policy now covers every generated file, in `pipeline::artifact_io`. REJECT UP FRONT, BEFORE ANY WORK RUNS. `reject_unsafe_destinations` checks each artifact path with `symlink_metadata` — which does not follow the final component, unlike `metadata`, whose answer would describe the TARGET — and refuses a symbolic link or an existing non-regular file (a directory, a FIFO, a device node). Running the check before the first byte is written is what covers the object file and the final binary or library too: an external assembler and linker write those, and this process never opens them itself. An existing REGULAR file is allowed, and deliberately: recompiling over yesterday's `main.s` is the normal case, and it is now an atomic replacement rather than a truncate-in-place. WRITE THROUGH A STAGED RENAME. `write_artifact` creates a private file in the destination's own directory with `O_CREAT | O_EXCL`, writes and syncs it, then renames it over the destination. `rename(2)` REPLACES a symlink rather than following it, so for the paths this process writes itself — assembly, source map, generated header, probe-key sidecar — the window between the up-front check and the write is closed rather than merely narrowed. The staged file is removed on any failure. Verified end to end: compiling `main.php` in a directory containing a `main.s -> victim.txt` symlink now refuses with a named diagnostic and leaves `victim.txt` byte-identical, while an ordinary compile and recompile still succeed. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Greptile SummaryThis PR prevents generated artifacts from overwriting symlink targets or existing non-regular files. It introduces a centralized artifact policy that:
The changes since the previous review add the required compiler-internals documentation. The prior documentation finding is therefore fully addressed. Confidence Score: 5/5The PR appears safe to merge; no actionable new issue or outstanding previous finding remains. The latest documentation accurately records the new artifact policy and its external-tool limitation, fully addressing the only unresolved previous thread. The other previous findings were fixed, withdrawn after clarification, or resolved.
|
| Filename | Overview |
|---|---|
| docs/internals/architecture.md | Documents destination preflight, atomic staged replacement, private probe-key creation, command-specific planning, and the external-tool race limitation. |
| src/pipeline.rs | Performs command-specific artifact destination validation before frontend or backend work begins. |
| src/pipeline/artifact_io.rs | Centralizes destination validation and staged atomic writes, with regression coverage for unsafe paths, permissions, cleanup, and compilation modes. |
| src/pipeline/backend.rs | Routes assembly, generated-header, and probe-key writes through the shared artifact policy. |
| src/source_map.rs | Routes source-map output through atomic staged replacement instead of direct truncating writes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Derive output paths and artifact plan] --> B[Inspect planned destinations]
B -->|Symlink, non-regular, or inspection failure| C[Refuse compilation]
B -->|Missing or regular files| D[Run compiler pipeline]
D --> E{Artifact writer}
E -->|Compiler process| F[Create exclusive sibling staging file]
F --> G[Write and sync]
G --> H[Rename over destination]
E -->|Assembler or linker| I[Write validated object or final output]
Reviews (3): Last reviewed commit: "docs(internals): document the generated-..." | Re-trigger Greptile
…es, harden the key Three review follow-ups on #888. VALIDATION NOW COVERS ONLY WHAT THE COMMAND WRITES. Every possible output was checked regardless of where the run stops, so an unrelated symlink or directory at `main.o`, `main` or `main.key` refused `--check`, `--emit-ir` and `--emit-asm` -- commands that create none of those files -- before any frontend work ran. A check that runs before the work has to describe the run that is about to happen: `ArtifactPlan::for_run` derives it from the flags that decide where `compile()` returns, and the probe-key sidecar is only a destination when `--with-monitoring` is on. Measured on a tree with all three planted: `--check` and `--emit-asm` now succeed, the full build still refuses at `main.o`, and the symlink target still reads SECRET. AN UNINSPECTABLE DESTINATION IS NO LONGER TREATED AS ABSENCE. Every `symlink_metadata` failure meant "does not exist, the write will create it", so a permission error, an I/O error or an `ELOOP` let exactly the paths that cannot be vouched for through the check and on to an external tool. Only `NotFound` means absence now; anything else is refused with the destination and the error named. THE PROBE KEY IS CREATED OWNER-ONLY, BEFORE THE RENAME. It was staged with the ambient umask and renamed into its well-known `.key` name, and only then narrowed to 0600 -- so on a shared build host the monitoring HMAC credential was readable by other local users for the window between the two, on every rebuild. `write_private_artifact` applies `mode(0o600)` at creation, which is an upper bound the umask can only narrow further, so the bytes never exist at the destination name under weaker permissions. The `restrict_to_owner` call after it stays as the belt to this braces. Tests: an `ELOOP` destination refused rather than waved through; a private artifact never group- or world-readable; and one that plants symlinks at `main`, `main.o` and `main.key` and walks `--check`, `--emit-ir`, `--emit-asm`, a full build and a full build without monitoring through the plan. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Review follow-up: AGENTS.md requires a compiler-internals change to be documented under `docs/internals/`, and this PR introduced a pipeline-wide policy with nothing written down. `docs/internals/architecture.md` gains a "Generated-artifact write policy" section under the compilation pipeline: why every output path is attacker-chosen in the first place (they are derived from the source filename), what the preflight check refuses and why it uses `symlink_metadata` rather than `metadata`, why only `NotFound` counts as absence, why the destination set is scoped to the command about to run, and what `O_EXCL`-staged replacement buys for the four paths this process writes itself. It also states the limitation plainly rather than leaving it to be discovered: the object file and the final binary are written by an external assembler and linker, so the preflight is all that covers them -- complete against a symlink already in the tree, which is the reported threat, and not against an attacker writing to the build directory concurrently. The reason staging those two is a separate change is named too: the linker's debug map records the object path as handed to it, and `dsymutil` names its bundle after the binary it is given. The module map gains the `artifact_io.rs` entry. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
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.
Closes #888.
What was wrong
Every output path is derived from the source filename, so a checked-out tree someone else controls — an untrusted pull request, a shared build directory — chooses them. A symlink named
main.s,main.map,libmain.hormain.keynext tomain.phpmade the compiler truncate and overwrite that symlink's target with the compiling user's permissions.Fix
One policy for every generated file, in a new
pipeline::artifact_io.Reject up front, before any work runs.
reject_unsafe_destinationschecks each artifact path withsymlink_metadata— which does not follow the final component, unlikemetadata, whose answer would describe the target — and refuses a symbolic link or an existing non-regular file (a directory, a FIFO, a device node).Running the check before the first byte is written is what covers the object file and the final binary or library too: an external assembler and linker write those, and this process never opens them itself, so refusing before any tool runs is the only place to cover them.
That answers the issue's "external invocations must not bypass the destination checks" for the threat the issue describes — a symlink that is already in the tree when the compile starts, which is what an untrusted pull request or a seeded shared directory gives you. It does not close the interval between the check and the tool invocation: an attacker running concurrently with write access to the build directory can still plant one there. That is a strictly stronger position to attack from, and from it a symlink race is the worst available option — the same access replaces
main.php, or drops an executable atmainthat gets run a second later. Staging the object and the binary would close it, at the cost of reworking the debug-info path (the linker's debug map records the object path as handed to it, anddsymutilnames its bundle after the binary it is given); that is a separate change with its own coverage, not part of a symlink fix.The check is also scoped to what the selected command actually writes:
--checkand--emit-irproduce none of these files,--emit-asmstops after the assembly, and the probe-key sidecar is a destination only under--with-monitoring. An unrelated symlink at an unused output path does not refuse a valid command.An existing regular file is allowed, and deliberately: recompiling over yesterday's
main.sis the normal case. It is now an atomic replacement rather than a truncate-in-place.Write through a staged rename.
write_artifactcreates a private file in the destination's own directory withO_CREAT | O_EXCL, writes and syncs it, then renames it over the destination.rename(2)replaces a symlink rather than following it, so for the four paths this process writes itself — assembly, source map, generated header, probe-key sidecar — the window between the up-front check and the write is closed, not merely narrowed. The staged file is removed on any failure.Verification
End to end, with the fixture above:
and an ordinary compile and recompile still succeed (
Compiled 'main.php' -> 'main', outputhi, twice).Eight unit tests cover the policy directly: a symlink destination refused with its target intact, a non-regular destination refused, missing and regular destinations accepted, an existing artifact replaced with no staging file left behind, a symlink planted after the check replaced rather than followed, a destination that cannot be inspected (a symlink loop) refused instead of waved through as absent, a private artifact that is never group- or world-readable, and one that plants symlinks at
main,main.oandmain.keyand walks--check,--emit-ir,--emit-asm, a full build and a full build without monitoring through the plan.cargo test --bin elephc(1906),cargo test --lib, and--test codegen_tests cli(63) pass;cargo buildis warning-free.🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr