Skip to content

fix: bind the plugin cache to a home-scoped attestation before promoting it to PATH (GHSA-4q78) - #636

Open
REPPL wants to merge 5 commits into
mainfrom
fix/owned-copy-attestation
Open

REPPL wants to merge 5 commits into
mainfrom
fix/owned-copy-attestation

Conversation

@REPPL

@REPPL REPPL commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Fixes GHSA-4q78-ccfv-f374 (CWE-345) under the maintainer's option B ruling of 2026-09-15. Held unarmed for the release-cutting session, which merges and reviews it before the v0.9.0 changelog roll.

The defect

The owned PATH copy was verified only against a binary-meta beside the artefact, in a directory named solely by the CLAUDE_PLUGIN_DATA environment variable, so whoever controlled that variable could have ahoy install bless bytes of their choosing as the owned abcd.

The fix

  • The bootstrap writes a home-scoped attestation (~/.abcd/cache-attestation: data dir, manifest-authenticated hash, trust, time; 0600; written through a mktemp file and renamed in) only after it has authenticated the cache against the release manifest. An offline bootstrap writes nothing and leaves an existing attestation untouched.
  • Install promotes a cache only when the attestation names that directory and the co-located record carries the attested hash, and it verifies the artefact against the attested hash, never a re-read of the record beside it. An unbound cache is refused with a note naming which check failed, and install degrades to the pinned symlink as it does with no cache. The plugin-root stamp route gets the same binding; dogfood checkouts are unaffected because the binding is on the cache, not the plugin-root binary.
  • The attestation's data_dir carries no control character on the write or on the read.

Two independent reviews shaped this: the first produced the ruling, the second found the re-read window, the predictable temp name and the injection, each fixed with a test watched failing first. A further commit set is in progress on this branch for the review's fourth finding, ruled by the maintainer as "the home anchor for the records install trusts is environment-independent": path-entry and the attestation resolve the home from the account database and fail closed when HOME disagrees, in Go, in the bootstrap and in the hook shims' ownership rung. It will be pushed to this branch when green; the commits below stand on their own.

Records

iss-2609012039102770 resolves with impact: fix; adr-2609151706587280 supersedes adr-46, carrying its five decisions forward under the same numbers; spc-35's wording is revised in place, dated; brief invariant 12 gains the clause; the decision log records the ruling.

Gates

make preflight and make fmt-check green at the pushed tip; RS001 to RS003, attribution and decisions-append gates clean over the range. Touches hooks/, so a code-owner review is required.

Resolves: iss-2609012039102770
Assisted-by: Claude:claude-fable-5-1

…ing it to PATH

The owned PATH copy was verified only against a binary-meta beside the
artefact, in a directory named solely by CLAUDE_PLUGIN_DATA, so whoever
controlled that variable could have `ahoy install` bless bytes of their
choosing (GHSA-4q78-ccfv-f374, CWE-345). The bootstrap now writes
~/.abcd/cache-attestation (data dir, manifest-authenticated hash, trust,
time; 0600; renamed in whole) only after it has authenticated the cache
against the release manifest, and install promotes a cache only when the
attestation names that directory and the co-located record carries the
attested hash. An unbound cache is refused with a note naming which of
the three checks failed, and install degrades to the pinned symlink as it
does with no cache. An offline bootstrap writes nothing and leaves an
existing attestation untouched. The plugin-root stamp route gets the same
binding; dogfood checkouts are unaffected because the binding is on the
cache, not the plugin-root binary.

Refs: iss-2609012039102770
Assisted-by: Claude:claude-fable-5-1
iss-2609012039102770 resolves with impact fix. adr-2609151706587280
supersedes adr-46, carrying its five decisions forward under the same
numbers and adding the cache attestation as the binding between an
environment-supplied data directory and the provenance install records;
adr-46 is marked superseded. spc-35's "taken from the harness or not at
all" is revised in place, dated. Brief invariant 12 gains the clause. The
decision log records that the maintainer ruled option B on 2026-09-15.

Resolves: iss-2609012039102770
Assisted-by: Claude:claude-fable-5-1
…ue, never the record beside it

The binding check compared the co-located binary-meta with the attestation
and the promotion then re-read that same attacker-writable record to decide
what the artefact must hash to. A writer in the attested directory who swaps
the (artefact, binary-meta) pair for a self-consistent forgery between the
two reads had the forgery promoted — the security review of the first cut
did it in 0.25 s with a flipper goroutine.

cacheBindingProblem now hands back the attestation it accepted, and
installOwnedEntry takes its three verdicts once — hazard, presence, binding —
and acts on those locals alone: `want` is the ATTESTED hash, and nothing under
the data dir is consulted after the binding except the artefact bytes, which
are hashed against it. A test seam (nil in production) occupies the window
deterministically; TestInstallRefusesPairFlippedAfterBinding was watched
failing with the forged pair promoted and no note, and passes with the pair
refused on the checksum, no PATH entry and no provenance written.

Assisted-by: Claude:claude-fable-5-1
…r a name it can predict

The attestation temp was `$HOME/.abcd/.cache-attestation.$$`, written with
`>` and chmod-ed by name; both follow a symlink pre-planted at that
predictable name, so a same-UID writer could have the run write the record's
bytes and mode onto a file of their choosing and then rename the planted link
itself into place as the attestation. mktemp creates a fresh exclusive regular
file under an unpredictable name and the chmod is by the name it returned.

TestBootstrapAttestationTempIgnoresAPlantedSymlink learns the script's pid
before it starts (a wrapper records $$, waits, then execs the script, which
keeps the pid), plants a symlink at the old predictable name, and was watched
failing with the victim's bytes and mode overwritten and the attestation
landing as a symlink; it passes with the victim untouched and a regular 0600
attestation in place.

Assisted-by: Claude:claude-fable-5-1
…write and on the read

CLAUDE_PLUGIN_DATA is an environment value written raw into a line-oriented
record whose Go reader parses last-wins and whose shell reader parses
first-wins: a value carrying a newline injected key=value lines of its own,
and a forged binary_sha256 line after the real one won in Go. The bootstrap
now strips \000-\037\177 from the value before the write — the same class
meta_field strips on read — and readCacheAttestation refuses a record whose
data_dir holds one, since no bootstrap wrote it.

TestBootstrapAttestationStripsControlCharactersFromDataDir (a data dir whose
name carries a newline and a forged record line) was watched failing with a
five-line record and the forged hash parsed; the two new
TestReadCacheAttestationIgnoresMalformed cases were watched failing with the
control-carrying record parsed. All pass.

Assisted-by: Claude:claude-fable-5-1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant