* fix(java): bound user-controlled reads in the graalvm native-image cataloger
Sizes, offsets and lengths in a native-image binary all come from the file
being parsed, and several were acted on directly.
The PE export directory sized a `make([]byte, Size)` from a uint32 in the
optional header, so an 8KB file claiming 4GB reserved 4GB before reading a
byte. It now reads what the file actually holds, bounded by the declared size,
and still requires the whole directory to be present. This also fixes a latent
bug: `unionreader.readerAtAdapter.ReadAt` can return a short read with a nil
error, and the old code discarded the count, so squashfs-sourced binaries could
parse zero padding as export data.
The bounds checks in `decompressSbom` and the PE export walk validated only the
end of a range, computed by adding to a value out of the file. Those sums wrap,
and a wrapped sum compares as in range while the slice that follows it panics.
They now subtract from the known-good length instead. The three
`address - sectionBase` subtractions are unsigned and underflowed on an address
below the base; they share one guarded helper now.
The embedded SBOM decompresses through an `io.LimitedReader`, since the
compressed bytes are bounded by the file but what they expand to is not. The
limit is checked before the decoder's own error, which would otherwise report a
size problem as "not a cyclonedx json document". Hitting it is logged, since
the caller reports parse failures at trace level and a dropped SBOM would
otherwise go unnoticed.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(java): stop at the first format that parses a native image
The format loop tried ELF, Mach-O and PE against every reader and kept going
after one of them parsed, so a file that parses as more than one format
contributes its packages once per format.
No input parses as two formats today, so this is not a behavior change in
practice: `debug/pe` rejects an ELF on the optional-header magic, and both
`debug/elf` and `debug/macho` reject anything without their own magic. It
removes the possibility rather than a live bug.
Note this does not deduplicate across readers, which is the outer loop: a
universal Mach-O carrying the same native image for two architectures still
yields its packages twice.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(java): bound the graalvm export directory read against the file
Follow-up to the hardening earlier in this branch, from review.
The export directory read was still bounded only by file size, so a large
mostly-empty file (cheap to ship inside a compressed layer) was read into
memory in full and then discarded. It now weighs the declared size against an
absolute cap and against the bytes really present before allocating anything,
matching `readDataFromRVA` in `syft/pkg/cataloger/internal/dotnet/pe`. A 64MB
file declaring a 4GB export directory went from ~140MB allocated to ~3.6KB.
Also from the same review:
- dropped the decompressed-SBOM ceiling from 100MB to 16MB. Decoding costs
roughly 48x the JSON size in allocation and 6x retained, so the old ceiling
was worth several GB of churn per file, and catalogers run concurrently.
- `symbolOffsets` is now single-address `symbolOffset`. Two same-typed address
arguments in a row could be transposed at a call site and would produce a
silently wrong SBOM rather than an error.
- `newMachO` now returns an error on a real read failure. It was falling
through and dropping the file with no error and no log at all.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(java): reject a short read of the graalvm export directory
Second round of review on this branch.
The export directory read introduced earlier in the branch only consulted the
byte count when `ReadAt` also returned an error, so a reader handing back fewer
bytes with a nil error was accepted and the tail of the buffer stayed zeroed,
then got parsed as a valid export directory. The count decides now, whatever
the error says. `io.ReadFull` would also cover it but spins forever on a reader
that keeps answering `(0, nil)`, which is the same hostile input this is meant
to reject, so the single read stays.
Also in here:
- the decompressed SBOM is buffered and size-checked before the decoder is
invoked, instead of inferring the overrun from `io.LimitedReader.N` after the
fact. That check was only correct because the cyclonedx decoder happens to
drain its reader before parsing, and nothing would have caught it if that
changed.
- four bounds guards, the `newMachO` error path and the SBOM ceiling had no
test that failed when they were removed. They do now, verified by deleting
each one and watching a named test go red. The ceiling in particular could be
raised to 2GB with the suite still green, since every case derived its sizes
from the constant.
- `nativeImagePE.reader` is dead now that the read takes the reader as an
argument.
- three comments claimed guarantees the code does not make: that `symbolOffset`
cannot be called with transposed arguments, that a rejected file cannot go
unrecorded when it is only logged at trace level, and a set of measured
allocation multipliers that nothing pins.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* chore(java): trim the graalvm cataloger comments
Comment pass over everything this branch added. Cut about a quarter of the
comment lines, mostly restatement and detail that was never going to stay true.
Dropped: a production comment naming a test, the measured allocation multiples
behind the two size ceilings, cross-references to the equivalent constants in
`dotnet/pe` and `elfutil`, and test docs that restated their own test name
before saying anything.
Kept the parts that fail silently if someone tidies them away: why the read is
a single `ReadAt` rather than `io.ReadFull`, the note that `readExportDirectory`
uses an RVA directly as a file offset, and the fixture notes explaining why a
`VirtualAddress` is deliberately huge or why no symbol sits at index 0.
Three that were stale rather than verbose:
- `nativeImagePE`'s doc still said it holds the underlying reader, which this
branch deleted.
- `TestFetchSbomSymbols_FindsAllThreeSymbols`' doc had ended up above
`TestFetchSbomSymbols_AddressBaseOutOfRangeGuard`, documenting the wrong
function.
- a comment referenced a commit sha that no longer exists on this branch.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): bound the bundle signature search by the file, not the headers
The single-file bundle marker search sized its buffer from `calculatePEEndOffset` /
`calculateELFEndOffset`, both of which sum user-controlled header fields. A tiny file
declaring 4GB sections drove an 8.6GB allocation, and on the ELF side the sum could
overflow negative and panic in `make`.
The search now clamps its window to the real file length before sizing anything. The PE
and ELF paths had grown two copies of the same find-and-read sequence, so they collapse
into one `bundle.ExtractDepsJSON`, which lets the signature search and the header reader
both go unexported.
Also guards `read7BitEncodedInt` against returning a negative length, which on a 32-bit
`int` would seek a manifest walk backwards and re-read the same bytes per declared file.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): stop a truncated version resource from spinning forever
`readIntoStruct` reported a truncated read as success and left the destination struct
zeroed. Its callers advance their loop counters by the offsets it updates, so the string
table loop consumed nothing, advanced nothing, and never reached its length bound: a
version resource whose header claims more content than the blob carries hung the parse.
A truncated read is now an error, and the two places where hitting the end of a blob is
normal say so explicitly. That distinction is what keeps a well-formed resource whose last
child is `VarFileInfo` from losing its `FileVersion` fallback. io.EOF and
io.ErrUnexpectedEOF mean the same thing here, so both take the same path.
The string table walk moves into its own function, since the two new arms put
`parseVersionResourceSection` over the funlen limit.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): bound PE resource parsing by what the file actually holds
Resource sizes and RVAs are user-controlled uint32s that sized buffers before anything
checked the file held that many bytes, so a small crafted section could reserve up to 4GB.
The walk now carries the section's reader and its base RVA for its whole lifetime, so
`reader.Size()` is the single authoritative bound and one `offsetOf` helper rejects every
RVA the section does not hold. That replaces the per-node section value the recursion used
to synthesize, whose size argument was only correct by convention.
Bounding the allocations alone turns the OOM into a hang, though: nothing in the format
stops entries from aliasing, so thousands of them may name one fat blob and every offset
still checks out. A 256KB section built that way drove 24GB of allocation over two minutes
with peak memory flat. Capping entry *count* does not help since the cost is per byte read,
so the walk now charges every read against a budget of a few times the section size. Real
binaries come in at about 1x, since a well-formed section's entries partition it.
A resource tree that stops early no longer fails the whole file. It says nothing about the
CLR directory or an embedded deps.json, and dropping the package from the SBOM over one
malformed section is worse than reporting the fields we did get.
Also widens the entry counts before summing them (two uint16s can sum past 0xFFFF and wrap),
and bounds the name length and section header count against the reader for the same reason.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): bound resource recursion depth and cap declared sizes absolutely
Review follow-ups on this branch. The headline one is a real crash: the resource
walk had no depth bound, and the `budget` comment claiming depth "falls out for
free" was wrong, since the budget scales with the section while the stack does
not. A chain of directories at distinct RVAs (which the `dirs` set does not
catch) recursed until the goroutine stack gave out. A 13.7MB crafted PE took the
whole process down with `fatal error: stack overflow`, which is unrecoverable,
so unlike every other bug on this branch no caller could log-and-continue past
it. Now capped at 32 levels, versus the 3 the format actually uses.
The other bounds moved from "the file" to a real ceiling. Clamping an allocation
to the bytes remaining in the file is not enough on its own: a mostly empty file
costs almost nothing inside a compressed layer, so a cheap artifact could still
authorize a multi-gigabyte read. Both the PE data directories and the bundle
marker search now cap the same way `elfutil` already does.
Also hardened in the bundle reader:
- `headerOffset` is validated against the file. It comes straight out of the
file bytes and everything downstream seeks to it, so it was the least
trustworthy value in there and the only one not being checked.
- a manifest entry count that could not fit in the remaining bytes is rejected
rather than walked for hundreds of millions of reads.
- a non-positive `searchLimit` falls back to the whole file instead of reading
nothing. It is only ever an optimization hint, so treating it as authoritative
let one bogus ELF `p_offset` hide a bundle from us entirely.
- a short read no longer aborts the search. `unionreader` can return fewer bytes
than the file reports, and the marker may well be in what we did get.
`pe.Read` now reports partial parses instead of choosing between dropping the
package and hiding the problem. It was inconsistent: a malformed resource tree
was non-fatal while an unreadable resource *section* one call away was fatal, and
the non-fatal path only reached a trace log, so the unknowns task could not see
that a file had been cataloged on incomplete evidence. Both now land on a new
`File.ParseErr`, which the dotnet and PE catalogers join into their unknowns and
still keep the package.
Worth noting for changelog purposes: the `isTruncated` guard fixes a
user-visible bug beyond the hardening. A version resource leaving 1 to 5
trailing bytes cuts the next struct in half, which reports as
`io.ErrUnexpectedEOF` rather than `io.EOF`; that used to propagate out and skip
the `VS_FIXEDFILEINFO` fallback, so those binaries lost their version entirely.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): treat a zero bundle offset as unbundled, not malformed
The apphost ships the 32-byte bundle signature compiled in with its 8-byte header
offset left as zero, and only `HostWriter.SetAsBundle` fills that slot in when you
publish as a single file. So a zero offset means "never bundled", which is what
every framework-dependent .NET executable looks like.
The bounds work on this branch folded that case in with the hostile ones, so the
most common shape a .NET binary takes started reporting a parse failure. It
surfaces through the PE cataloger rather than the dotnet one, since an apphost
carries no CLR metadata of its own: the package still comes out, with a spurious
unknown attached to it. `main` had this right and said so in a comment; the
regression is on this branch.
Reproduces on a fixture already in the repo. Of the 53 PE files in
`image-net8-app`, `/app/dotnetapp.exe` was the only one reporting anything, and
the whole image is clean again now.
The three outcomes are now distinct, since collapsing any two of them loses
something the caller needs: zero means there is no bundle, an error means we could
not tell, and a positive offset is the answer.
`Test_Read_DotNetDetection` gains the apphost and, more usefully, an assertion
that a well-formed binary reports no partial parse at all. That one line is what
would have caught this, and it now covers all six fixtures. The unit case that
pinned the old behavior asserted the bug, so it flips.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): measure readers with the shared helper and report refusals
Two things, both about not re-deriving what the campaign already has.
`internal/file.ReaderSize` exists on `main` for exactly this job, and `upx.go`
already uses it. Its doc comment is the problem statement here almost verbatim: a
declared size is worth nothing until you weigh it against a byte count the reader
can actually back, which it does by reading the last byte. This branch was
hand-rolling `Seek(0, io.SeekEnd)` in three places, one of them reimplementing the
helper's save-and-restore dance verbatim, and then absorbing the reader-lies case
downstream by tolerating short reads.
Adopting it means the bundle reader and `readDataFromRVA` now take the reader
types they were always being handed (`unionreader.UnionReader` and `io.ReaderAt`),
and the reads move to `ReadAt`, which leaves the caller's cursor alone and drops
two seeks. Net effect is a smaller diff than what it replaces, and the case the
`shortReader` fake was invented for is now caught up front instead of tolerated:
the fake was modeling a lie about length, which is not what a short squashfs read
actually does. It now models the real shape (honest length, read stops early) and
a second reader covers the lie.
Three places also still returned "there is nothing here" when what they meant was
"I could not tell", which is the thing `File.ParseErr` was added to stop:
- an ELF that `elfutil` refused to parse read as "not an ELF". `ErrDeclaredSizeExceeded`
is exported precisely so a refusal can be told apart from a broken file, and the
caller upstream already turns an error into an unknown.
- a `deps.json` past the 3MB cap returned "". That is the file's entire dependency
list, so this handed back an SBOM that looked complete and silently was not.
- the marker search clamped at 512MB and then reported no bundle. It cannot claim
there is no bundle when it declined to look everywhere one could be, so a miss
after clamping is now an error.
`readSizeRecorder` records `ReadAt` alongside `Read`, or the allocation-bound tests
would have quietly stopped measuring anything.
Also renames `findSignatureOffset` back to `findBundleHeaderOffset`, since it
returns a header offset and reading it as "no signature" is what the last bug was,
and fixes the stale `ExtractDepsJSONFromBundle` doc comment.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* test(dotnet): assert allocation sizes directly in the bundle search guards
The PE and ELF search guards asserted the largest read the reader was asked for.
That happened to track the allocation, since the whole buffer goes to ReadAt, but
nothing said so. Both now measure the bytes allocated with the same `measureAlloc`
helper the resource guards use, and keep the read-size check as a separate claim.
Also names the reason a reader can report a size it cannot back: `debug/elf` and
`debug/pe` hand out `io.NewSectionReader(r, 0, 1<<63-1)`, whose `Size()` is nominal
rather than measured, so answering a size is not on its own evidence the bytes are
there. Renames `lyingReader` to `unbackedSizeReader` and regroups the two reader
implementations, which had their methods interleaved.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Decoding an SPDX document into the syft model was quadratic in document
size. findRootPackages (spdxhelpers/to_syft_model.go), which locates the
packages the document DESCRIBES, scanned every relationship for every
package. On a 119MB SPDX JSON document (70,701 packages, 153,301
relationships) that is 1.1e10 comparisons and 91% of all CPU samples; on a
238MB document it is 4.3e10 comparisons and 95%. A 1.3GB document did not
finish in 91 minutes. Everything else on the decode path is map-indexed and
linear, and spdx/tools-golang's own parse is linear (4.5s and 9.3s for the
two documents above). The scan dates from 9480f10c (top-level SPDX package
support, 2023).
The function now indexes the DESCRIBES / DESCRIBED_BY relationships once,
counting how many times each element is described by the DOCUMENT, then
walks the packages once. Semantics are unchanged: results follow package
order, an element described by more than one edge still appears once per
edge (so the existing "exactly one root" check behaves as before), only
edges touching DOCUMENT count, and nil entries are skipped.
Measured on an Apple M1 Max, `syft convert <spdx> -o <format>=<file>`:
input target before (main) after
119MB spdx-json@2.3 68.2s / 1.3GB 10.4s / 1.3GB
119MB spdx-json@2.2 70.2s / 1.3GB 12.6s / 1.3GB
238MB spdx-json@2.3 291.2s / 2.3GB 20.9s / 2.6GB
1.3GB spdx-json@2.3 killed at 91min 115.5s / 13.6GB
1.3GB spdx-json@2.2 not attempted 136.3s / 14.1GB
1.3GB syft-json not attempted 102.6s / 14.1GB
1.3GB cyclonedx-json not attempted 114.1s / 13.1GB
Isolating the conversion step (ToSyftModel) from the tools-golang parse:
119MB 72.3s -> 1.5s, 238MB 265.8s -> 3.1s. The remaining decode time and
all of the memory are in tools-golang, which parses the document several
times over; that is a separate, upstream concern.
Output equivalence was checked by converting 10 SPDX documents (the 8 SPDX
fixtures in this repository plus the 119MB and 238MB documents) to
syft-json, spdx-json@2.3, spdx-json@2.2, spdx-tag-value and
cyclonedx-json@1.6 with both binaries and comparing the results with
timestamps and generated ids removed: all 50 pairs are identical.
Tests pin the original semantics (including the duplicate-edge corner case)
and guard the complexity with a 50k x 100k document that must complete in
under 10s; the old implementation takes about a minute on it.
Signed-off-by: juanjsebgarcia <github@juangarcia.co.uk>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* test(dotnet): speed up pe fixture tests
the pe package took ~90s under -race, nearly all of it loading docker archive
fixtures. two things:
the three bundle fixtures published with `SelfContained=true`, so each `hello.exe`
carried a full .NET runtime (~60MB, 33MB tar). the bundle format is what those
fixtures exercise and it is identical framework-dependent, so they now publish
with `SelfContained=false` (160KB exe, 2MB tar). the race detector multiplies the
layer inflate + diffID hashing by ~15x, which is why the size mattered so much.
`fixtureFile` also loaded the image and built a squashed resolver per table row,
so `image-net8-app` was loaded four times for four files in it. resolvers are now
built once per distinct fixture from the parent test.
87s -> 28.5s under -race, and ~60MB less fixture cache.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix(dotnet): read the right file type from V1 single-file bundles
V1 bundles (.NET Core 3.x) predate the `unknown` member at the head of the
manifest file type enum, so their types are all one less than V2+: deps.json is
2, runtimeconfig.json is 3. we were matching on the V2+ value, which silently
matched runtimeconfig.json instead, so every .NET Core 3.x single-file app
reported its runtimeconfig as the embedded deps.json and its dependencies went
unseen.
the existing fixture test asserted the buggy output (it expected
`runtimeOptions`, which only ever appears in a runtimeconfig). the assertions
now pin the framework moniker out of each fixture's real deps.json, so reading
the neighboring file can't pass.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* test(dotnet): drop the 64MB jruby binary from the net8-app fixture
the `image-net8-app` fixture wget'd a 64MB jruby release binary at build time to
serve as "a PE that is not a .NET binary", which was 64MB of the image's 65MB.
every test that loads that image pays for it, in both the dotnet cataloger and
the pe package, and it sits in the fixture cache twice.
the same jruby binary already has its own fixture in the binary cataloger
(`image-jruby`), asserted through the same `pe.Read`, so the coverage is not
lost. the negative case here now uses `/app/dotnetapp.exe` -- the native apphost
that is already in the image -- which pairs nicely with the single file case,
same apphost with the app bundled in and CLR evidence present.
also drops a github release download from the fixture build.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* fix oom when testing
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* right-size CI test runners
the `r` family and `ram=32+128` were sized for the tmpfs footprint rather
than the workload. with the caches back on disk the build+test peak fits in
16GB, so move to `m`/`c` at `ram=16+32`.
- ~27% cheaper per instance-hour on spot, and a much deeper pool to schedule
against (~20 x86 types vs 7)
- `cpu=4+8` with `ram=16+32` lets price-capacity-optimized opportunistically
land an 8 vCPU box when it's cheap
- plain `volume=100gb` replaces the provisioned throughput. a 4 vCPU instance
caps near 150MB/s of EBS and gp3's free baseline is already 125MB/s, so the
override bought nothing
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
---------
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
the base image is digest-pinned to a bullseye snapshot whose InRelease has
expired, so `apt-get update` now exits 100 on any cold fixture build. rather
than disable Check-Valid-Until (which just delays the next break), skip apt
entirely:
- `less` was never used and `unzip` already ships in the base image
- rezip with `jar cfM` from the base JDK instead of installing `zip`
the `-M` matters here: this fixture intentionally has no META-INF/MANIFEST.MF,
and plain `jar cf` would generate one.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
`archive.ubuntu.com/ubuntu/pool/main` only keeps the current version of a
package, so the pinned `zlib1g 1:1.3.dfsg-3.1ubuntu2.1` URL started 404ing.
`curl` without `-f` wrote the error page into `zlib1g.deb` and exited 0, so
the fixture image built clean and just contained a non-deb, failing
`TestDpkgArchiveCataloger` only when the fixture cache was cold.
Now points at the launchpad `+files` URL, which is immutable. Also added `-f`
to the other fixture Dockerfiles that curl a pinned artifact so the next bit
of URL rot fails the build instead of quietly poisoning a fixture.
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
* 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>
Alex Goodman
anchore-oss-update-botandanchore-oss-update-bot
dependabot[bot]
Juan
Rez Moss
Vaibhav Srivastava
Ankush Pathak
Pujitha PaladuguandPujitha Paladugu
Matt Van HornandChristopher Phillips
Sueun ChoandChristopher Phillips
addielarue
Xenira
