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 <willmurphyscode@users.noreply.github.com>

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

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

syft/pkg/cataloger/golang/upx_security_test.go

@@ -718,20 +718,42 @@ 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)
 
+	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},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
 			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
+
+			// 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)...)
@@ -739,12 +761,14 @@ func TestDecompressUPX_TailExtentsArePlacedPastTheLoader(t *testing.T) {
 			data = padTo(data, 1024)
 
 			out, err := unpack(t, data)
-	// 160 bytes of a declared 4096, short by construction like every other crafted fixture here
+			// 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) {

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