2 Commits

Include renames

Author	SHA1	Message	Date
Alex Goodman	3325493831	chore: pin the bounds from the unbounded allocation work (#5298) ... * build(deps): bump go-make to v0.8.1 Picks up golangci-lint 2.13.2 (built with go1.27) in place of 2.11.4 (go1.26). The old build panics outright on a go1.27 GOROOT with "file requires newer Go version go1.27", and an older mise-shim build fails the quieter way: it loses type info and reports zero findings with exit 0, so a ruleguard run comes back green without having evaluated anything. Also folds in what the newer linter flags: - `reflect.Ptr` -> `reflect.Pointer` in three files, applied by --fix - `tw.CellFormatting{MergeMode: ...}` -> `tw.CellMerging{Mode: ...}` in the cataloger info table, for a staticcheck SA1019 deprecation Note the deprecation text names `CellConfig.CellMerging.Mode`, but the field is `CellConfig.Merging`; `CellMerging` is the type. That command path has no test coverage, so the swap was checked by rendering the same hierarchical-merge table both ways and diffing: byte identical. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * chore(lint): tune goconst for data tables goconst got stricter in golangci-lint 2.13 and now counts strings in map literals and struct fields, which lights up every lookup table in the repo. It was already enabled, so this is main going red on a linter bump rather than anything new in the tree: 101 findings, and the default threshold of 3 means a vendor name appearing in three rows of a table reads as a magic constant. Raising `min-occurrences` to 5 takes that to 14. The rest split by whether the repeated string is data or an actual constant: - `cpegenerate` is excluded by path. It maps artifact names to vendors, products, and maven group ids, and repeats strings 35, 83, even 293 times, so no threshold reaches it. Hoisting those would trade a readable table for indirection. - the two SBOM fixture builders in `format/internal/testutil` get a function-level nolint each. The repeated names and versions are what the golden files assert against. - `mysql-cluster` and `php` were genuine repeated literals and are now constants. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * chore(test): extract measureAlloc into a shared helper The allocation-budget tests added during the unbounded allocation work grew a copy of the same helper in every package they touched: four identical `measureAlloc` definitions plus two more spelled out inline against `runtime.MemStats`. Now `testutils.MeasureAlloc`, at the repo root so `internal/spillbuf` can reach it too. The doc comment carries the two things that were only recorded in some of the copies: TotalAlloc is process-wide so callers must not use t.Parallel, and a budget assertion only means something if the fixture would really blow past it unguarded, so measure the unguarded path as well. No behavior change, the body was identical in all six. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * chore(lint): add ruleguard rules for overflow and swallowed EOF Two rules out of the recent unbounded allocation work (#5187, #5195, #5267, #5291, #5292, #5293, #5294). `noOverflowingBoundsCheck` flags `offset + size > limit` (and <, <=, >=) where an operand is unsigned, neither addend is constant, and the operands are at least 32 bits. Unsigned addition wraps, so an offset out of a file header can carry the sum past the wrap point and pass a check it should have failed. The fix is to subtract against the limit instead. Constant addends are excluded because `pos+4 <= uint32(len(buf))` is the normal way to step a cursor, and the 32-bit floor drops nibbles widened for a comparison. Would have caught four sites across #5195 and #5291. `noSwallowedEOF` flags `return nil` / `return nil, nil` on an io.EOF check. The caller gets a zero value it cannot tell from real data, and anything decoding a fixed-size struct off it parses the padding as content. That is the shape behind #5186. Both decide from the expression alone, and writing the fix deletes the pattern, so neither needs a suppression anywhere in the tree. That was the bar: rules for the allocation and decompression classes were tried and dropped, because the bound they look for lives in a preceding statement (or, for a decompressor constructor, always downstream), so no correct implementation can silence them. That would have meant ~15 permanent nolints on code that is already right. The rules here that do work don't judge whether a bound exists, they route to a wrapper that takes one: noDirectTempFiles, noDirectELFOpen. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * test: pin allocation budgets on the alpm, deb, and changelog bounds The bounds added for #5292, #5293 and #5294 are asserted by the error they return. That keeps passing if the bound is removed and something else rejects the input later, which is the failure these were written for. Each of these measures bytes instead, and proves the fixture is really a bomb first so a fixture that quietly stops being one fails rather than passing. - alpm mtree: 16MB of newlines, well inside the 64MB byte cap, costs 16.5GB uncapped against 238MB with maxMtreeLines. That 16.5GB is the measurement the line cap was added for and it was recorded only in the commit message. Note what the cap actually buys: 300k entries still cost a few hundred MB, per concurrent cataloger. - deb members: 1.28GB against 392KB. Both arms read with io.ReadAll on purpose, since the stream itself never holds much whatever the cap says; what the cap protects is a consumer that buffers, and the real one is a tar reader building entries. - snap changelog: 1.28GB against 175KB. This one has no explicit cap at all. It is held by bufio.Scanner refusing a token past MaxScanTokenSize, which is incidental, so switching to io.ReadAll or a bufio.Reader to pick up long lines would remove it silently. Nothing added for the kernel module reader: its copy is already io.Copy(tempFile, io.LimitReader(rc, max+1)), so peak disk is bounded by construction and heap is streaming. A heap budget would assert the wrong resource, and the three existing subtests cover the cap, at-cap fidelity, and spill cleanup. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * test: pin allocation budgets on the ai and alpm byte caps Follows the same pattern as the alpm line cap: measure bytes, and prove the fixture is really a bomb first so one that quietly stops being a bomb fails rather than passes. - safetensors: 252MB against 2.6KB. The existing over-cap case deliberately omits the body, since the guard rejects on the declared length before reading anything. That is the right test for the guard but it would keep passing if the check moved after the read, so this one delivers the bytes. - gguf: 268MB against 168MB. copyHeader io.Copy's whatever it is handed, so maxHeaderSize holds only because every caller wraps the reader first. This pins that the limit is where the bound lives, so dropping the wrapper at a call site fails here. - alpm byte cap: 740MB against 132MB, the counterpart to the line cap budget. Worth knowing for anyone writing the next one of these: a single enormous line does not exercise the alpm byte cap. go-mtree runs its own bufio.Scanner and refuses a token past 64KB, so that fixture fails as "token too long" before the byte cap is consulted. The fixture here uses ordinary 1KB lines, enough of them to pass 64MB while staying under the 300k line cap, so the byte cap is actually the thing under test. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * test: assert bytes retained instead of allocated for gguf copyHeader the bounded arm writes into a bytes.Buffer, whose doubling growth costs a multiple of what it holds and lands on a different power of two depending on the platform. it measured 168MB locally and 320MB on CI, past the budget. buf.Len() states the same property deterministically. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>	2026-09-17 14:11:16 -04:00
Alex Goodman	1d25dfe4da	fix(golang): bound allocations from UPX-packed binary headers (#5195) ... * fix(golang): bound UPX decompression by what the input can justify A UPX block's `sz_unc` was passed straight to `make([]byte, n)`, so four bytes of attacker input spanned the full uint32 range. A ~300KB file could drive multi-GB resident memory, which Go reports as a fatal runtime OOM that no `recover` on this path can contain. Syft parses binaries out of arbitrary images, so that is reachable from any scan. Two bounds do the work, and UPX's own invariants make them exact: - all blocks together reconstruct `p_filesize`, so a running remainder caps the sum. Bounding only per-block would leave the total at (block count x original size), so the remainder is the part that actually closes it - `p_filesize` sizes the output buffer, so it is capped both absolutely and against the size of the file on disk. The absolute cap alone left a ~60 byte header able to claim 500MB; the ratio alone cannot work either, since LZMA encodes a run of N equal bytes in O(log N) and a `go:embed` of 120MB of zeros legitimately packs 209x The output buffer is also allocated on the first block that survives validation rather than up front, so a header claiming a large size with nothing decodable behind it costs nothing. Alongside that, several ways the block loop could be steered off its own buffer: - `parseELFPTLoadOffsets` bounds-checked program headers with `phStart+phentsize > len(buf)`, which overflows for a `p_offset` near 2^64 and lets an out-of-range entry through into a slice index. Switched to the subtraction form, and a `phentsize` shorter than an ELF64 program header is rejected since the reads use fixed offsets - block placement had the same overflow shape, and on failure it skipped the copy and fell through. `outputOffset` derives from the rejected offset, so a `p_offset` at the uint64 ceiling wrapped it to 0 and the next block landed on the reconstructed ELF header, still returning success. Out-of-range placement now ends the block chain, keeping the blocks already placed since those often carry `.go.buildinfo` - the UPX 2-byte LZMA header carries `lc`/`lp` as nibbles and `pb` as three bits, so they can hold values LZMA does not permit. Folded into the props byte they wrap (`lc=15, lp=15, pb=7` gives 465, which truncates to 209) and the stream mis-decodes instead of failing - blocks decode directly into a slice of the output buffer instead of a per-block buffer that is then copied, so one block no longer doubles peak memory - the block count is capped: the size budget alone still allows millions of 12-byte blocks, each spinning up an LZMA reader Verified against the `image-small-upx` fixture, a real `upx --best --lzma` binary: four blocks, max `sz_unc` equal to `p_blocksize` exactly, blocks summing to 0.68 of `p_filesize`, and a 1.96x expansion against the packed file, so every bound holds with headroom on genuine output. Note `p_blocksize` is deliberately not used as a ceiling on `sz_unc`. It adds nothing the remainder does not already cover, and real output sits at exactly `p_blocksize`, so it would run with zero headroom against a value the format does not promise. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix(golang): report UPX binaries we cannot unpack as unknowns A packed Go binary we fail to unpack means its packages are silently missing from the SBOM. `getBuildInfo` checked the decompression error for nil and discarded it, so a rejected file surfaced only the original `buildinfo.Read` error, and that one is deliberately silenced because it is usually just "not a Go binary". `decompressUPX` now marks the cases worth reporting with `errUPXDecompress`: it got past the header and the method dispatch and still could not unpack the file. Everything else stays quiet, which matters more than it sounds. `upx` defaults to NRV2B unless `--lzma` is passed and only LZMA is implemented here, so treating any decompression failure as reportable would attach a golang-cataloger unknown to every packed non-Go binary in an image. Making the reportable case opt in rather than the quiet case a growing list also means a guard added later is silent by default. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * refactor(golang): drop the vendored xcoff parser The only thing the golang cataloger read from it was `TargetMachine`, which is the same two magic bytes `getGOARCHFromBin` had already matched on to dispatch there. So a full XCOFF walk (string table, symbol table, every relocation table) ran to recover a value that was already in hand. `getGOARCHFromBin` reads those two bytes directly now. That is also strictly more correct: `xcoff.NewFile` bailed with "no symbol table" when `symptr == 0`, so a stripped XCOFF binary reported no arch at all. Fixtures move to `testdata/xcoff/`, since they are no longer a package's own testdata. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * feat(internal/file): report a reader's real size behind one helper Bounds get written as "does this claim exceed what the reader actually holds", and the subtlety is that a reader's own answer cannot be trusted: an `io.SectionReader` reports the nominal length it was built with, so one constructed with `1<<63-1` will happily claim to hold 8EB. Confirming the last byte is readable is what separates a real size from a nominal one. Returns `(int64, bool)` rather than a bare size, since there are five ways to not know and collapsing them to 0 makes every caller reinvent "0 means stop bounding". The doc also names the wrapper trap: a type embedding `io.ReaderAt` as an interface promotes only `ReadAt`, so it answers no size at all however large the reader beneath it is, and a bound written against it silently does nothing. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix(golang): bound every section a reader can expand, not just the name table `CheckSectionNameTable` bounds the one section `elf.NewFile` expands while parsing, which is enough for a caller that only parses. It is not enough for one that goes on to read sections: those are expanded lazily, so a 260KB ELF declaring a compressed `.symtab` drove 1.3GB of allocation through `goversion`, which opens the file with `debug/elf` inside its own package and cannot be routed through `elfutil.NewFile`. `CheckAllSections` is that gate, and is deliberately named as the superset so the relationship to the narrow one is structural rather than documented. Also exports `ErrDeclaredSizeExceeded`. A refusal here costs the SBOM a package, so callers report it as an unknown, and matching on an error string to decide that is not something to build a reporting policy on. A ruleguard rule now flags `buildinfo.Read` and `version.ReadExeFromReader` outside `scan_binary.go`. Both open `debug/elf` internally and are only bounded there by the wrappers that gate the reader first, which the existing rule on `elf.NewFile` could not see through. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * feat(internal/spillbuf): sparse offset-addressed buffer that spills to disk For output that arrives out of order, at offsets the input itself declares: a decompressor placing extents, an archive rebuilding a file from chunks. A plain `[]byte` cannot do that without either pre-sizing to a length the input claims or growing to the furthest offset it names, and both hand a hostile input an allocation knob. The load-bearing property is that it reports only what it actually stored. `Size` is the contiguous run written from offset zero and a read past it is `io.EOF`, never the zeros an unwritten region would hand back for free. Sparse storage serving holes as real content is what lets a write offset stand in for output nothing produced, so anything sizing an allocation against this reader (`saferio` in `debug/elf` does exactly that) stays bounded by work actually done. `FirstGap` answers where the next write fits, so callers filling holes do not need the extent bookkeeping and cannot re-derive that rule wrongly, and it only reports gaps lying entirely inside `[0, within)`. The extent type is unexported for the same reason. The rest of the contract worth stating: reads and writes after `Close` fail with `os.ErrClosed` rather than looking like an empty buffer, and a write that fails leaves the buffer unchanged. Memory is bounded by the limit rather than by how much is written: filling 1MB and filling 64MB cost about the same. What lands on disk past that limit is the caller's bound to set, not this package's. Growth is geometric and capped, because sizing the tier to exactly what each write needs is quadratic once a caller streams through it in small chunks. The golang cataloger is the only consumer today, and nothing else in the tree has this shape. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix(golang): rebuild UPX binaries through spillbuf and read the reconstruction everywhere Two amplification paths were still reachable. A 128 byte input allocated 16MB by trusting `sz_cpr` before weighing it against the bytes the file actually holds, and the reconstruction was assembled in a `[]byte` sized from the header's declared original size, so a 301KB input allocated 300MB. The reconstruction is a `spillbuf.Buffer` now, which is what makes the second one go away: blocks are placed at the offsets the file declares, nothing is pre-sized from a claim, and the reader's length is the contiguous run actually rebuilt. That last part is the invariant everything downstream leans on. A reader reporting the declared size would serve the holes between placed blocks as zeros it never stored, and a 64 byte block parked at `p_filesize-64` then made a 300KB input hand out a 64MB reader: measured, that drove 269MB through `getBuildInfo` and returned nothing, silently. Placement and storage are separate concerns. `upx.go` places blocks at the offsets the file declares and asks the buffer how much was rebuilt and where the next gap is; it knows nothing about temp files or memory limits. The remaining bounds are ratios against the input wherever they can be, since an absolute ceiling only ever drops a large legitimate binary. The one absolute left is on `p_filesize`, because padding an input is nearly free, and the LZMA dictionary is paid for in its own block's `sz_cpr`. Interface change: `scanFile` takes a context and hands back the reconstruction for each binary that turned out to be packed, and the caller owns it. Everything after the build info read (crypto settings, arch, symbols, the version scan) reads it, so a packed binary no longer reports its packages with no symbols at all. Also narrows what gets reported. This cataloger runs on every executable in an image, so reporting every `getBuildInfo` error attached an unknown to every corrupt ELF, odd PE and non-Go Mach-O slice in it. Now only four gaps, the ones that cost the SBOM something syft could have had: a packed file we could not decode (`errUPXDecompress`), a header the size bounds refused (`errUPXSizeRefused`), a reconstruction that came up short (`errUPXPartial`), and an ELF expansion `elfutil` declined (`ErrDeclaredSizeExceeded`). An unimplemented UPX method stays quiet on purpose: `upx` defaults to NRV2B and only LZMA is implemented here, so reporting it would attach an unknown to most packed non-Go binaries in an image. That filtering is a new pattern. No other cataloger picks which of its errors become unknowns, and the reason this one does is that it runs on every executable in an image rather than on files a glob already selected. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * chore: ignore local spec/ planning notes `/specs` was already ignored; `/spec` is the same thing under the singular name and was not, so working notes landed in commits. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * refactor(golang): name what the unpack hands back, not where it stores it the scan's signatures said `*spillbuf.Buffer` all the way down, which told every reader below the scan that it cares whether the reconstruction lives in memory or on disk. it does not. two interfaces instead, each naming only what its side actually uses: - `unpackedContents` for the consumers: `ReaderAt`, `Closer`, and a `Size` that is the contiguous run rebuilt from zero - `blockSink` for the decoder, which additionally needs `WriterAt` and `FirstGap` to place blocks this costs one thing worth flagging. `Close` on a nil `*spillbuf.Buffer` was nil-safe, so callers just deferred it; widened into an interface, a nil pointer becomes a non-nil interface and the same call panics. `closeUnpacked` absorbs that, and it is now the only way an owner releases contents. `readerFor`, `seekerFor` and `closeUnpacked` are the three places that resolve the nil. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * docs(elfutil): say that CheckAllSections is the decompression-bomb gate the doc led with "bounds every section syft can drive debug/elf into decompressing", which describes the mechanism without ever naming what it is defending against. a reader landing on the call site could not tell that this is the decompression-bomb check. state the attack instead: a compressed section header declares its own decompressed size, debug/elf believes that number and allocates it on open, and nothing forces it to match what the compressed bytes actually yield. the 260KB ELF that drove 1.3GB of allocation now lives in the doc rather than only at the one call site that happened to mention it. comments only. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * docs(golang): move the overflow note onto the bounds test it describes the "subtraction form" note sat above the phStart assignment with ten lines of wrap analysis between it and the `if` it was talking about, so the subtraction it names reads as missing. it is `hdrLen-phStart < phentsize`. move it onto that test and name the form it is avoiding (`phStart+phentsize > hdrLen`, which can overflow past the buffer end) so the referent is not several paragraphs away. comments only. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * chore(golang): drop the internal/ README left over from xcoff that README existed to record where the vendored xcoff package came from and that Go keeps it internal, which is a provenance note worth having while third-party code sat there. dropping xcoff took the reason with it, and repurposing it to point at gotestdata just made it redundant: gotestdata has its own README covering why it is under internal/, why it is not testdata/, and what belongs in each. internal/ now holds one self-documenting directory. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>	2026-09-10 09:21:19 -04:00