mirror of
https://github.com/anchore/syft.git
synced 2026-10-11 21:57:22 +02:00
1d25dfe4da0cfbc3a59b7473b6c1201708dae25b
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
Alex Goodman
|
58a033f924 |
Prevent unnecessary allocations when parsing compressed ELF sections (#5187)
* fix(elf): bound compressed ELF section reads `debug/elf` takes a section's decompressed size from that section's own compression header, and a highly compressible stream really does deliver the bytes that header promises, so `internal/saferio` does not help: it faithfully allocates every one of them. A 2MB input file drives `elf.NewFile` to allocate over 10GB and return no error, which is a fatal OOM rather than a recoverable panic. Which sections get read is not up to the caller. `elf.NewFile` always reads the section-name string table, and `File.Symbols` reads `.symtab` plus whatever section its `Link` field points at, so being selective about sections is not enough to avoid it. New `elfutil.NewFile` is a drop-in for `elf.NewFile` that rejects a declared decompressed size over 128MB. Every production call site goes through it, and a ruleguard rule keeps the next one from going direct. The check runs in two parts, since `debug/elf` expands sections at two different times. The section-name string table is the only one `elf.NewFile` expands itself, so it is checked against the raw bytes before the call; everything else is expanded lazily by `(*Section).Open` and is checked after the parse, where names, types and decompressed sizes are already resolved. Only the sections syft can actually reach are bounded, which keeps the guard from costing real binaries. DWARF is excluded since nothing calls `File.DWARF`, so a large compressed `.debug_info` no longer skips the whole file, and sections `debug/elf` will not decompress anyway (`SHF_ALLOC`, `SHT_NOBITS`) are left alone. The legacy `.zdebug` form is matched on the section name the way `debug/elf` gates it rather than on the `ZLIB` magic, so an ordinary section starting with those four bytes is not mistaken for a compressed one. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix(elf): gate debug/buildinfo behind the compressed-section check `debug/buildinfo.Read` opens ELF files with `debug/elf` itself, and `elf.NewFile` expands the section-name string table as it parses, so the golang cataloger was still reachable by the same bomb `elfutil` exists to stop. A 261KB fixture drove 1.4GB of allocation through `buildinfo.Read` and returned no error. `elfutil.CheckSectionNameTable` is now exported for that case: callers that cannot use `NewFile` because the `debug/elf` call is made for them inside another package. Both `buildinfo.Read` call sites go through it, including the UPX-decompressed one. Also corrects claims that did not hold up: - the package doc's 2MB-to-10GB figure is not reachable with zlib (~1000:1), so it now carries the measured 510KB-to-2.6GB, and names zstd's 32767:1 since that is what makes the small inputs possible - `.go.buildinfo` was listed as a hot-path section elfutil covers, but it is read through `debug/buildinfo` and never touches `Section.Data` - the graalvm comment claimed routing size rejections away from `*elf.FormatError` improved reporting; both branches are skipped by the caller and only the FormatError branch logs, so it did the opposite - `sharedLibraries` logged short and truncated files as real ELF failures, since `debug/elf` returns a bare `io.EOF` rather than an `*elf.FormatError` for those Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * added test comments around the negative cases Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * additional tests Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * better decomposition and comments Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * use a less brittle constant for error detection Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> |
||
 Will Murphy
|
e38851143e |
chore: centralize temp files and prefer streaming IO (#4668)
* chore: centralize temp files and prefer streaming IO Catalogers that create temp files ad-hoc can easily forget cleanup, leaking files on disk. Similarly, io.ReadAll is convenient but risks OOM on large or malicious inputs. Introduce internal/tmpdir to manage all cataloger temp storage under a single root directory with automatic cleanup. Prefer streaming parsers (bufio.Scanner, json/yaml.NewDecoder, io.LimitReader) over buffering entire inputs into memory. Add ruleguard rules to enforce both practices going forward. Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * chore: go back to old release parsing Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * simplify to limit reader in version check Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * chore: regex change postponed Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> * simplify supplement release to limitreader Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> --------- Signed-off-by: Will Murphy <willmurphyscode@users.noreply.github.com> |
||
|
Alex Goodman
|
048df17e3d |
Use values in relationship To/From fields (#2871)
* use pkg values in relationship fields Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * add linter rule for using values in relationships Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * use new cmptest package for comparing relationships Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * create cmptest for common cmp.Diff options in test Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * condense matches for relationship ruleguard Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * remove relationship type from rules Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * restore build tag Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * suggest using values Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * nil check pkgs Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> |
||
|
Alex Goodman
|
ada8f009d2 |
Add relationships for ALPM packages (arch linux) (#2851)
* add alpm relationships Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * tweak reader linter rule to check for reader impl Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * update JSON schema with alpm dependency information Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> |
||
 William Murphy
|
3713d97b7b |
chore: use ruleguard to test for missing defer statements (#2837)
* chore: ruleguard to enforce defer use Signed-off-by: Will Murphy <will.murphy@anchore.com> * fix go.mod location Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close in linux release identifier Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: better lint suggestion Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: refactor binary classifier to defer close Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in gentoo cataloger Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: make go license parsing defer close readers Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer closing readers in alpine apm parser Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in graalvm parser Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in debian package parser Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in alpm parser Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in executable file cataloger Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in javascript license parser Signed-off-by: Will Murphy <will.murphy@anchore.com> * chore: defer close readers in go mod parser Signed-off-by: Will Murphy <will.murphy@anchore.com> --------- Signed-off-by: Will Murphy <will.murphy@anchore.com> |