From c59fbf5e9c3b856d042f02cb18ad4de077eaeab5 Mon Sep 17 00:00:00 2001 From: Dery Ferdika <114888155+DeryFerd@users.noreply.github.com> Date: Tue, 13 Oct 2026 04:15:34 +0700 Subject: [PATCH] fix(artifacts): bound archive decompression by source size (#2144) * fix(artifacts): bound archive decompression by source size A ZIP/APK/IPA/MSIX member is attacker-controlled and can expand far beyond the archive that carries it. Inventory hashed every member to EOF and extraction wrote every member with no ceiling, so a ~2 MiB archive could drive ~30 s of CPU during inventory and ~150 s plus 2 GiB of disk writes during extraction. A fixed per-entry byte cap is the wrong shape: it rejects legitimately large members inside large packages. The ceiling instead scales with the archive's own size (500x, with a 256 MiB floor), so large packages keep a proportionally large budget while a small archive cannot amplify into unbounded work. The cumulative declared size is charged from the central directory before any entry is decompressed, and zip.js already verifies streamed output against each declaration, so the check is exact and covers both inventory and extraction through the shared reader. * fix(artifacts): own decompression accounting per traversal * test(artifacts): verify declared ZIP output enforcement --------- Co-authored-by: baron --- src/artifacts/ZipArtifactReader.test.ts | 191 ++++++++++++++++++++++ src/artifacts/ZipArtifactReader.ts | 24 ++- src/domain/artifactDecompressionBudget.ts | 35 ++++ 3 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 src/artifacts/ZipArtifactReader.test.ts create mode 100644 src/domain/artifactDecompressionBudget.ts diff --git a/src/artifacts/ZipArtifactReader.test.ts b/src/artifacts/ZipArtifactReader.test.ts new file mode 100644 index 000000000..679882ec7 --- /dev/null +++ b/src/artifacts/ZipArtifactReader.test.ts @@ -0,0 +1,191 @@ +import { readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { + TextReader, + Uint8ArrayReader, + Uint8ArrayWriter, + ZipWriter, +} from "@zip.js/zip.js"; +import { describe, expect, it } from "vitest"; + +import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; + +import { ZipArtifactReader } from "./ZipArtifactReader.js"; +import { + ARTIFACT_DECOMPRESSION_RATIO, + artifactDecompressionBudgetBytes, + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES, +} from "../domain/artifactDecompressionBudget.js"; + +/** Small real ZIP carriers with forged sizes; metadata-only controls, never decompressed. */ +const writeZipWithDeclaredSize = async ( + directory: string, + declaredBytes: readonly number[], +): Promise => { + const path = join(directory, "fixture.zip"); + const writer = new ZipWriter(new Uint8ArrayWriter()); + for (const [index] of declaredBytes.entries()) + await writer.add( + `member-${index}.bin`, + new TextReader("real member bytes"), + ); + await writeFile(path, await writer.close()); + const bytes = await readFile(path); + let offset = 0; + for (const declared of declaredBytes) { + const centralDirectory = bytes.indexOf( + Buffer.from([0x50, 0x4b, 0x01, 0x02]), + offset, + ); + if (centralDirectory < 0) + throw new Error("Expected a ZIP central directory"); + // Metadata-only controls: these declarations do not describe valid member streams. + bytes.writeUInt32LE(declared, centralDirectory + 24); + offset = centralDirectory + 46; + } + await writeFile(path, bytes); + return path; +}; + +const enumerate = async (reader: ZipArtifactReader): Promise => { + let files = 0; + for await (const entry of reader.entries()) + if (entry.kind === "file") files += 1; + return files; +}; + +describe("ZipArtifactReader decompression budget", () => { + it("refuses an archive whose entries declare more than its size can justify", async () => { + const directory = await createTestTempDirectory("rea-zip-budget-bomb-"); + // A ~160-byte archive cannot declare 2 GiB of members without being a bomb. + const path = await writeZipWithDeclaredSize(directory, [2_147_483_647]); + const reader = new ZipArtifactReader(path, "zip"); + try { + await expect(enumerate(reader)).rejects.toMatchObject({ + name: "ArtifactReaderFailure", + reason: "limit", + }); + } finally { + await reader.close(); + } + }); + + it("charges all declarations in one traversal before yielding an over-budget member", async () => { + const directory = await createTestTempDirectory("rea-zip-budget-total-"); + const half = MINIMUM_ARTIFACT_DECOMPRESSION_BYTES / 2; + const path = await writeZipWithDeclaredSize(directory, [half, half + 1]); + const reader = new ZipArtifactReader(path, "zip"); + const iterator = reader.entries()[Symbol.asyncIterator](); + try { + expect((await iterator.next()).value?.declaredSize).toBe(half); + await expect(iterator.next()).rejects.toMatchObject({ reason: "limit" }); + } finally { + await iterator.return?.(); + await reader.close(); + } + }); + + it("keeps repeated and interleaved metadata traversals independently charged", async () => { + const directory = await createTestTempDirectory( + "rea-zip-budget-iterators-", + ); + const path = await writeZipWithDeclaredSize(directory, [ + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES / 2, + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES / 2, + ]); + const reader = new ZipArtifactReader(path, "zip"); + const first = reader.entries()[Symbol.asyncIterator](); + const second = reader.entries()[Symbol.asyncIterator](); + try { + expect((await first.next()).done).toBe(false); + expect((await second.next()).done).toBe(false); + expect((await first.next()).done).toBe(false); + expect((await second.next()).done).toBe(false); + expect((await first.next()).done).toBe(true); + expect((await second.next()).done).toBe(true); + expect(await enumerate(reader)).toBe(2); + } finally { + await Promise.all([first.return?.(), second.return?.()]); + await reader.close(); + } + }); + + it("streams a valid small member through open", async () => { + const directory = await createTestTempDirectory("rea-zip-budget-valid-"); + const path = join(directory, "valid.zip"); + const writer = new ZipWriter(new Uint8ArrayWriter()); + await writer.add("member.bin", new TextReader("valid member bytes")); + await writeFile(path, await writer.close()); + const reader = new ZipArtifactReader(path, "zip"); + try { + const entries = []; + for await (const entry of reader.entries()) entries.push(entry); + const entry = entries[0]; + if (entry === undefined) throw new Error("Expected a ZIP member"); + const chunks: Buffer[] = []; + for await (const chunk of await reader.open(entry)) + chunks.push(Buffer.from(chunk)); + expect(Buffer.concat(chunks).toString()).toBe("valid member bytes"); + } finally { + await reader.close(); + } + }); + + it("rejects under-declared member output before delivering its complete actual bytes", async () => { + const directory = await createTestTempDirectory("rea-zip-underdeclared-"); + const path = join(directory, "underdeclared.zip"); + const content = new Uint8Array(128 * 1024); + const writer = new ZipWriter(new Uint8ArrayWriter()); + await writer.add("member.bin", new Uint8ArrayReader(content), { + dataDescriptor: false, + bufferedWrite: true, + }); + const bytes = Buffer.from(await writer.close()); + const local = bytes.indexOf(Buffer.from([0x50, 0x4b, 0x03, 0x04])); + const central = bytes.indexOf(Buffer.from([0x50, 0x4b, 0x01, 0x02])); + if (local < 0 || central < 0) throw new Error("Expected ZIP entry headers"); + // Both headers agree and no descriptor remains: rejection must enforce + // actual decompressed output, not merely detect conflicting metadata. + expect(bytes.readUInt16LE(local + 6) & 8).toBe(0); + expect(bytes.readUInt16LE(local + 8)).toBe(8); + expect(bytes.readUInt32LE(local + 22)).toBe(content.length); + expect(bytes.readUInt32LE(central + 24)).toBe(content.length); + bytes.writeUInt32LE(1, local + 22); + bytes.writeUInt32LE(1, central + 24); + await writeFile(path, bytes); + const reader = new ZipArtifactReader(path, "zip"); + try { + const entries = []; + for await (const entry of reader.entries()) entries.push(entry); + const entry = entries[0]; + if (entry === undefined) throw new Error("Expected a ZIP member"); + expect(entry.declaredSize).toBe(1); + let deliveredBytes = 0; + await expect( + (async () => { + for await (const chunk of await reader.open(entry)) + deliveredBytes += Buffer.from(chunk).length; + })(), + ).rejects.toThrow("Invalid uncompressed size"); + expect(deliveredBytes).toBeLessThan(content.length); + } finally { + await reader.close(); + } + }); +}); + +describe("artifactDecompressionBudgetBytes", () => { + it("grants at least the floor and grows with the compressed size", () => { + expect(artifactDecompressionBudgetBytes(0)).toBe( + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES, + ); + expect(artifactDecompressionBudgetBytes(1024)).toBe( + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES, + ); + const large = MINIMUM_ARTIFACT_DECOMPRESSION_BYTES * 4; + expect(artifactDecompressionBudgetBytes(large)).toBe( + large * ARTIFACT_DECOMPRESSION_RATIO, + ); + }); +}); diff --git a/src/artifacts/ZipArtifactReader.ts b/src/artifacts/ZipArtifactReader.ts index 5ce2ee417..11c68557b 100644 --- a/src/artifacts/ZipArtifactReader.ts +++ b/src/artifacts/ZipArtifactReader.ts @@ -12,6 +12,11 @@ import { type ArtifactReader, } from "./ArtifactReader.js"; import type { ZipPackageFormat } from "../domain/zipPackageFormat.js"; +import { + ARTIFACT_DECOMPRESSION_RATIO, + artifactDecompressionBudgetBytes, + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES, +} from "../domain/artifactDecompressionBudget.js"; import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; import { NonRegularFileReadError, @@ -132,7 +137,6 @@ export class ZipArtifactReader implements ArtifactReader { string, { readonly source: Entry; readonly metadata: ArtifactEntry } >(); - /** Optionally bound each metadata read before allocating its backing buffer. */ constructor( path: string, @@ -153,6 +157,8 @@ export class ZipArtifactReader implements ArtifactReader { async *entries(signal?: AbortSignal): AsyncIterable { let index = 0; + // A traversal owns its accounting, including overlapping or repeated passes. + let declaredFileBytes = 0; for await (const entry of this.#reader.getEntriesGenerator({ decodeText: decodeZipEntryText, })) { @@ -160,6 +166,22 @@ export class ZipArtifactReader implements ArtifactReader { // ZIP member identity is its directory occurrence, not its filename. // Logical path uniqueness belongs to the artifact inventory boundary. const metadata = projectZipEntry(entry, index++); + // zip.js enforces each member's declared output size, so this sum bounds streamed work. + if ( + metadata.kind === "file" && + !metadata.encrypted && + metadata.declaredSize !== null && + Number.isSafeInteger(metadata.declaredSize) && + metadata.declaredSize >= 0 + ) { + declaredFileBytes += metadata.declaredSize; + const budget = artifactDecompressionBudgetBytes(this.#source.size); + if (declaredFileBytes > budget) + throw new ArtifactReaderFailure( + "limit", + `ZIP entries declare ${String(declaredFileBytes)} uncompressed bytes, exceeding this archive's ${String(budget)}-byte decompression budget (${String(MINIMUM_ARTIFACT_DECOMPRESSION_BYTES)}-byte minimum, ${String(ARTIFACT_DECOMPRESSION_RATIO)}x archive size); a small archive cannot expand this far`, + ); + } const stored = this.#entries.get(metadata.adapterKey); if (stored !== undefined && !sameArtifactEntry(metadata, stored.metadata)) throw new ArtifactReaderFailure( diff --git a/src/domain/artifactDecompressionBudget.ts b/src/domain/artifactDecompressionBudget.ts new file mode 100644 index 000000000..2f9396f47 --- /dev/null +++ b/src/domain/artifactDecompressionBudget.ts @@ -0,0 +1,35 @@ +/** + * Aggregate decompression ceiling for one archive-backed artifact operation. + * + * A ZIP/APK/IPA/MSIX member is attacker-controlled and can expand far beyond the + * archive that carries it, so a small input can drive unbounded decompression + * work during inventory hashing and extraction. A fixed per-entry byte cap is + * the wrong shape: it rejects legitimately large members inside large packages + * and cannot distinguish a bomb from a large file. + * + * The ceiling instead scales with the archive's own compressed size, so a large + * package keeps a proportionally large budget while a small archive cannot + * amplify into unbounded CPU time or disk writes. The floor keeps ordinary + * small archives usable, and the ratio sits above real-world artifact + * compression (measured ~3:1-15:1, up to ~350:1 for pure text) but below the + * DEFLATE ceiling (~1030:1) that a single-entry bomb relies on. + */ + +/** Allow a whole archive to expand by this factor before decompression is refused. */ +export const ARTIFACT_DECOMPRESSION_RATIO = 500; + +/** + * Smallest aggregate budget granted to any archive, so small packages stay usable. + * This floor keeps ordinary archives extractable and stays well above the largest + * single members real packages carry; it is not a per-entry cap. + */ +export const MINIMUM_ARTIFACT_DECOMPRESSION_BYTES = 256 * 1024 * 1024; + +/** Largest total decompressed bytes allowed for an archive of this compressed size. */ +export const artifactDecompressionBudgetBytes = ( + compressedBytes: number, +): number => + Math.max( + MINIMUM_ARTIFACT_DECOMPRESSION_BYTES, + Math.floor(compressedBytes) * ARTIFACT_DECOMPRESSION_RATIO, + );