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>

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
+// buildInfoFromUPX decompresses a UPX-packed binary and reads its build info.
-// 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 {
+// 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")
+		if !errors.Is(err, errUPXDecompress) {
-		return nil
+			// 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
 }

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

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")
 }