From b4ee6e4696b6e17cbc8d7028d2c3da431bd1f8a9 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Mon, 14 Sep 2026 18:17:56 -0400 Subject: [PATCH] fix(debian): bound the decompressed members of a .deb archive (#5293) --- syft/pkg/cataloger/debian/cataloger_test.go | 1 + syft/pkg/cataloger/debian/package.go | 12 +- .../pkg/cataloger/debian/parse_deb_archive.go | 127 +++++++- .../debian/parse_deb_archive_test.go | 281 ++++++++++++++++++ syft/pkg/cataloger/debian/parse_dpkg_db.go | 7 +- .../cataloger/debian/parse_dpkg_info_files.go | 38 ++- .../debian/parse_dpkg_info_files_test.go | 55 +++- 7 files changed, 498 insertions(+), 23 deletions(-) diff --git a/syft/pkg/cataloger/debian/cataloger_test.go b/syft/pkg/cataloger/debian/cataloger_test.go index 4d5deb9fb..8d38c7b33 100644 --- a/syft/pkg/cataloger/debian/cataloger_test.go +++ b/syft/pkg/cataloger/debian/cataloger_test.go @@ -285,6 +285,7 @@ func TestDpkgArchiveCataloger(t *testing.T) { pkgtest.NewCatalogTester(). WithImageResolver(t, tt.name). IgnoreLocationLayer(). // this fixture can be rebuilt, thus the layer ID will change + WithErrorAssertion(require.NoError). Expects(tt.expected, nil). TestCataloger(t, c) }) diff --git a/syft/pkg/cataloger/debian/package.go b/syft/pkg/cataloger/debian/package.go index 364963516..8ad569f0f 100644 --- a/syft/pkg/cataloger/debian/package.go +++ b/syft/pkg/cataloger/debian/package.go @@ -202,7 +202,11 @@ func getAdditionalFileListing(resolver file.Resolver, dbLocation file.Location, if md5Reader != nil && md5Location != nil { defer internal.CloseAndLogError(md5Reader, md5Location.AccessPath) // attach the file list - files = append(files, parseDpkgMD5Info(md5Reader)...) + records, err := parseDpkgMD5Info(md5Reader) + if err != nil { + log.Debugf("dpkg md5sums file %q: %v", md5Location.RealPath, err) + } + files = append(files, records...) // keep a record of the file where this was discovered locations = append(locations, *md5Location) @@ -213,7 +217,11 @@ func getAdditionalFileListing(resolver file.Resolver, dbLocation file.Location, if conffilesReader != nil && conffilesLocation != nil { defer internal.CloseAndLogError(conffilesReader, conffilesLocation.AccessPath) // attach the file list - files = append(files, parseDpkgConffileInfo(conffilesReader)...) + records, err := parseDpkgConffileInfo(conffilesReader) + if err != nil { + log.Debugf("dpkg conffiles file %q: %v", conffilesLocation.RealPath, err) + } + files = append(files, records...) // keep a record of the file where this was discovered locations = append(locations, *conffilesLocation) diff --git a/syft/pkg/cataloger/debian/parse_deb_archive.go b/syft/pkg/cataloger/debian/parse_deb_archive.go index b3b5f923f..4036e93d4 100644 --- a/syft/pkg/cataloger/debian/parse_deb_archive.go +++ b/syft/pkg/cataloger/debian/parse_deb_archive.go @@ -3,8 +3,10 @@ package debian import ( "archive/tar" "context" + "errors" "fmt" "io" + "math" "path/filepath" "regexp" "strings" @@ -13,6 +15,7 @@ import ( "github.com/mholt/archives" "github.com/anchore/syft/internal" + intFile "github.com/anchore/syft/internal/file" "github.com/anchore/syft/internal/unknown" "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/file" @@ -33,6 +36,7 @@ func parseDebArchive(ctx context.Context, _ file.Resolver, _ *generic.Environmen var metadata *pkg.DpkgArchiveEntry var licenses []string var unknownErr error + var sawControl, sawData bool for { header, err := arReader.Next() if err == io.EOF { @@ -43,19 +47,28 @@ func parseDebArchive(ctx context.Context, _ file.Resolver, _ *generic.Environmen } switch { - case strings.HasPrefix(header.Name, "control.tar"): + // a .deb has exactly one control.tar.* and one data.tar.* member; anything past the first of + // each is attacker-supplied filler that would otherwise buy a fresh decompression budget + case !sawControl && strings.HasPrefix(header.Name, "control.tar"): + sawControl = true // Decompress the control.tar.* file - dcReader, err := decompressionStream(ctx, arReader, header.Name) + dcReader, err := decompressionStream(ctx, arReader, header.Name, maxControlTarSize) if err != nil { return nil, nil, unknown.New(reader.Location, fmt.Errorf("failed to decompress control.tar.* file: %w", err)) } metadata, err = processControlTar(dcReader) - if err != nil { + switch { + case err != nil && metadata == nil: return nil, nil, unknown.New(reader.Location, fmt.Errorf("failed to process control.tar.* file: %w", err)) + case err != nil: + // metadata came back usable but incomplete (a clipped file listing); keep the package and + // record why it is partial + unknownErr = unknown.Append(unknownErr, reader.Location, fmt.Errorf("partial control.tar.* file: %w", err)) } - case strings.HasPrefix(header.Name, "data.tar"): + case !sawData && strings.HasPrefix(header.Name, "data.tar"): + sawData = true // Decompress the data.tar.* file - dcReader, err := decompressionStream(ctx, arReader, header.Name) + dcReader, err := decompressionStream(ctx, arReader, header.Name, maxDataTarSize) if err != nil { return nil, nil, unknown.New(reader.Location, fmt.Errorf("failed to decompress data.tar.* file: %w", err)) } @@ -64,15 +77,21 @@ func parseDebArchive(ctx context.Context, _ file.Resolver, _ *generic.Environmen unknownErr = unknown.Append(unknownErr, reader.Location, fmt.Errorf("failed to process data.tar.* file: %w", err)) } } + + if sawControl && sawData { + break + } } if metadata == nil { return nil, nil, unknown.New(reader.Location, fmt.Errorf("no application found described in .dpkg archive")) } + // a partial parse still yields a usable package, so report what went wrong alongside it rather than + // dropping either one return []pkg.Package{ newDebArchivePackage(ctx, reader.Location, *metadata, licenses), - }, nil, nil + }, nil, unknownErr } // this is the pattern you'd expect to see in a tar header for a debian package license file () @@ -81,6 +100,7 @@ var archiveHeaderLicensePathPattern = regexp.MustCompile(`^\.?/usr/share/doc/[^/ func processDataTar(dcReader io.ReadCloser) ([]string, error) { defer internal.CloseAndLogError(dcReader, "") var licenses []string + var copyrightFiles int tarReader := tar.NewReader(dcReader) for { @@ -96,6 +116,10 @@ func processDataTar(dcReader io.ReadCloser) ([]string, error) { // TODO: in the future we can add archive sub indexes to the locations to see where within // the dpkg archive the license was found if archiveHeaderLicensePathPattern.MatchString(header.Name) { + copyrightFiles++ + if copyrightFiles > maxCopyrightFiles { + return licenses, fmt.Errorf("copyright file count exceeds %d; remaining files not read", maxCopyrightFiles) + } licenses = append(licenses, parseLicensesFromCopyright(tarReader)...) } } @@ -103,6 +127,10 @@ func processDataTar(dcReader io.ReadCloser) ([]string, error) { return licenses, nil } +// processControlTar always returns whatever metadata it managed to parse, even alongside a non-nil +// error. A non-nil error with non-nil metadata means the package is usable but incomplete (e.g. a +// clipped file listing); nil metadata means nothing usable was found and the caller should treat it +// as fatal. func processControlTar(dcReader io.ReadCloser) (*pkg.DpkgArchiveEntry, error) { defer internal.CloseAndLogError(dcReader, "") @@ -111,6 +139,7 @@ func processControlTar(dcReader io.ReadCloser) (*pkg.DpkgArchiveEntry, error) { var metadata *pkg.DpkgArchiveEntry var files []pkg.DpkgFileRecord var confFileRecords []pkg.DpkgFileRecord + var listingErr error for { header, err := tarReader.Next() @@ -118,7 +147,7 @@ func processControlTar(dcReader io.ReadCloser) (*pkg.DpkgArchiveEntry, error) { break } if err != nil { - return nil, fmt.Errorf("failed to read control tar: %w", err) + return metadata, fmt.Errorf("failed to read control tar: %w", err) } switch filepath.Base(header.Name) { @@ -134,11 +163,15 @@ func processControlTar(dcReader io.ReadCloser) (*pkg.DpkgArchiveEntry, error) { entry := pkg.DpkgArchiveEntry(entries[0].toDpkgEntry()) metadata = &entry case "md5sums": - // parseDpkgMD5Info already streams via bufio.Scanner - files = parseDpkgMD5Info(tarReader) + // parseDpkgMD5Info streams via bufio.Scanner and reports its own clipping/scan errors + var err error + files, err = parseDpkgMD5Info(tarReader) + listingErr = errors.Join(listingErr, err) case "conffiles": - // parseDpkgConffileInfo already streams via bufio.Scanner - confFileRecords = parseDpkgConffileInfo(tarReader) + // parseDpkgConffileInfo streams via bufio.Scanner and reports its own clipping/scan errors + var err error + confFileRecords, err = parseDpkgConffileInfo(tarReader) + listingErr = errors.Join(listingErr, err) } } @@ -159,10 +192,36 @@ func processControlTar(dcReader io.ReadCloser) (*pkg.DpkgArchiveEntry, error) { } metadata.Files = files - return metadata, nil + + // a clipped or unreadable file listing leaves the package usable, so hand both back and let the + // caller decide + return metadata, listingErr } -func decompressionStream(ctx context.Context, r io.Reader, filePath string) (io.ReadCloser, error) { +// a .deb's member sizes are bounded by the archive, but what they decompress to is not: xz and zstd +// both exceed gzip's ~1032:1, so a few KB member can expand without limit. The two members get +// different budgets because they hold different things. +const ( + // control.tar holds only metadata (control, md5sums, conffiles). Even a package shipping a hundred + // thousand files lands around 12MB of md5sums text. + maxControlTarSize int64 = 16 * intFile.MB + + // data.tar is the package payload, which is legitimately large (texlive and cuda ship hundreds of + // MB), so this is only here to stop the tar walk from spinning forever on a bomb. It mirrors the + // perFileReadLimit already used for archive extraction in internal/file/copy.go. + maxDataTarSize int64 = 2 * intFile.GB + + // a real .deb ships exactly one copyright file per package. + maxCopyrightFiles = 1000 +) + +// sentinels so callers (and tests) can match with errors.Is instead of substring matching. +var ( + errDecompressedTooLarge = errors.New("decompressed stream is larger than the max allowed size") + errClippedFileListing = errors.New("file listing exceeds the max allowed entries") +) + +func decompressionStream(ctx context.Context, r io.Reader, filePath string, maxDecompressed int64) (io.ReadCloser, error) { format, stream, err := archives.Identify(ctx, filePath, r) if err != nil { return nil, fmt.Errorf("failed to identify compression format: %w", err) @@ -178,5 +237,45 @@ func decompressionStream(ctx context.Context, r io.Reader, filePath string) (io. return nil, fmt.Errorf("failed to create decompression reader: %w", err) } - return rc, nil + return newBoundedReadCloser(rc, maxDecompressed), nil +} + +// boundedReadCloser fails the read past a byte budget rather than truncating. +// +// io.LimitedReader is the obvious thing to reach for here and is wrong for this: past its limit it +// just stops returning bytes, and both consumers of this are tar readers, which read a truncated +// stream as a clean end of archive. Together that emits a package silently missing files, which is +// the exact failure the bound exists to catch. The signal is recoverable by checking +// LimitedReader.N after the walk finishes (the same shape as safeCopy in internal/file/copy.go, +// which counts bytes off io.Copy), but nothing obliges a caller to write that check and a missed +// one is silent all over again. Erroring at the Read is the version that cannot be forgotten. +// +// net/http's MaxBytesReader has precisely these semantics and is stdlib, but reaching into the HTTP +// package to parse a .deb leaves the next reader wondering what it is doing here. +type boundedReadCloser struct { + io.ReadCloser + remaining int64 + limit int64 +} + +func newBoundedReadCloser(rc io.ReadCloser, limit int64) *boundedReadCloser { + // budget is one byte past the cap so a stream of exactly limit bytes still reaches EOF normally, + // and only the byte after the cap trips the error. Clamp so a limit near math.MaxInt64 can't wrap + // remaining negative and fail every read immediately. + if limit < 0 || limit > math.MaxInt64-1 { + limit = math.MaxInt64 - 1 + } + return &boundedReadCloser{ReadCloser: rc, remaining: limit + 1, limit: limit} +} + +func (b *boundedReadCloser) Read(p []byte) (int, error) { + if b.remaining <= 0 { + return 0, fmt.Errorf("%w (%d bytes)", errDecompressedTooLarge, b.limit) + } + if int64(len(p)) > b.remaining { + p = p[:b.remaining] + } + n, err := b.ReadCloser.Read(p) + b.remaining -= int64(n) + return n, err } diff --git a/syft/pkg/cataloger/debian/parse_deb_archive_test.go b/syft/pkg/cataloger/debian/parse_deb_archive_test.go index c9baee738..f90e953d9 100644 --- a/syft/pkg/cataloger/debian/parse_deb_archive_test.go +++ b/syft/pkg/cataloger/debian/parse_deb_archive_test.go @@ -3,11 +3,22 @@ package debian import ( "archive/tar" "bytes" + "compress/gzip" + "context" + "errors" + "fmt" "io" + "strings" "testing" + "testing/iotest" + "github.com/blakesmith/ar" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/anchore/syft/internal/unknown" + "github.com/anchore/syft/syft/file" + "github.com/anchore/syft/syft/pkg/cataloger/generic" ) func TestProcessControlTar(t *testing.T) { @@ -85,3 +96,273 @@ func writeTarEntry(t *testing.T, tw *tar.Writer, name, content string) { _, err := tw.Write([]byte(content)) require.NoError(t, err) } + +func Test_boundedReadCloser(t *testing.T) { + payload := []byte("0123456789") + + t.Run("a stream at exactly the cap reads through whole", func(t *testing.T) { + // the positive assertion is the point: it proves the one-byte-past budget does not truncate + // at-cap content, which a bare "did not error" check would not catch + r := newBoundedReadCloser(io.NopCloser(bytes.NewReader(payload)), int64(len(payload))) + + got, err := io.ReadAll(r) + + require.NoError(t, err) + assert.Equal(t, payload, got) + }) + + t.Run("a stream past the cap errors rather than truncating", func(t *testing.T) { + r := newBoundedReadCloser(io.NopCloser(bytes.NewReader(payload)), int64(len(payload)-1)) + + _, err := io.ReadAll(r) + + require.ErrorIs(t, err, errDecompressedTooLarge) + }) + + t.Run("the underlying read error is not masked", func(t *testing.T) { + r := newBoundedReadCloser(io.NopCloser(iotest.ErrReader(errBoom)), 1024) + + _, err := io.ReadAll(r) + + require.ErrorIs(t, err, errBoom) + }) +} + +var errBoom = errors.New("boom") + +func Test_decompressionStream_boundsDecompressedSize(t *testing.T) { + // a highly compressible payload standing in for a decompression bomb: tiny compressed, large out + var raw bytes.Buffer + raw.Write(make([]byte, 512*1024)) + + var gz bytes.Buffer + gw := gzip.NewWriter(&gz) + _, err := gw.Write(raw.Bytes()) + require.NoError(t, err) + require.NoError(t, gw.Close()) + + require.Less(t, gz.Len(), 4096, "compressed payload should be tiny relative to what it expands to") + + rc, err := decompressionStream(context.Background(), bytes.NewReader(gz.Bytes()), "control.tar.gz", 64*1024) + require.NoError(t, err, "the bound is enforced on read, not on open") + t.Cleanup(func() { _ = rc.Close() }) + + _, err = io.ReadAll(rc) + + require.ErrorIs(t, err, errDecompressedTooLarge) +} + +func Test_processControlTar_rejectsABombedControlMember(t *testing.T) { + // end to end through the real consumer: a tar reader treats a truncated stream as a clean end of + // archive, so this asserts the bomb surfaces as an error instead of a package missing its files. + // "control" parses fully before the md5sums stream dies, so this also covers the Fix C contract: a + // control tar read failure must not drop the package that was already parsed. + var tarBuf bytes.Buffer + tw := tar.NewWriter(&tarBuf) + writeTarEntry(t, tw, "control", "Package: test-package\nVersion: 1.0.0\n") + writeTarEntry(t, tw, "md5sums", strings.Repeat("d41d8cd98f00b204e9800998ecf8427e usr/bin/x\n", 20000)) + require.NoError(t, tw.Close()) + + metadata, err := processControlTar(newBoundedReadCloser(io.NopCloser(bytes.NewReader(tarBuf.Bytes())), 4096)) + + require.ErrorIs(t, err, errDecompressedTooLarge) + require.NotNil(t, metadata, "a control tar read failure after \"control\" was parsed must not drop the package") + assert.Equal(t, "test-package", metadata.Package) +} + +func Test_processControlTar_clippedListingIsUsableButReported(t *testing.T) { + // the record cap must not drop the package: metadata comes back usable AND an error explains why the + // file list is short, so the caller can record it as an unknown instead of discarding the package + var tarBuf bytes.Buffer + tw := tar.NewWriter(&tarBuf) + writeTarEntry(t, tw, "control", "Package: test-package\nVersion: 1.0.0\n") + writeTarEntry(t, tw, "md5sums", strings.Repeat("d41d8cd98f00b204e9800998ecf8427e usr/bin/x\n", maxDpkgFileRecords+10)) + require.NoError(t, tw.Close()) + + metadata, err := processControlTar(io.NopCloser(bytes.NewReader(tarBuf.Bytes()))) + + require.Error(t, err, "clipping must be reported") + require.ErrorIs(t, err, errClippedFileListing) + require.NotNil(t, metadata, "the package must survive a clipped file listing") + assert.Equal(t, "test-package", metadata.Package) + assert.Len(t, metadata.Files, maxDpkgFileRecords) +} + +// buildDeb assembles a minimal .deb (an ar archive of debian-binary + control.tar.gz + data.tar.gz). +func buildDeb(t *testing.T, controlTar, dataTar []byte) []byte { + t.Helper() + + gzipped := func(b []byte) []byte { + var out bytes.Buffer + w := gzip.NewWriter(&out) + _, err := w.Write(b) + require.NoError(t, err) + require.NoError(t, w.Close()) + return out.Bytes() + } + + var buf bytes.Buffer + aw := ar.NewWriter(&buf) + require.NoError(t, aw.WriteGlobalHeader()) + + for _, m := range []struct { + name string + body []byte + }{ + {"debian-binary", []byte("2.0\n")}, + {"control.tar.gz", gzipped(controlTar)}, + {"data.tar.gz", gzipped(dataTar)}, + } { + require.NoError(t, aw.WriteHeader(&ar.Header{Name: m.name, Size: int64(len(m.body))})) + _, err := aw.Write(m.body) + require.NoError(t, err) + } + + return buf.Bytes() +} + +func Test_parseDebArchive_surfacesPartialParseAsUnknown(t *testing.T) { + // guards a silent failure: unknownErr was accumulated and then dropped on return, so a data.tar that + // failed to parse produced a clean-looking SBOM with no record that anything went wrong. The property + // that actually matters is that the returned error is a *unknown.CoordinateError: that's what decides + // between "recorded as an unknown" (this case) and "aborts the entire scan" (anything else), per + // internal/task/executor.go and syft/create_sbom.go. A plain substring match on "data.tar" would pass + // just as well if unknown.Append were swapped for errors.Join, which is the regression that matters. + var controlTar bytes.Buffer + ctw := tar.NewWriter(&controlTar) + writeTarEntry(t, ctw, "control", "Package: test-package\nVersion: 1.0.0\nArchitecture: all\n") + require.NoError(t, ctw.Close()) + + // a data.tar that is not a valid tar at all, so processDataTar errors + dataTar := []byte(strings.Repeat("not a tar file at all", 100)) + + debBytes := buildDeb(t, controlTar.Bytes(), dataTar) + + loc := file.NewVirtualLocation("/test.deb", "/test.deb") + pkgs, _, err := parseDebArchive(context.Background(), nil, &generic.Environment{}, + file.LocationReadCloser{ + Location: loc, + ReadCloser: io.NopCloser(bytes.NewReader(debBytes)), + }) + + require.Len(t, pkgs, 1, "the package must still be emitted") + assert.Equal(t, "test-package", pkgs[0].Name) + require.Error(t, err, "a failed data.tar parse must not be swallowed") + + coordinateErrors, remaining := unknown.ExtractCoordinateErrors(err) + require.NoError(t, remaining, "no non-coordinate errors should remain in the chain") + require.Len(t, coordinateErrors, 1) + assert.Equal(t, loc.Coordinates, coordinateErrors[0].Coordinates) + assert.Contains(t, coordinateErrors[0].Reason.Error(), "data.tar") +} + +func Test_parseDebArchive_controlTarBombRejectedByRealSizeBound(t *testing.T) { + // end to end through the real entrypoint using the real maxControlTarSize (16MB), not a swapped-in + // test value: nothing else in this suite exercises that constant at its actual call site. The bomb + // entry is written before "control", so "control" is never reached and processControlTar returns nil + // metadata alongside the error -- this exercises parseDebArchive's fatal branch (metadata == nil). + var controlTar bytes.Buffer + ctw := tar.NewWriter(&controlTar) + writeTarEntry(t, ctw, "bomb", strings.Repeat("\x00", 20*1024*1024)) + writeTarEntry(t, ctw, "control", "Package: test-package\nVersion: 1.0.0\n") + require.NoError(t, ctw.Close()) + + debBytes := buildDeb(t, controlTar.Bytes(), nil) + require.Less(t, len(debBytes), 64*1024, "compressed .deb should stay tiny relative to what it expands to") + + loc := file.NewVirtualLocation("/bomb.deb", "/bomb.deb") + pkgs, _, err := parseDebArchive(context.Background(), nil, &generic.Environment{}, + file.LocationReadCloser{ + Location: loc, + ReadCloser: io.NopCloser(bytes.NewReader(debBytes)), + }) + + require.Empty(t, pkgs, "no package should be emitted when the control tar never parses") + require.Error(t, err) + + coordinateErrors, remaining := unknown.ExtractCoordinateErrors(err) + require.NoError(t, remaining, "no non-coordinate errors should remain in the chain") + require.Len(t, coordinateErrors, 1) + require.ErrorIs(t, coordinateErrors[0].Reason, errDecompressedTooLarge) +} + +// arMember is one named, already-encoded ar entry body, for tests that need control over the raw +// member list (order, duplicates) that buildDeb's fixed three-member shape does not allow. +type arMember struct { + name string + body []byte +} + +// buildDebWithMembers is buildDeb generalized to an arbitrary member list. +func buildDebWithMembers(t *testing.T, members []arMember) []byte { + t.Helper() + + var buf bytes.Buffer + aw := ar.NewWriter(&buf) + require.NoError(t, aw.WriteGlobalHeader()) + + for _, m := range members { + require.NoError(t, aw.WriteHeader(&ar.Header{Name: m.name, Size: int64(len(m.body))})) + _, err := aw.Write(m.body) + require.NoError(t, err) + } + + return buf.Bytes() +} + +func Test_parseDebArchive_ignoresDuplicateArMembers(t *testing.T) { + // a .deb has exactly one control.tar.* and one data.tar.*; a second data.tar.* member is + // attacker-supplied filler that would otherwise buy a fresh decompression budget, and here would also + // fail to parse as a tar at all -- so this proves the walk stops after the first of each kind rather + // than merely surviving a well-formed duplicate. + gzipped := func(b []byte) []byte { + var out bytes.Buffer + w := gzip.NewWriter(&out) + _, err := w.Write(b) + require.NoError(t, err) + require.NoError(t, w.Close()) + return out.Bytes() + } + + var controlTar bytes.Buffer + ctw := tar.NewWriter(&controlTar) + writeTarEntry(t, ctw, "control", "Package: test-package\nVersion: 1.0.0\n") + require.NoError(t, ctw.Close()) + + var firstDataTar bytes.Buffer + dtw := tar.NewWriter(&firstDataTar) + require.NoError(t, dtw.Close()) + + debBytes := buildDebWithMembers(t, []arMember{ + {"debian-binary", []byte("2.0\n")}, + // the duplicate sits before control.tar.gz on purpose: the loop cannot have stopped yet at that + // point, so only the per-member guard can keep it from being read + {"data.tar.gz", gzipped(firstDataTar.Bytes())}, + {"data.tar.gz", []byte("not a tar file at all")}, // not even gzipped; would error if ever read + {"control.tar.gz", gzipped(controlTar.Bytes())}, + }) + + pkgs, _, err := parseDebArchive(context.Background(), nil, &generic.Environment{}, + file.LocationReadCloser{ + Location: file.NewVirtualLocation("/test.deb", "/test.deb"), + ReadCloser: io.NopCloser(bytes.NewReader(debBytes)), + }) + + require.NoError(t, err, "the second data.tar.* member must never be reached") + require.Len(t, pkgs, 1) +} + +func Test_processDataTar_capsCopyrightFiles(t *testing.T) { + // a real .deb ships exactly one copyright file per package; this asserts the walk stops and errors + // rather than accumulating an unbounded number of license reads + var tarBuf bytes.Buffer + tw := tar.NewWriter(&tarBuf) + for i := 0; i < maxCopyrightFiles+10; i++ { + writeTarEntry(t, tw, fmt.Sprintf("/usr/share/doc/pkg%d/copyright", i), "License: MIT\n") + } + require.NoError(t, tw.Close()) + + _, err := processDataTar(io.NopCloser(bytes.NewReader(tarBuf.Bytes()))) + + require.Error(t, err) +} diff --git a/syft/pkg/cataloger/debian/parse_dpkg_db.go b/syft/pkg/cataloger/debian/parse_dpkg_db.go index be9f04448..daea1b541 100644 --- a/syft/pkg/cataloger/debian/parse_dpkg_db.go +++ b/syft/pkg/cataloger/debian/parse_dpkg_db.go @@ -141,7 +141,12 @@ func (d *dpkgExtractedMetadata) toDpkgEntry() pkg.DpkgDBEntry { // there may be an optional conffiles section that we should persist as files if d.Conffiles != "" { - entry.Files = parseDpkgConffileInfo(strings.NewReader(d.Conffiles)) + var err error + entry.Files, err = parseDpkgConffileInfo(strings.NewReader(d.Conffiles)) + if err != nil { + // toDpkgEntry has no error return, a log line is all that's available here + log.Debugf("dpkg status conffiles field for package %q: %v", d.Package, err) + } } if entry.Files == nil { diff --git a/syft/pkg/cataloger/debian/parse_dpkg_info_files.go b/syft/pkg/cataloger/debian/parse_dpkg_info_files.go index d9d6802ba..b194184ba 100644 --- a/syft/pkg/cataloger/debian/parse_dpkg_info_files.go +++ b/syft/pkg/cataloger/debian/parse_dpkg_info_files.go @@ -2,6 +2,7 @@ package debian import ( "bufio" + "fmt" "io" "strings" @@ -9,10 +10,27 @@ import ( "github.com/anchore/syft/syft/pkg" ) -func parseDpkgMD5Info(reader io.Reader) (findings []pkg.DpkgFileRecord) { +// maxDpkgFileRecords bounds the file lists parsed out of md5sums and conffiles. A byte cap on the +// enclosing stream is not enough on its own: the shortest line that still yields a record is 4 bytes, +// so 16MB of control.tar buys millions of records. +// +// The number comes from measuring what a record actually retains, not from the struct size. A +// DpkgFileRecord is 32 bytes, but a populated one also heap-allocates a file.Digest and three string +// backing arrays: 121 bytes each with realistic Debian paths, 70 with minimal ones. At this bound that +// is ~24MB per package, and the effective ceiling is that times cataloger parallelism (NumCPU*4). +// The largest real Debian packages ship on the order of 10^4 files, so this leaves ~5x headroom. +const maxDpkgFileRecords = 200_000 + +// parseDpkgMD5Info returns the records it managed to parse, plus a non-nil error when the record bound +// was hit (wrapping errClippedFileListing) or the scanner failed partway through. +func parseDpkgMD5Info(reader io.Reader) ([]pkg.DpkgFileRecord, error) { + var findings []pkg.DpkgFileRecord scanner := bufio.NewScanner(reader) for scanner.Scan() { + if len(findings) >= maxDpkgFileRecords { + return findings, fmt.Errorf("%w: dpkg md5sums listing exceeds %d entries", errClippedFileListing, maxDpkgFileRecords) + } line := scanner.Text() fields := strings.SplitN(line, " ", 2) if len(fields) == 2 { @@ -29,13 +47,22 @@ func parseDpkgMD5Info(reader io.Reader) (findings []pkg.DpkgFileRecord) { }) } } - return findings + if err := scanner.Err(); err != nil { + return findings, fmt.Errorf("failed to scan dpkg md5sums: %w", err) + } + return findings, nil } -func parseDpkgConffileInfo(reader io.Reader) (findings []pkg.DpkgFileRecord) { +// parseDpkgConffileInfo returns the records it managed to parse, plus a non-nil error when the record +// bound was hit (wrapping errClippedFileListing) or the scanner failed partway through. +func parseDpkgConffileInfo(reader io.Reader) ([]pkg.DpkgFileRecord, error) { + var findings []pkg.DpkgFileRecord scanner := bufio.NewScanner(reader) for scanner.Scan() { + if len(findings) >= maxDpkgFileRecords { + return findings, fmt.Errorf("%w: dpkg conffiles listing exceeds %d entries", errClippedFileListing, maxDpkgFileRecords) + } line := strings.Trim(scanner.Text(), " \n") fields := strings.SplitN(line, " ", 2) @@ -70,5 +97,8 @@ func parseDpkgConffileInfo(reader io.Reader) (findings []pkg.DpkgFileRecord) { findings = append(findings, record) } } - return findings + if err := scanner.Err(); err != nil { + return findings, fmt.Errorf("failed to scan dpkg conffiles: %w", err) + } + return findings, nil } diff --git a/syft/pkg/cataloger/debian/parse_dpkg_info_files_test.go b/syft/pkg/cataloger/debian/parse_dpkg_info_files_test.go index 90aafd904..613883e6b 100644 --- a/syft/pkg/cataloger/debian/parse_dpkg_info_files_test.go +++ b/syft/pkg/cataloger/debian/parse_dpkg_info_files_test.go @@ -1,10 +1,13 @@ package debian import ( + "bufio" "os" + "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/anchore/syft/syft/file" @@ -45,7 +48,8 @@ func TestMD5SumInfoParsing(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { require.NoError(t, f.Close()) }) - actual := parseDpkgMD5Info(f) + actual, err := parseDpkgMD5Info(f) + require.NoError(t, err) if diff := cmp.Diff(test.expected, actual); diff != "" { t.Errorf("unexpected md5 files (-want +got):\n%s", diff) @@ -79,7 +83,8 @@ func TestConffileInfoParsing(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { require.NoError(t, f.Close()) }) - actual := parseDpkgConffileInfo(f) + actual, err := parseDpkgConffileInfo(f) + require.NoError(t, err) if diff := cmp.Diff(test.expected, actual); diff != "" { t.Errorf("unexpected md5 files (-want +got):\n%s", diff) @@ -88,3 +93,49 @@ func TestConffileInfoParsing(t *testing.T) { }) } } + +func Test_parseDpkgInfoFiles_boundRecordCount(t *testing.T) { + // a byte cap on the enclosing stream cannot bound these on its own, since the shortest line that + // still yields a record is a few bytes. exercises the real maxDpkgFileRecords const directly, since + // the limit is no longer injectable. + t.Run("md5sums stops at the record bound", func(t *testing.T) { + buf := strings.Repeat("d41d8cd98f00b204e9800998ecf8427e usr/bin/x\n", maxDpkgFileRecords+10) + + got, err := parseDpkgMD5Info(strings.NewReader(buf)) + + assert.Len(t, got, maxDpkgFileRecords) + require.ErrorIs(t, err, errClippedFileListing) + }) + + t.Run("conffiles stops at the record bound", func(t *testing.T) { + buf := strings.Repeat("/etc/x.conf\n", maxDpkgFileRecords+10) + + got, err := parseDpkgConffileInfo(strings.NewReader(buf)) + + assert.Len(t, got, maxDpkgFileRecords) + require.ErrorIs(t, err, errClippedFileListing) + }) + + t.Run("a listing under the bound is returned whole", func(t *testing.T) { + // guards against the bound clipping legitimate input + got, err := parseDpkgMD5Info(strings.NewReader( + "d41d8cd98f00b204e9800998ecf8427e usr/bin/a\n" + + "d41d8cd98f00b204e9800998ecf8427e usr/bin/b\n")) + + require.NoError(t, err) + require.Len(t, got, 2) + assert.Equal(t, "/usr/bin/a", got[0].Path) + assert.Equal(t, "/usr/bin/b", got[1].Path) + }) +} + +func Test_parseDpkgMD5Info_scannerError(t *testing.T) { + // a single line longer than bufio.MaxScanTokenSize with no trailing newline trips the scanner's + // internal error rather than yielding a clean (and silently incomplete) empty list + line := strings.Repeat("a", bufio.MaxScanTokenSize+1) + + got, err := parseDpkgMD5Info(strings.NewReader(line)) + + assert.Empty(t, got) + require.Error(t, err) +}