test(selfupdate): pin the archive path guards, reject zero-length binaries, add fuzzing - #64
Open
aaearon wants to merge 3 commits into
Open
test(selfupdate): pin the archive path guards, reject zero-length binaries, add fuzzing#64aaearon wants to merge 3 commits into
aaearon wants to merge 3 commits into
Conversation
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.
…ilures 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.
…e 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parts 2–3 of 8. Base:
test/isolation-harness(#63) — that must merge first.Defensive security work on our own updater. Hostile archive fixtures are built in-memory; nothing is written to disk or followed, and every case asserts rejection.
The headline defect
A
tar.TypeSymlinkentry namedgrantwithSize: 0madeextractBinaryreturn(empty, nil).applyBinaryhashes whatever it is handed, so an empty payload verifies against itself and installs — bricking the user's binary. Reproduced twice independently asbytes=0 err=<nil>.Zero-length binaries are now rejected in both extractors and at
applyBinaryTo.Also fixed
grantbeside it plus a guard-specificwantErrContains.path.Clean("//host/share/x")collapses to/host/share/x, sopath.IsAbsalways won. Reordered. Verified message-only: 35 hand-picked inputs plus every string of length ≤5 over{/ \ . : C c 1 a}— 37,449 strings, zero decision differences.fs.ModeSymlinkentry namedgrant.exewas accepted in production, unmutated. Nowf.Mode()&fs.ModeType != 0, mirroring tar.checkArchivePathand both extractors, seeded with the hostile cases.Notes a reviewer should not skip
tar.Readerforcesnb=0for header-only types, so symlink/hardlink/dir fixtures all degrade to "is empty".TypeContand vendor types are not header-only and carry readable bytes — those cases were added and fail on bytes returned. This is why the type guard must never be removed in favour of the zero-length backstop.zip.NewReaderparses only the central directory and never inflates skipped entries. Measured — a 1000:1 bomb inflated nothing in 126 µs. Pinned with a passing test.FuzzExtractFromZippreviously wedged to 0 exec/sec and still printedPASS. Diagnosed as throughput collapse on near-cap inputs (128 MiBio.ReadAllper exec), not a production hang. Fixed by cappingmaxDownloadBytesinside the targets; it now sustains the highest throughput of the three.len(data) != hdr.Sizecross-checks are unreachable — a successful capped read returns exactlyhdr.Size, and earlier exhaustion returnsio.ErrUnexpectedEOF. Kept as defense-in-depth, claiming no coverage.fsynctest uses a FIFO and is//go:build !windows. No portable Windows equivalent exists —FlushFileBufferssucceeds on both named pipes and regular files.17/17 mutations killed. Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed.