fix: bound archive recursion with a cycle guard and depth ceiling

a negative `MaxDepth` really was unbounded, and nothing noticed an archive nested inside a copy of itself. A zip quine at `-1` recursed until memory, disk or file descriptors ran out, running the full sub-pipeline at every level.

- the walk keeps a stack of the digests of the archives it's inside, and an archive identical to one of them is recorded as an unknown instead of being entered
- negative depth now stops at 16 levels (docs and CLI description updated)
- cancellation is checked between candidates, so a cancelled scan stops walking instead of finishing the archive tree

the three unknown-recording blocks in `processArchive` became a one-line helper to stay under `funlen`.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

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.
 //

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)