[PR #2144] [MERGED] fix(artifacts): bound archive decompression by source size #5056

Closed
opened 2026-10-31 01:01:27 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2144
Author: @DeryFerd
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/12/2026
Merged by: @baron

Base: main ← Head: fix/artifact-decompression-budget


📝 Commits (3)

  • 8b09b40 fix(artifacts): bound archive decompression by source size
  • bcdfbd5 fix(artifacts): own decompression accounting per traversal
  • cf1f3c2 test(artifacts): verify declared ZIP output enforcement

📊 Changes

3 files changed (+249 additions, -1 deletions)

View changed files

➕ src/artifacts/ZipArtifactReader.test.ts (+191 -0)
📝 src/artifacts/ZipArtifactReader.ts (+23 -1)
➕ src/domain/artifactDecompressionBudget.ts (+35 -0)

📄 Description

Summary

inspect-artifact and extract-artifact read a ZIP-backed package (.zip, .ipa, .apk, .msix, .appx, .hap, .hsp, app pack) with no ceiling on how far its members may expand. A member is attacker-controlled and can decompress far beyond the archive that carries it, so a small file can drive a long-running or disk-filling operation on the analyst's machine. This change bounds the total decompressed bytes an archive is allowed to produce, scaled to the archive's own size.

There is no linked issue. The behavior was reproduced against current main while reviewing the archive readers; the closest prior work is PR #1081, which proposed a fixed per-entry cap and was closed because it rejected legitimately large members inside large packages.

Problem and expected behavior

Smallest trigger: a ~2 MiB ZIP whose single member is 2 GiB of zeros. On the base revision, inspect-artifact hashes every member to EOF during inventory and extract-artifact writes every member to disk, so this tiny input produces about 30 seconds of CPU during inventory and, on extraction, roughly 150 seconds and 2 GiB written to the output directory. Nothing in the reader refuses it.

The violated invariant is that the work an operation performs must stay proportional to the artifact the analyst chose, not to what a hostile member declares. REA already treats the artifact as untrusted and applies per-document budgets in sibling readers (KeyedArchiveReader, AppleAssetCatalogAnalysis, InterfaceBuilderAnalysis all use 64 MiB), so archive-backed packages should carry an equivalent bound instead of none.

Change and scope

The ceiling scales with the archive's compressed size: max(256 MiB, 500 × archiveSize). A large package keeps a proportionally large budget, and a small archive cannot amplify into unbounded work. This is deliberately different from PR #1081's fixed per-entry cap, which would reject a single large member even when the surrounding package is legitimate. The ratio sits above real artifact compression (measured roughly 3:1 to 15:1, up to about 350:1 for pure text) and below the DEFLATE ceiling (about 1030:1) that a single-entry bomb relies on.

Enforcement is cumulative and happens during enumeration, before any member is decompressed. The reader charges each regular entry's declared uncompressed size against the budget and refuses the archive with the existing typed limit failure once the running total exceeds it. Because zip.js already verifies streamed output against each member's declaration, the declared total is an exact upper bound on the decompression work any later read can drive, and the check covers both inventory hashing and extraction through the shared reader.

The new src/domain/artifactDecompressionBudget.ts owns the policy so the number and its rationale live in one place. The reader change is additive: existing metadata binding, CRC, overlap, metadata-budget, encrypted-entry, and cancellation checks are unchanged. Not included: a configurable environment override, an output-size accounting pass during extraction itself, or changes to the non-ZIP readers. The declared-size bound already covers those paths, and adding an override or a second accounting layer would widen the change without closing a reachable gap.

Contract and boundary impact

  • Semantic owner and earliest changed stage: decompression policy in src/domain/artifactDecompressionBudget.ts; enforcement in ZipArtifactReader.entries() before any member is read.
  • CLI and MCP/tool-catalog contract: no schema change. An over-budget archive fails with the existing ArtifactReaderFailure reason: "limit", which the CLI/MCP error projection already maps to a truncated/limit outcome.
  • Provider, bridge, target-format, or platform compatibility: ZIP-backed package families are affected uniformly. Non-ZIP readers (ASAR, directory, Mach-O, DMG) are unchanged.
  • Evidence, artifact, provenance, or reconstruction contract: the archive is refused before extraction, so no partial output tree is published; inventory and extraction keep their current success shapes for accepted archives.
  • Process execution, authorization, cleanup, or containment impact: none. No new process, no change to output ownership, and the failure path releases the reader and any created output tree as before.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none. No schema or catalog change.

Evidence and regression coverage

  • Tests added or updated: src/artifacts/ZipArtifactReader.test.ts covers an archive that declares more than its size can justify (refused with reason: "limit"), an archive within its budget (enumerates normally), a small archive that still receives the floor, and the budget function's floor-and-scale behavior. The fixture forges the central-directory uncompressed size so the test is fast and deterministic without building a multi-gigabyte archive.
  • Base reproduction or other evidence: on the base revision, a 2 MiB ZIP with a 2 GiB member inventories in about 30 seconds (RSS stayed near 150 MB) and extracts in about 150 seconds while writing 2.0 GiB; a 512 MiB member inventories in about 11 seconds. After the change, both are refused in under 100 ms with no output written. A legitimate 4.9 MiB archive of the real dist/ tree (1364 entries, about 6:1 compression) still inventories normally.
  • User-visible CLI/MCP output (if applicable): an over-budget archive returns the typed limit failure, for example ZIP entries declare 2147483648 uncompressed bytes, exceeding this archive's 1043707000-byte decompression budget (268435456-byte minimum, 500x archive size); a small archive cannot expand this far.
  • Remaining proof gaps: the byte counts above come from a direct harness against the built reader on Windows with Node 24; Linux/macOS and the full CLI/MCP entry points were not exercised for this exact fixture. Extraction-side accounting was not added, so a member that lies about its size and passes the declaration check relies on zip.js's streamed verification, which is already enforced.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run src/artifacts/ZipArtifactReader.test.ts — 4 passed
  • npx vitest run tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/providers/native/zipBackpressureCancellation.test.ts tests/boundary/filesystem/ownedArtifactReaderCleanup.test.ts — 16 passed
  • npx tsc --noEmit -p tsconfig.json — passed
  • npx oxlint src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts src/domain/artifactDecompressionBudget.ts — 0 warnings, 0 errors
  • npx oxfmt --check <same files> — correct format
  • node scripts/verify-module-boundaries.mjs — clean across 1571 source files
  • Empirical probe against the built reader — 2 GiB and 512 MiB bombs refused under 100 ms; a legitimate 4.9 MiB archive still inventories

Compatibility, safety, and release

  • Breaking changes or migration steps: none for a supported contract. An archive that previously would have been processed (a genuine bomb) now fails with a typed limit error; archives within the budget are unaffected.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable. This path does not start a provider; it is the archive reader used by inspect-artifact and extract-artifact.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: this is the security change. It removes a resource-exhaustion path where a small attacker-controlled archive could consume unbounded CPU and disk. The bound is enforced before decompression, uses the existing typed limit failure, and preserves the reader's current containment and cleanup behavior.

Review checklist

  • This PR addresses a concrete problem or an agreed enhancement.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2144 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@baron](https://github.com/baron) **Base:** `main` ← **Head:** `fix/artifact-decompression-budget` --- ### 📝 Commits (3) - [`8b09b40`](https://github.com/morluto/rea/commit/8b09b402c9e4aa5f1c1fd638b7c3f70500c582b3) fix(artifacts): bound archive decompression by source size - [`bcdfbd5`](https://github.com/morluto/rea/commit/bcdfbd586e2748ecbb9fea8b33c48ed67c5725ec) fix(artifacts): own decompression accounting per traversal - [`cf1f3c2`](https://github.com/morluto/rea/commit/cf1f3c2f46cec9ec00e8ba5b7a3c0c1bf6b716a9) test(artifacts): verify declared ZIP output enforcement ### 📊 Changes **3 files changed** (+249 additions, -1 deletions) <details> <summary>View changed files</summary> ➕ `src/artifacts/ZipArtifactReader.test.ts` (+191 -0) 📝 `src/artifacts/ZipArtifactReader.ts` (+23 -1) ➕ `src/domain/artifactDecompressionBudget.ts` (+35 -0) </details> ### 📄 Description ## Summary `inspect-artifact` and `extract-artifact` read a ZIP-backed package (`.zip`, `.ipa`, `.apk`, `.msix`, `.appx`, `.hap`, `.hsp`, app pack) with no ceiling on how far its members may expand. A member is attacker-controlled and can decompress far beyond the archive that carries it, so a small file can drive a long-running or disk-filling operation on the analyst's machine. This change bounds the total decompressed bytes an archive is allowed to produce, scaled to the archive's own size. There is no linked issue. The behavior was reproduced against current `main` while reviewing the archive readers; the closest prior work is PR #1081, which proposed a fixed per-entry cap and was closed because it rejected legitimately large members inside large packages. ## Problem and expected behavior Smallest trigger: a ~2 MiB ZIP whose single member is 2 GiB of zeros. On the base revision, `inspect-artifact` hashes every member to EOF during inventory and `extract-artifact` writes every member to disk, so this tiny input produces about 30 seconds of CPU during inventory and, on extraction, roughly 150 seconds and 2 GiB written to the output directory. Nothing in the reader refuses it. The violated invariant is that the work an operation performs must stay proportional to the artifact the analyst chose, not to what a hostile member declares. REA already treats the artifact as untrusted and applies per-document budgets in sibling readers (`KeyedArchiveReader`, `AppleAssetCatalogAnalysis`, `InterfaceBuilderAnalysis` all use 64 MiB), so archive-backed packages should carry an equivalent bound instead of none. ## Change and scope The ceiling scales with the archive's compressed size: `max(256 MiB, 500 × archiveSize)`. A large package keeps a proportionally large budget, and a small archive cannot amplify into unbounded work. This is deliberately different from PR #1081's fixed per-entry cap, which would reject a single large member even when the surrounding package is legitimate. The ratio sits above real artifact compression (measured roughly 3:1 to 15:1, up to about 350:1 for pure text) and below the DEFLATE ceiling (about 1030:1) that a single-entry bomb relies on. Enforcement is cumulative and happens during enumeration, before any member is decompressed. The reader charges each regular entry's declared uncompressed size against the budget and refuses the archive with the existing typed `limit` failure once the running total exceeds it. Because zip.js already verifies streamed output against each member's declaration, the declared total is an exact upper bound on the decompression work any later read can drive, and the check covers both inventory hashing and extraction through the shared reader. The new `src/domain/artifactDecompressionBudget.ts` owns the policy so the number and its rationale live in one place. The reader change is additive: existing metadata binding, CRC, overlap, metadata-budget, encrypted-entry, and cancellation checks are unchanged. Not included: a configurable environment override, an output-size accounting pass during extraction itself, or changes to the non-ZIP readers. The declared-size bound already covers those paths, and adding an override or a second accounting layer would widen the change without closing a reachable gap. ## Contract and boundary impact - Semantic owner and earliest changed stage: decompression policy in `src/domain/artifactDecompressionBudget.ts`; enforcement in `ZipArtifactReader.entries()` before any member is read. - CLI and MCP/tool-catalog contract: no schema change. An over-budget archive fails with the existing `ArtifactReaderFailure` `reason: "limit"`, which the CLI/MCP error projection already maps to a truncated/limit outcome. - Provider, bridge, target-format, or platform compatibility: ZIP-backed package families are affected uniformly. Non-ZIP readers (ASAR, directory, Mach-O, DMG) are unchanged. - Evidence, artifact, provenance, or reconstruction contract: the archive is refused before extraction, so no partial output tree is published; inventory and extraction keep their current success shapes for accepted archives. - Process execution, authorization, cleanup, or containment impact: none. No new process, no change to output ownership, and the failure path releases the reader and any created output tree as before. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. No schema or catalog change. ## Evidence and regression coverage - Tests added or updated: `src/artifacts/ZipArtifactReader.test.ts` covers an archive that declares more than its size can justify (refused with `reason: "limit"`), an archive within its budget (enumerates normally), a small archive that still receives the floor, and the budget function's floor-and-scale behavior. The fixture forges the central-directory uncompressed size so the test is fast and deterministic without building a multi-gigabyte archive. - Base reproduction or other evidence: on the base revision, a 2 MiB ZIP with a 2 GiB member inventories in about 30 seconds (RSS stayed near 150 MB) and extracts in about 150 seconds while writing 2.0 GiB; a 512 MiB member inventories in about 11 seconds. After the change, both are refused in under 100 ms with no output written. A legitimate 4.9 MiB archive of the real `dist/` tree (1364 entries, about 6:1 compression) still inventories normally. - User-visible CLI/MCP output (if applicable): an over-budget archive returns the typed limit failure, for example `ZIP entries declare 2147483648 uncompressed bytes, exceeding this archive's 1043707000-byte decompression budget (268435456-byte minimum, 500x archive size); a small archive cannot expand this far`. - Remaining proof gaps: the byte counts above come from a direct harness against the built reader on Windows with Node 24; Linux/macOS and the full CLI/MCP entry points were not exercised for this exact fixture. Extraction-side accounting was not added, so a member that lies about its size and passes the declaration check relies on zip.js's streamed verification, which is already enforced. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run src/artifacts/ZipArtifactReader.test.ts` — 4 passed - `npx vitest run tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/providers/native/zipBackpressureCancellation.test.ts tests/boundary/filesystem/ownedArtifactReaderCleanup.test.ts` — 16 passed - `npx tsc --noEmit -p tsconfig.json` — passed - `npx oxlint src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts src/domain/artifactDecompressionBudget.ts` — 0 warnings, 0 errors - `npx oxfmt --check <same files>` — correct format - `node scripts/verify-module-boundaries.mjs` — clean across 1571 source files - Empirical probe against the built reader — 2 GiB and 512 MiB bombs refused under 100 ms; a legitimate 4.9 MiB archive still inventories ## Compatibility, safety, and release - Breaking changes or migration steps: none for a supported contract. An archive that previously would have been processed (a genuine bomb) now fails with a typed limit error; archives within the budget are unaffected. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. This path does not start a provider; it is the archive reader used by `inspect-artifact` and `extract-artifact`. - Package or release metadata impact: none. - Security, privacy, process, or containment review: this is the security change. It removes a resource-exhaustion path where a small attacker-controlled archive could consume unbounded CPU and disk. The bound is enforced before decompression, uses the existing typed limit failure, and preserves the reader's current containment and cleanup behavior. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:01:27 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#5056
No description provided.