diff --git a/cmd/syft/internal/options/pkg.go b/cmd/syft/internal/options/pkg.go index b72def965..3a3cc4f66 100644 --- a/cmd/syft/internal/options/pkg.go +++ b/cmd/syft/internal/options/pkg.go @@ -26,7 +26,7 @@ note: enabling this may result in a performance impact since all discovered comp note: this only applies to the java package cataloger, and is ignored when nested-archive-max-depth is not 0 (every archive is searched)`) descriptions.Add(&o.ExcludeBinaryOverlapByOwnership, `allows users to exclude synthetic binary packages from the sbom these packages are removed if an overlap with a non-synthetic package is found`) - descriptions.Add(&o.NestedArchiveMaxDepth, `maximum depth to recursively catalog nested archives (0 = disabled, -1 = unlimited)`) + descriptions.Add(&o.NestedArchiveMaxDepth, `maximum depth to recursively catalog nested archives (0 = disabled, -1 = as deep as 16 levels)`) descriptions.Add(&o.NestedArchiveMaxMemoryBytes, `maximum bytes of nested archive content held in memory at once (0 = default, -1 = unbounded)`) descriptions.Add(&o.NestedArchiveMaxDiskBytes, `maximum bytes of nested archive content on disk at once (0 = default, -1 = unbounded)`) } diff --git a/internal/task/archive_tasks.go b/internal/task/archive_tasks.go index cb1ce21db..8034fa3b2 100644 --- a/internal/task/archive_tasks.go +++ b/internal/task/archive_tasks.go @@ -8,6 +8,7 @@ import ( "io" "maps" "runtime/debug" + "slices" "time" "github.com/dustin/go-humanize" @@ -36,7 +37,7 @@ const ArchiveCatalogerTaskName = "archive-cataloger" // cfg.MaxDepth; the task drops itself from subPipeline so that is the only recursion. // // exclusions are the scan's exclusion patterns, applied inside each archive. Returns nil when archive -// cataloging is disabled (MaxDepth == 0; negative means unbounded). +// cataloging is disabled (MaxDepth == 0; negative means as deep as unboundedArchiveDepth). func NewArchiveCatalogerTask(cfg cataloging.ArchiveSearchConfig, subPipeline []Task, exclusions []string) Task { return newArchiveCatalogerTask(cfg.MaxDepth, archiveLimits(cfg), subPipeline, exclusions) } @@ -109,6 +110,10 @@ type archiveCataloger struct { // progress is the one row for the whole walk: its count is how many archives have been entered progress *monitor.TaskProgress + // ancestors are the digests of the archives the walk is currently inside. The walk is depth-first + // and sequential, so this is a stack. + ancestors []file.Digest + // the archive that spent the longest on its own extraction and cataloging, excluding nested archives slowest time.Duration slowestPath string @@ -129,13 +134,24 @@ func (c *archiveCataloger) logStats() { log.WithFields(fields...).Info("nested archive cataloging complete") } +// unboundedArchiveDepth is the depth a negative MaxDepth stops at. No real nesting comes close, and a +// hostile archive (a zip quine, a recursive bomb) must still end. +const unboundedArchiveDepth = 16 + // catalog processes every archive in the resolver, recursing into each up to maxDepth. func (c *archiveCataloger) catalog(ctx context.Context, resolver file.Resolver, depth int, builder sbomsync.Builder) error { - if c.maxDepth >= 0 && depth >= c.maxDepth { + maxDepth := c.maxDepth + if maxDepth < 0 { + maxDepth = unboundedArchiveDepth + } + if depth >= maxDepth { return nil } var errs error for _, candidate := range c.discoverArchives(resolver) { + if err := ctx.Err(); err != nil { + return err + } if err := c.processArchive(ctx, resolver, candidate, depth, builder); err != nil { errs = unknown.Append(errs, candidate.location, err) } @@ -175,10 +191,7 @@ func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver fi extracted, err := archive.Extract(ctx, archiveContent, location.FileSystemID, archivePath, c.limiter, c.exclusions) if errors.Is(err, archive.ErrDiskLimitReached) { // nothing is released while this archive waits, so skip it rather than block the scan - appendUnknowns(builder, ArchiveCatalogerTaskName, []unknown.CoordinateError{{ - Coordinates: location.Coordinates, - Reason: fmt.Errorf("archive skipped: its content would exceed the disk limit"), - }}) + recordArchiveUnknown(builder, location.Coordinates, "archive skipped: its content would exceed the disk limit") return nil } if err != nil { @@ -193,11 +206,16 @@ func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver fi } defer extracted.Cleanup() + // an archive whose bytes match one it is nested in (a zip quine) would recurse forever + if c.isAncestor(extracted.Digests) { + recordArchiveUnknown(builder, location.Coordinates, "archive not cataloged: it is identical to an archive that contains it") + return nil + } + c.ancestors = append(c.ancestors, extracted.Digests...) + defer func() { c.ancestors = c.ancestors[:len(c.ancestors)-len(extracted.Digests)] }() + if extracted.Truncated { - appendUnknowns(builder, ArchiveCatalogerTaskName, []unknown.CoordinateError{{ - Coordinates: location.Coordinates, - Reason: fmt.Errorf("archive cataloged from part of its contents: extraction stopped at the configured memory or disk limit"), - }}) + recordArchiveUnknown(builder, location.Coordinates, "archive cataloged from part of its contents: extraction stopped at the configured memory or disk limit") } traversal := &archive.Traversal{Location: location, Digests: extracted.Digests} @@ -216,6 +234,19 @@ func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver fi return c.catalog(ctx, extracted, depth+1, builder) } +func (c *archiveCataloger) isAncestor(digests []file.Digest) bool { + for _, d := range digests { + if slices.Contains(c.ancestors, d) { + return true + } + } + return false +} + +func recordArchiveUnknown(builder sbomsync.Builder, coordinates file.Coordinates, reason string) { + appendUnknowns(builder, ArchiveCatalogerTaskName, []unknown.CoordinateError{{Coordinates: coordinates, Reason: errors.New(reason)}}) +} + // runSubPipeline runs every task against the resolver into a throwaway SBOM, so results can be // attributed to the containing archive before merging into the shared SBOM. // diff --git a/internal/task/archive_tasks_test.go b/internal/task/archive_tasks_test.go index c57d1df27..e5ae4113c 100644 --- a/internal/task/archive_tasks_test.go +++ b/internal/task/archive_tasks_test.go @@ -6,6 +6,8 @@ import ( "bytes" "compress/gzip" "context" + "crypto/sha1" + "encoding/hex" "errors" "fmt" "io" @@ -314,6 +316,59 @@ func Test_archiveCataloger_depthBoundIsExact(t *testing.T) { assert.ElementsMatch(t, []string{"/outer.zip", "/outer.zip:nested/inner.zip"}, run(t, 2), "depth 2 descends one level") } +func Test_archiveCataloger_negativeDepthStopsAtTheCeiling(t *testing.T) { + // "unbounded" must still end, or a zip quine or recursive bomb runs forever + chain := makeZip(t, map[string][]byte{"deep.txt": []byte("deep")}) + for range unboundedArchiveDepth + 4 { + chain = makeZip(t, map[string][]byte{"n.zip": chain}) + } + rootDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "outer.zip"), chain, 0o600)) + + var seen []string + tsk := newTestTask(t, cataloging.DefaultArchiveSearchConfig().WithMaxDepth(-1), capturingTask(t, &seen, nil)) + _ = tsk.Execute(context.Background(), dirTestResolver{dir: rootDir}, sbomsync.NewBuilder(newTestSBOM())) + + assert.Len(t, seen, unboundedArchiveDepth) +} + +func Test_archiveCataloger_anArchiveIdenticalToAnAncestorIsNotEntered(t *testing.T) { + // a zip quine contains itself. Building one is impractical, so the walk is started as if it were + // already inside an archive with these exact bytes, which is the state a quine reaches one level down + outerZip := makeZip(t, map[string][]byte{"a.txt": []byte("a")}) + rootDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "outer.zip"), outerZip, 0o600)) + + var ran int + c := newTestCataloger(-1, archive.NewLimiter(archive.Limits{MaxMemoryBytes: -1, MaxDiskBytes: -1}), countingTask(&ran)) + sum := sha1.Sum(outerZip) + c.ancestors = []file.Digest{{Algorithm: "sha1", Value: hex.EncodeToString(sum[:])}} + + s := newTestSBOM() + require.NoError(t, c.catalog(context.Background(), dirTestResolver{dir: rootDir}, 0, sbomsync.NewBuilder(s))) + + assert.Zero(t, ran, "the sub-pipeline must not run inside a repeat of an ancestor") + reasons := s.Artifacts.Unknowns[file.Coordinates{RealPath: "/outer.zip"}] + require.Len(t, reasons, 1) + assert.Contains(t, reasons[0], "identical to an archive that contains it") + assert.Len(t, c.ancestors, 1, "the walk leaves the ancestor stack as it found it") +} + +func Test_archiveCataloger_stopsWhenTheContextIsCancelled(t *testing.T) { + rootDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "outer.zip"), makeZip(t, map[string][]byte{"a.txt": []byte("a")}), 0o600)) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var ran int + tsk := newTestTask(t, cataloging.DefaultArchiveSearchConfig().WithMaxDepth(1), countingTask(&ran)) + err := tsk.Execute(ctx, dirTestResolver{dir: rootDir}, sbomsync.NewBuilder(newTestSBOM())) + + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, ran) +} + func Test_NewArchiveCatalogerTask_dropsItselfFromSubPipeline(t *testing.T) { // a copy of itself in the sub-pipeline would process every nesting level twice cfg := cataloging.DefaultArchiveSearchConfig().WithMaxDepth(2) diff --git a/syft/cataloging/archive_search.go b/syft/cataloging/archive_search.go index 3d1db2783..05906839a 100644 --- a/syft/cataloging/archive_search.go +++ b/syft/cataloging/archive_search.go @@ -23,7 +23,7 @@ type ArchiveSearchConfig struct { // IncludeUnindexedArchives indicates whether to search within unindexed archive files (e.g., .tar*). IncludeUnindexedArchives bool `yaml:"include-unindexed-archives" json:"include-unindexed-archives" mapstructure:"include-unindexed-archives"` - // MaxDepth is how many levels of nested archives to catalog: 0 disables it, negative is unbounded. + // MaxDepth is how many levels of nested archives to catalog: 0 disables it, negative goes as deep as 16 levels. MaxDepth int `yaml:"max-depth" json:"max-depth" mapstructure:"max-depth"` // MaxMemoryBytes bounds the archive content held in memory at once; content that does not fit is diff --git a/syft/pkg/cataloger/java/capabilities.yaml b/syft/pkg/cataloger/java/capabilities.yaml index bde560445..4ccae2cef 100644 --- a/syft/pkg/cataloger/java/capabilities.yaml +++ b/syft/pkg/cataloger/java/capabilities.yaml @@ -8,7 +8,7 @@ configs: # AUTO-GENERATED - config structs and their fields - key: IncludeUnindexedArchives description: IncludeUnindexedArchives indicates whether to search within unindexed archive files (e.g., .tar*). - key: MaxDepth - description: 'MaxDepth is how many levels of nested archives to catalog: 0 disables it, negative is unbounded.' + description: 'MaxDepth is how many levels of nested archives to catalog: 0 disables it, negative goes as deep as 16 levels.' - key: MaxMemoryBytes description: MaxMemoryBytes bounds the archive content held in memory at once; content that does not fit is written to disk. Zero means DefaultArchiveMaxMemoryBytes, negative is unbounded. - key: MaxDiskBytes