fix(security): fail closed when checksums unavailable or mismatched#269
fix(security): fail closed when checksums unavailable or mismatched#269mitre88 wants to merge 1 commit intoGentleman-Programming:mainfrom
Conversation
|
Note: I can't add labels from a fork. This PR needs the |
Checksum verification was fail-open: if checksums.txt was missing, the archive name was absent from the file, or no sha256 tool was found, the installer warned and continued. This weakens the supply-chain trust boundary at install time. Changes: - install.sh: convert three warn+continue paths to fatal; add --insecure escape hatch for manual use in air-gapped or trusted environments - install.ps1: same for two warn+continue paths; add -Insecure switch - download.go: add mandatory checksum verification to the self-upgrade path (Download); introduces downloadToFile (SHA256 while streaming), fetchChecksums, expectedChecksumFor, resolveArchiveName, and resolveChecksumURL; existing downloadBinary kept for extraction - download_test.go: add TestExpectedChecksumFor, TestFetchChecksums, TestDownloadToFile, and TestDownload_ChecksumVerification covering all four failure modes from the issue Closes Gentleman-Programming#245
97d359b to
b70661e
Compare
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Solid security hardening — the fail-open to fail-closed transition is exactly right, and the test coverage for all four failure modes is thorough. The Go implementation correctly downloads to a temp dir, verifies the checksum, and only then extracts. LGTM.
A few minor suggestions (none blocking):
-
fetchChecksums: addio.LimitReader— the project already uses this pattern instrategy.go. Without it, a malicious server could force unbounded memory allocation. Something likeio.LimitReader(resp.Body, 1<<20)would be sufficient. -
downloadToFile: explicitf.Close()before return — the deferred close means write errors fromClose()are silently dropped. Minor, but good practice for data-integrity code. -
Constant-time comparison (
crypto/subtle.ConstantTimeCompare) for the digest check. Negligible real-world risk for a CLI tool, but it's the standard practice for checksum verification. -
downloadBinaryis now orphaned fromDownload()— consider a// Deprecatednote or removal in a follow-up to avoid confusion about which path is checksum-verified.
Nice work overall.
|
Gentle ping — this PR is ready for review. All CI checks pass. Would appreciate a look when possible. Thanks! |
|
Bump — all CI checks pass (Unit Tests, E2E Tests, lint). This PR has been approved. Ready to merge. Could a maintainer hit the merge button? Thanks! |
|
@Gentleman-Programming — this security fix has been approved and all CI passes (unit + E2E). It makes checksum validation fail-closed instead of fail-open. Could you merge? Thanks! |
Summary
Closes #245
Checksum verification in the binary install path was fail-open: if
checksums.txtcould not be downloaded, the archive name was absent from it, or no SHA256 tool was available, the installer logged a warning and continued. This allows installation of an unverified (potentially compromised) binary.This PR hardens all three paths to fail closed by default, with an explicit escape hatch for environments where skipping is intentional.
Changes
scripts/install.shCould not download checksums.txt→fatal(waswarn)Archive not found in checksums.txt→fatal(waswarn)No sha256sum/shasum found→fatal(waswarn+ fake match)--insecureflag to explicitly opt out of verificationscripts/install.ps1Could not download checksums.txt(catch block) →Stop-WithError(wasWrite-Warn)Archive not found in checksums.txt→Stop-WithError(wasWrite-Warn)-Insecureswitch parameterinternal/update/upgrade/download.goThe self-upgrade path (
Download) had zero checksum verification. New functions added:downloadToFile— downloads to a temp file and returns SHA256 hex digest (viaio.MultiWriter)fetchChecksums— fetcheschecksums.txt, returns error on HTTP non-200expectedChecksumFor— parses BSD-style checksums.txt, returns error if filename not listedresolveArchiveName/resolveChecksumURL— URL helpers (also improve testability)Downloadnow downloads to a temp file, verifies checksum, then extracts — fail closed at every stepinternal/update/upgrade/download_test.goNew table-driven tests covering all four failure modes from the issue:
TestExpectedChecksumFor— pure unit test for the checksum parserTestFetchChecksums— success + HTTP 404 → errorTestDownloadToFile— verifies SHA256 computed correctly while streamingTestDownload_ChecksumVerification— four sub-cases: match (success), mismatch, missingchecksums.txt, archive not listedTest plan
go test ./internal/update/upgrade/...— all existing + new tests passgo vet ./...— no issues./scripts/install.sh --insecure— installs with warning, no fatal./scripts/install.sh— fails whenchecksums.txtunreachable (e.g. no network)