* 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>
bullseye went EOL and deb.debian.org dropped it, so the elf executable
fixture image stopped building and took the whole unit test job with it
(fixture generation runs before any test does). main has been red on this
since 2026-09-08.
two separate breakages, both handled here:
- the base repo moved to `archive.debian.org`, whose Release files are
permanently expired, so `Acquire::Check-Valid-Until=false` is required
- `bullseye-security` is worse off: its index is still served but every
package in the pool behind it 404s, and it has not landed on the archive
yet. dropping it is the only option right now. these are throwaway
fixture build tools, so base bullseye versions are fine.
verified by rebuilding the image and running the fixture `verify` target,
which still reports `PASS: all security features accounted for`.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* chore(deps): pin stereoscope to the hardlink-inode-adoption branch
temporary pin to anchore/stereoscope#670 so the hardlink changes can be
exercised end to end. needs re-pinning to a release tag before merge.
pulls transitive bumps along with it: docker/cli, docker/go-connections and
gabriel-vasile/mimetype.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix: drop the image resolver hardlink workaround
stereoscope now adopts a hardlink's target at index time, so a hardlinked name
arrives already described as a regular file with the target's size, mime type
and content. the resolver-side fix from #5029 sat on top of that doing the same
job a second time, gated on a file type that no longer shows up, so both
`resolveHardLinkTarget` implementations and the extra tree walk they cost on
every `FilesByPath` are gone.
`file.NewVirtualLocationFromImage` stays as-is. it is exported and syft is v1,
so it keeps working for anyone using it, it just has no callers in syft now.
this picks up `FilesByMIMEType`, which #5029 explicitly could not fix, so
hardlinked names now reach mime-driven catalogers. on images built around
multi-call binaries (busybox and friends) that is a lot more file and executable
entries than before; distro and toolchain images move by about one entry.
one shape regresses: a hardlink stereoscope could not adopt (a link name that is
absent, empty, names a directory, names another un-adopted link, or points into
a lower layer) keeps `TypeHardLink`, and syft goes back to collapsing it onto
its target's path. no mainstream builder emits the cross-layer case.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* bump stereoscope to main
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
#5188 changed `pnpmV6LockYaml.Parse` to take a `*yaml.Node` and #5175 added
this test against the old `[]byte` signature. both were green on their own
branches, so main does not compile.
the v9 and v6 cases in the same test already go through the `yamlDocument`
helper, this is just the v5 one that was missed.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
parsePinnedVersion rejected any version constraint containing "!" to
skip the "!=" exclusion operator. PEP 440 epochs use "!" as well
(e.g. "1!2.0.0"), so an exact pin like "pkg == 1!2.0.0" was treated as
unparseable and returned "". parseRequirementsTxt drops a requirement
whose version resolves to "", so the package never made it into the
SBOM at all in either the pinned or guessed path.
Check for the "!=" operator as a substring instead of the bare "!"
character, which leaves the epoch separator alone.
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
pnpm v5 lockfiles encode resolved peer dependencies as an
underscore-delimited suffix of the dependency path, e.g.
/acorn-jsx/5.3.2_acorn@8.8.0 or a hashed form like
/webpack-cli/4.10.0_fzn43tb6bdtdxy2s3aqevve2su. Only the v6+
parenthesized form was stripped, so for v5 lockfiles the suffix leaked
into reported versions and PURLs, and peer variants of the same
name+version were not deduplicated.
Strip the suffix on the v5 code path (version < 6.0) wherever a version
is extracted: top-level dependencies values, packages keys, and
per-package dependencies map values. Only values with a leading digit
are stripped so link:/file:/git specifiers are unaffected.
Fixes#5174
Signed-off-by: codeAnqiang-ma <273298913+codeAnqiang-ma@users.noreply.github.com>
Co-authored-by: codeAnqiang-ma <273298913+codeAnqiang-ma@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(binary): detect multi-arch ingress-nginx
Add v1.9.6 ARM64, ARMv7, and s390x fixtures from the published controller image and match their version marker against the nearby Go build version.
Assisted-by: OpenAI Codex
Signed-off-by: Shurong Cao <170531907+CAOShurong@users.noreply.github.com>
* fix(binary): match ingress-nginx release marker on all arches
The release is injected with `-ldflags -X`, so it lands in its own aligned,
NUL-padded data symbol. Only that padding is portable -- the surrounding bytes
are an arch-specific float constant pool, which is why the existing matchers all
anchor on incidental junk like `$a` and `S=v<y5` and only ever worked on amd64.
Matching `v<version>` followed by two NULs finds exactly one hit in every v1.9.6
binary (amd64, arm, arm64, s390x) and in all nine amd64 releases already under
test. Two NULs matter -- with one, s390x matches a vendored `v1.19.0` earlier in
the file and reports the wrong version.
This also drops the ~8KB wildcard window the marker previously needed, so the new
fixtures are ordinary 369 byte snippets instead of 8.4KB ones.
Two things in the fixture tooling had to move for that to work:
- `config.yaml` pinned the multi-arch *index* digest for all four platforms, but
extraction runs `docker create <ref>` with no `--platform`, so every entry
resolved to whichever image was pulled last. `make download` failed outright on
`linux/arm/v7`, since the registry labels that platform `armv7` and docker
normalizes it to `arm`. Now pinned per-platform, matching the redis entry.
- `write-snippet` only recognized EM_X86_64 and EM_AARCH64, so anything else
landed in a `linux-unknown-<hex>` directory and had to be placed by hand.
Verified against the real binaries with `-must-use-original-binaries`.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(binary): put non-amd64 snippets where the tests look for them
`write-snippet` used to fall back to `unknown-<hex>` for any ELF machine it
didn't recognize, so three snippets were sitting in directories nothing reads:
helm/3.12.0/linux-unknown-454d5f53333930 (hex of "EM_S390")
helm/4.1.4/linux-unknown-454d5f53333930
redis-server/7.2.5/linux-unknown-454d5f333836 (hex of "EM_386")
The test table asks for `linux-s390x` and `linux-386`, so those three cases were
quietly falling through to downloading the full binaries instead. Renamed to
match, and taught the tool about EM_386 and EM_RISCV so it stops producing dead
directories (there is an existing `linux-riscv64` snippet it also couldn't have
written).
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Shurong Cao <170531907+CAOShurong@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Shurong Cao <170531907+CAOShurong@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(javascript): read every document of a pnpm-lock.yaml stream
pnpm-lock.yaml can be a multi-document YAML stream. pnpm keeps config
dependencies and the pinned package-manager version in a leading document
and the project's dependency graph in the next one. That layout has
shipped since pnpm 11 for projects using config dependencies, and pnpm 12
records the package-manager pin by default, so two documents is now the
common case.
The cataloger called yaml.Unmarshal, which reads the first document only.
On such a lockfile the SBOM contains pnpm's own release binaries and none
of the project's dependencies. Both documents declare the same
lockfileVersion and the result is well-formed and non-empty, so nothing
signals that it is wrong.
Split the stream and parse each document, merging the results. A
single-document lockfile yields exactly one document, so its behaviour is
unchanged. A document that omits lockfileVersion falls back to the
version from the first one rather than being dropped.
Fixes#5168
Signed-off-by: hamodywe <iosapk.org@gmail.com>
* refactor(javascript): stream pnpm-lock.yaml documents instead of buffering
The multi-document fix read the whole lockfile, decoded it to a `yaml.Node`
tree, re-marshaled every document back to `[]byte` and then re-parsed each one
twice. `yaml.Node` already has a `Decode` method, so the decoder can walk the
stream straight off the reader and hand each document's node to the parser.
On a 4.8MB single-document lockfile that takes total allocations from 1069MB to
250MB, which is also below where things sat before multi-document support. It
drops the `io.ReadAll` and its `noUnboundedReads` suppression, and puts pnpm in
line with every other YAML cataloger in the tree, all of which already stream.
`pnpmLockfileParser.Parse` now takes a `*yaml.Node` rather than `[]byte`. It is
package-private with two implementations and one production call site.
Documents accumulate into a single map keyed by `name@version`, so the stream
follows one collision rule rather than two: the last entry to appear wins,
which is what already happens within a document. That replaces the separate
first-wins-with-fill-in merge pass. A conflicting integrity between documents
is now traced rather than resolved silently.
Three behaviour fixes come with it:
- an empty or comment-only leading document no longer takes down the whole
file. It decodes to a null node, not a zero-kind one, so the old guard never
fired and the stream failed with `invalid lockfile version ""`.
- a malformed document no longer discards the documents that already parsed.
It is reported through `unknown` instead, matching how the rest of the
cataloger reports partial results.
- parse errors carry the document index, and their line numbers again point at
the user's file rather than into a re-marshaled buffer.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* chore(javascript): trim the duplicated pnpm multi-document comment
The six-line explanation of why a pnpm lockfile can hold two documents sat
verbatim in both the parser and its test. Keep it next to the code, where
someone debugging will be, and leave the test pointing at it.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: hamodywe <iosapk.org@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>
orderedIDSet.add scanned the entire existing slice for every inserted ID,
so building a Collection was quadratic in the number of packages sharing an
index key. This is most visible in idsByType: package types are few, so that
index accumulates nearly every package in the SBOM into a single set, and
each Add rescans it.
Keep the ordered slice (ordering and dedup semantics are unchanged) and add
a hash index for membership checks. The index is only built once a set grows
beyond a small threshold, so the many single-element sets held by idsByName
and idsByPath do not pay for a map, while the large idsByType sets get
constant-time lookups.
BenchmarkCollectionAdd (Apple M2 Pro), packages sharing one type:
packages before after speedup
1,000 1.37 ms 0.76 ms 1.8x
10,000 100.31 ms 9.80 ms 10.2x
50,000 2,423.79 ms 34.46 ms 70.3x
The tradeoff is memory: sets past the threshold allocate an index map,
about 14% more bytes at 50,000 packages, with allocation counts unchanged.
Signed-off-by: Oleksandr Vodotiiets <61548316+avodotiiets@users.noreply.github.com>
With search-remote-licenses enabled, every module name went to the proxy,
including toolchain binaries whose main module is synthesized from the
package path, such as cmd/cgo. The proxy answers 404, and the direct
fallback then treats the path as a repository host, producing requests
like https://cmd/cgo/info/refs?service=git-upload-pack.
Reuse isStandardImportPath, already in this package, to skip the remote
search for paths whose first element carries no dot. Those are never
publishable module paths, so neither a proxy nor a repository can resolve
them.
Fixes#3149
Signed-off-by: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com>
* 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>
`readerAtAdapter.ReadAt` seeks and then issues a single `Read`, which breaks the
`io.ReaderAt` contract in both directions against the squashfs reader it exists
to wrap:
- `squashfs.File.Read` copies against the decompressed block length but advances
its block cursor by the nominal block size, so a block that decompresses short
silently stops copying and returns fewer bytes with a nil error. `ReadAt`
forbids that, and callers rely on it: anything decoding a fixed-size structure
off the result gets zero padding it has no way to detect and parses it as real
data. The GraalVM PE export table and the UPX block reader both size a buffer
from a header field and then ignore `n` entirely, so a crafted image drives
them straight through the padding.
- a read landing exactly on the end of the file returns a *full* buffer paired
with `io.EOF`. `bytes.Reader.ReadAt` returns nil there, and the callers that
treat any error as fatal were written against that, so squashfs-resident
binaries sized near a read boundary were being skipped outright.
`io.ReadFull` normalizes both: it fills the buffer across short reads, and it
clears the error once the buffer is full. A genuinely short tail is reported as
`io.EOF`, which is what `ReadAt` implementations return at the end of a file, and
what the buffering branch of `GetUnionReader` already returns.
Affects squashfs-backed sources (snaps), so in practice the binary catalogers
reading structure out of executables.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
dependabot[bot]
Alex Goodman
anchore-oss-update-botandanchore-oss-update-bot
Rez Moss
Vaibhav Srivastava
Ankush Pathak
Pujitha PaladuguandPujitha Paladugu
Matt Van HornandChristopher Phillips
Sueun ChoandChristopher Phillips
addielarue
Xenira
Hamody WeandAlex Goodman
Oleksandr Vodotiiets
Keith Zantow
Luan Taraschi
Weston Steimel