diff --git a/CHANGELOG.md b/CHANGELOG.md index f5229a0..a66848c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Security + +- `grant update` now refuses to install a zero-length binary from a release archive +- `grant update` now rejects non-regular zip entries, matching the existing tar behaviour + ### Fixed - `grant favorites add` now fails immediately without a terminal instead of authenticating first diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..ea55a51 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,7 +108,12 @@ Custom `SCAAccessService` follows SDK conventions: - Asset selection: `grant-cli___.tar.gz` (`.zip` on windows) — must stay in sync with `.goreleaser.yaml` - Integrity: SHA-256 of the archive checked against the release's `checksums.txt` (GNU `*filename` binary marker tolerated). **Trust model:** `checksums.txt` comes from the same origin as the archive, so it defends against corrupted/tampered downloads in transit, **not** against a compromised GitHub account or release pipeline. Signature verification would be needed for that. Note the checksum covers the *archive*, not the extracted binary — hence the independent size checks below - Extraction: `archive/tar`+`compress/gzip` / `archive/zip`. Rejects absolute, drive-absolute (`C:\`), UNC and `..` paths (gosec G305); accepts only a single `grant`/`grant.exe` at the archive root (nested entries and duplicate candidates are errors). Size cap is 128 MiB (`maxDownloadBytes`), enforced by `readCapped`, which probes one byte past the cap — a bare `io.LimitReader` reports a *successful* short read and would silently install a truncated binary (gosec G110). `maxDownloadBytes` is a var only so tests can shrink it — mutate it exclusively through the `withMaxDownloadBytes(t, n)` helper (restores via `t.Cleanup`), and never call `t.Parallel()` in a test that does - - Apply: `github.com/minio/selfupdate` v0.6.0 owns the staged-file write, the two-rename swap including the Windows path, and rollback — do not hand-roll this. grant adds the `fsync` of the staged file (minio does not sync) plus a best-effort directory sync. Seams: `applyWithOptions`, `prepareFn`, `commitFn` in `internal/selfupdate/apply.go` + - **Zero-length binaries are refused twice**, in `extractBinary` and again at `applyBinaryTo`: the checksum covers the *archive*, not the extracted bytes, so an empty payload verifies against itself and would replace a working binary with nothing. Backstops, not the fix — they do not catch a non-regular entry carrying non-empty bytes (`tar.TypeCont` and vendor types `'A'..'Z'` are not header-only), which is why the type guards below must never be removed in favour of them + - **Path-guard order in `checkArchivePath` is load-bearing**: the UNC (`//`) arm must precede `path.IsAbs`, because `path.Clean` collapses `//host/share/x` to `/host/share/x` and the absolute arm would otherwise make the UNC arm dead code + - **Non-regular entries are rejected symmetrically and deliberately so**: tar filters on `hdr.Typeflag != tar.TypeReg`, zip on `f.Mode()&fs.ModeType != 0`. `IsDir()` alone accepted a `fs.ModeSymlink` entry named `grant.exe`, extracting the link-target string as the binary. The two formats must reject the same shapes + - The tar/zip declared-size asymmetry is **intentional**: tar checks every entry because reaching the next header inflates the current one, while `zip.NewReader` reads only the central directory and never opens a skipped entry. Neither cap is an *aggregate* one — nothing bounds total inflated bytes or entry count, so a tar bomb can burn CPU inside `extractBinary` — but `verifyChecksum` runs first, so reaching it requires control of `checksums.txt`, which the trust model above already excludes + - `internal/selfupdate/fuzz_test.go` carries fuzz targets for `checkArchivePath` and both extractors; run them by hand (never in CI) and commit any `testdata/fuzz/` entry a real failure produces, as a permanent regression seed + - Apply: `github.com/minio/selfupdate` v0.6.0 owns the staged-file write, the two-rename swap including the Windows path, and rollback — do not hand-roll this. grant adds the `fsync` of the staged file (minio does not sync) plus a best-effort directory sync. Seams: `applyWithOptions`, `prepareFn`, `commitFn`, `syncStagedFileFn` in `internal/selfupdate/apply.go`; `syncStagedFileFn` is a test seam only, and the real `fsync` failure is exercised on Unix alone via a FIFO staged path (`//go:build !windows` — no portable Windows equivalent exists) - **Atomicity, precisely:** each rename is atomic, so the installed binary is never partially written. The *pair* is not: a kill between the two renames, or a failed second rename whose rollback also fails, leaves the binary path absent with `.grant.old`/`.grant.new` beside it. `InterruptedUpdate()` detects that state and `recoveryHint()` prints the `mv` command that fixes it. Do not describe this as fully atomic - `selfUpdater` in `cmd/interfaces.go` is defined over grant-owned types: `UpdateSelf(ctx, current string) (newVersion string, updated bool, err error)` - **End-to-end apply test:** `internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`, run with `go test -tags=selfupdate_e2e ./internal/selfupdate/`. Everything else in `apply_test.go` swaps inert byte blobs; this compiles two real fixture binaries (a dependency-free module built with `-ldflags -X main.version=...`, so no network), executes one, and replaces it through `applyBinaryTo`/`applyWithOptions` **while a process is still running from that image**. That running child is what makes the Windows path real — a running `.exe` cannot be deleted, only renamed. `heldProcess.alive()` asserts the child survived the swap so the held cases cannot silently degrade into the idle cases. Covers success, rollback after a failed second rename (the restored file must still *execute*), and debris diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..e49e080 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -152,28 +152,29 @@ premise does not hold). | ELV-25 | cmd/root dispatch | `cmd/root.go:631-636` | Swap the dispatch order: test `if flags.groups` before `if flags.group != ""`. `--group` and `--groups` are **not** mutually exclusive (`root.go:136-142` pairs neither), so their precedence is unspecified and unpinned | CONFIRMED | test | `TestRootElevate_GroupAndGroupsPrecedence` | PR4 | todo | | ELV-26 | cmd test quality | `cmd/root_elevate_test.go:248` | No production site. The `multi-CSP concurrent fetch - parallel execution` case duplicates the line-174 success setup, adds sleeps, and asserts **no** elapsed time. Real concurrency is covered by `TestFetchEligibility_ConcurrentExecution` | CONFIRMED | test | Delete the duplicate case or give it a real elapsed-time assertion | PR4 | todo | | ELV-27 | cmd test quality | `cmd/root_test.go` (`TestFetchEligibility_ConcurrentExecution`) | Claim: the `<350ms` bound for two concurrent 200ms sleeps is flaky. **Not demonstrated** — 50/50 runs passed. The wall-clock sensitivity remains a plausible overloaded-CI risk, so widen the bound; do not claim an observed flake | OVERSTATED | test | Widen the bound in `TestFetchEligibility_ConcurrentExecution` | PR4 | todo | -| SFU-01 | internal/selfupdate | `internal/selfupdate/selfupdate.go:345` | `case path.IsAbs(cleaned):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath` — one guard-specific `wantErrContains` per arm, with a valid `grant` entry beside each malicious one so the "no binary" fallback cannot be the reason for the error | PR2 | todo | -| SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | todo | -| SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | todo | -| SFU-04 | internal/selfupdate | `internal/selfupdate/selfupdate.go:343` | `case name == "":` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/empty_name` | PR2 | todo | -| SFU-05 | internal/selfupdate | `internal/selfupdate/selfupdate.go:339` | `normalized := strings.ReplaceAll(name, "\\", "/")` → `normalized := name` (backslash traversal and backslash UNC then slip through) | CONFIRMED | test | `TestCheckArchivePath/backslash_traversal`, `.../backslash_unc` | PR2 | todo | -| SFU-06 | internal/selfupdate | `internal/selfupdate/selfupdate.go:359` (`hasDriveLetter`) | Narrow the check to uppercase drive letters only, so lowercase `c:\...` passes | CONFIRMED | test | `TestCheckArchivePath/lowercase_drive`, `.../forward_slash_drive` | PR2 | todo | -| SFU-07 | internal/selfupdate | `internal/selfupdate/selfupdate.go:392` | `if hdr.Typeflag != tar.TypeReg \|\| !isBinaryEntry(hdr.Name) {` → drop the `hdr.Typeflag != tar.TypeReg` operand. Probe (`Typeflag: tar.TypeSymlink, Name: "grant", Size: 0, Linkname: "/etc/passwd"`): baseline `bytes=0 err=archive does not contain a grant binary`; mutated `bytes=0 err=` — i.e. a **zero-byte self-destruct**. No symlink case exists anywhere in the package | CONFIRMED | test + prod-fix | `TestExtractBinary_RejectsNonRegularEntries` (symlink / hardlink / directory named `grant`, plus a zip directory entry). **Production:** reject a zero-length extracted binary *and* add a second non-empty check at the apply boundary (`internal/selfupdate/apply.go:50`). CHANGELOG `### Security` | PR2 | todo | -| SFU-08 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389-391` | Delete the tar declared-size guard (`if hdr.Size > maxDownloadBytes`) | CONFIRMED | test | `TestExtractFromTarGz_RejectsOversizeDecoy` — oversized **decoy** beside a valid binary | PR2 | todo | -| SFU-09 | internal/selfupdate | `internal/selfupdate/selfupdate.go:430-432` | Delete the zip declared-size guard (`if maxDownloadBytes >= 0 && f.UncompressedSize64 > uint64(maxDownloadBytes)`) | CONFIRMED | test | `TestExtractFromZip_RejectsOversizeDecoy` | PR2 | todo | -| SFU-10 | internal/selfupdate | `internal/selfupdate/apply.go:74` | Delete the `if err := syncStagedFile(target); err != nil { ... }` call. Per the consistency review this is **not** a production gap — `applyWithOptions` already returns a wrapped sync error before commit and `syncStagedFile` (`:110`) already returns `f.Sync()` errors. Scope is a seam plus tests; **no CHANGELOG entry** | CONFIRMED | test | `TestApplyWithOptions_SyncsBeforeCommit` via a `syncStagedFileFn` seam (call-order + abort-before-commit) | PR3 | todo | -| SFU-11 | internal/selfupdate | `internal/selfupdate/apply.go:110-115` | In `syncStagedFile`, ignore the `f.Sync()` error: `_ = f.Sync(); return nil` | CONFIRMED | test | `TestApplyWithOptions_AbortsOnSyncError` | PR3 | todo | -| SFU-12 | internal/selfupdate | `internal/selfupdate/apply.go:154-156` | In `InterruptedUpdate`, delete the target-exists guard (`if _, err := os.Stat(targetPath); err == nil \|\| !errors.Is(err, os.ErrNotExist) { return "", false }`). The untested case is target **present** and `.old` present — the documented Windows steady state | CONFIRMED | test | `TestInterruptedUpdate_TargetPresentWithOldBackup` | PR3 | todo | -| SFU-13 | internal/selfupdate | `internal/selfupdate/selfupdate.go:197-199` | Delete the non-200 check `if resp.StatusCode != http.StatusOK { ... }` in `fetchLatestRelease` | CONFIRMED | test | `newFixtureServerWith(t, opts)` → `TestFetchLatestRelease_Non200` | PR3 | todo | -| SFU-14 | internal/selfupdate | `internal/selfupdate/selfupdate.go:202-204` | In `fetchLatestRelease`, swallow the `json.Unmarshal` error on an empty body: `_ = json.Unmarshal(body, &rel)` | CONFIRMED | test | `TestFetchLatestRelease_EmptyBody` | PR3 | todo | -| SFU-15 | internal/selfupdate | `internal/selfupdate/selfupdate.go:205-207` | Delete `if rel.TagName == "" { return nil, errors.New("GitHub release response has no tag_name") }` | CONFIRMED | test | `TestFetchLatestRelease_EmptyTagName`; also non-200 on the **asset** and **checksums** downloads (`:229`) | PR3 | todo | -| SFU-16 | internal/selfupdate | `internal/selfupdate/version.go` (`comparePreRelease`, numeric-vs-numeric branch) | Invert the numeric-vs-numeric comparison so `rc.10` sorts before `rc.2` | CONFIRMED | test | `TestCompareVersions_NumericPrereleaseOrdering` (`rc.10` vs `rc.2`) | PR3 | todo | -| SFU-17 | internal/selfupdate | `internal/selfupdate/version.go:194` | `if !isAllDigits(part) {` → `if false {` in the core `MAJOR.MINOR.PATCH` loop, so `"1.+5.3"` is accepted | CONFIRMED | test | `TestParseVersion/invalid_core_segment` (`"1.+5.3"`) | PR3 | todo | -| SFU-18 | internal/selfupdate | `internal/selfupdate/selfupdate.go:284-287` | `if len(fields) != 2 { return fmt.Errorf("malformed line in %s: %q", ...) }` → `continue` | CONFIRMED | test | `TestVerifyChecksum_MalformedLine` | PR3 | todo | -| SFU-19 | internal/selfupdate | `internal/selfupdate/selfupdate.go:316-317` | In `extractBinary`, replace the `default:` unsupported-format error with `return extractFromTarGz(archive)` | CONFIRMED | test | `TestExtractBinary_UnsupportedFormat` | PR3 | todo | -| SFU-20 | internal/selfupdate | `internal/selfupdate/selfupdate.go:402` | Delete `if int64(len(data)) != hdr.Size { ... }` (tar truncation cross-check). **Unreachable by construction**: a successful capped read returns exactly `hdr.Size`, and earlier exhaustion returns `io.ErrUnexpectedEOF`. Hand-patched proof: header declares 40 with body `"bin"` → `bytes=40 err=`; header declares 2000 → `bytes=0 err=... unexpected EOF` | CONFIRMED | wont-fix | none — keep as defense-in-depth, comment it as unreachable, and claim no coverage. Rename `TestExtractBinaryRejectsTruncatedEntry` → `...TruncatedArchive` | PR2 | todo | -| SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | todo | -| SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | todo | +| SFU-01 | internal/selfupdate | `internal/selfupdate/selfupdate.go:345` | `case path.IsAbs(cleaned):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath` — one guard-specific `wantErrContains` per arm, with a valid `grant` entry beside each malicious one so the "no binary" fallback cannot be the reason for the error | PR2 | done | +| SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | done | +| SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | done | +| SFU-04 | internal/selfupdate | `internal/selfupdate/selfupdate.go:343` | `case name == "":` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/empty_name` | PR2 | done | +| SFU-05 | internal/selfupdate | `internal/selfupdate/selfupdate.go:339` | `normalized := strings.ReplaceAll(name, "\\", "/")` → `normalized := name` (backslash traversal and backslash UNC then slip through) | CONFIRMED | test | `TestCheckArchivePath/backslash_traversal`, `.../backslash_unc` | PR2 | done | +| SFU-06 | internal/selfupdate | `internal/selfupdate/selfupdate.go:359` (`hasDriveLetter`) | Narrow the check to uppercase drive letters only, so lowercase `c:\...` passes | CONFIRMED | test | `TestCheckArchivePath/lowercase_drive`, `.../forward_slash_drive` | PR2 | done | +| SFU-07 | internal/selfupdate | `internal/selfupdate/selfupdate.go:392` | `if hdr.Typeflag != tar.TypeReg \|\| !isBinaryEntry(hdr.Name) {` → drop the `hdr.Typeflag != tar.TypeReg` operand. Probe (`Typeflag: tar.TypeSymlink, Name: "grant", Size: 0, Linkname: "/etc/passwd"`): baseline `bytes=0 err=archive does not contain a grant binary`; mutated `bytes=0 err=` — i.e. a **zero-byte self-destruct**. No symlink case exists anywhere in the package | CONFIRMED | test + prod-fix | `TestExtractBinaryRejectsNonRegularEntries` (symlink / hardlink / directory named `grant`, plus a zip directory entry). **Correction (post-review):** the header-only fixtures (symlink/hardlink/dir) fail on the *message* only (`grant in archive is empty` instead of `does not contain a grant binary`), because Go's `tar.Reader` forces a zero-length body for header-only types and the empty-binary backstop catches them first. That pinned the wording, not the behaviour. Two content-carrying cases were added — `tar.TypeCont` and vendor type `'Z'`, both non-header-only and therefore with readable bodies — which fail on the **bytes returned** (`expected rejection, got 44 bytes`) and convert the whole class to a behavioural pin. This is also the residual gap the backstop cannot cover. **Production:** reject a zero-length extracted binary *and* add a second non-empty check at the apply boundary (`internal/selfupdate/apply.go:50`). CHANGELOG `### Security` | PR2 | done | +| SFU-08 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389-391` | Delete the tar declared-size guard (`if hdr.Size > maxDownloadBytes`) | CONFIRMED | test | `TestExtractFromTarGzRejectsOversizeDecoy` — oversized **decoy** beside a valid binary | PR2 | done | +| SFU-09 | internal/selfupdate | `internal/selfupdate/selfupdate.go:430-432` | Delete the zip declared-size guard (`if maxDownloadBytes >= 0 && f.UncompressedSize64 > uint64(maxDownloadBytes)`) | CONFIRMED | test | `TestExtractBinaryRejectsOversizedEntry/zip`, asserting the pre-filter's exact wording ("declares 300 bytes, over the 64 byte limit") rather than readCapped's "exceeds". **Correction:** a zip oversize *decoy* cannot kill this mutation and must not — `zip.NewReader` never opens a skipped entry, so the asymmetry is intentional (SFU-22). `TestExtractFromZipIgnoresOversizeDecoy` pins that instead | PR2 | done | +| SFU-10 | internal/selfupdate | `internal/selfupdate/apply.go:74` | Delete the `if err := syncStagedFile(target); err != nil { ... }` call. Per the consistency review this is **not** a production gap — `applyWithOptions` already returns a wrapped sync error before commit and `syncStagedFile` (`:110`) already returns `f.Sync()` errors. Scope is a seam plus tests; **no CHANGELOG entry** | CONFIRMED | test | `TestApplyWithOptionsSyncsBeforeCommit` + `TestApplyWithOptionsAbortsOnSyncError`, via a `syncStagedFileFn` seam (call-order + abort-before-commit) | PR3 | done | +| SFU-11 | internal/selfupdate | `internal/selfupdate/apply.go:110-115` | In `syncStagedFile`, ignore the `f.Sync()` error: `_ = f.Sync(); return nil` | CONFIRMED | test | `TestSyncStagedFileReportsSyncError` (`apply_unix_test.go`). **Correction:** the seam cannot kill this mutation — it lives *inside* `syncStagedFile`, so a stubbed seam never runs it. The kill needs a real failing `fsync`: a FIFO at the staged path (`fsync(2)` on a FIFO returns `EINVAL`). Unix only; the file is **build-excluded** on Windows via `//go:build !windows` (not `t.Skip`), where no portable equivalent exists. **Correction (post-review):** asserting only `err != nil` made the test degradable — on a Unix where `os.OpenFile(fifo, O_RDWR)` fails, `syncStagedFile` returns the *open* error and the test passes with the mutation applied. Verified by probe (no staged file → `err=open .../.grant.new: no such file or directory`, which satisfies `err != nil`). It now asserts `errors.Is(err, syscall.EINVAL) \|\| errors.Is(err, syscall.ENOTSUP)` | PR3 | done | +| SFU-12 | internal/selfupdate | `internal/selfupdate/apply.go:154-156` | In `InterruptedUpdate`, delete the target-exists guard (`if _, err := os.Stat(targetPath); err == nil \|\| !errors.Is(err, os.ErrNotExist) { return "", false }`). The untested case is target **present** and `.old` present — the documented Windows steady state | CONFIRMED | test | `TestInterruptedUpdate/target_present_with_backup_present` | PR3 | done | +| SFU-13 | internal/selfupdate | `internal/selfupdate/selfupdate.go:197-199` | Delete the non-200 check `if resp.StatusCode != http.StatusOK { ... }` in `fetchLatestRelease` | CONFIRMED | test | `TestFetchLatestRelease/not_found` and `/rate_limited` already kill this once the message is asserted; `newFixtureServerWith(t, opts)` was added for the other rows | PR3 | done | +| SFU-14 | internal/selfupdate | `internal/selfupdate/selfupdate.go:202-204` | In `fetchLatestRelease`, swallow the `json.Unmarshal` error on an empty body: `_ = json.Unmarshal(body, &rel)` | CONFIRMED | test | `TestFetchLatestReleaseRejectsBadPayloads/empty_body` — asserting the *decode* message, since an empty body also yields an empty `tag_name` | PR3 | done | +| SFU-15 | internal/selfupdate | `internal/selfupdate/selfupdate.go:205-207` | Delete `if rel.TagName == "" { return nil, errors.New("GitHub release response has no tag_name") }` | CONFIRMED | test | `TestFetchLatestReleaseRejectsBadPayloads/empty_tag_name`; non-200 on the **asset** and **checksums** downloads plus the empty-download guard are `TestUpdateSelfFailsOnAssetDownloadStatus`, which must assert the inner `download returned status N` message — the `failed to download X` wrapper alone is satisfied by the empty-download error | PR3 | done | +| SFU-16 | internal/selfupdate | `internal/selfupdate/version.go` (`comparePreRelease`, numeric-vs-numeric branch) | Invert the numeric-vs-numeric comparison so `rc.10` sorts before `rc.2` | CONFIRMED | test | `TestCompareVersions/numeric_prerelease_compares_numerically_mirrored` (`rc.10` vs `rc.2` → +1). **Note:** a full inversion of the branch is caught by the pre-existing case too; the mutation that genuinely needs the mirror is dropping the `case aNum > bNum: return 1` arm, which was the one reverified | PR3 | done | +| SFU-17 | internal/selfupdate | `internal/selfupdate/version.go:194` | `if !isAllDigits(part) {` → `if false {` in the core `MAJOR.MINOR.PATCH` loop, so `"1.+5.3"` is accepted | CONFIRMED | test | `TestParseVersion/non_numeric` (`"1.x.3"`) and `/empty_core_segment` (`"1.2."`), asserting `is not a non-negative integer`. **Correction:** `"1.+5.3"` does NOT reach the guard — `ParseVersion` splits build metadata at the first `+` before parsing the core, so it fails with `expected MAJOR.MINOR.PATCH` mutated or not. It is kept as a case, documented as such. Only the message distinguishes the guard from `strconv.Atoi` | PR3 | done | +| SFU-18 | internal/selfupdate | `internal/selfupdate/selfupdate.go:284-287` | `if len(fields) != 2 { return fmt.Errorf("malformed line in %s: %q", ...) }` → `continue` | CONFIRMED | test | `TestVerifyChecksum/malformed_line_with_one_field` and `/malformed_line_with_three_fields` | PR3 | done | +| SFU-19 | internal/selfupdate | `internal/selfupdate/selfupdate.go:316-317` | In `extractBinary`, replace the `default:` unsupported-format error with `return extractFromTarGz(archive)` | CONFIRMED | test | `TestExtractBinary/unknown_archive_format` with `wantErrContains: "unsupported archive format"` | PR3 | done | +| SFU-20 | internal/selfupdate | `internal/selfupdate/selfupdate.go:402` | Delete `if int64(len(data)) != hdr.Size { ... }` (tar truncation cross-check). **Unreachable by construction**: a successful capped read returns exactly `hdr.Size`, and earlier exhaustion returns `io.ErrUnexpectedEOF`. Hand-patched proof: header declares 40 with body `"bin"` → `bytes=40 err=`; header declares 2000 → `bytes=0 err=... unexpected EOF` | CONFIRMED | wont-fix | none — keep as defense-in-depth, comment it as unreachable, and claim no coverage. Rename `TestExtractBinaryRejectsTruncatedEntry` → `...TruncatedArchive` | PR2 | done | +| SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | done | +| SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | done | +| SFU-23 | internal/selfupdate | `internal/selfupdate/selfupdate.go:441` | `extractFromZip` filtered only on `f.FileInfo().IsDir()`, with no non-regular guard at all. A zip entry carrying `fs.ModeSymlink` named `grant.exe` was accepted **unmutated, in production**: probe returned `bytes=11 body="/etc/passwd" err=`. Not exploitable — extraction is in-memory, the link is never followed, and the bytes are checksum-gated and attacker-chosen either way — so the impact is only that grant installs a file whose content is the link-target string. It was an undocumented tar/zip asymmetry | CONFIRMED | test + prod-fix | **Production:** `if f.Mode()&fs.ModeType != 0 \|\| !isBinaryEntry(f.Name)`, mirroring the tar `Typeflag` guard. **Test:** `TestExtractBinaryRejectsNonRegularEntries/zip_symlink-mode_entry_named_grant`. Reverting the guard fails it behaviourally (`expected rejection, got 11 bytes`) | PR2 | done | | CACHE-01 | internal/cache | `internal/cache/cached_eligibility.go:84` | When `c.refresh` is true, skip the write: guard `Set(c.store, key, *resp)` with `if !c.refresh`. `--refresh` must bypass the **read** but still **write** | CONFIRMED | test | `TestCachedEligibility_RefreshStillWrites` | PR6 | todo | | CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | todo | | CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | todo | diff --git a/internal/selfupdate/apply.go b/internal/selfupdate/apply.go index c9f1d65..510b895 100644 --- a/internal/selfupdate/apply.go +++ b/internal/selfupdate/apply.go @@ -38,6 +38,12 @@ import ( var ( prepareFn = minio.PrepareAndCheckBinary commitFn = minio.CommitBinary + // syncStagedFileFn is a seam only: syncStagedFile already reports its + // errors and applyWithOptions already aborts on them. It exists so tests + // can assert the call ORDER (sync strictly before commit) and the + // abort-before-commit behavior without needing a filesystem on which + // fsync fails. + syncStagedFileFn = syncStagedFile ) // applyBinary replaces the currently running executable with newBinary. @@ -48,6 +54,12 @@ func applyBinary(newBinary []byte) error { // applyBinaryTo replaces the binary at targetPath. An empty targetPath means // the running executable. func applyBinaryTo(newBinary []byte, targetPath string) error { + // Independent of the extractor's own empty check: the checksum minio + // verifies is computed here, from these bytes, so an empty payload would + // verify against itself and replace a working binary with nothing. + if len(newBinary) == 0 { + return errors.New("refusing to install an empty binary") + } sum := sha256.Sum256(newBinary) return applyWithOptions(bytes.NewReader(newBinary), minio.Options{ TargetPath: targetPath, @@ -71,7 +83,7 @@ func applyWithOptions(update io.Reader, opts minio.Options) error { // minio closes the staged file but never syncs it; do that before any // rename so a crash cannot promote a partially materialized file. - if err := syncStagedFile(target); err != nil { + if err := syncStagedFileFn(target); err != nil { _ = os.Remove(stagedPath(target)) return fmt.Errorf("failed to flush the staged binary to disk: %w", err) } diff --git a/internal/selfupdate/apply_test.go b/internal/selfupdate/apply_test.go index 30083e2..d37575c 100644 --- a/internal/selfupdate/apply_test.go +++ b/internal/selfupdate/apply_test.go @@ -84,6 +84,29 @@ func TestApplyBinaryToReplacesTarget(t *testing.T) { } } +// TestApplyBinaryToRejectsEmptyBinary is the second, independent guard against +// the zero-byte self-destruct: whatever the extractor did, the apply boundary +// refuses to install nothing. minio would otherwise happily verify an empty +// payload against a checksum computed from that same empty payload. +func TestApplyBinaryToRejectsEmptyBinary(t *testing.T) { + path := writeFakeBinary(t) + + err := applyBinaryTo(nil, path) + if err == nil { + t.Fatal("expected an empty binary to be refused, got nil") + } + if !strings.Contains(err.Error(), "empty binary") { + t.Errorf("error = %q, want it to name the empty binary", err) + } + if got := readFile(t, path); got != oldBinaryContents { + t.Errorf("target was modified: %q", got) + } + + if err := applyBinaryTo([]byte{}, path); err == nil { + t.Fatal("expected a zero-length slice to be refused, got nil") + } +} + // TestApplyBinaryFailsBeforeTouchingTarget covers the failures that happen // while staging, i.e. before the original binary is renamed at all. func TestApplyBinaryFailsBeforeTouchingTarget(t *testing.T) { @@ -320,6 +343,22 @@ func TestInterruptedUpdate(t *testing.T) { t.Error("missing binary without a backup must not be reported as interrupted") } }) + + // The Windows steady state after EVERY successful update: minio cannot + // remove the backup while a process still runs from that image, so it + // hides it and leaves it behind. Both files present is healthy - if the + // target-exists guard broke, every Windows user would be told their + // install is interrupted and handed a recovery command that would + // overwrite a perfectly good binary with the previous version. + t.Run("target present with backup present", func(t *testing.T) { + path := writeFakeBinary(t) + if err := os.WriteFile(oldPathFor(path), []byte(oldBinaryContents), 0o755); err != nil { //nolint:gosec // test fixture + t.Fatalf("write backup: %v", err) + } + if hint, ok := InterruptedUpdate(path); ok { + t.Errorf("a present binary with a leftover backup is not interrupted, got hint %q", hint) + } + }) } // TestApplyBinaryStagedFileIsSynced pins that grant syncs the staged file @@ -352,6 +391,87 @@ func TestApplyBinaryStagedFileIsSynced(t *testing.T) { } } +// withSyncStagedFileFn swaps the sync seam for one test and restores it via +// t.Cleanup. Not parallel: syncStagedFileFn is package-global. +func withSyncStagedFileFn(t *testing.T, fn func(string) error) { + t.Helper() + orig := syncStagedFileFn + syncStagedFileFn = fn + t.Cleanup(func() { syncStagedFileFn = orig }) +} + +// TestApplyWithOptionsSyncsBeforeCommit pins the ORDER: the staged file must +// be fsynced before minio renames anything, because minio writes and closes it +// without syncing. A commit that happens first would leave a crash window in +// which a zero-length .new file gets renamed into place. +func TestApplyWithOptionsSyncsBeforeCommit(t *testing.T) { + path := writeFakeBinary(t) + + var calls []string + var syncedPath string + withSyncStagedFileFn(t, func(target string) error { + calls = append(calls, "sync") + syncedPath = target + return syncStagedFile(target) + }) + + origCommit := commitFn + t.Cleanup(func() { commitFn = origCommit }) + commitFn = func(opts minio.Options) error { + calls = append(calls, "commit") + return origCommit(opts) + } + + if err := applyBinaryTo([]byte(newBinaryContents), path); err != nil { + t.Fatalf("applyBinaryTo: %v", err) + } + + want := []string{"sync", "commit"} + if len(calls) != len(want) || calls[0] != want[0] || calls[1] != want[1] { + t.Errorf("call order = %v, want %v", calls, want) + } + if syncedPath != path { + t.Errorf("synced %q, want the target %q", syncedPath, path) + } +} + +// TestApplyWithOptionsAbortsOnSyncError pins that a sync failure aborts BEFORE +// the commit: the target is untouched, the staged file is cleaned up, and the +// error says what failed. +func TestApplyWithOptionsAbortsOnSyncError(t *testing.T) { + path := writeFakeBinary(t) + + withSyncStagedFileFn(t, func(string) error { return errTestApply }) + + committed := false + origCommit := commitFn + t.Cleanup(func() { commitFn = origCommit }) + commitFn = func(opts minio.Options) error { + committed = true + return origCommit(opts) + } + + err := applyBinaryTo([]byte(newBinaryContents), path) + if err == nil { + t.Fatal("expected the sync failure to abort the update, got nil") + } + if !errors.Is(err, errTestApply) { + t.Errorf("error does not wrap the sync failure: %v", err) + } + if !strings.Contains(err.Error(), "flush the staged binary to disk") { + t.Errorf("error = %q, want it to name the failed flush", err) + } + if committed { + t.Error("commit ran despite the sync failure") + } + if got := readFile(t, path); got != oldBinaryContents { + t.Errorf("target was modified: %q", got) + } + if _, err := os.Stat(stagedPath(path)); err == nil { + t.Error("staged file was left behind after the aborted apply") + } +} + // TestApplyBinaryToSymlink documents what happens when the install path is a // symlink: the link itself is replaced by a regular file and the link target // is left untouched. On Linux and macOS os.Executable resolves symlinks, so diff --git a/internal/selfupdate/apply_unix_test.go b/internal/selfupdate/apply_unix_test.go new file mode 100644 index 0000000..8915610 --- /dev/null +++ b/internal/selfupdate/apply_unix_test.go @@ -0,0 +1,39 @@ +//go:build !windows + +package selfupdate + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" +) + +// TestSyncStagedFileReportsSyncError pins that syncStagedFile propagates a +// failing f.Sync() rather than swallowing it. A silent failure would defeat the +// whole point of the fsync: minio would rename a file that may not be on disk. +// +// fsync(2) on a FIFO returns EINVAL, which is the only portable-ish way to make +// Sync fail on a file that opens cleanly. Unix only - Windows has no mkfifo. +func TestSyncStagedFileReportsSyncError(t *testing.T) { + target := filepath.Join(t.TempDir(), "grant") + staged := stagedPath(target) + + if err := syscall.Mkfifo(staged, 0o600); err != nil { + t.Skipf("mkfifo unavailable: %v", err) + } + t.Cleanup(func() { _ = os.Remove(staged) }) + + // Assert the error is the fsync failure, not an open failure. Without this + // the test degrades silently: on a Unix where os.OpenFile(fifo, O_RDWR) + // fails, syncStagedFile returns the OPEN error, `err != nil` still holds, + // and the test would pass with the mutation applied. + err := syncStagedFile(target) + if err == nil { + t.Fatal("expected the failing fsync to be reported, got nil") + } + if !errors.Is(err, syscall.EINVAL) && !errors.Is(err, syscall.ENOTSUP) { + t.Fatalf("expected the fsync error (EINVAL/ENOTSUP), got %v - the FIFO probably failed to open, which makes this test inert", err) + } +} diff --git a/internal/selfupdate/archive_security_test.go b/internal/selfupdate/archive_security_test.go new file mode 100644 index 0000000..040b3a8 --- /dev/null +++ b/internal/selfupdate/archive_security_test.go @@ -0,0 +1,462 @@ +package selfupdate + +// SECURITY FIXTURES — defensive, not offensive. +// +// grant update downloads a release archive it did not build the moment it runs +// and unpacks it. This file exercises the guards that already exist in +// extractFromTarGz/extractFromZip/checkArchivePath against archives shaped the +// way a tampered release would be shaped: absolute paths, Windows +// drive-absolute and UNC paths, "..", empty names, non-regular entries and +// oversized entries. +// +// Every fixture here is built in memory and is only ever fed to the in-memory +// extractor. Nothing in this file writes to the filesystem, creates or follows +// a link, executes anything, or reaches the network — the payload bytes are +// inert ASCII. Their sole purpose is to make a regression in those guards fail +// the build. +// +// Most cases assert REJECTION. The non-regular-entry cases deliberately assert +// the opposite — that the hostile entry is SKIPPED and a valid binary beside it +// is returned — because a rejection assertion there cannot tell which guard +// fired; see TestExtractBinaryRejectsNonRegularEntries. + +import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" + "io/fs" + "strings" + "testing" +) + +// Hostile entry names, one per guard in checkArchivePath. The payload is inert +// text: nothing dereferences these names. +const ( + hostileTraversalName = "../evil" + hostileBackslashTraversalName = `..\evil` + hostileAbsoluteName = "/etc/passwd" + hostileDriveName = `C:\grant.exe` + hostileDriveLowerName = `c:\grant.exe` + hostileDriveSlashName = "C:/grant.exe" + hostileUNCName = "//host/share/grant" + hostileBackslashUNCName = `\\host\share\grant` + hostileEmptyName = "" + hostileSymlinkTarget = "/etc/passwd" + hostilePayload = "inert fixture payload, never written to disk" +) + +// fixtureBinaryContents is the benign "grant binary" placed beside a hostile +// entry so that a rejection can never be attributed to the archive simply not +// containing a binary. +const fixtureBinaryContents = "\x7fELF fake grant binary" + +// tarEntry describes one tar member, including the fields buildTarGz cannot +// express: a non-regular type flag, a link name, and a declared size that +// disagrees with the body. +type tarEntry struct { + name string + body string + typeflag byte // zero value means tar.TypeReg + linkname string + size int64 // zero means len(body) +} + +// zipEntry describes one zip member. A name ending in "/" plus fs.ModeDir +// produces a directory entry. +type zipEntry struct { + name string + body string + mode fs.FileMode +} + +// buildTarGzEntries builds an in-memory tar.gz from fully specified entries. +func buildTarGzEntries(t testing.TB, entries []tarEntry) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + for _, e := range entries { + typeflag := e.typeflag + if typeflag == 0 { + typeflag = tar.TypeReg + } + size := e.size + if size == 0 { + size = int64(len(e.body)) + } + hdr := &tar.Header{ + Name: e.name, + Mode: 0o755, + Size: size, + Typeflag: typeflag, + Linkname: e.linkname, + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatalf("write tar header %q: %v", e.name, err) + } + if e.body != "" { + if _, err := tw.Write([]byte(e.body)); err != nil { + t.Fatalf("write tar body %q: %v", e.name, err) + } + } + } + if err := tw.Close(); err != nil { + t.Fatalf("close tar: %v", err) + } + if err := gz.Close(); err != nil { + t.Fatalf("close gzip: %v", err) + } + return buf.Bytes() +} + +// buildZipEntries builds an in-memory zip from fully specified entries. +func buildZipEntries(t testing.TB, entries []zipEntry) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for _, e := range entries { + hdr := &zip.FileHeader{Name: e.name, Method: zip.Deflate} + if e.mode != 0 { + hdr.SetMode(e.mode) + } + w, err := zw.CreateHeader(hdr) + if err != nil { + t.Fatalf("create zip entry %q: %v", e.name, err) + } + if _, err := w.Write([]byte(e.body)); err != nil { + t.Fatalf("write zip entry %q: %v", e.name, err) + } + } + if err := zw.Close(); err != nil { + t.Fatalf("close zip: %v", err) + } + return buf.Bytes() +} + +// buildHostileTarGz / buildHostileZip are buildTarGzEntries / buildZipEntries +// under a name that says what the call site is doing: assembling a +// deliberately malformed archive that the extractor must refuse. They exist so +// a reader of a test case does not have to infer intent from the entry names. +func buildHostileTarGz(t testing.TB, entries []tarEntry) []byte { + t.Helper() + return buildTarGzEntries(t, entries) +} + +func buildHostileZip(t testing.TB, entries []zipEntry) []byte { + t.Helper() + return buildZipEntries(t, entries) +} + +// validTarBinary is the benign entry added beside every hostile one. +func validTarBinary() tarEntry { + return tarEntry{name: "grant", body: fixtureBinaryContents} +} + +func validZipBinary() zipEntry { + return zipEntry{name: "grant.exe", body: fixtureBinaryContents} +} + +// TestCheckArchivePath pins each guard to its own diagnostic. The guards are +// ordered most-specific-first: the UNC arm must be reached before the +// absolute-path arm, because path.Clean collapses "//host/share/x" to +// "/host/share/x" and path.IsAbs would otherwise always win and make the UNC +// arm dead code. +func TestCheckArchivePath(t *testing.T) { + tests := []struct { + name string + entry string + wantErrContains string // empty means the path must be accepted + }{ + {name: "plain name", entry: "grant"}, + {name: "dot slash prefix", entry: "./grant"}, + {name: "nested name", entry: "nested/dir/grant"}, + {name: "name containing dots", entry: "grant..md"}, + {name: "inner parent segment is cleaned away", entry: "a/../grant"}, + + {name: "empty name", entry: hostileEmptyName, wantErrContains: "empty name"}, + {name: "absolute", entry: hostileAbsoluteName, wantErrContains: "illegal absolute path"}, + {name: "traversal", entry: hostileTraversalName, wantErrContains: "illegal path traversal"}, + {name: "backslash traversal", entry: hostileBackslashTraversalName, wantErrContains: "illegal path traversal"}, + {name: "bare parent", entry: "..", wantErrContains: "illegal path traversal"}, + {name: "drive absolute", entry: hostileDriveName, wantErrContains: "illegal drive-absolute path"}, + {name: "lowercase drive", entry: hostileDriveLowerName, wantErrContains: "illegal drive-absolute path"}, + {name: "forward slash drive", entry: hostileDriveSlashName, wantErrContains: "illegal drive-absolute path"}, + {name: "unc", entry: hostileUNCName, wantErrContains: "illegal UNC path"}, + {name: "backslash unc", entry: hostileBackslashUNCName, wantErrContains: "illegal UNC path"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkArchivePath(tt.entry) + if tt.wantErrContains == "" { + if err != nil { + t.Fatalf("checkArchivePath(%q) = %v, want nil", tt.entry, err) + } + return + } + if err == nil { + t.Fatalf("checkArchivePath(%q) = nil, want an error containing %q", tt.entry, tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("checkArchivePath(%q) = %q, want it to contain %q", tt.entry, err, tt.wantErrContains) + } + }) + } +} + +// TestExtractBinaryRejectsNonRegularEntries pins the highest-severity finding +// in the audit: dropping the `hdr.Typeflag != tar.TypeReg` operand (or the zip +// `f.Mode()&fs.ModeType != 0` mirror) makes a symlink, hardlink or directory +// entry named "grant" eligible to become the installed binary. For the +// header-only types that means a successful extraction of ZERO bytes; nothing +// downstream catches it, because the checksum covers the archive, not the +// extracted binary, so an empty payload verifies against itself and +// self-destructs the installed binary. +// +// The cases are written as SUCCESS assertions, not rejections, and that shape +// is the point. Asserting only "an error occurred, containing 'does not +// contain a grant binary'" cannot distinguish the type guard skipping the +// entry from isBinaryEntry failing to match it from checkArchivePath rejecting +// it — every one of those produces the same generic fallback. Neutering +// isBinaryEntry to return false passed such a table unchanged. +// +// Instead each archive pairs the hostile entry with a VALID binary under the +// other accepted name. isBinaryEntry accepts both "grant" and "grant.exe" in +// either format, so with the type guard present the non-regular entry is +// skipped and the valid one is returned; remove the guard and both entries +// match, giving "archive contains more than one grant binary". That is a +// behavioral kill: it survives no mutation of the type guard, and it fails +// loudly if isBinaryEntry stops matching, because then nothing is extracted at +// all. +// +// The valid binary is deliberately the FIRST entry in every archive. Put the +// header-only hostile entry first and, with the guard removed, the zero-length +// backstop fires before the second binary is ever reached — still a failure, +// but one that reports "is empty" instead of naming the duplicate. Ordering it +// this way makes all seven cases fail identically on the guard's own semantics. +// +// Nothing here is written, linked, followed or executed: the extractor is +// in-memory and the link targets are inert strings. +func TestExtractBinaryRejectsNonRegularEntries(t *testing.T) { + tests := []struct { + name string + assetName string + archive []byte + }{ + { + name: "tar symlink named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "grant", typeflag: tar.TypeSymlink, linkname: hostileSymlinkTarget}, + }), + }, + { + name: "tar hardlink named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "other", body: hostilePayload}, + {name: "grant", typeflag: tar.TypeLink, linkname: "other"}, + }), + }, + { + // A directory entry's name normalizes to "grant", so isBinaryEntry + // matches it and only the type guard keeps it out. + name: "tar directory named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "grant/", typeflag: tar.TypeDir}, + }), + }, + { + // Not header-only: tar.Reader will hand out this body, so without + // the type guard the extractor would return attacker-chosen bytes + // and the zero-length backstop would never fire. + name: "tar continuation entry named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "grant", typeflag: tar.TypeCont, body: hostilePayload}, + }), + }, + { + // Vendor-reserved type flags ('A'..'Z') are likewise not + // header-only. Same behavioral pin, a different byte. + name: "tar vendor-type entry named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "grant", typeflag: 'Z', body: hostilePayload}, + }), + }, + { + name: "zip symlink-mode entry named grant.exe is skipped for the valid grant", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildHostileZip(t, []zipEntry{ + {name: "grant", body: fixtureBinaryContents}, + {name: "grant.exe", mode: fs.ModeSymlink | 0o777, body: hostileSymlinkTarget}, + }), + }, + { + // The zip mirror of the tar directory case. IsDir() would also + // catch this one; the symlink case above is what separates + // f.Mode()&fs.ModeType from f.FileInfo().IsDir(). + name: "zip directory named grant is skipped for the valid grant.exe", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildHostileZip(t, []zipEntry{ + {name: "grant.exe", body: fixtureBinaryContents}, + {name: "grant/", mode: fs.ModeDir | 0o755}, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := extractBinary(tt.archive, tt.assetName) + if err != nil { + t.Fatalf("the non-regular entry must be skipped and the valid binary returned, got: %v", err) + } + if string(got) != fixtureBinaryContents { + t.Errorf("extracted %q, want the valid binary %q", string(got), fixtureBinaryContents) + } + }) + } +} + +// TestExtractBinaryRejectsNonRegularOnlyArchive keeps the rejection half of the +// class covered: an archive whose ONLY "grant" is a non-regular entry must be +// refused outright rather than silently yielding empty or link-target bytes. +// The diagnostic is the generic fallback by construction — after the guard +// skips the entry there is no binary left — so this case is deliberately +// wording-shaped and carries no mutation-killing weight of its own. The +// paired-binary cases above are what pin the guard. +func TestExtractBinaryRejectsNonRegularOnlyArchive(t *testing.T) { + tests := []struct { + name string + assetName string + archive []byte + }{ + { + name: "tar symlink is the only grant", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant", typeflag: tar.TypeSymlink, linkname: hostileSymlinkTarget}, + }), + }, + { + name: "tar vendor-type entry carrying bytes is the only grant", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant", typeflag: 'Z', body: hostilePayload}, + }), + }, + { + name: "zip symlink-mode entry is the only grant", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildHostileZip(t, []zipEntry{ + {name: "grant.exe", mode: fs.ModeSymlink | 0o777, body: hostileSymlinkTarget}, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := extractBinary(tt.archive, tt.assetName) + if err == nil { + t.Fatalf("expected rejection, got %d bytes", len(got)) + } + if !strings.Contains(err.Error(), "does not contain a grant binary") { + t.Errorf("error = %q, want it to contain %q", err, "does not contain a grant binary") + } + if len(got) != 0 { + t.Errorf("returned %d bytes alongside the error", len(got)) + } + }) + } +} + +// TestExtractBinaryRejectsEmptyBinary pins the backstop guard: even a +// well-formed regular entry named "grant" must not extract to zero bytes. It +// is a backstop, not a class fix — it does not stop a malformed non-regular +// entry that carries non-empty bytes, which is why +// TestExtractBinaryRejectsNonRegularEntries stays type-specific. +func TestExtractBinaryRejectsEmptyBinary(t *testing.T) { + t.Run("tar.gz", func(t *testing.T) { + archive := buildTarGzEntries(t, []tarEntry{{name: "grant"}}) + got, err := extractBinary(archive, "grant-cli_0.7.0_linux_amd64.tar.gz") + if err == nil { + t.Fatalf("expected rejection, got %d bytes", len(got)) + } + if !strings.Contains(err.Error(), "is empty") { + t.Errorf("error = %q, want it to mention that the entry is empty", err) + } + }) + + t.Run("zip", func(t *testing.T) { + archive := buildZipEntries(t, []zipEntry{{name: "grant.exe"}}) + got, err := extractBinary(archive, "grant-cli_0.7.0_windows_amd64.zip") + if err == nil { + t.Fatalf("expected rejection, got %d bytes", len(got)) + } + if !strings.Contains(err.Error(), "is empty") { + t.Errorf("error = %q, want it to mention that the entry is empty", err) + } + }) +} + +// TestExtractFromTarGzRejectsOversizeDecoy covers the tar declared-size +// pre-filter specifically. The oversized entry is NOT the binary, so +// readCapped never sees it: only the header check can reject this archive, and +// deleting that check makes the archive extract successfully. +func TestExtractFromTarGzRejectsOversizeDecoy(t *testing.T) { + withMaxDownloadBytes(t, 64) + + archive := buildTarGzEntries(t, []tarEntry{ + {name: "decoy.bin", body: strings.Repeat("A", 300)}, + validTarBinary(), + }) + + got, err := extractBinary(archive, "grant-cli_0.7.0_linux_amd64.tar.gz") + if err == nil { + t.Fatalf("expected rejection, got %d bytes", len(got)) + } + // "declares" is the header pre-filter's wording; readCapped says "exceeds". + if !strings.Contains(err.Error(), "declares 300 bytes, over the 64 byte limit") { + t.Errorf("error = %q, want the tar declared-size rejection", err) + } +} + +// TestExtractFromZipIgnoresOversizeDecoy pins the tar/zip asymmetry as +// INTENTIONAL. The tar size check runs for every entry because tar is a +// stream: reaching the next header means inflating the current entry's body. +// zip.NewReader parses only the central directory and never opens a skipped +// entry (f.Open() is called only for the binary), so an oversized non-binary +// entry costs nothing and is correctly ignored. Do not "fix" this by moving +// the zip check earlier — it would reject archives that are not a threat. +// +// Note what tar's check is and is not: hdr.Size bounds each ENTRY, and +// readCapped bounds the binary, but nothing bounds TOTAL inflated bytes or +// entry count. A tar bomb of many just-under-cap entries still burns CPU +// inside extractBinary, because Next() must inflate every skipped entry to +// reach the following header. That is accepted, not overlooked: verifyChecksum +// runs BEFORE extractBinary, so an attacker must already control +// checksums.txt — which the documented trust model excludes anyway. +func TestExtractFromZipIgnoresOversizeDecoy(t *testing.T) { + withMaxDownloadBytes(t, 64) + + archive := buildZipEntries(t, []zipEntry{ + {name: "decoy.bin", body: strings.Repeat("A", 300)}, + validZipBinary(), + }) + + got, err := extractBinary(archive, "grant-cli_0.7.0_windows_amd64.zip") + if err != nil { + t.Fatalf("an oversized non-binary zip entry must be ignored, got: %v", err) + } + if string(got) != fixtureBinaryContents { + t.Errorf("extracted %q, want the binary beside the decoy", string(got)) + } +} diff --git a/internal/selfupdate/fuzz_test.go b/internal/selfupdate/fuzz_test.go new file mode 100644 index 0000000..1239c04 --- /dev/null +++ b/internal/selfupdate/fuzz_test.go @@ -0,0 +1,171 @@ +package selfupdate + +// Coverage-guided fuzzing for the archive-parsing surface — defensive, like +// archive_security_test.go. grant update feeds these functions bytes it just +// downloaded, so they are the one part of the CLI that parses input an +// attacker could shape. Everything runs in memory: nothing is written, linked, +// followed or executed, and every target asserts that grant REJECTS or safely +// accepts, never that it does anything with the payload. +// +// These targets are cheap under a normal `go test` run: the seed corpus is +// executed as ordinary test cases and nothing more. Extended fuzzing is +// deliberately NOT wired into CI; run it by hand, e.g. +// +// go test ./internal/selfupdate/ -run=Fuzz -fuzz=FuzzCheckArchivePath -fuzztime=30s +// +// Any input that trips an assertion is written to testdata/fuzz// by +// the toolchain; commit that file and it becomes a permanent regression seed. + +import ( + "archive/tar" + "io/fs" + "path" + "strings" + "testing" +) + +// fuzzHasDriveLetter is the oracle's OWN drive-letter predicate, deliberately +// not the production hasDriveLetter. An oracle that calls the code under test +// is blind to defects inside it: with the shared call, narrowing +// hasDriveLetter to uppercase-only drives survived 2.5M execs untouched, while +// TestCheckArchivePath/lowercase_drive caught it instantly. Keep this +// independent — if it ever drifts from the production predicate, the fuzzer +// reports it, which is the whole point. +func fuzzHasDriveLetter(normalized string) bool { + if len(normalized) < 2 || normalized[1] != ':' { + return false + } + c := normalized[0] + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') +} + +// FuzzCheckArchivePath checks a NECESSARY CONDITION on the guard, not its full +// contract: anything it accepts must be relative, non-escaping and not +// Windows-absolute. It cannot prove the converse — an over-eager guard that +// rejects legitimate names passes this target trivially, because the property +// body returns early on any error. TestCheckArchivePath owns the +// accept-and-diagnostic side. +func FuzzCheckArchivePath(f *testing.F) { + seeds := []string{ + "grant", + "grant.exe", + "./grant", + "nested/dir/grant", + hostileEmptyName, + hostileTraversalName, + hostileBackslashTraversalName, + hostileAbsoluteName, + hostileDriveName, + hostileDriveLowerName, + hostileDriveSlashName, + hostileUNCName, + hostileBackslashUNCName, + "..", + "a/../../b", + "\x00grant", + strings.Repeat("../", 64) + "grant", + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, name string) { + if err := checkArchivePath(name); err != nil { + return // rejected: nothing further to prove + } + + if name == "" { + t.Fatal("an empty entry name must be rejected") + } + normalized := strings.ReplaceAll(name, `\`, "/") + cleaned := path.Clean(normalized) + switch { + case strings.HasPrefix(normalized, "//"): + t.Fatalf("accepted a UNC path: %q", name) + case path.IsAbs(cleaned): + t.Fatalf("accepted an absolute path: %q", name) + case fuzzHasDriveLetter(normalized): + t.Fatalf("accepted a drive-absolute path: %q", name) + case cleaned == ".." || strings.HasPrefix(cleaned, "../"): + t.Fatalf("accepted a traversal path: %q", name) + } + }) +} + +// fuzzMaxDownloadBytes bounds what a single fuzz exec can allocate. The +// production cap is 128 MiB and readCapped will io.ReadAll up to it per exec: +// with the real cap FuzzExtractFromZip collapses to 0 exec/sec on transient +// near-cap inputs (worker RSS ~118 MB) while still reporting PASS, which makes +// its exec count incomparable to the other targets'. Shrinking the cap keeps +// the fuzzer exploring archive SHAPE rather than SIZE. It sits well above every +// seed (the largest body is 4 KiB), so no seed is rejected by the cap itself. +const fuzzMaxDownloadBytes = 64 << 10 + +// assertExtractInvariant is the property both archive targets share: an +// extractor either fails, or returns a NON-EMPTY binary. A successful +// extraction of zero bytes is the self-destruct case — the checksum covers the +// archive, not the extracted bytes, so empty output would be installed. +func assertExtractInvariant(t *testing.T, got []byte, err error) { + t.Helper() + if err != nil { + if len(got) != 0 { + t.Fatalf("returned %d bytes alongside an error: %v", len(got), err) + } + return + } + if len(got) == 0 { + t.Fatal("reported success with an empty binary") + } +} + +func FuzzExtractFromTarGz(f *testing.F) { + withMaxDownloadBytes(f, fuzzMaxDownloadBytes) + + good := buildTarGzEntries(f, []tarEntry{{name: "grant", body: fixtureBinaryContents}}) + seeds := [][]byte{ + good, + good[:len(good)/2], // truncated gzip stream + []byte("not a gzip stream"), + nil, + buildTarGzEntries(f, []tarEntry{{name: hostileTraversalName, body: hostilePayload}, {name: "grant", body: fixtureBinaryContents}}), + buildTarGzEntries(f, []tarEntry{{name: hostileUNCName, body: hostilePayload}, {name: "grant", body: fixtureBinaryContents}}), + buildTarGzEntries(f, []tarEntry{{name: hostileDriveName, body: hostilePayload}, {name: "grant", body: fixtureBinaryContents}}), + buildTarGzEntries(f, []tarEntry{{name: "grant", typeflag: tar.TypeSymlink, linkname: hostileSymlinkTarget}}), + buildTarGzEntries(f, []tarEntry{{name: "grant", typeflag: tar.TypeDir}}), + buildTarGzEntries(f, []tarEntry{{name: "grant"}}), // zero-length binary + buildTarGzEntries(f, []tarEntry{{name: "grant", body: strings.Repeat("A", 4096)}}), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, archive []byte) { + got, err := extractFromTarGz(archive) + assertExtractInvariant(t, got, err) + }) +} + +func FuzzExtractFromZip(f *testing.F) { + withMaxDownloadBytes(f, fuzzMaxDownloadBytes) + + good := buildZipEntries(f, []zipEntry{{name: "grant.exe", body: fixtureBinaryContents}}) + seeds := [][]byte{ + good, + good[:len(good)/2], // truncated central directory + []byte("not a zip file"), + nil, + buildZipEntries(f, []zipEntry{{name: hostileTraversalName, body: hostilePayload}, {name: "grant.exe", body: fixtureBinaryContents}}), + buildZipEntries(f, []zipEntry{{name: hostileUNCName, body: hostilePayload}, {name: "grant.exe", body: fixtureBinaryContents}}), + buildZipEntries(f, []zipEntry{{name: hostileDriveName, body: hostilePayload}, {name: "grant.exe", body: fixtureBinaryContents}}), + buildZipEntries(f, []zipEntry{{name: "grant/", mode: fs.ModeDir | 0o755}}), + buildZipEntries(f, []zipEntry{{name: "grant.exe"}}), // zero-length binary + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, archive []byte) { + got, err := extractFromZip(archive) + assertExtractInvariant(t, got, err) + }) +} diff --git a/internal/selfupdate/selfupdate.go b/internal/selfupdate/selfupdate.go index e497db6..4932790 100644 --- a/internal/selfupdate/selfupdate.go +++ b/internal/selfupdate/selfupdate.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "path" "runtime" @@ -342,10 +343,14 @@ func checkArchivePath(name string) error { switch { case name == "": return errors.New("archive contains an entry with an empty name") - case path.IsAbs(cleaned): - return fmt.Errorf("archive contains illegal absolute path %q", name) + // The UNC arm must precede the absolute arm: path.Clean collapses + // "//host/share/x" to "/host/share/x", so path.IsAbs would always match + // first and this arm would be unreachable. Specificity before generality - + // the rejection is the same either way, only the diagnostic differs. case strings.HasPrefix(normalized, "//"): return fmt.Errorf("archive contains illegal UNC path %q", name) + case path.IsAbs(cleaned): + return fmt.Errorf("archive contains illegal absolute path %q", name) case hasDriveLetter(normalized): return fmt.Errorf("archive contains illegal drive-absolute path %q", name) case cleaned == ".." || strings.HasPrefix(cleaned, "../"): @@ -399,9 +404,21 @@ func extractFromTarGz(archive []byte) ([]byte, error) { if err != nil { return nil, fmt.Errorf("failed to read %s from archive: %w", hdr.Name, err) } + // Defense in depth, and UNREACHABLE by construction: a successful + // readCapped returns exactly hdr.Size bytes, and a stream that runs + // out early fails inside readCapped with io.ErrUnexpectedEOF. Kept + // because the invariant is worth stating, but no test claims coverage + // of this branch - it cannot be provoked through tar.Reader. if int64(len(data)) != hdr.Size { return nil, fmt.Errorf("%s in archive is truncated: got %d bytes, header declares %d", hdr.Name, len(data), hdr.Size) } + // A zero-length binary must never reach the apply step: the checksum + // covers the archive, not the extracted bytes, so an empty payload + // would be hashed and installed over the working binary. This is a + // backstop for the type and size guards above, not a replacement. + if len(data) == 0 { + return nil, fmt.Errorf("%s in archive is empty", hdr.Name) + } found = data } if found == nil { @@ -421,7 +438,12 @@ func extractFromZip(archive []byte) ([]byte, error) { if err := checkArchivePath(f.Name); err != nil { return nil, err } - if f.FileInfo().IsDir() || !isBinaryEntry(f.Name) { + // Mirrors the tar `Typeflag != tar.TypeReg` guard: only a regular file + // may be the binary. IsDir() alone would accept a symlink-mode entry, + // whose "contents" are the link target string. Not exploitable — + // extraction is in-memory and never follows the link — but the two + // formats must reject the same shapes. + if f.Mode()&fs.ModeType != 0 || !isBinaryEntry(f.Name) { continue } if found != nil { @@ -434,9 +456,17 @@ func extractFromZip(archive []byte) ([]byte, error) { if err != nil { return nil, err } + // Defense in depth, and UNREACHABLE for the same reason as the tar + // cross-check above: readCapped either returns the full entry or + // fails. No test claims coverage of this branch. if uint64(len(data)) != f.UncompressedSize64 { return nil, fmt.Errorf("%s in archive is truncated: got %d bytes, directory declares %d", f.Name, len(data), f.UncompressedSize64) } + // See extractFromTarGz: an empty payload would be checksum-valid and + // would self-destruct the installed binary. + if len(data) == 0 { + return nil, fmt.Errorf("%s in archive is empty", f.Name) + } found = data } if found == nil { diff --git a/internal/selfupdate/selfupdate_test.go b/internal/selfupdate/selfupdate_test.go index 7e675d6..daa3d6b 100644 --- a/internal/selfupdate/selfupdate_test.go +++ b/internal/selfupdate/selfupdate_test.go @@ -2,7 +2,6 @@ package selfupdate import ( "archive/tar" - "archive/zip" "bytes" "compress/gzip" "crypto/sha256" @@ -154,12 +153,15 @@ func TestVerifyChecksum(t *testing.T) { sum := sha256.Sum256(payload) good := hex.EncodeToString(sum[:]) + // wantErrContains, not wantErr: a malformed line that is skipped instead of + // rejected still ends in an error ("has no entry for"), so only the + // message can tell the two apart. tests := []struct { - name string - checksums string - filename string - data []byte - wantErr bool + name string + checksums string + filename string + data []byte + wantErrContains string }{ { name: "match", @@ -187,101 +189,104 @@ func TestVerifyChecksum(t *testing.T) { data: payload, }, { - name: "mismatch", - checksums: "0000000000000000000000000000000000000000000000000000000000000000 grant-cli_0.7.0_linux_amd64.tar.gz\n", - filename: "grant-cli_0.7.0_linux_amd64.tar.gz", - data: payload, - wantErr: true, + name: "mismatch", + checksums: "0000000000000000000000000000000000000000000000000000000000000000 grant-cli_0.7.0_linux_amd64.tar.gz\n", + filename: "grant-cli_0.7.0_linux_amd64.tar.gz", + data: payload, + wantErrContains: "checksum mismatch", }, { - name: "filename absent", - checksums: good + " some-other-file.tar.gz\n", - filename: "grant-cli_0.7.0_linux_amd64.tar.gz", - data: payload, - wantErr: true, + name: "filename absent", + checksums: good + " some-other-file.tar.gz\n", + filename: "grant-cli_0.7.0_linux_amd64.tar.gz", + data: payload, + wantErrContains: "has no entry for", }, { - name: "malformed line", - checksums: "deadbeef\n", - filename: "grant-cli_0.7.0_linux_amd64.tar.gz", - data: payload, - wantErr: true, + name: "malformed line with one field", + checksums: "deadbeef\n", + filename: "grant-cli_0.7.0_linux_amd64.tar.gz", + data: payload, + wantErrContains: "malformed line", }, { - name: "empty checksums", - checksums: "", - filename: "grant-cli_0.7.0_linux_amd64.tar.gz", - data: payload, - wantErr: true, + name: "malformed line with three fields", + checksums: good + " extra grant-cli_0.7.0_linux_amd64.tar.gz\n", + filename: "grant-cli_0.7.0_linux_amd64.tar.gz", + data: payload, + wantErrContains: "malformed line", + }, + { + name: "empty checksums", + checksums: "", + filename: "grant-cli_0.7.0_linux_amd64.tar.gz", + data: payload, + wantErrContains: "has no entry for", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := verifyChecksum([]byte(tt.checksums), tt.filename, tt.data) - if tt.wantErr && err == nil { - t.Fatal("expected error, got nil") + if tt.wantErrContains == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return } - if !tt.wantErr && err != nil { - t.Fatalf("unexpected error: %v", err) + if err == nil { + t.Fatalf("expected an error containing %q, got nil", tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) } }) } } -// buildTarGz builds an in-memory tar.gz archive from name -> contents. -func buildTarGz(t *testing.T, entries [][2]string) []byte { +// buildTarGz builds an in-memory tar.gz archive from name -> contents. It is a +// convenience wrapper over buildTarGzEntries (archive_security_test.go) for the +// common case of well-formed regular files; tests that need a non-regular type +// flag, a link name or a lying declared size call buildTarGzEntries directly. +func buildTarGz(t testing.TB, entries [][2]string) []byte { t.Helper() - var buf bytes.Buffer - gz := gzip.NewWriter(&buf) - tw := tar.NewWriter(gz) + full := make([]tarEntry, 0, len(entries)) for _, e := range entries { - hdr := &tar.Header{Name: e[0], Mode: 0o755, Size: int64(len(e[1])), Typeflag: tar.TypeReg} - if err := tw.WriteHeader(hdr); err != nil { - t.Fatalf("write tar header: %v", err) - } - if _, err := tw.Write([]byte(e[1])); err != nil { - t.Fatalf("write tar body: %v", err) - } + full = append(full, tarEntry{name: e[0], body: e[1]}) } - if err := tw.Close(); err != nil { - t.Fatalf("close tar: %v", err) - } - if err := gz.Close(); err != nil { - t.Fatalf("close gzip: %v", err) - } - return buf.Bytes() + return buildTarGzEntries(t, full) } -// buildZip builds an in-memory zip archive from name -> contents. -func buildZip(t *testing.T, entries [][2]string) []byte { +// buildZip builds an in-memory zip archive from name -> contents. Wrapper over +// buildZipEntries, mirroring buildTarGz. +func buildZip(t testing.TB, entries [][2]string) []byte { t.Helper() - var buf bytes.Buffer - zw := zip.NewWriter(&buf) + full := make([]zipEntry, 0, len(entries)) for _, e := range entries { - w, err := zw.Create(e[0]) - if err != nil { - t.Fatalf("create zip entry: %v", err) - } - if _, err := w.Write([]byte(e[1])); err != nil { - t.Fatalf("write zip entry: %v", err) - } + full = append(full, zipEntry{name: e[0], body: e[1]}) } - if err := zw.Close(); err != nil { - t.Fatalf("close zip: %v", err) - } - return buf.Bytes() + return buildZipEntries(t, full) } +// TestExtractBinary drives the whole extractor. Two conventions matter here: +// +// - wantErrContains, not a bare wantErr bool. Several of the path guards are +// interchangeable as far as "an error happened" is concerned, so only +// message discrimination can tell them apart - and a guard that is silently +// replaced by a later, more general one is exactly the regression this +// table exists to catch. +// - every hostile entry sits BESIDE a valid "grant". Without it the archive +// also fails the "does not contain a grant binary" check, so removing the +// guard under test still produces an error and the case proves nothing. func TestExtractBinary(t *testing.T) { const binContents = "\x7fELF fake grant binary" tests := []struct { - name string - assetName string - archive []byte - wantErr bool - want string + name string + assetName string + archive []byte + wantErrContains string // empty means the extraction must succeed + want string }{ { name: "tar.gz with decoy files", @@ -306,55 +311,124 @@ func TestExtractBinary(t *testing.T) { name: "tar.gz path traversal rejected", assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", archive: buildTarGz(t, [][2]string{ - {"../evil", "pwned"}, + {hostileTraversalName, hostilePayload}, {"grant", binContents}, }), - wantErr: true, + wantErrContains: "illegal path traversal", + }, + { + name: "tar.gz backslash traversal rejected", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGz(t, [][2]string{ + {hostileBackslashTraversalName, hostilePayload}, + {"grant", binContents}, + }), + wantErrContains: "illegal path traversal", }, { name: "zip path traversal rejected", assetName: "grant-cli_0.7.0_windows_amd64.zip", archive: buildZip(t, [][2]string{ - {"../evil", "pwned"}, + {hostileTraversalName, hostilePayload}, {"grant.exe", binContents}, }), - wantErr: true, + wantErrContains: "illegal path traversal", }, { name: "tar.gz absolute path rejected", assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", archive: buildTarGz(t, [][2]string{ - {"/etc/passwd", "pwned"}, + {hostileAbsoluteName, hostilePayload}, + {"grant", binContents}, }), - wantErr: true, + wantErrContains: "illegal absolute path", }, { name: "tar.gz windows drive-absolute path rejected", assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", archive: buildTarGz(t, [][2]string{ - {`C:\grant.exe`, "pwned"}, + {hostileDriveName, hostilePayload}, + {"grant", binContents}, }), - wantErr: true, + wantErrContains: "illegal drive-absolute path", + }, + { + name: "tar.gz lowercase drive-absolute path rejected", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGz(t, [][2]string{ + {hostileDriveLowerName, hostilePayload}, + {"grant", binContents}, + }), + wantErrContains: "illegal drive-absolute path", + }, + { + name: "tar.gz forward-slash drive-absolute path rejected", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGz(t, [][2]string{ + {hostileDriveSlashName, hostilePayload}, + {"grant", binContents}, + }), + wantErrContains: "illegal drive-absolute path", }, { name: "zip windows drive-absolute path rejected", assetName: "grant-cli_0.7.0_windows_amd64.zip", archive: buildZip(t, [][2]string{ - {`C:\grant.exe`, "pwned"}, + {hostileDriveName, hostilePayload}, + {"grant.exe", binContents}, }), - wantErr: true, + wantErrContains: "illegal drive-absolute path", }, { name: "tar.gz UNC path rejected", assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", - archive: buildTarGz(t, [][2]string{{"//host/share/grant", "pwned"}}), - wantErr: true, + archive: buildTarGz(t, [][2]string{ + {hostileUNCName, hostilePayload}, + {"grant", binContents}, + }), + wantErrContains: "illegal UNC path", }, { - name: "nested binary is not selected", + name: "tar.gz backslash UNC path rejected", assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", - archive: buildTarGz(t, [][2]string{{"nested/dir/grant", "pwned"}}), - wantErr: true, + archive: buildTarGz(t, [][2]string{ + {hostileBackslashUNCName, hostilePayload}, + {"grant", binContents}, + }), + wantErrContains: "illegal UNC path", + }, + { + name: "zip UNC path rejected", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildZip(t, [][2]string{ + {hostileUNCName, hostilePayload}, + {"grant.exe", binContents}, + }), + wantErrContains: "illegal UNC path", + }, + { + name: "tar.gz empty entry name rejected", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGzEntries(t, []tarEntry{ + {name: hostileEmptyName, body: hostilePayload}, + {name: "grant", body: binContents}, + }), + wantErrContains: "empty name", + }, + { + name: "zip empty entry name rejected", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildZipEntries(t, []zipEntry{ + {name: hostileEmptyName, body: hostilePayload}, + {name: "grant.exe", body: binContents}, + }), + wantErrContains: "empty name", + }, + { + name: "nested binary is not selected", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGz(t, [][2]string{{"nested/dir/grant", hostilePayload}}), + wantErrContains: "does not contain a grant binary", }, { name: "dot-slash prefixed binary is accepted", @@ -369,7 +443,7 @@ func TestExtractBinary(t *testing.T) { {"grant", binContents}, {"grant.exe", "a different binary"}, }), - wantErr: true, + wantErrContains: "more than one grant binary", }, { name: "zip with two candidate binaries rejected", @@ -378,46 +452,51 @@ func TestExtractBinary(t *testing.T) { {"grant.exe", binContents}, {"grant", "a different binary"}, }), - wantErr: true, + wantErrContains: "more than one grant binary", }, { - name: "tar.gz without binary", - assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", - archive: buildTarGz(t, [][2]string{{"README.md", "nope"}}), - wantErr: true, + name: "tar.gz without binary", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildTarGz(t, [][2]string{{"README.md", "nope"}}), + wantErrContains: "does not contain a grant binary", }, { - name: "zip without binary", - assetName: "grant-cli_0.7.0_windows_amd64.zip", - archive: buildZip(t, [][2]string{{"README.md", "nope"}}), - wantErr: true, + name: "zip without binary", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildZip(t, [][2]string{{"README.md", "nope"}}), + wantErrContains: "does not contain a grant binary", }, { - name: "corrupt gzip", - assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", - archive: []byte("not a gzip stream"), - wantErr: true, + name: "corrupt gzip", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: []byte("not a gzip stream"), + wantErrContains: "failed to open gzip stream", }, { - name: "corrupt zip", - assetName: "grant-cli_0.7.0_windows_amd64.zip", - archive: []byte("not a zip file"), - wantErr: true, + name: "corrupt zip", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: []byte("not a zip file"), + wantErrContains: "failed to open zip archive", }, { - name: "unknown archive format", - assetName: "grant-cli_0.7.0_linux_amd64.rar", - archive: []byte("whatever"), - wantErr: true, + // Kills a default arm that falls through to tar.gz: that would + // fail on the gzip header instead, with a different message. + name: "unknown archive format", + assetName: "grant-cli_0.7.0_linux_amd64.rar", + archive: []byte("whatever"), + wantErrContains: "unsupported archive format", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := extractBinary(tt.archive, tt.assetName) - if tt.wantErr { + if tt.wantErrContains != "" { if err == nil { - t.Fatalf("expected error, got %d bytes", len(got)) + t.Fatalf("expected an error containing %q, got %d bytes", tt.wantErrContains, len(got)) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) } return } @@ -453,6 +532,66 @@ func newFixtureServer(t *testing.T, archiveName string, archive, checksums []byt return srv } +// fixtureServerOpts configures newFixtureServerWith. A nil handler keeps the +// default (the same behavior newFixtureServer provides); releaseBody replaces +// only the JSON body while keeping the default 200 handler. +type fixtureServerOpts struct { + archiveName string + archive []byte + checksums []byte + + // releaseBody receives the server URL and returns the releases/latest + // body. Ignored when releaseHandler is set. + releaseBody func(srvURL string) string + + releaseHandler http.HandlerFunc + archiveHandler http.HandlerFunc + checksumsHandler http.HandlerFunc +} + +// newFixtureServerWith is newFixtureServer with per-path handler overrides, so +// tests can drive a non-200 on either download, an empty body, or a release +// payload the happy path never produces. It is a sibling rather than a change +// to newFixtureServer's signature: the simple form has three call sites that +// have no interest in any of this. +func newFixtureServerWith(t *testing.T, opts fixtureServerOpts) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + release := opts.releaseHandler + if release == nil { + release = func(w http.ResponseWriter, r *http.Request) { + if opts.releaseBody != nil { + fmt.Fprint(w, opts.releaseBody(srv.URL)) + return + } + fmt.Fprintf(w, `{"tag_name":"v0.7.0","assets":[ + {"name":"checksums.txt","browser_download_url":"%[1]s/download/checksums.txt"}, + {"name":%[2]q,"browser_download_url":"%[1]s/download/%[2]s"} + ]}`, srv.URL, opts.archiveName) + } + } + checksums := opts.checksumsHandler + if checksums == nil { + checksums = func(w http.ResponseWriter, r *http.Request) { + w.Write(opts.checksums) //nolint:errcheck // test server + } + } + archive := opts.archiveHandler + if archive == nil { + archive = func(w http.ResponseWriter, r *http.Request) { + w.Write(opts.archive) //nolint:errcheck // test server + } + } + + mux.HandleFunc("/repos/aaearon/grant-cli/releases/latest", release) + mux.HandleFunc("/download/checksums.txt", checksums) + mux.HandleFunc("/download/"+opts.archiveName, archive) + return srv +} + func checksumsFor(name string, data []byte) []byte { sum := sha256.Sum256(data) return fmt.Appendf(nil, "%s %s\n", hex.EncodeToString(sum[:]), name) @@ -565,11 +704,130 @@ func TestUpdateSelf(t *testing.T) { }) } +// TestFetchLatestReleaseRejectsBadPayloads covers the two release-payload +// failures the happy path cannot reach. Both assert the SPECIFIC message: an +// empty body also produces an empty tag_name, so a test that only demanded +// "some error" would pass with the JSON error check deleted. +func TestFetchLatestReleaseRejectsBadPayloads(t *testing.T) { + tests := []struct { + name string + body string + wantErrContains string + }{ + { + name: "empty body", + body: "", + wantErrContains: "failed to decode GitHub release response", + }, + { + name: "empty tag_name", + body: `{"tag_name":"","assets":[]}`, + wantErrContains: "no tag_name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := tt.body + srv := newFixtureServerWith(t, fixtureServerOpts{ + archiveName: "grant-cli_0.7.0_linux_amd64.tar.gz", + releaseBody: func(string) string { return body }, + }) + + u := New("aaearon/grant-cli", "v0.6.1") + u.apiBaseURL = srv.URL + + rel, err := u.fetchLatestRelease(t.Context()) + if err == nil { + t.Fatalf("expected an error, got release %+v", rel) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + } + }) + } +} + +// TestUpdateSelfFailsOnAssetDownloadStatus covers a non-200 on each of the two +// asset downloads. Neither is reachable through the release-lookup fixture, +// and both must abort before anything is applied. +func TestUpdateSelfFailsOnAssetDownloadStatus(t *testing.T) { + archiveName := "grant-cli_0.7.0_linux_amd64.tar.gz" + archive := buildTarGz(t, [][2]string{{"grant", "new binary"}}) + + tests := []struct { + name string + mutate func(o *fixtureServerOpts) + wantErrContains string + }{ + { + name: "archive download returns 500", + mutate: func(o *fixtureServerOpts) { + o.archiveHandler = func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + } + }, + // The status must be named. A 500 with an empty body also trips + // the empty-download check, so asserting only the "failed to + // download " wrapper would pass with the status check + // deleted. + wantErrContains: "failed to download " + archiveName + ": download returned status 500", + }, + { + name: "checksums download returns 404", + mutate: func(o *fixtureServerOpts) { + o.checksumsHandler = func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + } + }, + wantErrContains: "failed to download checksums.txt: download returned status 404", + }, + { + name: "archive download returns an empty body", + mutate: func(o *fixtureServerOpts) { + o.archiveHandler = func(w http.ResponseWriter, r *http.Request) {} + }, + wantErrContains: "failed to download " + archiveName + ": download was empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := fixtureServerOpts{ + archiveName: archiveName, + archive: archive, + checksums: checksumsFor(archiveName, archive), + } + tt.mutate(&opts) + srv := newFixtureServerWith(t, opts) + + u := New("aaearon/grant-cli", "v0.6.1") + u.apiBaseURL = srv.URL + u.goos, u.goarch = "linux", "amd64" + + applied := false + u.applyFn = func([]byte) error { applied = true; return nil } + + _, _, err := u.UpdateSelf(t.Context(), "0.6.1") + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + } + if applied { + t.Error("a failed download must never reach the apply step") + } + }) + } +} + // withMaxDownloadBytes shrinks the download/decompression cap for one test and // restores it via t.Cleanup, so an early t.Fatal cannot leak the change into // another test. maxDownloadBytes is package-global mutable state: callers of -// this helper MUST NOT call t.Parallel(). -func withMaxDownloadBytes(t *testing.T, limit int64) { +// this helper MUST NOT call t.Parallel(). It takes testing.TB so the fuzz +// targets can bound each exec through the same seam (see fuzz_test.go). +func withMaxDownloadBytes(t testing.TB, limit int64) { t.Helper() orig := maxDownloadBytes maxDownloadBytes = limit @@ -619,14 +877,18 @@ func TestExtractBinaryRejectsOversizedEntry(t *testing.T) { withMaxDownloadBytes(t, 64) big := strings.Repeat("A", 300) + // The declared-size pre-filters must be what rejects these, not readCapped + // downstream: "declares" is the pre-filter's wording, "exceeds" is + // readCapped's. Asserting the exact wording is what keeps the pre-filters + // from being deleted as redundant. t.Run("tar.gz", func(t *testing.T) { archive := buildTarGz(t, [][2]string{{"grant", big}}) got, err := extractBinary(archive, "grant-cli_0.7.0_linux_amd64.tar.gz") if err == nil { t.Fatalf("expected error, got %d bytes", len(got)) } - if !strings.Contains(err.Error(), "limit") { - t.Errorf("error should mention the limit: %v", err) + if !strings.Contains(err.Error(), "declares 300 bytes, over the 64 byte limit") { + t.Errorf("error should be the tar declared-size rejection: %v", err) } }) @@ -636,8 +898,8 @@ func TestExtractBinaryRejectsOversizedEntry(t *testing.T) { if err == nil { t.Fatalf("expected error, got %d bytes", len(got)) } - if !strings.Contains(err.Error(), "limit") { - t.Errorf("error should mention the limit: %v", err) + if !strings.Contains(err.Error(), "declares 300 bytes, over the 64 byte limit") { + t.Errorf("error should be the zip declared-size rejection: %v", err) } }) @@ -654,9 +916,16 @@ func TestExtractBinaryRejectsOversizedEntry(t *testing.T) { }) } -// TestExtractBinaryRejectsTruncatedEntry covers a tar whose header declares -// more bytes than the stream actually carries. -func TestExtractBinaryRejectsTruncatedEntry(t *testing.T) { +// TestExtractBinaryRejectsTruncatedArchive covers a gzip stream that is cut +// short mid-entry. +// +// It pins GZIP-STREAM truncation, and nothing else. In particular it does NOT +// cover the `len(data) != hdr.Size` cross-check in extractFromTarGz: that +// branch is unreachable, because a stream that runs out early fails inside +// readCapped with io.ErrUnexpectedEOF long before the comparison. The +// cross-check is retained as defense in depth with no coverage claimed - see +// the comment on it in selfupdate.go. +func TestExtractBinaryRejectsTruncatedArchive(t *testing.T) { var buf bytes.Buffer gz := gzip.NewWriter(&buf) tw := tar.NewWriter(gz) diff --git a/internal/selfupdate/version_test.go b/internal/selfupdate/version_test.go index 4fc3840..baee876 100644 --- a/internal/selfupdate/version_test.go +++ b/internal/selfupdate/version_test.go @@ -3,15 +3,21 @@ package selfupdate import ( "math" "strconv" + "strings" "testing" ) func TestParseVersion(t *testing.T) { + // wantErrContains, not wantErr: several rejections are interchangeable as + // "an error happened" but come from different guards. In particular the + // core MAJOR.MINOR.PATCH loop checks isAllDigits before calling + // strconv.Atoi, and Atoi would reject almost the same inputs - only the + // message distinguishes the two, so only the message can pin the guard. tests := []struct { - name string - input string - want Version - wantErr bool + name string + input string + want Version + wantErrContains string // empty means the version must parse }{ {name: "plain", input: "1.2.3", want: Version{Major: 1, Minor: 2, Patch: 3}}, {name: "lowercase v prefix", input: "v1.2.3", want: Version{Major: 1, Minor: 2, Patch: 3}}, @@ -31,30 +37,41 @@ func TestParseVersion(t *testing.T) { {name: "prerelease and build", input: "1.2.3-rc.1+sha.abc123", want: Version{Major: 1, Minor: 2, Patch: 3, Prerelease: "rc.1", Build: "sha.abc123"}}, {name: "build metadata containing hyphen", input: "1.2.3+build-5", want: Version{Major: 1, Minor: 2, Patch: 3, Build: "build-5"}}, - {name: "empty", input: "", wantErr: true}, - {name: "too few parts", input: "1.2", wantErr: true}, - {name: "too many parts", input: "1.2.3.4", wantErr: true}, - {name: "non numeric", input: "1.x.3", wantErr: true}, - {name: "negative", input: "1.-2.3", wantErr: true}, - {name: "just v", input: "v", wantErr: true}, - {name: "leading zero in core", input: "01.2.3", wantErr: true}, - {name: "leading zero in patch", input: "1.2.03", wantErr: true}, - {name: "empty prerelease", input: "1.2.3-", wantErr: true}, - {name: "empty prerelease identifier", input: "1.2.3-rc..1", wantErr: true}, - {name: "leading zero in numeric prerelease", input: "1.2.3-01", wantErr: true}, - {name: "illegal prerelease character", input: "1.2.3-rc_1", wantErr: true}, - {name: "empty build metadata", input: "1.2.3+", wantErr: true}, - {name: "illegal build character", input: "1.2.3+build_5", wantErr: true}, - {name: "plus signed core", input: "1.+2.3", wantErr: true}, + {name: "empty", input: "", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + {name: "too few parts", input: "1.2", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + {name: "too many parts", input: "1.2.3.4", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + // These two are what pin the isAllDigits guard in the core loop: + // without it strconv.Atoi rejects them too, but with its own wording. + {name: "non numeric", input: "1.x.3", wantErrContains: `"x" is not a non-negative integer`}, + {name: "empty core segment", input: "1.2.", wantErrContains: `"" is not a non-negative integer`}, + {name: "negative", input: "1.-2.3", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + {name: "just v", input: "v", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + {name: "leading zero in core", input: "01.2.3", wantErrContains: "has a leading zero"}, + {name: "leading zero in patch", input: "1.2.03", wantErrContains: "has a leading zero"}, + {name: "empty prerelease", input: "1.2.3-", wantErrContains: "empty pre-release"}, + {name: "empty prerelease identifier", input: "1.2.3-rc..1", wantErrContains: "empty identifier in pre-release"}, + {name: "leading zero in numeric prerelease", input: "1.2.3-01", wantErrContains: "has a leading zero"}, + {name: "illegal prerelease character", input: "1.2.3-rc_1", wantErrContains: "illegal character in pre-release"}, + {name: "empty build metadata", input: "1.2.3+", wantErrContains: "empty build metadata"}, + {name: "illegal build character", input: "1.2.3+build_5", wantErrContains: "illegal character in build metadata"}, + // A "+" is split off as build metadata BEFORE the core is parsed, so + // these are rejected for having two core segments, not for the sign. + // They therefore do NOT exercise the isAllDigits guard, despite Atoi + // being happy to read "+2" as 2 - the cases above are what do. + {name: "plus signed core", input: "1.+2.3", wantErrContains: "expected MAJOR.MINOR.PATCH"}, + {name: "plus signed core alternate", input: "1.+5.3", wantErrContains: "expected MAJOR.MINOR.PATCH"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := ParseVersion(tt.input) - if tt.wantErr { + if tt.wantErrContains != "" { if err == nil { t.Fatalf("expected error for %q, got %+v", tt.input, got) } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("ParseVersion(%q) = %q, want it to contain %q", tt.input, err, tt.wantErrContains) + } return } if err != nil { @@ -128,7 +145,11 @@ func TestCompareVersions(t *testing.T) { // SemVer 2.0.0 precedence rules. {name: "prerelease sorts before release", a: "1.0.0-rc.1", b: "1.0.0", want: -1}, {name: "release sorts after prerelease", a: "1.0.0", b: "1.0.0-rc.1", want: 1}, + // Both directions. One direction alone is not enough: a numeric + // comparison that always returns -1 satisfies the first case by + // coincidence, because the numeric-vs-alphanumeric arm returns -1 too. {name: "numeric prerelease compares numerically", a: "1.0.0-rc.2", b: "1.0.0-rc.10", want: -1}, + {name: "numeric prerelease compares numerically mirrored", a: "1.0.0-rc.10", b: "1.0.0-rc.2", want: 1}, {name: "numeric identifier below alphanumeric", a: "1.0.0-1", b: "1.0.0-alpha", want: -1}, {name: "alpha before beta", a: "1.0.0-alpha", b: "1.0.0-beta", want: -1}, {name: "larger identifier set wins", a: "1.0.0-alpha", b: "1.0.0-alpha.1", want: -1},