* 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>
Adding tests for the Binary classifier cataloger
Tip
TL;DR to add a test for a new classifier:
- head to the correct directory:
cd testdata- add a new entry to
config.yamlto track where to get the binary from (verify the entry withmake list)- run
make downloadto get the binary- run
make add-snippetand follow the prompts (use/to search; answeryat the(Y/n/q)prompt to write the snippet)- add a new test case to
Test_Cataloger_PositiveCasesin../classifier_cataloger_test.go- if you added a new classifier, regenerate the capabilities catalog from the repo root and commit the result:
go generate ./internal/capabilities
To test the binary cataloger we run it against a set of files ("test fixtures"). There are two kinds of test fixtures:
- Full binaries: files downloaded and cached at test runtime
- Snippets: ~100 byte files checked into the repo
The upside with snippets is that they live with the test, don't necessarily require network access or hosting concerns, and are easy to add. The downside is that they are not the entire real binary so modifications may require recreating the snippet entirely.
The upside with full binaries is that they are the "Real McCoy" and allows the business logic to change without needing to update the fixture. The downside is that they require network access and take up a lot of space. For instance, downloading all binaries for testing today requires downloading ~15GB of container images and ends up being ~500MB of disk space.
You can find the test fixtures at the following locations:
syft/pkg/cataloger/binary/testdata/
└── classifiers/
├── bin/ # full binaries
├── ...
└── snippets/ # snippets
And use tooling to list and manage the fixtures:
make list- list all fixturesmake download- download binaries that are not covered by a snippetmake download-all- download all binariesmake add-snippet- add a new snippet based off of a configured binarycapture-snippet.sh- add a new snippet based off of a binary on your local machine (not recommended, but allowed)
There is a config.yaml that tracks all binaries that the tests can use. This makes it possible to download it at any time from a hosted source. Today the only method allowed is to download a container image and extract files out.
Here is an example entry in config.yaml for a binary reference from a container image:
from-images:
# note the group-name is assumed from the single binary name extracted: "redis-server"
- version: 7.2.3
images:
# note we're pulling the same binary from multiple images (representing different architectures)
- ref: redis:7.2.3@sha256:d4c84914b872521e215f77d8845914c2268a96b0e35bacd5691e1f5e1f88b500
platform: linux/amd64
- ref: redis:7.2.3@sha256:a0a0c38b31011b813cddf78d997f8ccba13019c27efd386984b0cfc1e4b618ff
platform: linux/arm64
# the paths to extract from the binary...
paths:
- /usr/local/bin/redis-server
# since there are multiple binaries in the image, we need to specify the group-name
- name: ruby-bullseye-shared-libs
version: 2.7.7
images:
- ref: ruby:2.7.7-bullseye@sha256:055191740a063f33fef1f09423e5ed8f91143aae62a3772a90910118464c5120
platform: linux/amd64
paths:
- /usr/local/bin/ruby
- /usr/local/lib/libruby.so.2.7.7
- /usr/local/lib/libruby.so.2.7
Note
You will need a system with
go,bash,strings, andxxdinstalled to capture test snippets.
Testing
The test cases have been setup to allow testing against full binaries or a mix of both (default).
To force running only against the original full binaries (instead of snippets), run with the
-must-use-original-binaries flag:
go test -v ./syft/pkg/cataloger/binary -run Test_Cataloger_PositiveCases/ -must-use-original-binaries
To run against a single fixture, append its name to the -run filter:
go test -v ./syft/pkg/cataloger/binary -run Test_Cataloger_PositiveCases/<name> -must-use-original-binaries
Adding a new test fixture
Adding a snippet (recommended)
Even if you are adding a snippet, it is best practice to:
- create that snippet from a full binary (not craft a snippet by hand)
- track where the binary is from and how to download it in
config.yaml
-
Follow the steps above to add a full binary
-
Run
make add-snippetand follow the prompts to create a new snippet (answeryat the(Y/n/q)prompt to write the snippet)- you should see your binary in the list of binaries to choose from. If not, check step 2
- if the search results in no matching snippets, you can specify your own search with
--search-for <grep-pattern> - you should see a new snippet file created in
snippets/
-
Write a test that references your new snippet by
<name>/<version>/<architecture><name>is the name of the binary (e.g.curl) or the name inconfig.yamlif specified- note that your test does not know about if it's running against a snippet or a full binary
Adding a full binary
- Add a new entry to
config.yamlwith the following fields
- if you are adding a single binary, the
namefield does not need to be specified - the
namefield is useful for distinguishing a quality about the binary (e.g.javavsjava-jre-ibm)
- Run
make downloadand ensure your new binary is downloaded
Adding a custom snippet
If you need to add a snippet that is not based off of a full binary, you can use the capture-snippet.sh script.
./capture-snippet.sh <binary-path> <version> [--search-for <pattern>] [--length <length>] [--prefix-length <prefix_length>] [--group <name>]
This is not recommended because it is not reproducible and does not allow for the test to be run against a full binary.