From e1e3bc7ff64ac85650aa7191838be9015e294a5e Mon Sep 17 00:00:00 2001 From: deveshctl Date: Mon, 13 Jul 2026 11:29:54 +0530 Subject: [PATCH 1/3] fix(image): cap metadata, layer, and daemon-spool reads against DoS Three resource-exhaustion paths in the archive/spool pipeline sat outside the existing MaxLayerBlobSize cap, each reachable from 'layerx ' with no daemon: - A gzip-bomb layer expanded without bound because the analysis path passed the decompressed reader straight to ParseLayerTar. It now mirrors the extraction path with io.LimitReader(dec, MaxLayerBlobSize+1). - manifest.json / image config / legacy root *.json were read with an unbounded io.ReadAll that trusted the tar header's declared size. A new MaxMetadataSize (64 MiB) cap plus a shared readMetadataEntry helper fails fast on an inflated header and bounds the stream when the header understates. - The daemon ImageSave response was spooled to a temp file with no ceiling, letting a rogue DOCKER_HOST fill the disk. A new MaxArchiveSize (64 GiB) cap via spoolFromDaemon bounds both daemon spool sites. Also add TestEfficiency_Golden pinning the full EfficiencyResult for a fixed multi-layer fixture, so a refactor touching the waste denominator fails loudly. --- CHANGELOG.md | 13 ++++++++++ image/copyctx.go | 25 ++++++++++++++++++ image/copyctx_test.go | 36 ++++++++++++++++++++++++++ image/docker.go | 38 ++++++++++++++++++++++----- image/docker_test.go | 43 ++++++++++++++++++++++++++++++ image/efficiency_test.go | 56 ++++++++++++++++++++++++++++++++++++++++ image/extractor.go | 16 +++++++----- 7 files changed, 214 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d339803..3b03bc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The `--json` write confirmation now reads `layerx: wrote analysis to `, matching the `layerx:` prefix used elsewhere on stderr. +### Security +- Layer analysis now caps each decompressed layer at 16 GiB, closing a + gzip-bomb path where a tiny compressed blob could expand without bound and + exhaust memory during `layerx `. The extraction path already + enforced this cap; the analysis path now matches it. +- Archive descriptors (`manifest.json`, image config, legacy root `*.json`) + are now capped at 64 MiB and reject a tar header that overstates its size, + preventing a crafted archive from forcing an unbounded allocation before any + layer is read. +- The image-save stream from a container engine is now capped at 64 GiB while + spooling to a temp file, so a rogue or compromised `DOCKER_HOST` cannot fill + the local disk with an endless response. + ### Fixed - File extraction (`x` key) no longer silently overwrites a dangling symlink at the destination path. If a broken symlink exists at the target, the diff --git a/image/copyctx.go b/image/copyctx.go index 5ee0336..e18e58f 100644 --- a/image/copyctx.go +++ b/image/copyctx.go @@ -2,6 +2,7 @@ package image import ( "context" + "fmt" "io" ) @@ -31,3 +32,27 @@ func copyCtx(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { } } } + +// MaxArchiveSize bounds the total bytes spooled from a daemon's ImageSave +// response to a temp file. A rogue or compromised daemon (a hostile +// DOCKER_HOST=tcp://…, an SSH-tunnelled Podman) can stream an unbounded +// chunked response and fill the local disk before parsing ever begins. The +// per-blob MaxLayerBlobSize cap only fires later, during the tar walk, so the +// spool itself needs its own ceiling. 64 GiB comfortably exceeds any real +// multi-layer image while stopping an endless stream from exhausting /tmp. +const MaxArchiveSize = 64 << 30 // 64 GiB + +// spoolFromDaemon copies an ImageSave stream to dst, aborting with a clear +// error once the source exceeds MaxArchiveSize rather than filling the disk. +// Use this for daemon-sourced streams (untrusted DOCKER_HOST); it is not for +// on-disk archives, whose size is already bounded by the filesystem. +func spoolFromDaemon(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { + n, err := copyCtx(ctx, dst, io.LimitReader(src, MaxArchiveSize+1)) + if err != nil { + return n, err + } + if n > MaxArchiveSize { + return n, fmt.Errorf("image archive exceeds %d bytes: refusing to spool an unbounded daemon response", MaxArchiveSize) + } + return n, nil +} diff --git a/image/copyctx_test.go b/image/copyctx_test.go index 09df066..056bfdd 100644 --- a/image/copyctx_test.go +++ b/image/copyctx_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "io" "sync" "testing" ) @@ -141,3 +142,38 @@ func TestCopyCtx_ReadError(t *testing.T) { t.Fatalf("dst received %d bytes, want %d", dst.Len(), len(chunk)) } } + +func TestSpoolFromDaemon_HappyPath(t *testing.T) { + src := bytes.NewReader(bytes.Repeat([]byte("layerx"), 200_000)) // ~1.2 MB + want := src.Len() + var dst bytes.Buffer + + n, err := spoolFromDaemon(context.Background(), &dst, src) + if err != nil { + t.Fatalf("spoolFromDaemon: unexpected error: %v", err) + } + if int(n) != want { + t.Fatalf("spoolFromDaemon returned %d bytes, want %d", n, want) + } + if dst.Len() != want { + t.Fatalf("dst received %d bytes, want %d", dst.Len(), want) + } +} + +// infiniteReader models a rogue daemon that never stops streaming. It fills +// every buffer and never returns EOF; spoolFromDaemon's internal LimitReader +// is what must bound it. Bytes are discarded, so the test allocates nothing +// beyond the copy buffer regardless of MaxArchiveSize. +type infiniteReader struct{} + +func (infiniteReader) Read(p []byte) (int, error) { return len(p), nil } + +func TestSpoolFromDaemon_RejectsUnboundedStream(t *testing.T) { + n, err := spoolFromDaemon(context.Background(), io.Discard, infiniteReader{}) + if err == nil { + t.Fatalf("spoolFromDaemon accepted an unbounded stream; want cap error") + } + if n <= MaxArchiveSize { + t.Fatalf("spoolFromDaemon returned %d bytes, want > MaxArchiveSize (%d)", n, int64(MaxArchiveSize)) + } +} diff --git a/image/docker.go b/image/docker.go index da5dd1d..b25e9fe 100644 --- a/image/docker.go +++ b/image/docker.go @@ -548,7 +548,7 @@ func parseLayers(ctx context.Context, r io.Reader) ([]Layer, error) { defer os.Remove(spoolPath) defer spool.Close() - if _, err := copyCtx(ctx, spool, r); err != nil { + if _, err := spoolFromDaemon(ctx, spool, r); err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, err } @@ -645,7 +645,12 @@ func parseLayers(ctx context.Context, r io.Reader) ([]Layer, error) { if err != nil { return nil, fmt.Errorf("decompressing layer %s: %w", hdr.Name, err) } - tree, parseErr := ParseLayerTar(dec) + // Cap the decompressed body. tar.Reader bounds the compressed entry, + // but a gzip bomb can expand a small blob into an arbitrarily large + // tar. MaxLayerBlobSize matches the ceiling findFileInLayer enforces + // on the same reader so the analysis path is as bomb-resistant as the + // extraction path. + tree, parseErr := ParseLayerTar(io.LimitReader(dec, MaxLayerBlobSize+1)) dec.Close() if parseErr != nil { return nil, fmt.Errorf("parsing layer %s: %w", hdr.Name, parseErr) @@ -682,15 +687,15 @@ func scanResolveMetadata(spool *os.File) ([]byte, map[string][]byte, map[string] switch { case hdr.Name == "manifest.json": - data, err := io.ReadAll(tr) + data, err := readMetadataEntry(tr, hdr.Size, hdr.Name) if err != nil { - return nil, nil, nil, fmt.Errorf("reading manifest.json: %w", err) + return nil, nil, nil, err } manifestData = data case strings.HasSuffix(hdr.Name, ".json") && !strings.Contains(hdr.Name, "/"): - data, err := io.ReadAll(tr) + data, err := readMetadataEntry(tr, hdr.Size, hdr.Name) if err != nil { - return nil, nil, nil, fmt.Errorf("reading %s: %w", hdr.Name, err) + return nil, nil, nil, err } rootJSON[hdr.Name] = data default: @@ -700,6 +705,25 @@ func scanResolveMetadata(spool *os.File) ([]byte, map[string][]byte, map[string] return manifestData, rootJSON, headers, nil } +// readMetadataEntry reads a small archive descriptor (manifest.json, config, +// legacy root *.json) into memory, capped at MaxMetadataSize. It fails fast +// when the tar header overstates the size and enforces the same ceiling on the +// stream in case the header understates it — a hostile archive can lie in +// either direction. name is used only for the error message. +func readMetadataEntry(tr io.Reader, declaredSize int64, name string) ([]byte, error) { + if declaredSize > MaxMetadataSize { + return nil, fmt.Errorf("invalid image archive: %s too large: %d bytes (limit %d)", name, declaredSize, MaxMetadataSize) + } + data, err := io.ReadAll(io.LimitReader(tr, MaxMetadataSize+1)) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", name, err) + } + if int64(len(data)) > MaxMetadataSize { + return nil, fmt.Errorf("invalid image archive: %s too large: exceeds %d bytes", name, MaxMetadataSize) + } + return data, nil +} + func readEntryFromSpool(spool *os.File, name string) ([]byte, error) { if _, err := spool.Seek(0, io.SeekStart); err != nil { return nil, fmt.Errorf("seek spool: %w", err) @@ -714,7 +738,7 @@ func readEntryFromSpool(spool *os.File, name string) ([]byte, error) { return nil, fmt.Errorf("reading image archive: %w", err) } if hdr.Name == name { - return io.ReadAll(tr) + return readMetadataEntry(tr, hdr.Size, name) } } } diff --git a/image/docker_test.go b/image/docker_test.go index fa017b8..dd1c245 100644 --- a/image/docker_test.go +++ b/image/docker_test.go @@ -154,6 +154,49 @@ func TestParseLayers_MissingManifest(t *testing.T) { assert.Contains(t, err.Error(), "manifest.json not found") } +// tarWithForgedHeaderSize writes a single entry whose tar header DECLARES +// declaredSize bytes while the body only carries len(body). A hostile archive +// uses this to trick an unbounded io.ReadAll into pre-sizing a huge buffer. +// tw.Close is deliberately skipped: a correct close validates that the full +// declared size was written and would fail the fixture. parseLayers reads via +// its own tar.Reader, which surfaces the declared hdr.Size during Next() — +// that is what readMetadataEntry's fast-fail path checks, before the body is +// consumed — so the fixture stays a few bytes on disk while claiming gigabytes. +func tarWithForgedHeaderSize(t *testing.T, name string, declaredSize int64, body []byte) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + require.NoError(t, tw.WriteHeader(&tar.Header{Name: name, Size: declaredSize, Mode: 0644})) + _, _ = tw.Write(body) + return &buf +} + +func TestParseLayers_ManifestExceedsMetadataCap(t *testing.T) { + tarBuf := tarWithForgedHeaderSize(t, "manifest.json", MaxMetadataSize+1, []byte("[]")) + + _, err := parseLayers(context.Background(), tarBuf) + require.Error(t, err) + assert.Contains(t, err.Error(), "manifest.json too large") +} + +// readMetadataEntry must also reject a stream that overruns the cap when the +// header understated the size (a lying header in the opposite direction). +func TestReadMetadataEntry_StreamOverrunRejected(t *testing.T) { + // Header claims 1 byte; body is MaxMetadataSize+1 bytes. The io.LimitReader + // truncates and the post-read length check flags the overrun. + oversized := bytes.Repeat([]byte("x"), int(MaxMetadataSize)+1) + _, err := readMetadataEntry(bytes.NewReader(oversized), 1, "config.json") + require.Error(t, err) + assert.Contains(t, err.Error(), "config.json too large") +} + +func TestReadMetadataEntry_AcceptsSmallDescriptor(t *testing.T) { + body := []byte(`{"ok":true}`) + got, err := readMetadataEntry(bytes.NewReader(body), int64(len(body)), "manifest.json") + require.NoError(t, err) + assert.Equal(t, body, got) +} + func TestParseLayers_MalformedManifest(t *testing.T) { tarBuf := buildTar(t, map[string][]byte{ "manifest.json": []byte("not valid json{{{"), diff --git a/image/efficiency_test.go b/image/efficiency_test.go index 05cca6b..caa4a26 100644 --- a/image/efficiency_test.go +++ b/image/efficiency_test.go @@ -308,3 +308,59 @@ func TestEfficiencyFromAnalysis_MatchesEfficiency(t *testing.T) { assert.InDelta(t, fromLayers.Score, fromAnalysis.Score, 0.0001) assert.Equal(t, fromLayers.WastedFiles, fromAnalysis.WastedFiles) } + +// TestEfficiency_Golden pins every EfficiencyResult field for one fixed +// multi-layer fixture, with each expected number derived below. Unlike the +// focused tests above (each isolating one behaviour), this locks the whole +// result — WastedBytes, Score, and the ordered WastedFiles slice — so a +// refactor that silently changes the denominator (e.g. reverting to raw +// cumulative bytes) or the waste accounting fails loudly here. +// +// Fixture (three layers): +// +// L0: /bin/app = 1000, /lib/shared.so = 500 +// L1: /bin/app = 1200 (rewrite, same run), /etc/config = 300 (new) +// L2: /bin/app = 1500 (rewrite, same run), /etc/config = 300 (unchanged) +// +// Per-path runs and waste: +// - /bin/app: one run [(L0,1000),(L1,1200),(L2,1500)]. All but the last +// occurrence are shadowed → waste = 1000 + 1200 = 2200; 3 byte-contributing +// occurrences. +// - /lib/shared.so: single occurrence → no waste. +// - /etc/config: L1 Added (300); L2 is unchanged carryover (identical size), +// which pathRuns does not record → run [(L1,300)] → no waste. +// +// Totals: +// - WastedBytes = 2200. +// - liveBytes (final stacked tree) = /bin/app 1500 + /lib/shared.so 500 + +// /etc/config 300 = 2300. +// - totalBytes = 2300 + 2200 = 4500. +// - Score = 1 - 2200/4500 = 0.51111…. +// - WastedFiles = exactly one entry (/bin/app); the zero-waste paths are +// omitted. +func TestEfficiency_Golden(t *testing.T) { + layers := []Layer{ + {Index: 0, Size: 1500, Tree: makeTree( + makeDir("bin", "/bin", makeFile("app", "/bin/app", 1000)), + makeDir("lib", "/lib", makeFile("shared.so", "/lib/shared.so", 500)), + )}, + {Index: 1, Size: 1500, Tree: makeTree( + makeDir("bin", "/bin", makeFile("app", "/bin/app", 1200)), + makeDir("etc", "/etc", makeFile("config", "/etc/config", 300)), + )}, + {Index: 2, Size: 1800, Tree: makeTree( + makeDir("bin", "/bin", makeFile("app", "/bin/app", 1500)), + makeDir("etc", "/etc", makeFile("config", "/etc/config", 300)), + )}, + } + + result := Efficiency(layers) + + assert.Equal(t, int64(2200), result.WastedBytes) + assert.InDelta(t, 1.0-2200.0/4500.0, result.Score, 1e-9) + + require.Len(t, result.WastedFiles, 1) + assert.Equal(t, "/bin/app", result.WastedFiles[0].Path) + assert.Equal(t, int64(2200), result.WastedFiles[0].TotalWasted) + assert.Equal(t, 3, result.WastedFiles[0].LayerCount) +} \ No newline at end of file diff --git a/image/extractor.go b/image/extractor.go index 8a231fe..0552fb8 100644 --- a/image/extractor.go +++ b/image/extractor.go @@ -32,6 +32,14 @@ const MaxSaveSize = 2 << 30 // 2 GiB // claims a single blob is petabytes. const MaxLayerBlobSize = 16 << 30 // 16 GiB +// MaxMetadataSize bounds the small text descriptors read whole into memory +// during archive resolution: manifest.json, the image config, and any +// root-level *.json (legacy config payloads). Real descriptors are a few KiB; +// a hostile tar can declare an 8 GiB manifest.json header and OOM the process +// via io.ReadAll before any layer is touched. 64 MiB is orders of magnitude +// above any legitimate descriptor while cheap to reject. +const MaxMetadataSize = 64 << 20 // 64 MiB + type FileContent struct { Path string Data []byte @@ -332,7 +340,7 @@ func (e *DockerExtractor) loadLayerSource(ctx context.Context, imageRef string, _ = os.Remove(spoolPath) } - if _, err := copyCtx(ctx, spool, rc); err != nil { + if _, err := spoolFromDaemon(ctx, spool, rc); err != nil { cleanup() if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, nil, nil, err @@ -398,11 +406,7 @@ func readManifestFromSpool(spool *os.File) ([]byte, error) { return nil, fmt.Errorf("reading image archive: %w", err) } if hdr.Name == "manifest.json" { - data, err := io.ReadAll(tr) - if err != nil { - return nil, fmt.Errorf("reading manifest.json: %w", err) - } - return data, nil + return readMetadataEntry(tr, hdr.Size, "manifest.json") } } } From cad136cb4794dc4282791775ddaa8d4ffe313bf1 Mon Sep 17 00:00:00 2001 From: Devesh Pharswan <87819719+deveshctl@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:09:05 +0530 Subject: [PATCH 2/3] Updated nightly.yml --- .github/workflows/nightly.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index a510da8..f8688d0 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -16,7 +16,7 @@ jobs: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: - go-version: '1.26' + go-version: '1.26.5' # Longer-budget fuzz across the same targets PRs run. 5 minutes per # target is enough to surface coverage-guided crashes the 30s PR smoke # cannot. Crashes are surfaced by go test's non-zero exit; failing @@ -32,6 +32,6 @@ jobs: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: - go-version: '1.26' + go-version: '1.26.5' - run: go install golang.org/x/vuln/cmd/govulncheck@latest - run: govulncheck ./... From 13bf65157ab1a11dfe97e7acbd2c39c2fb881339 Mon Sep 17 00:00:00 2001 From: deveshctl Date: Mon, 13 Jul 2026 16:18:11 +0530 Subject: [PATCH 3/3] test: correct TestEfficiency_Golden expectations for Modified re-emit The golden fixture's /etc/config appears at the same 300-byte size in L1 and L2. The test assumed Stack would classify the L2 re-emit as Unchanged carryover and omit it from the waste run, expecting WastedBytes=2200 and a single WastedFile. Stack has no identical-size shortcut for files: a path present in both the cumulative tree and the incoming layer is always Modified, so /etc/config opens a second Added/Modified occurrence that pathRuns records. The real result is a run [(L1,300),(L2,300)] charging 300 wasted bytes, giving WastedBytes=2500, Score=1-2500/4800, and two WastedFiles (/bin/app 2200, /etc/config 300). Correct the expectations and the derivation comment to match the actual waste-accounting semantics. Production code is unchanged; only the test's predicted numbers were wrong. --- image/efficiency_test.go | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/image/efficiency_test.go b/image/efficiency_test.go index caa4a26..b427861 100644 --- a/image/efficiency_test.go +++ b/image/efficiency_test.go @@ -320,24 +320,28 @@ func TestEfficiencyFromAnalysis_MatchesEfficiency(t *testing.T) { // // L0: /bin/app = 1000, /lib/shared.so = 500 // L1: /bin/app = 1200 (rewrite, same run), /etc/config = 300 (new) -// L2: /bin/app = 1500 (rewrite, same run), /etc/config = 300 (unchanged) +// L2: /bin/app = 1500 (rewrite, same run), /etc/config = 300 (rewrite) // -// Per-path runs and waste: +// Per-path runs and waste. Stack classifies a path present in both the +// cumulative tree and the new layer as Modified regardless of whether its +// bytes changed — there is no identical-size shortcut for files — so a file +// re-emitted at the same size still opens a fresh Added/Modified occurrence +// that pathRuns records: // - /bin/app: one run [(L0,1000),(L1,1200),(L2,1500)]. All but the last // occurrence are shadowed → waste = 1000 + 1200 = 2200; 3 byte-contributing // occurrences. // - /lib/shared.so: single occurrence → no waste. -// - /etc/config: L1 Added (300); L2 is unchanged carryover (identical size), -// which pathRuns does not record → run [(L1,300)] → no waste. +// - /etc/config: L1 Added (300), L2 Modified (300) → run [(L1,300),(L2,300)]; +// the shadowed L1 copy → waste = 300; 2 occurrences. // // Totals: -// - WastedBytes = 2200. +// - WastedBytes = 2200 + 300 = 2500. // - liveBytes (final stacked tree) = /bin/app 1500 + /lib/shared.so 500 + // /etc/config 300 = 2300. -// - totalBytes = 2300 + 2200 = 4500. -// - Score = 1 - 2200/4500 = 0.51111…. -// - WastedFiles = exactly one entry (/bin/app); the zero-waste paths are -// omitted. +// - totalBytes = 2300 + 2500 = 4800. +// - Score = 1 - 2500/4800 = 0.47916…. +// - WastedFiles = two entries, sorted by TotalWasted desc: /bin/app (2200) +// then /etc/config (300); /lib/shared.so is omitted (zero waste). func TestEfficiency_Golden(t *testing.T) { layers := []Layer{ {Index: 0, Size: 1500, Tree: makeTree( @@ -356,11 +360,14 @@ func TestEfficiency_Golden(t *testing.T) { result := Efficiency(layers) - assert.Equal(t, int64(2200), result.WastedBytes) - assert.InDelta(t, 1.0-2200.0/4500.0, result.Score, 1e-9) + assert.Equal(t, int64(2500), result.WastedBytes) + assert.InDelta(t, 1.0-2500.0/4800.0, result.Score, 1e-9) - require.Len(t, result.WastedFiles, 1) + require.Len(t, result.WastedFiles, 2) assert.Equal(t, "/bin/app", result.WastedFiles[0].Path) assert.Equal(t, int64(2200), result.WastedFiles[0].TotalWasted) assert.Equal(t, 3, result.WastedFiles[0].LayerCount) -} \ No newline at end of file + assert.Equal(t, "/etc/config", result.WastedFiles[1].Path) + assert.Equal(t, int64(300), result.WastedFiles[1].TotalWasted) + assert.Equal(t, 2, result.WastedFiles[1].LayerCount) +}