From b13dbf12fd599c0ac02d7e3db8c78e90d7894bee Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:05:27 +0200 Subject: [PATCH 1/6] test(selfupdate): pin archive extraction and path guards Close the archive-extraction findings from the mutation audit (SFU-01..09, SFU-20..22). Tests - buildTarGzEntries/buildZipEntries give full control over type flag, link name and declared size; buildTarGz/buildZip become thin wrappers. - TestExtractBinary switches to wantErrContains and puts a valid "grant" beside every hostile entry, so a rejection can never be attributed to the archive simply not containing a binary. - New TestCheckArchivePath pins one diagnostic per guard, including the previously missing empty-name, lowercase and forward-slash drive forms and the backslash traversal/UNC forms. - TestExtractBinaryRejectsNonRegularEntries covers symlink, hardlink and directory entries named "grant" in tar plus a zip directory entry. - Native fuzz targets for checkArchivePath and both extractors, seeded with the hostile cases; extended fuzzing stays out of CI. - TestExtractBinaryRejectsTruncatedEntry renamed to ...TruncatedArchive: it pins gzip-stream truncation, not the unreachable size cross-checks. Production - Refuse a zero-length extracted binary, and again at the apply boundary: the checksum covers the archive, not the extracted bytes, so an empty payload verifies against itself. - Check the normalized "//" prefix before path.Clean/path.IsAbs, which collapses UNC paths and made that arm unreachable. Rejection unchanged. --- CHANGELOG.md | 4 + CLAUDE.md | 6 + docs/mutation-ledger.md | 24 +- internal/selfupdate/apply.go | 6 + internal/selfupdate/apply_test.go | 23 ++ internal/selfupdate/archive_security_test.go | 342 +++++++++++++++++++ internal/selfupdate/fuzz_test.go | 139 ++++++++ internal/selfupdate/selfupdate.go | 28 +- internal/selfupdate/selfupdate_test.go | 246 ++++++++----- 9 files changed, 718 insertions(+), 100 deletions(-) create mode 100644 internal/selfupdate/archive_security_test.go create mode 100644 internal/selfupdate/fuzz_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f5229a0..fec9dda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ 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 + ### Fixed - `grant favorites add` now fails immediately without a terminal instead of authenticating first diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..b3241ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,6 +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 + - **Zero-length binaries are refused twice**, in `extractBinary` and again at the apply boundary (`applyBinaryTo`). The checksum covers the *archive*, not the extracted bytes, and `applyBinary` hashes whatever it is handed — so an empty payload verifies against itself and would replace a working binary with nothing. The extractor's type/size guards are the real fix; these two checks are backstops, which is why the non-regular-entry tests stay type-specific + - **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. Specificity before generality; the rejection is identical either way, only the diagnostic differs + - **Security fixtures**: `internal/selfupdate/archive_security_test.go` builds deliberately malformed archives (absolute, drive-absolute, UNC, `..`, empty-name, non-regular and oversized entries) entirely in memory and asserts REJECTION — nothing is written, linked, followed or executed. `buildTarGzEntries`/`buildZipEntries` are the full-control builders (`buildHostileTarGz`/`buildHostileZip` are the same thing under an intent-revealing name); `buildTarGz`/`buildZip` are the well-formed-file wrappers. Hostile literals live in `hostile*` constants. `TestExtractBinary` uses `wantErrContains`, not `wantErr bool`, and every hostile entry sits beside a valid `grant` — otherwise the "no binary" fallback, not the guard under test, is what produces the error + - `internal/selfupdate/fuzz_test.go` has native fuzz targets for `checkArchivePath` and both extractors, asserting contracts (nothing accepted escapes; a successful extraction is never empty). Their seed corpora run under a normal `go test`; extended fuzzing is run by hand (`-fuzz=FuzzCheckArchivePath -fuzztime=30s`), never in CI + - 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. Pinned by `TestExtractFromZipIgnoresOversizeDecoy` + - The `len(data) != hdr.Size` / `!= f.UncompressedSize64` cross-checks are **unreachable** (a successful `readCapped` returns exactly the declared size; early exhaustion fails as `io.ErrUnexpectedEOF`). Kept as defense in depth with **no coverage claimed** — `TestExtractBinaryRejectsTruncatedArchive` pins gzip-stream truncation, not those branches - 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` - **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)` diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..f60d0a9 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -152,15 +152,15 @@ 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-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). Reverified: with the mutation the type-specific cases fail on the *message* (`grant in archive is empty` instead of `does not contain a grant binary`), because the new empty-binary backstop catches the bytes while the type tests catch the classification — exactly why both are kept. **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 | `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 | @@ -171,9 +171,9 @@ premise does not hold). | 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-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 | | 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..a2aaa00 100644 --- a/internal/selfupdate/apply.go +++ b/internal/selfupdate/apply.go @@ -48,6 +48,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, diff --git a/internal/selfupdate/apply_test.go b/internal/selfupdate/apply_test.go index 30083e2..3fc12de 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) { diff --git a/internal/selfupdate/archive_security_test.go b/internal/selfupdate/archive_security_test.go new file mode 100644 index 0000000..f52e6cb --- /dev/null +++ b/internal/selfupdate/archive_security_test.go @@ -0,0 +1,342 @@ +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. Every case asserts REJECTION: the test fails if grant accepts +// the archive. Their sole purpose is to make a regression in those guards fail +// the build. + +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 turns a +// zero-length symlink, hardlink or directory entry named "grant" into a +// successful extraction of ZERO bytes. Nothing downstream catches that — the +// checksum covers the archive, not the extracted binary, and applyBinary +// hashes whatever it is handed, so an empty payload verifies against itself +// and self-destructs the installed binary. +// +// The fixtures carry no body, so extraction can only ever produce empty bytes; +// they are never written, linked or followed. +func TestExtractBinaryRejectsNonRegularEntries(t *testing.T) { + tests := []struct { + name string + assetName string + archive []byte + wantErrContains string + }{ + { + name: "tar symlink named grant", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant", typeflag: tar.TypeSymlink, linkname: hostileSymlinkTarget}, + }), + wantErrContains: "does not contain a grant binary", + }, + { + name: "tar hardlink named grant", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "other", body: hostilePayload}, + {name: "grant", typeflag: tar.TypeLink, linkname: "other"}, + }), + wantErrContains: "does not contain a grant binary", + }, + { + name: "tar directory named grant", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant/", typeflag: tar.TypeDir}, + }), + wantErrContains: "does not contain a grant binary", + }, + { + name: "zip directory named grant", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildHostileZip(t, []zipEntry{ + {name: "grant/", mode: fs.ModeDir | 0o755}, + }), + wantErrContains: "does not contain a grant binary", + }, + } + + 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(), tt.wantErrContains) { + t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + } + }) + } +} + +// 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. +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..46f2617 --- /dev/null +++ b/internal/selfupdate/fuzz_test.go @@ -0,0 +1,139 @@ +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" +) + +// FuzzCheckArchivePath asserts the guard's CONTRACT rather than its wording: +// any name it accepts must be relative, non-escaping and not Windows-absolute. +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 hasDriveLetter(normalized): + t.Fatalf("accepted a drive-absolute path: %q", name) + case cleaned == ".." || strings.HasPrefix(cleaned, "../"): + t.Fatalf("accepted a traversal path: %q", name) + } + }) +} + +// 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) { + 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) { + 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..c4ec4eb 100644 --- a/internal/selfupdate/selfupdate.go +++ b/internal/selfupdate/selfupdate.go @@ -342,10 +342,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 +403,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 { @@ -434,9 +450,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..d119028 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" @@ -229,59 +228,49 @@ func TestVerifyChecksum(t *testing.T) { } } -// 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 +295,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}, + }), + 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}, }), - wantErr: true, + 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}, + }), + 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}, }), - wantErr: true, + 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 +427,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 +436,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 } @@ -619,14 +682,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 +703,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 +721,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) From 5a820a90c7ee58f375862333a925de5f9d56d086 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:11:26 +0200 Subject: [PATCH 2/6] test(selfupdate): cover sync, recovery detection and release fetch failures Close the remaining self-update findings (SFU-10..19). No behaviour change. - syncStagedFileFn seam (test-only): pins that the staged file is fsynced strictly before commit, and that a sync failure aborts before minio renames anything. syncStagedFile's own error propagation is exercised on Unix via a FIFO staged path, since fsync on a FIFO fails; skipped on Windows. - InterruptedUpdate: target present with a leftover .old backup is the documented Windows steady state and must not be reported as interrupted. - newFixtureServerWith(t, opts) adds per-path handler overrides beside the existing newFixtureServer, covering non-200 on both asset downloads, an empty download body, an empty release body and an empty tag_name. - Version parser: the mirrored numeric pre-release comparison, and message assertions that pin the core all-digits guard against strconv.Atoi. "1.+5.3" is documented as NOT reaching that guard: "+" is split off as build metadata first. - verifyChecksum rejects malformed lines with one and with three fields. - extractBinary's unsupported-format arm asserts its own message. --- CLAUDE.md | 2 +- docs/mutation-ledger.md | 20 +- internal/selfupdate/apply.go | 8 +- internal/selfupdate/apply_test.go | 97 ++++++++++ internal/selfupdate/apply_unix_test.go | 30 +++ internal/selfupdate/selfupdate_test.go | 252 ++++++++++++++++++++++--- internal/selfupdate/version_test.go | 61 ++++-- 7 files changed, 409 insertions(+), 61 deletions(-) create mode 100644 internal/selfupdate/apply_unix_test.go diff --git a/CLAUDE.md b/CLAUDE.md index b3241ce..1a5703d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -114,7 +114,7 @@ Custom `SCAAccessService` follows SDK conventions: - `internal/selfupdate/fuzz_test.go` has native fuzz targets for `checkArchivePath` and both extractors, asserting contracts (nothing accepted escapes; a successful extraction is never empty). Their seed corpora run under a normal `go test`; extended fuzzing is run by hand (`-fuzz=FuzzCheckArchivePath -fuzztime=30s`), never in CI - 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. Pinned by `TestExtractFromZipIgnoresOversizeDecoy` - The `len(data) != hdr.Size` / `!= f.UncompressedSize64` cross-checks are **unreachable** (a successful `readCapped` returns exactly the declared size; early exhaustion fails as `io.ErrUnexpectedEOF`). Kept as defense in depth with **no coverage claimed** — `TestExtractBinaryRejectsTruncatedArchive` pins gzip-stream truncation, not those branches - - 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` + - 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** — `syncStagedFile` already returns `f.Sync()` errors and `applyWithOptions` already aborts before commit on them; the seam exists to assert the call *order* and the abort. The real `f.Sync()` failure is exercised on Unix only, via a FIFO staged path (`fsync` on a FIFO returns `EINVAL`); there is no portable equivalent, so that test is skipped on Windows - **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 f60d0a9..6768dd6 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -161,16 +161,16 @@ premise does not hold). | 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). Reverified: with the mutation the type-specific cases fail on the *message* (`grant in archive is empty` instead of `does not contain a grant binary`), because the new empty-binary backstop catches the bytes while the type tests catch the classification — exactly why both are kept. **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 | `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-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; skipped on Windows, where no portable equivalent exists | 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 | diff --git a/internal/selfupdate/apply.go b/internal/selfupdate/apply.go index a2aaa00..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. @@ -77,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 3fc12de..d37575c 100644 --- a/internal/selfupdate/apply_test.go +++ b/internal/selfupdate/apply_test.go @@ -343,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 @@ -375,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..f5699b4 --- /dev/null +++ b/internal/selfupdate/apply_unix_test.go @@ -0,0 +1,30 @@ +//go:build !windows + +package selfupdate + +import ( + "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) }) + + if err := syncStagedFile(target); err == nil { + t.Fatal("expected the failing fsync to be reported, got nil") + } +} diff --git a/internal/selfupdate/selfupdate_test.go b/internal/selfupdate/selfupdate_test.go index d119028..92dc139 100644 --- a/internal/selfupdate/selfupdate_test.go +++ b/internal/selfupdate/selfupdate_test.go @@ -153,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", @@ -186,43 +189,56 @@ 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) } }) } @@ -516,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) @@ -628,6 +704,124 @@ 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 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}, From 77a462e12f27202919ae81ceee21840149722050 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:13:56 +0200 Subject: [PATCH 3/6] fix(selfupdate): reject non-regular zip entries and harden the archive tests Adversarial review of PR2+PR3 found four gaps in the extraction guards and their tests, plus doc nits. - extractFromZip filtered only on IsDir(), so a zip entry carrying fs.ModeSymlink named grant.exe was accepted in production and extracted the link-target string as the binary. Not exploitable (extraction is in-memory, the link is never followed, and the bytes are checksum-gated either way), but it was an undocumented tar/zip asymmetry. Now mirrors the tar typeflag guard. - TestExtractBinaryRejectsNonRegularEntries only pinned the error MESSAGE: all its fixtures are header-only tar types, whose bodies Go forces to zero length, so the empty-binary backstop caught them first. Added tar.TypeCont and vendor type 'Z' cases, which have readable bodies and therefore fail on the bytes returned - a behavioral pin for the whole class. - The FIFO fsync test asserted only err != nil, so an open failure would make it pass with the mutation applied. It now asserts EINVAL/ENOTSUP. - Both archive fuzz targets now shrink maxDownloadBytes to 64 KiB. FuzzExtractFromZip previously collapsed to 0 exec/sec while still reporting PASS, because readCapped can io.ReadAll 128 MiB per exec. Docs: CLAUDE.md corrected on the build-exclusion vs skip wording, the type-specific claim, and the backstop's residual gap; mutation ledger updated for SFU-07 and SFU-11 and gains SFU-23. --- CHANGELOG.md | 1 + CLAUDE.md | 7 ++-- docs/mutation-ledger.md | 5 ++- internal/selfupdate/apply_unix_test.go | 11 ++++- internal/selfupdate/archive_security_test.go | 42 +++++++++++++++++++- internal/selfupdate/fuzz_test.go | 13 ++++++ internal/selfupdate/selfupdate.go | 8 +++- internal/selfupdate/selfupdate_test.go | 5 ++- 8 files changed, 81 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fec9dda..a66848c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. ### 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 diff --git a/CLAUDE.md b/CLAUDE.md index 1a5703d..00f4977 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,13 +108,14 @@ 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 - - **Zero-length binaries are refused twice**, in `extractBinary` and again at the apply boundary (`applyBinaryTo`). The checksum covers the *archive*, not the extracted bytes, and `applyBinary` hashes whatever it is handed — so an empty payload verifies against itself and would replace a working binary with nothing. The extractor's type/size guards are the real fix; these two checks are backstops, which is why the non-regular-entry tests stay type-specific + - **Zero-length binaries are refused twice**, in `extractBinary` and again at the apply boundary (`applyBinaryTo`). The checksum covers the *archive*, not the extracted bytes, and `applyBinary` hashes whatever it is handed — so an empty payload verifies against itself and would replace a working binary with nothing. The extractor's type/size guards are the real fix; these two checks are backstops with a **known gap**: they do not cover a non-regular entry that carries *non-empty* bytes. Go's `tar.Reader` forces a zero-length body only for header-only types (symlink, hardlink, dir, char, block, fifo); `tar.TypeCont` and the vendor types `'A'..'Z'` have readable bodies, so with the type guard removed they would extract attacker-chosen bytes and the zero-length backstop would never fire. That is precisely why the type guard must never be removed, and why `TestExtractBinaryRejectsNonRegularEntries` includes content-carrying cases (`tar.TypeCont`, vendor `'Z'`) that fail on the *bytes returned* rather than on the error wording — the header-only fixtures alone would pin only the message - **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. Specificity before generality; the rejection is identical either way, only the diagnostic differs - **Security fixtures**: `internal/selfupdate/archive_security_test.go` builds deliberately malformed archives (absolute, drive-absolute, UNC, `..`, empty-name, non-regular and oversized entries) entirely in memory and asserts REJECTION — nothing is written, linked, followed or executed. `buildTarGzEntries`/`buildZipEntries` are the full-control builders (`buildHostileTarGz`/`buildHostileZip` are the same thing under an intent-revealing name); `buildTarGz`/`buildZip` are the well-formed-file wrappers. Hostile literals live in `hostile*` constants. `TestExtractBinary` uses `wantErrContains`, not `wantErr bool`, and every hostile entry sits beside a valid `grant` — otherwise the "no binary" fallback, not the guard under test, is what produces the error - - `internal/selfupdate/fuzz_test.go` has native fuzz targets for `checkArchivePath` and both extractors, asserting contracts (nothing accepted escapes; a successful extraction is never empty). Their seed corpora run under a normal `go test`; extended fuzzing is run by hand (`-fuzz=FuzzCheckArchivePath -fuzztime=30s`), never in CI + - `internal/selfupdate/fuzz_test.go` has native fuzz targets for `checkArchivePath` and both extractors, asserting contracts (nothing accepted escapes; a successful extraction is never empty). Their seed corpora run under a normal `go test`; extended fuzzing is run by hand (`-fuzz=FuzzCheckArchivePath -fuzztime=30s`), never in CI. Both archive targets shrink `maxDownloadBytes` to `fuzzMaxDownloadBytes` (64 KiB) through `withMaxDownloadBytes` — with the production 128 MiB cap, `readCapped` can `io.ReadAll` that much per exec and `FuzzExtractFromZip` collapses to **0 exec/sec while still reporting `PASS`**, making its exec count worthless. The cap keeps the fuzzer on archive *shape* rather than *size*. `withMaxDownloadBytes` takes `testing.TB` for this reason + - **Non-regular entries are rejected symmetrically**: tar filters on `hdr.Typeflag != tar.TypeReg`, zip on `f.Mode()&fs.ModeType != 0`. `IsDir()` alone was not enough — a zip entry with `fs.ModeSymlink` named `grant.exe` was accepted, extracting the link-target string as the binary. Never exploitable (extraction is in-memory and never follows a link, and the bytes are attacker-chosen either way), but 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. Pinned by `TestExtractFromZipIgnoresOversizeDecoy` - The `len(data) != hdr.Size` / `!= f.UncompressedSize64` cross-checks are **unreachable** (a successful `readCapped` returns exactly the declared size; early exhaustion fails as `io.ErrUnexpectedEOF`). Kept as defense in depth with **no coverage claimed** — `TestExtractBinaryRejectsTruncatedArchive` pins gzip-stream truncation, not those branches - - 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** — `syncStagedFile` already returns `f.Sync()` errors and `applyWithOptions` already aborts before commit on them; the seam exists to assert the call *order* and the abort. The real `f.Sync()` failure is exercised on Unix only, via a FIFO staged path (`fsync` on a FIFO returns `EINVAL`); there is no portable equivalent, so that test is skipped on Windows + - 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** — `syncStagedFile` already returns `f.Sync()` errors and `applyWithOptions` already aborts before commit on them; the seam exists to assert the call *order* and the abort. The real `f.Sync()` failure is exercised on Unix only, via a FIFO staged path (`fsync` on a FIFO returns `EINVAL`); there is no portable equivalent, so that file is **build-excluded** on Windows (`//go:build !windows`, not `t.Skip` — the `t.Skipf` inside fires only when `mkfifo` itself is unavailable). The test asserts the error *is* `EINVAL`/`ENOTSUP`, not merely non-nil: without that, a Unix where `os.OpenFile(fifo, O_RDWR)` fails would return the **open** error, the test would still pass, and the mutation would silently survive - **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 6768dd6..e49e080 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -158,11 +158,11 @@ premise does not hold). | 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). Reverified: with the mutation the type-specific cases fail on the *message* (`grant in archive is empty` instead of `does not contain a grant binary`), because the new empty-binary backstop catches the bytes while the type tests catch the classification — exactly why both are kept. **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-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; skipped on Windows, where no portable equivalent exists | 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 | @@ -174,6 +174,7 @@ premise does not hold). | 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_unix_test.go b/internal/selfupdate/apply_unix_test.go index f5699b4..8915610 100644 --- a/internal/selfupdate/apply_unix_test.go +++ b/internal/selfupdate/apply_unix_test.go @@ -3,6 +3,7 @@ package selfupdate import ( + "errors" "os" "path/filepath" "syscall" @@ -24,7 +25,15 @@ func TestSyncStagedFileReportsSyncError(t *testing.T) { } t.Cleanup(func() { _ = os.Remove(staged) }) - if err := syncStagedFile(target); err == nil { + // 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 index f52e6cb..7ace440 100644 --- a/internal/selfupdate/archive_security_test.go +++ b/internal/selfupdate/archive_security_test.go @@ -209,8 +209,18 @@ func TestCheckArchivePath(t *testing.T) { // hashes whatever it is handed, so an empty payload verifies against itself // and self-destructs the installed binary. // -// The fixtures carry no body, so extraction can only ever produce empty bytes; -// they are never written, linked or followed. +// Most fixtures carry no body, so extraction could only ever produce empty +// bytes; they are never written, linked or followed. Those cases pin the +// classification, but with the type operand dropped they fail on the error +// MESSAGE (the empty-binary backstop catches them and says "is empty"), not on +// behavior — because Go's tar.Reader forces a zero-length body for the +// header-only types (symlink, hardlink, dir, char, block, fifo). +// +// The `tar.TypeCont` and vendor-type cases are different, and they are the +// reason this test is a behavioral pin rather than a wording pin: those types +// are NOT header-only, so their bodies are readable. With the type operand +// dropped they extract non-empty attacker-chosen bytes and the zero-length +// backstop never fires. They must fail on the bytes returned. func TestExtractBinaryRejectsNonRegularEntries(t *testing.T) { tests := []struct { name string @@ -243,6 +253,34 @@ func TestExtractBinaryRejectsNonRegularEntries(t *testing.T) { }), wantErrContains: "does not contain a grant binary", }, + { + // Not header-only: the body is readable, so this case fails on the + // bytes returned rather than on the wording. + name: "tar continuation entry named grant carrying bytes", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant", typeflag: tar.TypeCont, body: hostilePayload}, + }), + wantErrContains: "does not contain a grant binary", + }, + { + // Vendor-reserved type flags ('A'..'Z') are likewise not + // header-only. Same behavioral pin, a different byte. + name: "tar vendor-type entry named grant carrying bytes", + assetName: "grant-cli_0.7.0_linux_amd64.tar.gz", + archive: buildHostileTarGz(t, []tarEntry{ + {name: "grant", typeflag: 'Z', body: hostilePayload}, + }), + wantErrContains: "does not contain a grant binary", + }, + { + name: "zip symlink-mode entry named grant", + assetName: "grant-cli_0.7.0_windows_amd64.zip", + archive: buildHostileZip(t, []zipEntry{ + {name: "grant.exe", mode: fs.ModeSymlink | 0o777, body: hostileSymlinkTarget}, + }), + wantErrContains: "does not contain a grant binary", + }, { name: "zip directory named grant", assetName: "grant-cli_0.7.0_windows_amd64.zip", diff --git a/internal/selfupdate/fuzz_test.go b/internal/selfupdate/fuzz_test.go index 46f2617..30799eb 100644 --- a/internal/selfupdate/fuzz_test.go +++ b/internal/selfupdate/fuzz_test.go @@ -73,6 +73,15 @@ func FuzzCheckArchivePath(f *testing.F) { }) } +// 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 @@ -91,6 +100,8 @@ func assertExtractInvariant(t *testing.T, got []byte, err error) { } func FuzzExtractFromTarGz(f *testing.F) { + withMaxDownloadBytes(f, fuzzMaxDownloadBytes) + good := buildTarGzEntries(f, []tarEntry{{name: "grant", body: fixtureBinaryContents}}) seeds := [][]byte{ good, @@ -116,6 +127,8 @@ func FuzzExtractFromTarGz(f *testing.F) { } func FuzzExtractFromZip(f *testing.F) { + withMaxDownloadBytes(f, fuzzMaxDownloadBytes) + good := buildZipEntries(f, []zipEntry{{name: "grant.exe", body: fixtureBinaryContents}}) seeds := [][]byte{ good, diff --git a/internal/selfupdate/selfupdate.go b/internal/selfupdate/selfupdate.go index c4ec4eb..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" @@ -437,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 { diff --git a/internal/selfupdate/selfupdate_test.go b/internal/selfupdate/selfupdate_test.go index 92dc139..daa3d6b 100644 --- a/internal/selfupdate/selfupdate_test.go +++ b/internal/selfupdate/selfupdate_test.go @@ -825,8 +825,9 @@ func TestUpdateSelfFailsOnAssetDownloadStatus(t *testing.T) { // 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 From e7ef36b916c05e4439344d46de76b2dcee6c8b64 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:26:56 +0200 Subject: [PATCH 4/6] test(selfupdate): pin the non-regular-entry guards behaviorally TestExtractBinaryRejectsNonRegularEntries asserted only the generic "does not contain a grant binary" fallback, which every failure mode produces: neutering isBinaryEntry to return false left the whole table passing. It could not tell the type guard from the name match from checkArchivePath. Each case now pairs the hostile entry with a valid binary under the other accepted name, and asserts that extraction SUCCEEDS and returns that binary. Removing either type guard makes both entries match and the extractor reports "archive contains more than one grant binary"; the valid binary is placed first so the zero-length backstop cannot fire before the duplicate is seen. Neutering isBinaryEntry now fails too. The rejection half of the class is kept as TestExtractBinaryRejectsNonRegularOnlyArchive, explicitly labelled as wording-shaped and carrying no mutation-killing weight of its own. Also records, on the zip oversize-decoy test, that tar's per-entry size check is not an aggregate cap: nothing bounds total inflated bytes or entry count, but verifyChecksum runs before extractBinary, so reaching it requires control of checksums.txt. --- internal/selfupdate/archive_security_test.go | 164 ++++++++++++++----- 1 file changed, 123 insertions(+), 41 deletions(-) diff --git a/internal/selfupdate/archive_security_test.go b/internal/selfupdate/archive_security_test.go index 7ace440..040b3a8 100644 --- a/internal/selfupdate/archive_security_test.go +++ b/internal/selfupdate/archive_security_test.go @@ -12,9 +12,13 @@ package selfupdate // 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. Every case asserts REJECTION: the test fails if grant accepts -// the archive. Their sole purpose is to make a regression in those guards fail +// 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" @@ -202,92 +206,159 @@ func TestCheckArchivePath(t *testing.T) { } // TestExtractBinaryRejectsNonRegularEntries pins the highest-severity finding -// in the audit: dropping the `hdr.Typeflag != tar.TypeReg` operand turns a -// zero-length symlink, hardlink or directory entry named "grant" into a -// successful extraction of ZERO bytes. Nothing downstream catches that — the -// checksum covers the archive, not the extracted binary, and applyBinary -// hashes whatever it is handed, so an empty payload verifies against itself -// and self-destructs the installed binary. +// 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. // -// Most fixtures carry no body, so extraction could only ever produce empty -// bytes; they are never written, linked or followed. Those cases pin the -// classification, but with the type operand dropped they fail on the error -// MESSAGE (the empty-binary backstop catches them and says "is empty"), not on -// behavior — because Go's tar.Reader forces a zero-length body for the -// header-only types (symlink, hardlink, dir, char, block, fifo). +// 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. // -// The `tar.TypeCont` and vendor-type cases are different, and they are the -// reason this test is a behavioral pin rather than a wording pin: those types -// are NOT header-only, so their bodies are readable. With the type operand -// dropped they extract non-empty attacker-chosen bytes and the zero-length -// backstop never fires. They must fail on the bytes returned. +// 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 - wantErrContains string + name string + assetName string + archive []byte }{ { - name: "tar symlink named grant", + 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}, }), - wantErrContains: "does not contain a grant binary", }, { - name: "tar hardlink named grant", + 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"}, }), - wantErrContains: "does not contain a grant binary", }, { - name: "tar directory named grant", + // 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}, }), - wantErrContains: "does not contain a grant binary", }, { - // Not header-only: the body is readable, so this case fails on the - // bytes returned rather than on the wording. - name: "tar continuation entry named grant carrying bytes", + // 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}, }), - wantErrContains: "does not contain a grant binary", }, { // Vendor-reserved type flags ('A'..'Z') are likewise not // header-only. Same behavioral pin, a different byte. - name: "tar vendor-type entry named grant carrying bytes", + 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}, }), - wantErrContains: "does not contain a grant binary", }, { - name: "zip symlink-mode entry named grant", + 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}, }), - wantErrContains: "does not contain a grant binary", }, { - name: "zip directory named grant", + // 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}, }), - wantErrContains: "does not contain a grant binary", + }, + } + + 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}, + }), }, } @@ -297,8 +368,11 @@ func TestExtractBinaryRejectsNonRegularEntries(t *testing.T) { if err == nil { t.Fatalf("expected rejection, got %d bytes", len(got)) } - if !strings.Contains(err.Error(), tt.wantErrContains) { - t.Errorf("error = %q, want it to contain %q", err, tt.wantErrContains) + 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)) } }) } @@ -362,6 +436,14 @@ func TestExtractFromTarGzRejectsOversizeDecoy(t *testing.T) { // 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) From a36a3fcd4611ac42c3f47a2ae38dcc89ece219eb Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:26:56 +0200 Subject: [PATCH 5/6] test(selfupdate): make the FuzzCheckArchivePath oracle independent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property body called the production hasDriveLetter, so a defect inside it was invisible: narrowing it to uppercase-only drives survived 2.5M execs while TestCheckArchivePath/lowercase_drive caught it instantly. The oracle now uses its own inlined drive-letter predicate and the mutation fails on seed corpus entry #9. Also corrects the doc comment: the target checks a necessary condition on accepted names, not the guard's full contract — an over-eager guard passes it trivially. --- internal/selfupdate/fuzz_test.go | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/internal/selfupdate/fuzz_test.go b/internal/selfupdate/fuzz_test.go index 30799eb..1239c04 100644 --- a/internal/selfupdate/fuzz_test.go +++ b/internal/selfupdate/fuzz_test.go @@ -24,8 +24,27 @@ import ( "testing" ) -// FuzzCheckArchivePath asserts the guard's CONTRACT rather than its wording: -// any name it accepts must be relative, non-escaping and not Windows-absolute. +// 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", @@ -65,7 +84,7 @@ func FuzzCheckArchivePath(f *testing.F) { t.Fatalf("accepted a UNC path: %q", name) case path.IsAbs(cleaned): t.Fatalf("accepted an absolute path: %q", name) - case hasDriveLetter(normalized): + 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) From 00a6d1a110380a9497fd0eba7368ebac29d003e9 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:26:56 +0200 Subject: [PATCH 6/6] docs: prune the selfupdate archive notes to durable facts The archive-hardening work added eight PR-length paragraphs of test-file structure, fuzz exec-rate diagnostics and mutation-survival rationale to the grant update bullets. Per the repo's own convention that belongs in the PR description; CLAUDE.md keeps policy and architecture. Retained: the load-bearing UNC-before-path.IsAbs ordering, the deliberately symmetric tar/zip non-regular type guards, the intentional declared-size asymmetry, the fact that fuzz targets exist and that a genuine testdata/fuzz failure is committed. Adds one sentence that neither size cap is an aggregate cap, and why that is accepted. --- CLAUDE.md | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 00f4977..ea55a51 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,14 +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 - - **Zero-length binaries are refused twice**, in `extractBinary` and again at the apply boundary (`applyBinaryTo`). The checksum covers the *archive*, not the extracted bytes, and `applyBinary` hashes whatever it is handed — so an empty payload verifies against itself and would replace a working binary with nothing. The extractor's type/size guards are the real fix; these two checks are backstops with a **known gap**: they do not cover a non-regular entry that carries *non-empty* bytes. Go's `tar.Reader` forces a zero-length body only for header-only types (symlink, hardlink, dir, char, block, fifo); `tar.TypeCont` and the vendor types `'A'..'Z'` have readable bodies, so with the type guard removed they would extract attacker-chosen bytes and the zero-length backstop would never fire. That is precisely why the type guard must never be removed, and why `TestExtractBinaryRejectsNonRegularEntries` includes content-carrying cases (`tar.TypeCont`, vendor `'Z'`) that fail on the *bytes returned* rather than on the error wording — the header-only fixtures alone would pin only the message - - **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. Specificity before generality; the rejection is identical either way, only the diagnostic differs - - **Security fixtures**: `internal/selfupdate/archive_security_test.go` builds deliberately malformed archives (absolute, drive-absolute, UNC, `..`, empty-name, non-regular and oversized entries) entirely in memory and asserts REJECTION — nothing is written, linked, followed or executed. `buildTarGzEntries`/`buildZipEntries` are the full-control builders (`buildHostileTarGz`/`buildHostileZip` are the same thing under an intent-revealing name); `buildTarGz`/`buildZip` are the well-formed-file wrappers. Hostile literals live in `hostile*` constants. `TestExtractBinary` uses `wantErrContains`, not `wantErr bool`, and every hostile entry sits beside a valid `grant` — otherwise the "no binary" fallback, not the guard under test, is what produces the error - - `internal/selfupdate/fuzz_test.go` has native fuzz targets for `checkArchivePath` and both extractors, asserting contracts (nothing accepted escapes; a successful extraction is never empty). Their seed corpora run under a normal `go test`; extended fuzzing is run by hand (`-fuzz=FuzzCheckArchivePath -fuzztime=30s`), never in CI. Both archive targets shrink `maxDownloadBytes` to `fuzzMaxDownloadBytes` (64 KiB) through `withMaxDownloadBytes` — with the production 128 MiB cap, `readCapped` can `io.ReadAll` that much per exec and `FuzzExtractFromZip` collapses to **0 exec/sec while still reporting `PASS`**, making its exec count worthless. The cap keeps the fuzzer on archive *shape* rather than *size*. `withMaxDownloadBytes` takes `testing.TB` for this reason - - **Non-regular entries are rejected symmetrically**: tar filters on `hdr.Typeflag != tar.TypeReg`, zip on `f.Mode()&fs.ModeType != 0`. `IsDir()` alone was not enough — a zip entry with `fs.ModeSymlink` named `grant.exe` was accepted, extracting the link-target string as the binary. Never exploitable (extraction is in-memory and never follows a link, and the bytes are attacker-chosen either way), but 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. Pinned by `TestExtractFromZipIgnoresOversizeDecoy` - - The `len(data) != hdr.Size` / `!= f.UncompressedSize64` cross-checks are **unreachable** (a successful `readCapped` returns exactly the declared size; early exhaustion fails as `io.ErrUnexpectedEOF`). Kept as defense in depth with **no coverage claimed** — `TestExtractBinaryRejectsTruncatedArchive` pins gzip-stream truncation, not those branches - - 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** — `syncStagedFile` already returns `f.Sync()` errors and `applyWithOptions` already aborts before commit on them; the seam exists to assert the call *order* and the abort. The real `f.Sync()` failure is exercised on Unix only, via a FIFO staged path (`fsync` on a FIFO returns `EINVAL`); there is no portable equivalent, so that file is **build-excluded** on Windows (`//go:build !windows`, not `t.Skip` — the `t.Skipf` inside fires only when `mkfifo` itself is unavailable). The test asserts the error *is* `EINVAL`/`ENOTSUP`, not merely non-nil: without that, a Unix where `os.OpenFile(fifo, O_RDWR)` fails would return the **open** error, the test would still pass, and the mutation would silently survive + - **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