Materialize per-layer artifacts with explicit whiteout handling - #456
Materialize per-layer artifacts with explicit whiteout handling#456chruffins wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2ea330e. Configure here.
2ea330e to
57b4c9c
Compare
57b4c9c to
d2068dc
Compare
a08da7b to
6e93aa4
Compare
4a6cbdf to
85d7ec5
Compare
85d7ec5 to
70c6422
Compare
de55693 to
e8b5a05
Compare
e8b5a05 to
0374d70
Compare
93d7a10 to
335b70d
Compare
3ad12a5 to
eb2b97b
Compare
68bf31c to
d883208
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := io.Copy(file, tr); err != nil { |
There was a problem hiding this comment.
Decompression bomb: layer extraction has no cumulative size cap
Semgrep rule: go.lang.security.decompression_bomb.potential-dos-via-decompression-bomb
The rule fired on line 291 (io.Copy(io.Discard, reader)), but that copy targets io.Discard — it burns CPU, not disk. The real instance of the same class is here: every tar.TypeReg entry is written through this unbounded io.Copy(file, tr), and neither extractTarFile nor the loop in unpackLayerBlob enforces any ceiling on total unpacked bytes or entry count.
Why this is a true positive
- Layer blobs and media types come from a remote OCI manifest (
lib/images/oci.go:474), i.e. a customer-supplied image reference — the compressed input is attacker-controlled. stats.unpackedBytesis accumulated but never compared against a limit, andheader.Sizeis trusted for accounting only.- The diffID check in
materializeLayerArtifact(lib/images/layer_artifact.go:173) runs after extraction finishes, and is skipped entirely whendesc.DiffID == "", so it cannot bound the write. - Disk admission control in
lib/resources/resource.gosums images already marked ready; it does not throttle an in-progress unpack. On a shared hypervisor host, a ~1 KB gzip layer expanding to hundreds of GB fills the data partition for every co-tenant VM.
Note this path is currently reached only from layer_artifact_test.go — nothing else calls materializeLayerArtifact yet — so this is latent, and worth fixing before it is wired into the pull path.
Recommended fix — thread an explicit budget through the unpack loop and bound the per-file copy:
const (
maxUnpackedBytes = 32 << 30 // 32 GiB per layer
maxEntries = 1 << 20
)
// unpackLayerBlob: budget := int64(maxUnpackedBytes), and in the loop
if stats.entries++; stats.entries > maxEntries {
return nil, fmt.Errorf("layer exceeds %d entries", maxEntries)
}
// extractTarFile: refuse to write past the remaining budget
func extractTarFile(tr *tar.Reader, target string, header *tar.Header, budget *int64) error {
// ...
n, err := io.CopyN(file, tr, *budget+1)
if err != nil && err != io.EOF {
_ = file.Close()
return err
}
if n > *budget {
_ = file.Close()
return fmt.Errorf("layer exceeds unpacked size limit of %d bytes", maxUnpackedBytes)
}
*budget -= n
// ...
}Do not apply Semgrep's suggested autofix at line 291 (io.CopyN(io.Discard, reader, 1024*1024*256)). Truncating that drain leaves the TeeReader hash incomplete, yielding a wrong stats.diffID and spurious "diff id mismatch" failures for any layer over 256 MB. If you want the trailing-data read bounded too, cap it well above the largest expected layer and treat hitting the cap as an error rather than silently stopping.
If you consider this an accepted risk, suppress with either:
- inline on the flagged line:
// nosemgrep: go.lang.security.decompression_bomb.potential-dos-via-decompression-bomb(or bare// nosemgrepto silence all rules on that line) - or exclude the file by adding
lib/images/layer_artifact.goto.semgrepignore
04e6138 to
36ccb28
Compare
36ccb28 to
06f8b54
Compare
06f8b54 to
041b07a
Compare
041b07a to
aa3a5ea
Compare
fde98d0 to
a0d2aba
Compare
9d06e12 to
da5b9d4
Compare
14cee93 to
4c270b0
Compare
4c270b0 to
fcd3558
Compare

summary
Stacked stage of the image-storage project. Adds a per-layer artifact store at
images/layers/<layer-blob-digest>/:materializeLayerArtifact): reads the compressed layer blob from the existing shared OCI cache (no new downloader), unpacks it into an isolated temp directory, converts to erofs, and installslayer.erofsatomically beside anartifact.jsonrecord. Keyed by layer blob digest plus format/options, so identical layers shared across image versions materialize once..wh.<name>and.wh..wh..opqmarkers are recorded in the artifact record (dir, target, opaque) instead of being assumed to compose.applyLayerTree): merges one unpacked layer into a target tree with correct OCI semantics — whiteouts/opaque markers remove or mask lower-layer content first, then the layer's own entries are copied on top, with whiteout-then-recreate pairs resolved correctly. Raw tar whiteout files are interpreted explicitly; they are never passed to overlayfs. Symlinks are removed rather than followed during deletion; hardlinks within a layer stay linked; path traversal is confined.validation
Synthetic OCI layouts and hand-built trees cover: materialization fields and reuse, missing-blob errors, whiteout/opaque inventory, whiteout/opaque/type-replacement/same-layer-recreate semantics, and symlink/hardlink edge cases.
go test ./lib/imagesgreen including-race; Docker Hub-backed tests are intermittently rate-limited in this environment.Note
Medium Risk
New tar unpack and layer-merge logic is security- and correctness-sensitive (path escapes, whiteout semantics, device nodes); it reuses the existing EROFS toolchain but is foundational for future image assembly.
Overview
Introduces a content-addressed per-layer store under
images/layers/<layer-blob-digest>/, with path helpers forlayer.erofsandartifact.json.materializeLayerArtifactbuilds or reuses a layer artifact from the existing OCI cache blob: unpack (gzip/zstd tar) into a temp dir, record metadata including whiteout inventory (.wh.*and opaque.wh..wh..opq), convert to EROFS via existingconvertToErofs, and install atomically. Missing blobs fail clearly; completed artifacts are not rebuilt.applyLayerTreeis the composition primitive that merges one unpacked layer onto a target tree with explicit OCI semantics—apply whiteouts/opaque dirs against lower content first, then copy layer entries (hardlinks, symlinks, type replacements) without leaking whiteout marker files. Tar extraction uses path confinement and full entry types where supported.Tests cover materialization, reuse, whiteout recording, composition edge cases, and symlink/hardlink behavior.
Reviewed by Cursor Bugbot for commit 2ea330e. Configure here.