From ffb691b0fe8ea8b93d6927d507b1028a703cb1d4 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Tue, 18 Aug 2026 15:28:25 -0400 Subject: [PATCH] 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 --- syft/pkg/cataloger/golang/scan_binary.go | 36 +- syft/pkg/cataloger/golang/upx.go | 396 +++++++++++---- .../pkg/cataloger/golang/upx_security_test.go | 464 ++++++++++++++++++ syft/pkg/cataloger/golang/upx_test.go | 53 +- 4 files changed, 806 insertions(+), 143 deletions(-) create mode 100644 syft/pkg/cataloger/golang/upx_security_test.go diff --git a/syft/pkg/cataloger/golang/scan_binary.go b/syft/pkg/cataloger/golang/scan_binary.go index e88aa91f2..ba57469f1 100644 --- a/syft/pkg/cataloger/golang/scan_binary.go +++ b/syft/pkg/cataloger/golang/scan_binary.go @@ -150,17 +150,11 @@ func getBuildInfo(r io.ReaderAt, location file.Location) (bi *debug.BuildInfo, e return bi, nil } - // if direct read fails and this looks like a UPX-compressed binary, - // try to decompress and read the buildinfo from the decompressed data - if isUPXCompressed(r) { - log.WithFields("path", location.RealPath).Trace("detected UPX-compressed Go binary, attempting decompression to read the build info") - decompressed, decompErr := decompressUPX(r) - if decompErr == nil { - bi, err = readBuildInfo(decompressed) - if err == nil { - return bi, nil - } - } + // 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 { + return upxBI, nil } // note: the stdlib does not export the error we need to check for @@ -177,3 +171,23 @@ 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 { + decompressed, err := decompressUPX(r) + if err != nil { + log.WithFields("path", location.RealPath, "error", err).Trace("not a readable UPX-packed binary") + return nil + } + + log.WithFields("path", location.RealPath).Trace("decompressed a UPX-packed binary to read the build info") + + bi, err := readBuildInfo(decompressed) + if err != nil { + log.WithFields("path", location.RealPath, "error", err). + Trace("unable to read build info from the decompressed UPX binary") + return nil + } + return bi +} diff --git a/syft/pkg/cataloger/golang/upx.go b/syft/pkg/cataloger/golang/upx.go index 334e5d59d..c48a5b521 100644 --- a/syft/pkg/cataloger/golang/upx.go +++ b/syft/pkg/cataloger/golang/upx.go @@ -22,8 +22,7 @@ package golang // // # Key Functions // -// - isUPXCompressed: detects UPX magic bytes ("UPX!") in the binary -// - decompressUPX: main entry point; decompresses all blocks and reconstructs the ELF +// - decompressUPX: main entry point; locates the header, decompresses all blocks and reconstructs the ELF // - decompressLZMA: handles UPX's custom 2-byte LZMA header format // - unfilter49: reverses the CTO (call trick optimization) filter for x86/x64 code // - parseELFPTLoadOffsets: extracts PT_LOAD segment offsets for proper block placement @@ -91,25 +90,30 @@ package golang // - Header corruption: checksums or version fields modified. // Recovery: ignore validation and use PackHeader values as authoritative source. // -// This would require parsing of the PackHeader, located in the final 36 bytes of the file, contains -// metadata recoverable even if p_info is corrupted (not parsed today): +// All of that would require parsing the PackHeader, which occupies the final 36 bytes of the file and +// carries metadata that survives a corrupted p_info. Not parsed today: // -// Offset Size Field Description -// ────────────────────────────────────────────────────────── -// 0x00 4 UPX magic "UPX!" (0x21585055) -// 0x04 1 version UPX version -// 0x05 1 format Executable format -// 0x06 1 method Compression method -// 0x07 1 level Compression level (1-10) -// 0x08 4 u_adler Uncompressed data checksum -// 0x0C 4 c_adler Compressed data checksum -// 0x10 4 u_len Uncompressed length -// 0x14 4 c_len Compressed length -// 0x18 4 u_file_size Original file size ← Recovery point -// 0x1C 1 filter Filter ID -// 0x1D 1 filter_cto Filter CTO parameter -// 0x1E 1 n_mru MRU parameter -// 0x1F 1 header_checksum Header checksum +// Offset Size Field Description +// ────────────────────────────────────────────────────────── +// 0x00 4 UPX magic "UPX!" (0x21585055) +// 0x04 1 version UPX version +// 0x05 1 format Executable format +// 0x06 1 method Compression method +// 0x07 1 level Compression level (1-10) +// 0x08 4 u_adler Uncompressed data checksum +// 0x0C 4 c_adler Compressed data checksum +// 0x10 4 u_len Uncompressed length +// 0x14 4 c_len Compressed length +// 0x18 4 u_file_size Original file size ← Recovery point +// 0x1C 1 filter Filter ID +// 0x1D 1 filter_cto Filter CTO parameter +// 0x1E 1 n_mru MRU parameter +// 0x1F 1 header_checksum Header checksum +// +// Confirmed against the image-small-upx fixture, whose trailing 36 bytes decode as version=14, +// format=22, method=14 (LZMA), level=10, u_len=5811393, c_len=2966652, u_file_size=5811393, +// filter=0x49, filter_cto=0x24. Note u_file_size matches the p_filesize in p_info, and filter and +// filter_cto match the b_ftid and b_cto8 of the filtered block, so the recovery path above is real. import ( "bytes" @@ -119,8 +123,15 @@ import ( "io" "github.com/ulikunitz/xz/lzma" + + intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/log" ) +// upxMagicScanWindow is how far into the file the "UPX!" magic is searched for. UPX places l_info +// just past the ELF headers and its loader stub, so this covers real output with room to spare. +const upxMagicScanWindow = 8192 + // UPX compression method constants const ( upxMethodLZMA uint8 = 14 // M_LZMA in UPX source @@ -131,17 +142,46 @@ const ( upxFilterCTO uint8 = 0x49 // CTO (call trick optimization) filter for x86/x64 ) +// bounds on what a UPX header may claim before we act on it +const ( + // maxUPXOriginalSize is the absolute ceiling on p_filesize. A ratio against the input size cannot be + // the only bound: LZMA encodes a run of N identical bytes in O(log N) bytes, so a legitimate binary + // embedding a large compressible asset expands by hundreds of times (a `go:embed` of 120MB of zeros + // packs to 609KB, a 209x ratio). It is the ceiling and maxUPXExpansion together that bound this. + maxUPXOriginalSize = 500 * intFile.MB + + // maxUPXExpansion bounds p_filesize against the size of the file actually on disk, so a few dozen + // header bytes cannot claim the absolute ceiling. Real `upx --best --lzma` output expands 1.96x on the + // image-small-upx fixture, and the most compressible case above is 209x against the packed asset, so + // this leaves a wide margin; the point is only that the claim has to be paid for in input bytes. + // Applied only when the input size can be determined, otherwise the ceiling stands alone. + maxUPXExpansion = 1024 + + // maxUPXBlocks bounds the block loop. Real UPX emits one block for the ELF headers plus one per + // PT_LOAD extent (four on the image-small-upx fixture), so single digits; this leaves room for an + // unusual layout while keeping a file from driving unbounded iterations with minimum-size blocks. + maxUPXBlocks = 1024 + + // maxUPXLZMALiteralBits caps lc+lp. The decoder allocates and initializes a 0x300<<(lc+lp) entry + // probability array per block, independent of block size, so the library's own ceiling of 12 costs + // 6.3MB of stores for a one-byte block. Across maxUPXBlocks blocks that adds up, and UPX emits lc+lp + // of 3 or less, so this stays well clear of real output while cutting the per-block cost by 16x. + maxUPXLZMALiteralBits = 8 +) + var ( // upxMagic is the magic bytes that identify a UPX-packed binary upxMagic = []byte("UPX!") errNotUPX = errors.New("not a UPX-compressed binary") errUnsupportedUPXMethod = errors.New("unsupported UPX compression method") + 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") ) // upxInfo contains parsed UPX header information type upxInfo struct { - magicOffset int64 version uint8 format uint8 originalSize uint32 // p_filesize - original uncompressed file size @@ -159,8 +199,11 @@ type blockInfo struct { dataOffset int64 } -// upxDecompressor is a function that decompresses data using a specific method -type upxDecompressor func(compressedData []byte, uncompressedSize uint32) ([]byte, error) +// upxDecompressor decompresses compressedData into dst, which the caller has already sized to the +// block's declared uncompressed size and bounds-checked against the output buffer. Writing into a +// caller-owned slice rather than returning a fresh one is what keeps a single block from doubling +// peak memory; implementations must fill dst exactly and must not grow it. +type upxDecompressor func(compressedData, dst []byte) error // upxDecompressors maps compression methods to their decompressor functions var upxDecompressors = map[uint8]upxDecompressor{ @@ -211,18 +254,6 @@ func unfilter49(data []byte, cto8 byte) { } } -// isUPXCompressed checks if the reader contains a UPX-compressed binary -func isUPXCompressed(r io.ReaderAt) bool { - // UPX magic can be at various offsets depending on the binary format - // scan the first 4KB for the magic bytes - buf := make([]byte, 4096) - n, err := r.ReadAt(buf, 0) - if err != nil && !errors.Is(err, io.EOF) { - return false - } - return bytes.Contains(buf[:n], upxMagic) -} - // decompressUPX attempts to decompress a UPX-compressed ELF binary. // It reads blocks and places them at correct file offsets based on ELF PT_LOAD segments. // @@ -233,10 +264,10 @@ func isUPXCompressed(r io.ReaderAt) bool { // ptLoadOffsets := parseELFPTLoadOffsets(block1Data) // // - Block 1: placed at offset 0 (contains ELF header + program headers) -// - Block 2: placed at offset 0 (overwrites/extends) +// - Block 2: placed immediately after block 1 (running outputOffset, not offset 0) // - Block 3+: placed at ptLoadOffsets[blockNum-2] // -// Why this matters: Simply concatenating decompressed blocks produces invalid output. +// 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). @@ -246,81 +277,147 @@ func decompressUPX(r io.ReaderAt) (io.ReaderAt, error) { return nil, err } - // allocate buffer for the full decompressed output - output := make([]byte, info.originalSize) + output, err := decompressUPXBlocks(r, info) + if err != nil { + return nil, err + } + if output == nil { + // a plausible header with nothing decodable behind it is not a packed binary + return nil, fmt.Errorf("%w: no decodable blocks", errUPXImplausibleHeader) + } + + return bytes.NewReader(output), nil +} + +// decompressUPXBlocks walks the b_info chain, decoding each block into its place in the reconstructed +// file. It returns nil (with a nil error) when no block survived validation. A short or malformed chain +// yields the blocks placed so far rather than an error, since those are often enough to recover +// .go.buildinfo; only a block that claims more output than the header allows, or one that fails to +// decode, fails the file. +func decompressUPXBlocks(r io.ReaderAt, info *upxInfo) ([]byte, error) { + // output is allocated on the first block that survives validation rather than up front, so a header + // that claims a large p_filesize but carries no decodable block costs nothing. It is the dominant + // allocation on this path, bounded by parseUPXInfo. Blocks decode directly into slices of it rather + // than into a per-block buffer that is then copied, which keeps a single block from doubling peak + // memory. Also live during a decode: the LZMA decoder's dictionary (up to 128MB, see maxDictSize) and + // the compressed block being read. + var output []byte + + // UPX packs the original file as a series of blocks that together reconstruct p_filesize. Track the + // unclaimed remainder so the block headers cannot drive more decompression than the file itself + // declares: without this, every block may individually claim the whole original size, and the total + // work becomes (block count x original size). This is the bound that does the real work here. + remaining := info.originalSize currentOffset := info.firstBlockOff outputOffset := uint64(0) blockNum := 0 - - // track PT_LOAD segment offsets for proper block placement var ptLoadOffsets []uint64 - for { + for blockNum < maxUPXBlocks { block, err := readBlockInfo(r, currentOffset) if err != nil { - return nil, fmt.Errorf("failed to read block info at offset %d: %w", currentOffset, err) - } - - // check for end marker (sz_unc == 0) - if block.uncompressedSize == 0 { + // a real UPX file runs out of b_info structures before it runs out of blocks: the bytes after + // the last block are the loader stub, not another header. Keep what is placed so far. + log.WithFields("block", blockNum+1, "offset", currentOffset, "error", err). + Trace("UPX block info unreadable, using partial output") break } + if block.uncompressedSize == 0 { + break // end marker + } - // non-LZMA method on first block is an error; on subsequent blocks it indicates end of data - if block.method != upxMethodLZMA { + // an unknown method on the first block means we cannot read this file at all; on a later block it + // marks the end of the block chain, since the trailing loader bytes are not a b_info. + decompressor, ok := upxDecompressors[block.method] + if !ok { if blockNum == 0 { return nil, fmt.Errorf("%w: method %d", errUnsupportedUPXMethod, block.method) } break } + + // the blocks together reconstruct p_filesize, so the running remainder bounds the total output. + // Deliberately the only size check on a block: comparing sz_unc against p_blocksize as well adds + // 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", + errUPXOutputExceeded, blockNum+1, block.uncompressedSize, remaining, info.originalSize) + } + remaining -= block.uncompressedSize blockNum++ - decompressor, ok := upxDecompressors[block.method] - if !ok { - return nil, fmt.Errorf("%w: method %d", errUnsupportedUPXMethod, block.method) + if output == nil { + output = make([]byte, info.originalSize) } - // read compressed data for this block - compressedData := make([]byte, block.compressedSize) - _, err = r.ReadAt(compressedData, block.dataOffset) - if err != nil { - return nil, fmt.Errorf("failed to read compressed data: %w", err) - } - - // decompress this block - blockData, err := decompressor(compressedData, block.uncompressedSize) - if err != nil { - return nil, fmt.Errorf("failed to decompress block: %w", err) - } - - // apply CTO filter reversal if needed - if block.filterID == upxFilterCTO { - unfilter49(blockData, block.filterCTO) - } - - // first block contains ELF headers - parse PT_LOAD segments for subsequent blocks - if blockNum == 1 { - ptLoadOffsets = parseELFPTLoadOffsets(blockData) - } - - // determine where to place this block in the output + // blocks 1 and 2 run sequentially; blocks 3+ go to their PT_LOAD segment offset destOffset := outputOffset if blockNum > 2 && len(ptLoadOffsets) > blockNum-2 { - // blocks 3+ go to their respective PT_LOAD segment offsets destOffset = ptLoadOffsets[blockNum-2] } - // copy block data to output at correct offset - if destOffset+uint64(len(blockData)) <= uint64(len(output)) { - copy(output[destOffset:], blockData) + dst, ok := blockDest(output, destOffset, block.uncompressedSize) + if !ok { + // the file does not describe the layout it claims. Keep the blocks placed so far, but stop: + // outputOffset derives from destOffset, so continuing would carry the bad offset forward. + log.WithFields("block", blockNum, "offset", destOffset, "outputSize", len(output)). + Trace("UPX block placement out of range, using partial output") + break + } + + if err := decompressBlock(r, block, decompressor, dst); err != nil { + return nil, err + } + + // the first block carries the original ELF headers, which place every block after the second + if blockNum == 1 { + ptLoadOffsets = parseELFPTLoadOffsets(dst) } outputOffset = destOffset + uint64(block.uncompressedSize) currentOffset = block.dataOffset + int64(block.compressedSize) } - return bytes.NewReader(output), nil + if blockNum == maxUPXBlocks { + // truncated output, not a failure: the blocks placed so far may still carry .go.buildinfo. Logged + // because a legitimate file this far outside real UPX layout is worth knowing about. + log.WithFields("blocks", blockNum).Trace("UPX block count hit the cap, using partial output") + } + + return output, nil +} + +// blockDest returns the slice of output that a block of the given size occupies at destOffset, or false +// if it does not fit. destOffset comes from an ELF p_offset in the file, so the check is written as a +// subtraction to keep a uint64 overflow from slipping an out-of-range offset past destOffset+size. The +// result is capped to its own length so a decompressor that appends cannot reach the next block's region. +func blockDest(output []byte, destOffset uint64, size uint32) ([]byte, bool) { + outLen := uint64(len(output)) + if destOffset > outLen || uint64(size) > outLen-destOffset { + return nil, false + } + end := destOffset + uint64(size) + return output[destOffset:end:end], true +} + +// decompressBlock reads one block's compressed data and decodes it into dst, reversing the CTO filter +// if the block declares one. +func decompressBlock(r io.ReaderAt, block *blockInfo, decompressor upxDecompressor, dst []byte) error { + compressedData := make([]byte, block.compressedSize) + if _, err := r.ReadAt(compressedData, block.dataOffset); err != nil { + return fmt.Errorf("failed to read compressed data: %w", err) + } + + if err := decompressor(compressedData, dst); err != nil { + return fmt.Errorf("failed to decompress block: %w", err) + } + + if block.filterID == upxFilterCTO { + unfilter49(dst, block.filterCTO) + } + return nil } // parseELFPTLoadOffsets extracts PT_LOAD segment file offsets from ELF headers. @@ -345,10 +442,23 @@ func parseELFPTLoadOffsets(elfHeader []byte) []uint64 { phentsize := binary.LittleEndian.Uint16(elfHeader[0x36:0x38]) phnum := binary.LittleEndian.Uint16(elfHeader[0x38:0x3a]) + const elf64PhdrSize = 56 // fixed size of an ELF64 program header entry + + // the reads below use fixed offsets up to byte 16 of each entry, so a shorter entry must not be + // accepted. Loop-invariant, so it is checked once here rather than per iteration. + if phentsize < elf64PhdrSize { + return nil + } + + hdrLen := uint64(len(elfHeader)) var offsets []uint64 for i := range phnum { phStart := phoff + uint64(i)*uint64(phentsize) - if phStart+uint64(phentsize) > uint64(len(elfHeader)) { + // subtraction form so a large phoff cannot overflow phStart+phentsize past the buffer end. + // note: the `break` is load-bearing for that argument. It guarantees phoff <= hdrLen before any + // i >= 1 is reached, which is what keeps phStart itself from wrapping; a `continue` here would + // let phoff near 2^64 wrap into a small in-range phStart and read a bogus p_offset. + if phStart > hdrLen || hdrLen-phStart < uint64(phentsize) { break } @@ -365,10 +475,46 @@ func parseELFPTLoadOffsets(elfHeader []byte) []uint64 { return offsets } +// inputSize reports the size of the file behind r, or 0 if it cannot be determined. Both *bytes.Reader +// and *io.SectionReader answer directly; the UnionReader that reaches this path in a real scan only has +// Seek, so fall back to a save-and-restore seek. +func inputSize(r io.ReaderAt) int64 { + if sr, ok := r.(interface{ Size() int64 }); ok { + return sr.Size() + } + s, ok := r.(io.Seeker) + if !ok { + return 0 + } + cur, err := s.Seek(0, io.SeekCurrent) + if err != nil { + return 0 + } + size, err := s.Seek(0, io.SeekEnd) + if err != nil { + return 0 + } + if _, err := s.Seek(cur, io.SeekStart); err != nil { + return 0 + } + return size +} + +// maxOriginalSize is the ceiling p_filesize may claim for this input: the absolute cap, further limited +// by what the bytes on disk can plausibly expand to. A header only a few dozen bytes long must not be +// able to claim the absolute cap, since p_filesize sizes the output buffer directly. +func maxOriginalSize(r io.ReaderAt) uint64 { + size := inputSize(r) + if size <= 0 { + // no size available, so the absolute ceiling is all we have + return maxUPXOriginalSize + } + return min(uint64(size)*maxUPXExpansion, maxUPXOriginalSize) +} + // parseUPXInfo locates and parses the UPX header information func parseUPXInfo(r io.ReaderAt) (*upxInfo, error) { - // scan for the UPX! magic in the first 8KB - buf := make([]byte, 8192) + buf := make([]byte, upxMagicScanWindow) n, err := r.ReadAt(buf, 0) if err != nil && !errors.Is(err, io.EOF) { return nil, fmt.Errorf("failed to read header: %w", err) @@ -392,7 +538,7 @@ func parseUPXInfo(r io.ReaderAt) (*upxInfo, error) { // offset 4: p_filesize (4 bytes) - original file size // offset 8: p_blocksize (4 bytes) // - // b_info structures follow (12 bytes each): + // b_info structures follow (12 bytes each) and are read via ReadAt, not from buf: // offset 0: sz_unc (4 bytes) - uncompressed size of this block // offset 4: sz_cpr (4 bytes) - compressed size (may have filter bits) // offset 8: b_method (1 byte) @@ -400,15 +546,17 @@ func parseUPXInfo(r io.ReaderAt) (*upxInfo, error) { // offset 10: b_cto8 (1 byte) - filter parameter // offset 11: unused (1 byte) - if magicIdx+32 > n { - return nil, fmt.Errorf("UPX header truncated") + // the reads below reach magic+20 (the end of p_info); b_info comes from ReadAt later. Wrapped as an + // implausible header rather than its own error so the caller keeps treating it as "not really UPX". + const lInfoAndPInfoSize = 20 + if magicIdx+lInfoAndPInfoSize > n { + return nil, fmt.Errorf("%w: header runs past the end of the scan window", errUPXImplausibleHeader) } lInfoBase := buf[magicIdx:] pInfoBase := buf[magicIdx+8:] // p_info starts 8 bytes after magic info := &upxInfo{ - magicOffset: int64(magicIdx), version: lInfoBase[6], format: lInfoBase[7], originalSize: binary.LittleEndian.Uint32(pInfoBase[4:8]), @@ -416,9 +564,26 @@ func parseUPXInfo(r io.ReaderAt) (*upxInfo, error) { firstBlockOff: int64(magicIdx + 8 + 12), // magic + l_info remainder + p_info } - // sanity check - if info.originalSize == 0 || info.originalSize > 500*1024*1024 { - return nil, fmt.Errorf("invalid original size: %d", info.originalSize) + // the magic is found by an unanchored substring scan, so a stray "UPX!" in unrelated data (e.g. a + // string constant) can be read as a header. These checks are false-positive suppression, not + // hardening: the fields are attacker-controlled, and what actually bounds the work is the size limit + // below plus the running remainder in decompressUPX. + if info.version == 0 || info.format == 0 { + // l_version is the packheader version (11-14 in the wild) and l_format is a UPX_F_* id starting + // at 1, so neither is ever zero in real output. + return nil, fmt.Errorf("%w: version=%d format=%d", errUPXImplausibleHeader, info.version, info.format) + } + // p_blocksize is only checked for being set; it is not used as a bound. UPX derives it from a PT_LOAD + // extent, and on real output the largest block equals it exactly, so treating it as a ceiling on + // sz_unc would run with zero headroom against a value the format does not actually promise. + if info.blockSize == 0 { + return nil, fmt.Errorf("%w: p_blocksize is zero", errUPXImplausibleHeader) + } + // p_filesize sizes the output buffer, so this is the bound that matters most on this path + limit := maxOriginalSize(r) + if info.originalSize == 0 || uint64(info.originalSize) > limit { + return nil, fmt.Errorf("%w: p_filesize %d exceeds the %d byte limit for a %d byte input", + errUPXImplausibleHeader, info.originalSize, limit, inputSize(r)) } return info, nil @@ -435,8 +600,9 @@ func readBlockInfo(r io.ReaderAt, offset int64) (*blockInfo, error) { szUnc := binary.LittleEndian.Uint32(buf[0:4]) szCpr := binary.LittleEndian.Uint32(buf[4:8]) - // the compressed size may have filter info in the high bits - // for some formats, but for LZMA it's typically clean + // note: the high 8 bits of sz_cpr are masked off because some UPX formats store flags there. For the + // ELF format this code handles, filter data lives in b_ftid/b_cto8 instead, so the mask only matters + // for a block whose compressed size exceeds 16MB, where it would truncate the read. Not a size bound. block := &blockInfo{ uncompressedSize: szUnc, compressedSize: szCpr & 0x00ffffff, // lower 24 bits @@ -449,7 +615,9 @@ func readBlockInfo(r io.ReaderAt, offset int64) (*blockInfo, error) { return block, nil } -// nextPowerOf2 returns the smallest power of 2 >= n +// nextPowerOf2 returns the smallest power of 2 >= n, saturating at 2^31 rather than overflowing to 0, so +// the result is not >= n for n above 2^31. The saturation is unreachable from the only caller (dst is +// bounded by p_filesize, far below 2^31); it is here so the helper is not a trap if reused. func nextPowerOf2(n uint32) uint32 { if n == 0 { return 1 @@ -458,6 +626,9 @@ func nextPowerOf2(n uint32) uint32 { if n&(n-1) == 0 { return n } + if n > 1<<31 { + return 1 << 31 + } // find the highest set bit and shift left by 1 n-- n |= n >> 1 @@ -477,9 +648,9 @@ func nextPowerOf2(n uint32) uint32 { // - Byte 2+: raw LZMA stream (starts with 0x00 for range decoder init) // // Standard LZMA props encoding: props = lc + lp*9 + pb*9*5 -func decompressLZMA(compressedData []byte, uncompressedSize uint32) ([]byte, error) { +func decompressLZMA(compressedData, dst []byte) error { if len(compressedData) < 3 { - return nil, fmt.Errorf("compressed data too short") + return errors.New("compressed data too short") } // parse UPX's 2-byte LZMA header @@ -487,12 +658,26 @@ func decompressLZMA(compressedData []byte, uncompressedSize uint32) ([]byte, err lp := compressedData[1] >> 4 lc := compressedData[1] & 0x0f + // the header nibbles can hold values outside the LZMA ranges. This is a correctness check, not a + // bound: the library derives lc/lp/pb back out of the props byte by modular arithmetic, so it cannot + // be handed an out-of-range value. What it prevents is the uint8 math below wrapping (lc=15, lp=15, + // pb=7 gives 465, which truncates to 209) and silently decoding with parameters that never existed. + // lc+lp is capped separately, and that one is about allocation: see maxUPXLZMALiteralBits. + if lc > 8 || lp > 4 || pb > 4 { + return fmt.Errorf("%w: lc=%d lp=%d pb=%d", errUPXInvalidLZMAParams, lc, lp, pb) + } + if uint16(lc)+uint16(lp) > maxUPXLZMALiteralBits { + return fmt.Errorf("%w: lc+lp=%d exceeds %d", errUPXInvalidLZMAParams, lc+lp, maxUPXLZMALiteralBits) + } + // convert to standard LZMA properties byte props := lc + lp*9 + pb*9*5 // raw LZMA stream starts at byte 2 (includes 0x00 init byte) lzmaStream := compressedData[2:] + uncompressedSize := uint32(len(dst)) + // compute dictionary size: must be at least as large as uncompressed size // use next power of 2 for efficiency, with reasonable min/max bounds. // note: if you're seeing that testing small binaries works and large ones don't, @@ -507,21 +692,16 @@ func decompressLZMA(compressedData []byte, uncompressedSize uint32) ([]byte, err binary.LittleEndian.PutUint32(header[1:5], dictSize) binary.LittleEndian.PutUint64(header[5:13], uint64(uncompressedSize)) - // combine header + raw stream - var fullStream []byte - fullStream = append(fullStream, header...) - fullStream = append(fullStream, lzmaStream...) - - reader, err := lzma.NewReader(bytes.NewReader(fullStream)) + // MultiReader rather than concatenating, so the compressed block is not copied a second time + reader, err := lzma.NewReader(io.MultiReader(bytes.NewReader(header), bytes.NewReader(lzmaStream))) if err != nil { - return nil, fmt.Errorf("failed to create LZMA reader: %w", err) + return fmt.Errorf("failed to create LZMA reader: %w", err) } - decompressed := make([]byte, uncompressedSize) - _, err = io.ReadFull(reader, decompressed) - if err != nil { - return nil, fmt.Errorf("failed to decompress LZMA data: %w", err) + // dst is a slice of the caller's output buffer, sized to the block's declared uncompressed size + if _, err := io.ReadFull(reader, dst); err != nil { + return fmt.Errorf("failed to decompress LZMA data: %w", err) } - return decompressed, nil + return nil } diff --git a/syft/pkg/cataloger/golang/upx_security_test.go b/syft/pkg/cataloger/golang/upx_security_test.go new file mode 100644 index 000000000..7c3c428f5 --- /dev/null +++ b/syft/pkg/cataloger/golang/upx_security_test.go @@ -0,0 +1,464 @@ +package golang + +import ( + "bytes" + "encoding/binary" + "io" + "runtime/debug" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/ulikunitz/xz/lzma" + + intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/syft/file" +) + +// buildUPXLZMAStream encodes data into the compressed-block form decompressLZMA expects: UPX's custom +// 2-byte props header followed by the raw LZMA range-coded stream (the standard 13-byte .lzma header is +// stripped because decompressLZMA reconstructs its own). Uses the default lc=3/lp=0/pb=2 properties and a +// 64KB dictionary so the size math in decompressLZMA lines up for the small payloads used in tests. +func buildUPXLZMAStream(t *testing.T, data []byte) []byte { + t.Helper() + var buf bytes.Buffer + w, err := lzma.WriterConfig{ + Properties: &lzma.Properties{LC: 3, LP: 0, PB: 2}, + DictCap: 1 << 16, + SizeInHeader: true, + Size: int64(len(data)), + }.NewWriter(&buf) + require.NoError(t, err) + _, err = w.Write(data) + require.NoError(t, err) + require.NoError(t, w.Close()) + + raw := buf.Bytes()[13:] // strip standard 13-byte lzma header + // UPX 2-byte header: byte 0 is (t<<3)|pb where t = lc+lp, byte 1 is (lp<<4)|lc. Real `upx --best + // --lzma` output for lc=3/lp=0/pb=2 is 0x1a 0x03, confirmed against the image-small-upx fixture. + return append([]byte{0x1a, 0x03}, raw...) +} + +// padTo grows data to at least size bytes so its declared p_filesize stays within maxUPXExpansion of the +// fixture's own length. Real UPX files are far larger than their headers; the crafted ones here are not, +// and the input-size bound is deliberately sensitive to that. +func padTo(data []byte, size int) []byte { + if len(data) >= size { + return data + } + return append(data, make([]byte, size-len(data))...) +} + +// buildUPXHeader assembles the l_info + p_info prefix common to every crafted fixture below. +func buildUPXHeader(originalSize, blockSize uint32) []byte { + lInfo := []byte{ + 0, 0, 0, 0, // l_checksum + 'U', 'P', 'X', '!', // magic + 0, 0, // l_lsize + 14, 22, // l_version, l_format + } + pInfo := make([]byte, 12) + binary.LittleEndian.PutUint32(pInfo[4:8], originalSize) // p_filesize + binary.LittleEndian.PutUint32(pInfo[8:12], blockSize) // p_blocksize + return append(lInfo, pInfo...) +} + +// buildUPXFile assembles a minimal but structurally valid UPX container: l_info + p_info followed by one +// b_info + compressed stream per payload, terminated by a zero end-marker block. declaredSizes, when +// non-nil, overrides each block's sz_unc so a fixture can lie about what its stream decompresses to. +func buildUPXFile(t *testing.T, originalSize, blockSize uint32, payloads [][]byte, declaredSizes []uint32) []byte { + t.Helper() + data := buildUPXHeader(originalSize, blockSize) + for i, p := range payloads { + stream := buildUPXLZMAStream(t, p) + szUnc := uint32(len(p)) + if declaredSizes != nil { + szUnc = declaredSizes[i] + } + b := make([]byte, 12) + binary.LittleEndian.PutUint32(b[0:4], szUnc) // sz_unc + binary.LittleEndian.PutUint32(b[4:8], uint32(len(stream))) // sz_cpr + b[8] = 14 // b_method = LZMA + data = append(data, b...) + data = append(data, stream...) + } + return append(data, make([]byte, 12)...) // end marker: sz_unc == 0 +} + +// readAll drains an io.ReaderAt returned by decompressUPX. +func readAll(t *testing.T, r io.ReaderAt) []byte { + t.Helper() + require.NotNil(t, r) + out, err := io.ReadAll(io.NewSectionReader(r, 0, 1<<62)) + require.NoError(t, err) + return out +} + +func TestDecompressUPX_OversizedBlockRejected(t *testing.T) { + // a single block may not claim more output than the file's own p_filesize leaves. sz_unc used to be + // passed straight to make(), and the reporter's proof-of-concept drove it across the uint32 range. + cases := map[string]uint32{ + "beyond the declared original size": 8192, + "full uint32 range": 0xFFFFFFFF, + } + for name, szUnc := range cases { + t.Run(name, func(t *testing.T) { + data := buildUPXFile(t, 4096, 4096, [][]byte{bytes.Repeat([]byte("A"), 32)}, []uint32{szUnc}) + + _, err := decompressUPX(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXOutputExceeded) + }) + } +} + +func TestDecompressUPX_CumulativeExceedsOriginalSize(t *testing.T) { + // each block individually fits within p_blocksize, but together they claim more than the file's + // declared original size. Without the running remainder every block may claim the full size and the + // total decompression work becomes (block count x original size). The streams are real 2048-byte + // payloads so the failure below is the budget and not a short decode. + payload := bytes.Repeat([]byte("A"), 2048) + data := buildUPXFile(t, 4096, 2048, [][]byte{payload, payload, payload}, nil) // 3 x 2048 > 4096 + + _, err := decompressUPX(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXOutputExceeded) +} + +func TestDecompressUPX_BudgetAllowsExactlyOriginalSize(t *testing.T) { + // the bound must not reject a file whose blocks sum to exactly the declared original size, which is + // what a well-formed UPX binary does. + payload := bytes.Repeat([]byte("A"), 2048) + data := buildUPXFile(t, 4096, 2048, [][]byte{payload, payload}, nil) + + _, err := decompressUPX(bytes.NewReader(data)) + require.NoError(t, err) +} + +func TestDecompressUPX_BlockCountBounded(t *testing.T) { + // without a cap the block loop runs until the budget is spent one byte at a time, so a small file can + // drive hundreds of millions of iterations. Each block here places a single byte sequentially, so the + // number of blocks actually processed is directly observable in the output. + const blocks = maxUPXBlocks + 76 + payloads := make([][]byte, blocks) + for i := range payloads { + payloads[i] = []byte("A") + } + data := buildUPXFile(t, 4096, 4096, payloads, nil) + + out, err := decompressUPX(bytes.NewReader(data)) + require.NoError(t, err, "the cap stops the loop, it does not fail the file") + assert.Equal(t, maxUPXBlocks, bytes.Count(readAll(t, out), []byte("A")), + "exactly maxUPXBlocks blocks should have been placed") +} + +// buildELF64 assembles a minimal ELF64 header followed by the given program-header bytes. +func buildELF64(phoff uint64, phentsize, phnum uint16, phdrs []byte) []byte { + hdr := make([]byte, 64) + copy(hdr, []byte{0x7f, 'E', 'L', 'F'}) + hdr[4] = 2 // ELFCLASS64 + binary.LittleEndian.PutUint64(hdr[0x20:0x28], phoff) + binary.LittleEndian.PutUint16(hdr[0x36:0x38], phentsize) + binary.LittleEndian.PutUint16(hdr[0x38:0x3a], phnum) + return append(hdr, phdrs...) +} + +func TestParseELFPTLoadOffsets_ShortPhentsizeNoPanic(t *testing.T) { + // a program-header entry smaller than an ELF64 phdr would let the fixed-offset p_offset read run past + // the entry; the parser must reject it rather than index out of range. + phdr := []byte{1, 0, 0, 0, 0, 0, 0, 0} // ptype = PT_LOAD, only 8 bytes + elf := buildELF64(64, 8, 1, phdr) + + require.NotPanics(t, func() { + assert.Empty(t, parseELFPTLoadOffsets(elf)) + }) +} + +func TestParseELFPTLoadOffsets_OverflowPhoffNoPanic(t *testing.T) { + // a phoff near the top of the uint64 range must not overflow the bounds check into a huge slice index. + phdr := make([]byte, 56) + binary.LittleEndian.PutUint32(phdr[0:4], 1) // PT_LOAD + elf := buildELF64(0xFFFFFFFFFFFFFFF0, 56, 1, phdr) + + require.NotPanics(t, func() { + assert.Empty(t, parseELFPTLoadOffsets(elf)) + }) +} + +// buildPoisonELF returns an ELF whose second PT_LOAD segment declares p_offset at the top of the uint64 +// range, along with the payload set that drives block 3 to that offset. +func buildPoisonELF(t *testing.T) []byte { + t.Helper() + phdrs := make([]byte, 112) // two ELF64 program headers + binary.LittleEndian.PutUint32(phdrs[0:4], 1) // phdr[0] PT_LOAD + binary.LittleEndian.PutUint64(phdrs[8:16], 0) // p_offset 0 + binary.LittleEndian.PutUint32(phdrs[56:60], 1) // phdr[1] PT_LOAD + binary.LittleEndian.PutUint64(phdrs[64:72], 0xFFFFFFFFFFFFFFFF) // p_offset at the uint64 ceiling + elf := buildELF64(64, 56, 2, phdrs) + + // sanity: the crafted offset really is parsed out, so the tests below exercise the placement guard + require.Equal(t, []uint64{0, 0xFFFFFFFFFFFFFFFF}, parseELFPTLoadOffsets(elf)) + return elf +} + +func TestDecompressUPX_OutOfRangePlacementStopsWithPartialOutput(t *testing.T) { + // block 3 is directed at a p_offset past the end of the output buffer. The blocks placed before it are + // often enough to recover .go.buildinfo, so they are kept, but the loop must stop rather than continue + // from a bad offset. + elf := buildPoisonELF(t) + payloads := [][]byte{elf, bytes.Repeat([]byte("B"), 32), bytes.Repeat([]byte("C"), 32)} + data := buildUPXFile(t, 8192, 8192, payloads, nil) + + out, err := decompressUPX(bytes.NewReader(data)) + require.NoError(t, err) + got := readAll(t, out) + assert.Equal(t, elf, got[:len(elf)], "block 1 stays placed") + assert.NotContains(t, string(got), "CCCC", "the out-of-range block is not placed") +} + +func TestDecompressUPX_OutOfRangePlacementDoesNotPoisonLaterBlocks(t *testing.T) { + // regression: the placement guard used to skip the copy and fall through, but outputOffset is derived + // from the rejected destOffset. With p_offset at the uint64 ceiling and a 1-byte block 3, outputOffset + // wrapped to 0, and block 4 was then written over the reconstructed ELF header at offset 0. + elf := buildPoisonELF(t) + attacker := bytes.Repeat([]byte{0xDE, 0xAD, 0xBE, 0xEF}, 8) + payloads := [][]byte{elf, bytes.Repeat([]byte("B"), 32), {0x41}, attacker} + data := buildUPXFile(t, 8192, 8192, payloads, nil) + + out, err := decompressUPX(bytes.NewReader(data)) + require.NoError(t, err) + got := readAll(t, out) + assert.Equal(t, []byte{0x7f, 'E', 'L', 'F'}, got[:4], "the ELF header must not be overwritten") + assert.NotContains(t, string(got), string(attacker), "the block after a bad offset is not placed") +} + +func TestNextPowerOf2(t *testing.T) { + cases := []struct{ in, want uint32 }{ + {0, 1}, + {1, 1}, + {3, 4}, + {1 << 20, 1 << 20}, + {(1 << 20) + 1, 1 << 21}, + {1 << 31, 1 << 31}, + {(1 << 31) + 1, 1 << 31}, // saturates rather than wrapping to 0 + {0xFFFFFFFF, 1 << 31}, // saturates rather than wrapping to 0 + } + for _, c := range cases { + assert.Equalf(t, c.want, nextPowerOf2(c.in), "nextPowerOf2(%d)", c.in) + } +} + +func TestDecompressLZMA_RoundTrip(t *testing.T) { + // happy path: a stream built with valid LZMA parameters round-trips (and confirms the parameter + // validation does not reject legitimate values). + data := bytes.Repeat([]byte("hello UPX "), 16) + dst := make([]byte, len(data)) + require.NoError(t, decompressLZMA(buildUPXLZMAStream(t, data), dst)) + assert.Equal(t, data, dst) +} + +func TestDecompressLZMA_InvalidParams(t *testing.T) { + // the header nibbles can hold values the LZMA props byte cannot represent. Rejecting them keeps the + // uint8 props arithmetic from wrapping into a valid-looking but wrong value. + cases := map[string][]byte{ + // byte 0 low 3 bits carry pb; byte 1 is (lp<<4)|lc + "pb above range": {0x07, 0x03, 0x00, 0x00}, // pb = 7 + "lc above range": {0x02, 0x0f, 0x00, 0x00}, // lc = 15 + "lp above range": {0x02, 0x53, 0x00, 0x00}, // lp = 5 + "lc+lp above budget": {0x02, 0x48, 0x00, 0x00}, // lc = 8, lp = 4, sum = 12 + } + for name, stream := range cases { + t.Run(name, func(t *testing.T) { + err := decompressLZMA(stream, make([]byte, 32)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXInvalidLZMAParams) + }) + } +} + +func TestDecompressLZMA_LiteralBitBudgetAllowsRealValues(t *testing.T) { + // UPX emits lc=3/lp=0, and the budget must stay clear of anything real. lc+lp at exactly the cap is + // accepted (it fails later on the stream contents, not on the parameters). + stream := []byte{0x02, 0x44, 0x00, 0x00} // lc = 4, lp = 4, sum = 8 + err := decompressLZMA(stream, make([]byte, 32)) + require.Error(t, err) + assert.NotErrorIs(t, err, errUPXInvalidLZMAParams, "the cap itself must not reject lc+lp == the cap") +} + +func TestUnfilter49(t *testing.T) { + // the CTO filter stores CALL/JMP operands big-endian with cto8 as a marker byte. Reversing it must + // restore the little-endian relative address: 0x12345678 - (pos+1) - (cto8<<24) at pos 0. + const cto8 = 0x12 + data := []byte{0xE8, cto8, 0x34, 0x56, 0x78} + unfilter49(data, cto8) + assert.Equal(t, []byte{0xE8, 0x77, 0x56, 0x34, 0x00}, data) +} + +func TestUnfilter49_LeavesUnmarkedBytesAlone(t *testing.T) { + // a CALL whose next byte is not the cto8 marker was not transformed by the filter, so it must be + // left exactly as-is. + data := []byte{0xE8, 0x99, 0x34, 0x56, 0x78} + want := bytes.Clone(data) + unfilter49(data, 0x12) + assert.Equal(t, want, data) +} + +func TestParseUPXInfo_ImplausibleHeader(t *testing.T) { + // a coincidental "UPX!" match surrounded by zeroed fields must not be accepted as a real UPX header. + build := func(version, format byte, originalSize, blockSize uint32) []byte { + lInfo := []byte{0, 0, 0, 0, 'U', 'P', 'X', '!', 0, 0, version, format} + pInfo := make([]byte, 12) + binary.LittleEndian.PutUint32(pInfo[4:8], originalSize) + binary.LittleEndian.PutUint32(pInfo[8:12], blockSize) + return append(append(append([]byte{}, lInfo...), pInfo...), make([]byte, 32)...) + } + + cases := map[string][]byte{ + "zero version": build(0, 22, 0x1000, 0x1000), + "zero format": build(14, 0, 0x1000, 0x1000), + "zero block size": build(14, 22, 0x1000, 0), + "zero original size": build(14, 22, 0, 0x1000), + "original size over ceiling": build(14, 22, maxUPXOriginalSize+1, 0x1000), + } + for name, data := range cases { + t.Run(name, func(t *testing.T) { + _, err := parseUPXInfo(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXImplausibleHeader) + }) + } +} + +func TestParseUPXInfo_BlockSizeAboveOriginalSizeAccepted(t *testing.T) { + // p_blocksize is not a bound and must not be a rejection reason. UPX derives it from a PT_LOAD extent + // and real output has the largest block equal to it exactly, so a check against p_filesize would run + // with no headroom against something the format does not promise. The remainder budget bounds output. + data := padTo(append(buildUPXHeader(0x1000, 0x2000), make([]byte, 32)...), 128) + + info, err := parseUPXInfo(bytes.NewReader(data)) + require.NoError(t, err) + assert.Equal(t, uint32(0x2000), info.blockSize) +} + +func TestParseUPXInfo_HighExpansionAccepted(t *testing.T) { + // LZMA encodes a run of N identical bytes in O(log N), so a `go:embed` of 120MB of zeros packs + // 127504546 bytes into 608940 and `upx --best --lzma` produces exactly that. maxUPXExpansion must be + // loose enough for it: the ratio here is 209x against a 610KB input, well inside the 1024x allowance. + data := padTo(buildUPXHeader(127504546, 126628216), 610*1024) + + info, err := parseUPXInfo(bytes.NewReader(data)) + require.NoError(t, err, "a high but legitimate expansion ratio must not be rejected") + assert.Equal(t, uint32(127504546), info.originalSize) +} + +func TestParseUPXInfo_CeilingAcceptedWhenTheInputPaysForIt(t *testing.T) { + // the absolute ceiling is legal, but only for a file large enough to claim it + data := padTo(buildUPXHeader(maxUPXOriginalSize, maxUPXOriginalSize), maxUPXOriginalSize/maxUPXExpansion) + + info, err := parseUPXInfo(bytes.NewReader(data)) + require.NoError(t, err) + assert.Equal(t, uint32(maxUPXOriginalSize), info.originalSize) +} + +func TestParseUPXInfo_TinyInputCannotClaimALargeOriginalSize(t *testing.T) { + // regression: p_filesize sizes the output buffer directly, so without a bound against the actual input + // a header of a few dozen bytes could claim the absolute ceiling. A 128 byte file may claim 128KB. + data := padTo(buildUPXHeader(maxUPXOriginalSize, 4096), 128) + + _, err := parseUPXInfo(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXImplausibleHeader) + + withinAllowance := padTo(buildUPXHeader(128*maxUPXExpansion, 4096), 128) + _, err = parseUPXInfo(bytes.NewReader(withinAllowance)) + assert.NoError(t, err, "exactly the allowance is legal; only past it is not") +} + +// seekOnlyReader has ReadAt and Seek but no Size(), which is the shape the readers in a real scan have: +// GetReaders hands back the *os.File itself for a non-macho binary. +type seekOnlyReader struct{ r *bytes.Reader } + +func (s *seekOnlyReader) ReadAt(p []byte, off int64) (int, error) { return s.r.ReadAt(p, off) } +func (s *seekOnlyReader) Seek(off int64, whence int) (int64, error) { + return s.r.Seek(off, whence) +} + +func TestInputSize(t *testing.T) { + // the whole input-size bound goes inert if this returns 0, and the reader that reaches it in a real + // scan is an *os.File, which answers Seek but not Size(). Both paths have to work. + data := make([]byte, 4096) + + assert.Equal(t, int64(4096), inputSize(bytes.NewReader(data)), "*bytes.Reader answers Size() directly") + assert.Equal(t, int64(4096), inputSize(&seekOnlyReader{bytes.NewReader(data)}), "Seek-only must fall back, not give up") + + t.Run("seeking restores the original offset", func(t *testing.T) { + r := &seekOnlyReader{bytes.NewReader(data)} + _, err := r.Seek(100, io.SeekStart) + require.NoError(t, err) + require.Equal(t, int64(4096), inputSize(r)) + at, err := r.Seek(0, io.SeekCurrent) + require.NoError(t, err) + assert.Equal(t, int64(100), at, "measuring the size must not move the caller's cursor") + }) + + t.Run("unknown size falls back to the absolute ceiling", func(t *testing.T) { + // io.ReaderAt with neither Size() nor Seek: the ratio cannot be applied, so only the cap remains + plain := struct{ io.ReaderAt }{bytes.NewReader(data)} + assert.Zero(t, inputSize(plain)) + assert.Equal(t, uint64(maxUPXOriginalSize), maxOriginalSize(plain)) + }) +} + +func TestDecompressUPX_AllocationTracksTheInput(t *testing.T) { + // the property every bound in this file exists to protect, asserted directly rather than through an + // error value: a small file cannot drive a large allocation. Pre-fix a ~100 byte input reached + // make([]byte, 0xFFFFFFFF); an intermediate version capped that at the absolute 500MB ceiling, which + // a 100 byte file could still claim in full. + tiny := padTo(buildUPXHeader(maxUPXOriginalSize, maxUPXOriginalSize), 128) + + allocated := measureAlloc(t, func() { + _, err := decompressUPX(bytes.NewReader(tiny)) + require.Error(t, err) + }) + assert.Less(t, allocated, uint64(1<<20), "a 128 byte input must not drive a megabyte of allocation") + t.Logf("128 byte input allocated %d bytes", allocated) +} + +func TestDecompressUPX_HeaderWithoutDecodableBlocksAllocatesNothing(t *testing.T) { + // a header declaring a large p_filesize whose first b_info is the end marker used to allocate the whole + // output buffer and return success. output is now allocated on the first block that survives + // validation, so this costs nothing and reports that the file is not really packed. + data := padTo(append(buildUPXHeader(4*intFile.MB, 4096), make([]byte, 12)...), 8*1024) + + allocated := measureAlloc(t, func() { + _, err := decompressUPX(bytes.NewReader(data)) + require.Error(t, err) + assert.ErrorIs(t, err, errUPXImplausibleHeader) + }) + assert.Less(t, allocated, uint64(256<<10), "no block survived validation, so no output buffer is due") +} + +// 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. +func TestGetBuildInfo_MaliciousUPXRejected(t *testing.T) { + // a plausible ELF prefix so the file looks like an executable, then a UPX header whose single block + // claims the full uint32 range. Pre-fix this reached make([]byte, 0xFFFFFFFF). + elf := make([]byte, 64) + copy(elf, []byte{0x7f, 'E', 'L', 'F'}) + elf[4] = 2 + + data := append(elf, buildUPXFile(t, 4096, 4096, + [][]byte{bytes.Repeat([]byte("A"), 32)}, []uint32{0xFFFFFFFF})...) + + // go's own test timeout covers "did not return promptly"; a goroutine plus time.After here would + // assert on an already-finished test if it ever fired. + var bi *debug.BuildInfo + var err error + allocated := measureAlloc(t, func() { + bi, err = getBuildInfo(bytes.NewReader(data), file.NewLocation("/malicious")) + }) + assert.Nil(t, bi) + assert.Error(t, err) + assert.Less(t, allocated, uint64(1<<20), "the rejection must not cost a megabyte") +} diff --git a/syft/pkg/cataloger/golang/upx_test.go b/syft/pkg/cataloger/golang/upx_test.go index 99e720d12..ae33daf2c 100644 --- a/syft/pkg/cataloger/golang/upx_test.go +++ b/syft/pkg/cataloger/golang/upx_test.go @@ -8,44 +8,47 @@ import ( "github.com/stretchr/testify/require" ) -func TestIsUPXCompressed(t *testing.T) { +// isUPXCompressed used to pre-screen for the magic before parseUPXInfo scanned for it again. The +// function is gone; these are the cases it covered, now asserted on parseUPXInfo's errNotUPX result. +func TestParseUPXInfo_MagicDetection(t *testing.T) { tests := []struct { - name string - data []byte - expected bool + name string + data []byte + foundMagic bool // the magic was located (the header may still be rejected as implausible) }{ { - name: "contains UPX magic at start", - data: append([]byte("UPX!"), make([]byte, 100)...), - expected: true, + name: "contains UPX magic at start", + data: append([]byte("UPX!"), make([]byte, 100)...), + foundMagic: true, }, { - name: "contains UPX magic with offset", - data: append(append(make([]byte, 500), []byte("UPX!")...), make([]byte, 100)...), - expected: true, + name: "contains UPX magic with offset", + data: append(append(make([]byte, 500), []byte("UPX!")...), make([]byte, 100)...), + foundMagic: true, }, { - name: "no UPX magic", - data: []byte("\x7FELF" + string(make([]byte, 100))), - expected: false, + name: "no UPX magic", + data: []byte("\x7FELF" + string(make([]byte, 100))), }, { - name: "empty data", - data: []byte{}, - expected: false, + name: "empty data", + data: []byte{}, }, { - name: "partial UPX magic", - data: []byte("UPX"), - expected: false, + name: "partial UPX magic", + data: []byte("UPX"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - reader := bytes.NewReader(tt.data) - result := isUPXCompressed(reader) - assert.Equal(t, tt.expected, result) + _, err := parseUPXInfo(bytes.NewReader(tt.data)) + require.Error(t, err, "none of these fixtures is a usable UPX header") + if tt.foundMagic { + assert.NotErrorIs(t, err, errNotUPX, "the magic was found, so the header was parsed and rejected") + } else { + assert.ErrorIs(t, err, errNotUPX) + } }) } } @@ -84,8 +87,10 @@ func TestParseUPXInfo_ValidHeader(t *testing.T) { 14, 0, 0, 0, // method=LZMA, filter info } + // padded so the declared 1MB stays within maxUPXExpansion of the fixture's own size; a real UPX file + // carries the compressed data this header describes, and the bound is measured against that. data := append(append(lInfo, pInfo...), bInfo...) - data = append(data, make([]byte, 100)...) // padding + data = append(data, make([]byte, 0x100000/maxUPXExpansion)...) reader := bytes.NewReader(data) info, err := parseUPXInfo(reader) @@ -108,7 +113,7 @@ func TestDecompressUPX_UnsupportedMethod(t *testing.T) { pInfo := []byte{ 0, 0, 0, 0, // p_progid 0x00, 0x01, 0x00, 0x00, // p_filesize = 256 bytes (small for test) - 0, 0, 0x10, 0, // p_blocksize + 0x00, 0x01, 0x00, 0x00, // p_blocksize = 256 (UPX never sets this above p_filesize) } bInfo := []byte{