Skip to content

fix(security): fail closed when the snapshot checksum is unavailable - #205

Merged
kuny0707 merged 1 commit into
tronprotocol:developfrom
barbatos2011:security/snapshot-fail-closed
Aug 3, 2026
Merged

fix(security): fail closed when the snapshot checksum is unavailable#205
kuny0707 merged 1 commit into
tronprotocol:developfrom
barbatos2011:security/snapshot-fail-closed

Conversation

@barbatos2011

Copy link
Copy Markdown

Whether a snapshot was verified at all was decided by an attacker-controllable response. Found by an audit of develop at 0c654cd.

The problem

Download gated verification on pre.HasMD5Sidecar, a flag set from a plaintext HEAD of the .md5sum URL. Anyone on the path to a mainnet mirror — all bare-IP http:// — could answer that HEAD with a 404, and the tarball was then extracted with expectedMD5 empty, no error, exit 0. The CLI reported (md5 sidecar absent — not verified) as a successful download. The attacker's own response was, in effect, a switch that turned integrity checking off.

What changed

The sidecar is fetched unconditionally whenever the operator has not opted out. A 404, a non-200, or a transport failure returns a typed VerificationUnavailableError before any tarball byte is requested, so nothing is written.

Driven A/B against a binary built from develop:

sidecar behaviour develop this branch
valid digest extracted, verified identical
404 extracted, exit 0, attacker state on disk refused, 0 files in destination
500 extracted, exit 0 refused, 0 files
transport reset extracted, exit 0 refused, cause wrapped, 0 files

A request recorder confirms the refusal precedes any tarball GET.

The MCP snapshot_download tool had no opt-out at all, so it always extracted unverified on a 404 — it gains a no_verify arg alongside the CLI's existing --no-verify, which survives the --detach re-exec.

Does this break real downloads?

No. All eight mirrors in SourceTable were re-probed live and every one returns 200 with a coreutils-format body, so none of the in-repo callers — the recipe, build-nile-fixture.sh, the CI job — needs the opt-out, and none is changed here.

The docs that said sidecars are sometimes absent are corrected rather than deleted: they now say what actually happens.

Behaviour changes worth reviewing

  • A download whose sidecar is genuinely unavailable now exits 1 with VERIFICATION_UNAVAILABLE instead of succeeding. The mainnet mirrors index a backup directory as soon as it exists, so a run landing between tarball and sidecar publication would now fail — the error's first suggestion is to retry, for exactly that reason.
  • One failure path changes error_code from DOWNLOAD_ERROR to VERIFICATION_UNAVAILABLE; exit code stays 1.
  • Adds one additive optional field to snapshot-download.schema.json, so SchemaVersion goes 1.12.2 → 1.12.3.

Testing

go test ./... -race -count=1, go vet ./... and gofmt clean on the branch, plus the A/B matrix above and a live probe of all eight mirrors.

Ordering

This overlaps #204, which touches the same verification block and also bumps SchemaVersion to 1.12.3. Suggest merging #204 first, then rebasing this and bumping to 1.12.4 with make snapshot-schema-baseline. Both are independently correct against develop as it stands.

@barbatos2011
barbatos2011 force-pushed the security/snapshot-fail-closed branch 2 times, most recently from 7f9732c to ed5051f Compare August 3, 2026 08:47
@barbatos2011

Copy link
Copy Markdown
Author

Rebased on top of #204 to resolve the schema-version and download.go collision. This branch now contains #204's three commits as well, so its diff shows both until #204 merges — review the last commit (fix(security): fail closed when the snapshot checksum is unavailable) for the change specific to this PR.

The two fixes are integrated rather than merely stacked: the sidecar is fetched unconditionally (fail-closed) and the cleartext-transport warning still fires before it, DownloadResult carries both verification_skipped and the sha256/plaintext_transport fields, and both surfaces' JSON payloads report all of them. SchemaVersion is 1.12.5 here (develop took 1.12.3, #204 takes 1.12.4) with a regenerated baseline.

One test fixture changed as a consequence: #204's transport tests used a fake mirror that served no .md5sum, which was fine when a missing sidecar meant "skip verification" but is now correctly a hard error. The fixture serves a real sidecar, so those tests still exercise what they were written for. TestDownload_MissingSidecarFailsClosed and TestDownload_WarnsOnPlaintextMirror both pass together.

Whether a snapshot was verified at all was decided by pre.HasMD5Sidecar,
a flag set from a plaintext HEAD of the .md5sum URL. Anyone on the path
could answer that HEAD with a 404 and the tarball was then extracted with
expectedMD5 empty, no error, exit 0 - the attacker-controlled response was
effectively a 'skip integrity checking' switch, and the CLI reported
'(md5 sidecar absent - not verified)' as a success.

The sidecar is now fetched unconditionally whenever the operator has not
opted out, and a 404, non-200 or transport failure returns a typed
VerificationUnavailableError before any tarball byte is requested, so
nothing is written. Driven A/B against a binary built from develop: base
extracts attacker state with exit 0 on a 404, a 500 and a transport reset;
this refuses all three with an empty destination.

The MCP snapshot_download tool had no opt-out at all, so it always
extracted unverified on a 404; it gains a no_verify arg alongside the
CLI's existing --no-verify, which survives the --detach re-exec.

All eight mirrors were re-probed and every one returns 200 with a
coreutils-format body, so no in-repo caller needs the opt-out. The docs
still say sidecars can be absent - corrected to say the download now
refuses rather than extracting.

Adds one additive optional field to snapshot-download.schema.json, so
SchemaVersion goes 1.12.2 -> 1.12.3 with a regenerated baseline.
@barbatos2011
barbatos2011 force-pushed the security/snapshot-fail-closed branch from ed5051f to 1156b1e Compare August 3, 2026 09:00
@kuny0707
kuny0707 merged commit d2e9d4d into tronprotocol:develop Aug 3, 2026
14 checks passed
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.

2 participants