diff --git a/syft/pkg/cataloger/golang/scan_binary.go b/syft/pkg/cataloger/golang/scan_binary.go index ba57469f1..b0ccf1aff 100644 --- a/syft/pkg/cataloger/golang/scan_binary.go +++ b/syft/pkg/cataloger/golang/scan_binary.go @@ -2,6 +2,7 @@ package golang import ( "debug/buildinfo" + "errors" "fmt" "io" "runtime/debug" @@ -38,6 +39,11 @@ func scanFile(location file.Location, reader unionreader.UnionReader, captureSym bi, err := getBuildInfo(r, location) if err != nil { log.WithFields("file", location.RealPath, "error", err).Trace("unable to read golang buildinfo") + if errors.Is(err, errUPXDecompress) { + // a packed Go binary we could not unpack means its packages are missing from the SBOM. + // Every other failure here is usually just "not a Go binary", which is not worth reporting. + errs = unknown.Appendf(errs, location, "unable to read golang buildinfo: %w", err) + } continue } @@ -153,7 +159,11 @@ func getBuildInfo(r io.ReaderAt, location file.Location) (bi *debug.BuildInfo, e // if the direct read fails the file may be UPX-packed, in which case .go.buildinfo is compressed. // decompressUPX locates the header itself and reports errNotUPX when there is none, so there is no // cheaper pre-check to make here: scanning for the magic separately reads the same 8KB twice. - if upxBI := buildInfoFromUPX(r, location); upxBI != nil { + upxBI, upxErr := buildInfoFromUPX(r, location) + if upxErr != nil { + return nil, upxErr + } + if upxBI != nil { return upxBI, nil } @@ -172,13 +182,26 @@ func getBuildInfo(r io.ReaderAt, location file.Location) (bi *debug.BuildInfo, e return bi, err } -// buildInfoFromUPX decompresses a UPX-packed binary and reads its build info. A nil result means there -// was nothing to unpack or nothing Go inside it, so the caller falls through to its normal handling. -func buildInfoFromUPX(r io.ReaderAt, location file.Location) *debug.BuildInfo { +// buildInfoFromUPX decompresses a UPX-packed binary and reads its build info. +// +// A nil result with a nil error means there is nothing to report: no UPX header, a header that did not +// belong to real UPX output, a compression method we have not implemented, or a file that unpacked fine +// and simply is not Go. The caller falls through to its normal handling in all of those cases. +// +// A non-nil error means decompressUPX got far enough to be confident this is a packed binary and still +// could not unpack it, which is a gap in the SBOM rather than a file to skip quietly. decompressUPX marks +// exactly those cases with errUPXDecompress, so new guards there stay quiet unless they opt in. +func buildInfoFromUPX(r io.ReaderAt, location file.Location) (*debug.BuildInfo, error) { decompressed, err := decompressUPX(r) if err != nil { - log.WithFields("path", location.RealPath, "error", err).Trace("not a readable UPX-packed binary") - return nil + if !errors.Is(err, errUPXDecompress) { + // not a packed binary we can say anything about: a stray "UPX!" in a string constant, an + // implausible header, or NRV2B/NRV2D output, which upx produces unless --lzma was passed. + // Reporting these would attach an unknown to every packed non-Go binary in an image. + log.WithFields("path", location.RealPath, "error", err).Trace("not a readable UPX-packed binary") + return nil, nil + } + return nil, err } log.WithFields("path", location.RealPath).Trace("decompressed a UPX-packed binary to read the build info") @@ -187,7 +210,7 @@ func buildInfoFromUPX(r io.ReaderAt, location file.Location) *debug.BuildInfo { if err != nil { log.WithFields("path", location.RealPath, "error", err). Trace("unable to read build info from the decompressed UPX binary") - return nil + return nil, nil } - return bi + return bi, nil } diff --git a/syft/pkg/cataloger/golang/upx.go b/syft/pkg/cataloger/golang/upx.go index c48a5b521..23ba4f8de 100644 --- a/syft/pkg/cataloger/golang/upx.go +++ b/syft/pkg/cataloger/golang/upx.go @@ -178,6 +178,13 @@ var ( errUPXOutputExceeded = errors.New("UPX blocks decompress to more than the declared original size") errUPXImplausibleHeader = errors.New("implausible UPX header") errUPXInvalidLZMAParams = errors.New("invalid LZMA parameters") + + // errUPXDecompress marks a file that got past the header and the method dispatch and still could not + // be unpacked, which is a gap in the SBOM rather than a file to skip. It is deliberately the only + // signal the caller acts on: everything else this file returns (a stray magic, an implausible header, + // a method we have not implemented) means "not something we can catalog" and stays quiet, so a new + // guard added later is silent by default instead of turning into SBOM noise. + errUPXDecompress = errors.New("unable to decompress UPX-compressed Go binary") ) // upxInfo contains parsed UPX header information @@ -270,7 +277,9 @@ func unfilter49(data []byte, cto8 byte) { // Why this matters: simply concatenating decompressed blocks produces invalid output. // Each block corresponds to a PT_LOAD segment and must be placed at its correct file offset. // -// Returns the decompressed binary as a bytes.Reader (implements io.ReaderAt). +// Returns the decompressed binary as a bytes.Reader (implements io.ReaderAt). errNotUPX and +// errUPXImplausibleHeader mean the file is not really packed; only errUPXDecompress means a packed file +// we could not read. func decompressUPX(r io.ReaderAt) (io.ReaderAt, error) { info, err := parseUPXInfo(r) if err != nil { @@ -342,7 +351,7 @@ func decompressUPXBlocks(r io.ReaderAt, info *upxInfo) ([]byte, error) { // nothing here (remaining is already <= p_filesize) and real output sits at exactly p_blocksize, // so that check would run with no headroom against a value the format does not guarantee. if block.uncompressedSize > remaining { - return nil, fmt.Errorf("%w: block %d claims %d with %d left of %d", + return nil, fmt.Errorf("%w: %w: block %d claims %d with %d left of %d", errUPXDecompress, errUPXOutputExceeded, blockNum+1, block.uncompressedSize, remaining, info.originalSize) } remaining -= block.uncompressedSize @@ -368,7 +377,7 @@ func decompressUPXBlocks(r io.ReaderAt, info *upxInfo) ([]byte, error) { } if err := decompressBlock(r, block, decompressor, dst); err != nil { - return nil, err + return nil, fmt.Errorf("%w: %w", errUPXDecompress, err) } // the first block carries the original ELF headers, which place every block after the second diff --git a/syft/pkg/cataloger/golang/upx_security_test.go b/syft/pkg/cataloger/golang/upx_security_test.go index 7c3c428f5..815160c04 100644 --- a/syft/pkg/cataloger/golang/upx_security_test.go +++ b/syft/pkg/cataloger/golang/upx_security_test.go @@ -108,6 +108,7 @@ func TestDecompressUPX_OversizedBlockRejected(t *testing.T) { _, err := decompressUPX(bytes.NewReader(data)) require.Error(t, err) assert.ErrorIs(t, err, errUPXOutputExceeded) + assert.ErrorIs(t, err, errUPXDecompress, "a plausible header that fails to unpack is reportable") }) } } @@ -434,10 +435,27 @@ func TestDecompressUPX_HeaderWithoutDecodableBlocksAllocatesNothing(t *testing.T _, err := decompressUPX(bytes.NewReader(data)) require.Error(t, err) assert.ErrorIs(t, err, errUPXImplausibleHeader) + assert.NotErrorIs(t, err, errUPXDecompress, "nothing was packed, so there is nothing to report") }) assert.Less(t, allocated, uint64(256<<10), "no block survived validation, so no output buffer is due") } +func TestDecompressUPX_UnsupportedMethodIsNotReportable(t *testing.T) { + // upx defaults to NRV2B unless --lzma is passed, and only LZMA is implemented here. A packed non-Go + // binary must not turn into an SBOM unknown from the golang cataloger, so the unsupported-method error + // must stay clear of errUPXDecompress. + block := make([]byte, 12) + binary.LittleEndian.PutUint32(block[0:4], 256) // sz_unc + binary.LittleEndian.PutUint32(block[4:8], 32) // sz_cpr + block[8] = 2 // b_method = NRV2B + data := padTo(append(buildUPXHeader(4096, 4096), block...), 128) + + _, err := decompressUPX(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUnsupportedUPXMethod) + assert.NotErrorIs(t, err, errUPXDecompress, "an unimplemented method is a capability gap, not a gap in the SBOM") +} + // TestGetBuildInfo_MaliciousUPXRejected exercises the entry point the reported vulnerability was reachable // through. The unit tests above call decompressUPX directly; this one goes through getBuildInfo, which is // what parseGoBinary uses and what the default-enabled go-module-binary-cataloger reaches. @@ -460,5 +478,6 @@ func TestGetBuildInfo_MaliciousUPXRejected(t *testing.T) { }) assert.Nil(t, bi) assert.Error(t, err) + assert.ErrorIs(t, err, errUPXDecompress, "a packed binary we cannot unpack is worth reporting") assert.Less(t, allocated, uint64(1<<20), "the rejection must not cost a megabyte") }