From c60aabad37447894c229f3f48a718954a881aed9 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 29 Sep 2026 15:00:42 -0400 Subject: [PATCH] test(golang): make the UPX fixture exercise loader padding (#5354) UPX pads to a 4 byte boundary before it writes the loader stub. The image-small-upx build happened to need no padding, so the cataloger tests over it passed with or without the fix in #5347. - build the fixture with -X main.Version=1.0.11, which needs 2 bytes of padding, so the existing cataloger tests fail without the fix - add TestImageSmallUPXNeedsLoaderPadding, which fails if a change to the fixture's inputs means it no longer needs padding - run the crafted loader test on both sides of the boundary, so a skip that always rounds up is caught too Signed-off-by: Will Murphy --- syft/pkg/cataloger/golang/cataloger_test.go | 25 ++++---- .../testdata/image-small-upx/Dockerfile | 4 +- .../pkg/cataloger/golang/upx_security_test.go | 60 +++++++++++++------ syft/pkg/cataloger/golang/upx_test.go | 34 +++++++++++ 4 files changed, 92 insertions(+), 31 deletions(-) diff --git a/syft/pkg/cataloger/golang/cataloger_test.go b/syft/pkg/cataloger/golang/cataloger_test.go index 860171f70..4d504ff65 100644 --- a/syft/pkg/cataloger/golang/cataloger_test.go +++ b/syft/pkg/cataloger/golang/cataloger_test.go @@ -59,9 +59,10 @@ func Test_PackageCataloger_Binary(t *testing.T) { { name: "upx compressed binary", fixture: "image-small-upx", + // v1.0.11, not v1.0.0: see the fixture's Dockerfile wantErr: require.NoError, expectedPkgs: []string{ - "anchore.io/not/real @ v1.0.0 (/run-me)", + "anchore.io/not/real @ v1.0.11 (/run-me)", "github.com/andybalholm/brotli @ v1.1.1 (/run-me)", "github.com/dsnet/compress @ v0.0.2-0.20210315054119-f66993602bf5 (/run-me)", "github.com/golang/snappy @ v0.0.4 (/run-me)", @@ -75,17 +76,17 @@ func Test_PackageCataloger_Binary(t *testing.T) { "github.com/anchore/archiver/v3 @ v3.5.3-0.20241210171143-5b1d8d1c7c51 (/run-me)", }, expectedRels: []string{ - "github.com/andybalholm/brotli @ v1.1.1 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/dsnet/compress @ v0.0.2-0.20210315054119-f66993602bf5 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/golang/snappy @ v0.0.4 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/klauspost/compress @ v1.17.11 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/klauspost/pgzip @ v1.2.6 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/anchore/archiver/v3 @ v3.5.3-0.20241210171143-5b1d8d1c7c51 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/nwaples/rardecode @ v1.1.3 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/pierrec/lz4/v4 @ v4.1.21 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/ulikunitz/xz @ v0.5.12 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "github.com/xi2/xz @ v0.0.0-20171230120015-48954b6210f8 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", - "stdlib @ go1.23.2 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.0 (/run-me)", + "github.com/andybalholm/brotli @ v1.1.1 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/dsnet/compress @ v0.0.2-0.20210315054119-f66993602bf5 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/golang/snappy @ v0.0.4 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/klauspost/compress @ v1.17.11 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/klauspost/pgzip @ v1.2.6 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/anchore/archiver/v3 @ v3.5.3-0.20241210171143-5b1d8d1c7c51 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/nwaples/rardecode @ v1.1.3 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/pierrec/lz4/v4 @ v4.1.21 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/ulikunitz/xz @ v0.5.12 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "github.com/xi2/xz @ v0.0.0-20171230120015-48954b6210f8 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", + "stdlib @ go1.23.2 (/run-me) [dependency-of] anchore.io/not/real @ v1.0.11 (/run-me)", }, }, { diff --git a/syft/pkg/cataloger/golang/testdata/image-small-upx/Dockerfile b/syft/pkg/cataloger/golang/testdata/image-small-upx/Dockerfile index bdd8901d7..7ee71b619 100644 --- a/syft/pkg/cataloger/golang/testdata/image-small-upx/Dockerfile +++ b/syft/pkg/cataloger/golang/testdata/image-small-upx/Dockerfile @@ -9,7 +9,9 @@ COPY go.mod go.sum ./ RUN go mod download COPY main.go main.go -RUN CGO_ENABLED=0 GOOS=linux go build -ldflags "-X main.Version=1.0.0" -o run-me . +# 1.0.11 is chosen so UPX has to pad before its loader stub (1.0.0 needed none, which hid a bug). +# TestImageSmallUPXNeedsLoaderPadding checks the padding is still there. +RUN CGO_ENABLED=0 GOOS=linux go build -ldflags "-X main.Version=1.0.11" -o run-me . RUN upx --best --lzma --exact run-me FROM scratch diff --git a/syft/pkg/cataloger/golang/upx_security_test.go b/syft/pkg/cataloger/golang/upx_security_test.go index c7e3168c3..dd703ec6d 100644 --- a/syft/pkg/cataloger/golang/upx_security_test.go +++ b/syft/pkg/cataloger/golang/upx_security_test.go @@ -718,33 +718,57 @@ func TestDecompressUPX_TailExtentsArePlacedPastTheLoader(t *testing.T) { // front of it. Every other crafted fixture here has l_lsize == 0, so without this skipLoader was only // reachable through the Docker-backed fixture. // - // UPX pads to a 4 byte boundary before it writes the stub and l_lsize counts from there, so the head - // extent here deliberately ends off that boundary: its end plus l_lsize lands inside the stub. + // UPX pads to a 4 byte boundary before it writes the stub and l_lsize counts from there, so each case + // ends the head extent the given number of bytes short of a boundary. The head is a stored block so its + // length alone decides where it ends; a compressed one ends wherever the encoder happens to stop. // // note: block 1 here is not an ELF, so there are no PT_LOAD offsets and the tail lands sequentially. // The hole-filling path this feeds in production is covered by TestFirstHole and by the sparse // fixtures in upx_bounds_regression_test.go. const loaderSize = 64 - head := bytes.Repeat([]byte("H"), 128) tail := bytes.Repeat([]byte("T"), 32) - data := buildUPXHeaderWithLoader(4096, 4096, loaderSize) - data = append(data, blockFor(t, head)...) - require.NotZero(t, len(data)%4, "the head extent has to end off a 4 byte boundary for the padding to exist") - data = append(data, make([]byte, 4-len(data)%4)...) // the padding UPX writes in front of the stub - data = append(data, make([]byte, 12)...) // end marker: closes the first run of extents - data = append(data, make([]byte, loaderSize-12)...) // the loader stub the chain skips over - data = append(data, blockFor(t, tail)...) - data = append(data, make([]byte, 12)...) // end marker for the tail run - data = padTo(data, 1024) + tests := []struct { + name string + padding int + }{ + {name: "no padding", padding: 0}, + {name: "1 byte of padding", padding: 1}, + {name: "2 bytes of padding", padding: 2}, + {name: "3 bytes of padding", padding: 3}, + } - out, err := unpack(t, data) - // 160 bytes of a declared 4096, short by construction like every other crafted fixture here - require.ErrorIs(t, err, errUPXPartial) - got := readAll(t, out) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := buildUPXHeaderWithLoader(4096, 4096, loaderSize) - assert.Equal(t, head, got[:len(head)], "the head extent stays where it was placed") - assert.Contains(t, string(got), string(tail), "the extent behind the loader must be placed too") + // trim a 128 byte head so that it, plus its b_info, ends tt.padding bytes short of a boundary + headLen := 128 - (len(data)+12+128+tt.padding)%4 + head := bytes.Repeat([]byte("H"), headLen) + stored := make([]byte, 12) + binary.LittleEndian.PutUint32(stored[0:4], uint32(headLen)) // sz_unc + binary.LittleEndian.PutUint32(stored[4:8], uint32(headLen)) // sz_cpr, equal for a stored block + stored[8] = upxMethodStored + + data = append(data, stored...) + data = append(data, head...) + require.Equal(t, tt.padding, (4-len(data)%4)%4, "the head should end tt.padding bytes short of a boundary") + data = append(data, make([]byte, tt.padding)...) // the padding UPX writes in front of the stub + data = append(data, make([]byte, 12)...) // end marker: closes the first run of extents + data = append(data, make([]byte, loaderSize-12)...) // the loader stub the chain skips over + data = append(data, blockFor(t, tail)...) + data = append(data, make([]byte, 12)...) // end marker for the tail run + data = padTo(data, 1024) + + out, err := unpack(t, data) + // a few hundred bytes of a declared 4096, short by construction like every other crafted fixture here + require.ErrorIs(t, err, errUPXPartial) + got := readAll(t, out) + + assert.Equal(t, head, got[:len(head)], "the head extent stays where it was placed") + assert.Contains(t, string(got), string(tail), "the extent behind the loader must be placed too") + }) + } } func TestDecompressUPX_UnsupportedMethodIsNotReportable(t *testing.T) { diff --git a/syft/pkg/cataloger/golang/upx_test.go b/syft/pkg/cataloger/golang/upx_test.go index 2def6178a..05df518e2 100644 --- a/syft/pkg/cataloger/golang/upx_test.go +++ b/syft/pkg/cataloger/golang/upx_test.go @@ -2,10 +2,14 @@ package golang import ( "bytes" + "io" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + stereoscopeFile "github.com/anchore/stereoscope/pkg/file" + "github.com/anchore/stereoscope/pkg/imagetest" ) // every case here is one the magic scan must reject, asserted on parseUPXInfo's errNotUPX result. @@ -104,3 +108,33 @@ func TestParseUPXInfo_ValidHeader(t *testing.T) { assert.Equal(t, uint8(22), info.format) assert.Equal(t, uint32(0x100000), info.originalSize) } + +// TestImageSmallUPXNeedsLoaderPadding checks the fixture rather than the code. UPX pads to a 4 byte +// boundary before its loader stub, and the cataloger tests over image-small-upx only catch an unpacker that +// ignores that padding if there is some. A build that needs none passes either way, which is how the bug +// went unnoticed. The case without padding is covered by TestDecompressUPX_TailExtentsArePlacedPastTheLoader. +func TestImageSmallUPXNeedsLoaderPadding(t *testing.T) { + img := imagetest.GetFixtureImage(t, "docker-archive", "image-small-upx") + rc, err := img.OpenPathFromSquash(stereoscopeFile.Path("/run-me")) + require.NoError(t, err) + data, err := io.ReadAll(rc) + require.NoError(t, rc.Close()) + require.NoError(t, err) + + r := bytes.NewReader(data) + info, err := parseUPXInfo(r, int64(len(data))) + require.NoError(t, err) + require.NotZero(t, info.loaderSize, "real UPX output carries a loader stub") + + // walk the first run of extents to the end marker in front of the stub + extentsEnd := info.firstBlockOff + for blockNum := 0; ; blockNum++ { + block, _, err := readChainBlock(r, extentsEnd, blockNum, info.inputLen) + require.NoError(t, err) + if block == nil { + break + } + extentsEnd = block.dataOffset + int64(block.compressedSize) + } + require.NotZero(t, extentsEnd%4, "the fixture no longer needs padding: change the version string in its Dockerfile") +}