From be14aa0de92eda99b7862196400e394154996cec Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:29:51 +0800 Subject: [PATCH] fix(artifacts): share decoding budgets and centralize xar validation --- src/artifacts/ArtifactDecodedBudget.ts | 57 +++++++ src/artifacts/ArtifactReader.ts | 3 + src/artifacts/CpioArtifactReader.ts | 52 +++--- src/artifacts/InstallerPackage.fixture.ts | 20 ++- src/artifacts/InstallerPackageLimits.test.ts | 157 ++++++++++++++++++ src/artifacts/XarArtifactReader.ts | 77 ++++++--- src/artifacts/XarValidation.ts | 63 +++++++ .../extraction/ArtifactExtraction.ts | 6 +- src/artifacts/inventory/scanReader.ts | 1 + 9 files changed, 383 insertions(+), 53 deletions(-) create mode 100644 src/artifacts/ArtifactDecodedBudget.ts create mode 100644 src/artifacts/InstallerPackageLimits.test.ts create mode 100644 src/artifacts/XarValidation.ts diff --git a/src/artifacts/ArtifactDecodedBudget.ts b/src/artifacts/ArtifactDecodedBudget.ts new file mode 100644 index 0000000000..b529653e8e --- /dev/null +++ b/src/artifacts/ArtifactDecodedBudget.ts @@ -0,0 +1,57 @@ +import { Transform, type TransformCallback } from "node:stream"; +import { ArtifactReaderFailure } from "./ArtifactReader.js"; + +/** One decoded-byte ceiling shared by an archive and all nested readers. */ +export class ArtifactDecodedBudget { + #used = 0; + + constructor(private readonly maximum = 512 * 1024 * 1024) {} + + /** Charge decoding work, including repeated reads, before exposing a chunk. */ + consume(bytes: number, path: string): void { + if ( + !Number.isSafeInteger(bytes) || + bytes < 0 || + bytes > this.maximum - this.#used + ) + throw new ArtifactReaderFailure( + "limit", + `Decoded archive budget of ${this.maximum} bytes exhausted at ${path}`, + ); + this.#used += bytes; + } +} + +/** Decoder-stage budget/cancellation guard, independent of member metadata. */ +export class ArtifactBudgetTransform extends Transform { + constructor( + private readonly budget: ArtifactDecodedBudget, + private readonly path: string, + private readonly signal?: AbortSignal, + ) { + super(); + } + + override _transform( + chunk: Buffer, + _encoding: BufferEncoding, + done: TransformCallback, + ): void { + if (this.signal?.aborted) { + done( + new ArtifactReaderFailure( + "cancelled", + `Archive decoding cancelled at ${this.path}`, + ), + ); + return; + } + try { + this.budget.consume(chunk.length, this.path); + } catch (cause: unknown) { + done(cause instanceof Error ? cause : new Error(String(cause))); + return; + } + done(null, chunk); + } +} diff --git a/src/artifacts/ArtifactReader.ts b/src/artifacts/ArtifactReader.ts index c23512263d..688e4740f4 100644 --- a/src/artifacts/ArtifactReader.ts +++ b/src/artifacts/ArtifactReader.ts @@ -1,6 +1,7 @@ import type { Readable } from "node:stream"; import type { ArtifactCommand } from "../domain/artifactGraph.js"; import type { ZipPackageFormat } from "../domain/zipPackageFormat.js"; +import type { ArtifactDecodedBudget } from "./ArtifactDecodedBudget.js"; /** Archive-neutral entry metadata. Reader adapters never choose output paths. */ export interface ArtifactEntry { @@ -46,6 +47,8 @@ export interface ArtifactChecksumObservation { /** Read-only adapter over one directory, archive, or virtual container. */ export interface ArtifactReader { readonly format: "directory" | ZipPackageFormat | "asar" | "pkg" | "file"; + /** Shared safety budget for compressed representations and nested archives. */ + readonly decodedBudget?: ArtifactDecodedBudget; entries(signal?: AbortSignal): AsyncIterable; open(entry: ArtifactEntry, signal?: AbortSignal): Promise; /** Checksums observed after the complete member stream has been consumed. */ diff --git a/src/artifacts/CpioArtifactReader.ts b/src/artifacts/CpioArtifactReader.ts index 19bfe2d18d..df62306d90 100644 --- a/src/artifacts/CpioArtifactReader.ts +++ b/src/artifacts/CpioArtifactReader.ts @@ -1,4 +1,8 @@ -import { Readable, Transform, type TransformCallback } from "node:stream"; +import { Readable } from "node:stream"; +import { + ArtifactBudgetTransform, + ArtifactDecodedBudget, +} from "./ArtifactDecodedBudget.js"; import { createGunzip } from "node:zlib"; import type { ArtifactCommand } from "../domain/artifactGraph.js"; @@ -15,8 +19,6 @@ const MAX_LINK_BUFFER_BYTES = 16 * 1024 * 1024; const MAX_LINK_BUFFER_TOTAL = 64 * 1024 * 1024; /** Longest symlink target read; PATH_MAX is 1024 on macOS and 4096 on Linux. */ const MAX_SYMLINK_TARGET_BYTES = 4096; -/** Total decompressed cpio bytes accepted from one archive (gzip-bomb budget). */ -const MAX_TOTAL_DECOMPRESSED_BYTES = 512 * 1024 * 1024; const TRAILER = "TRAILER!!!"; const S_IFMT = 0o170000; const S_IFDIR = 0o040000; @@ -88,29 +90,6 @@ class ByteSource { } } -/** Bound total decompressed bytes so a gzip bomb cannot exhaust the process. */ -class DecodedBudget extends Transform { - #seen = 0; - - override _transform( - chunk: Buffer, - _encoding: BufferEncoding, - done: TransformCallback, - ): void { - this.#seen += chunk.length; - if (this.#seen > MAX_TOTAL_DECOMPRESSED_BYTES) { - done( - new ArtifactReaderFailure( - "limit", - `cpio archive decodes beyond ${MAX_TOTAL_DECOMPRESSED_BYTES} bytes`, - ), - ); - return; - } - done(null, chunk); - } -} - interface CpioHeader { readonly format: "odc" | "newc" | "crc"; readonly mode: number; @@ -252,6 +231,7 @@ const symlinkTargetLimitation = ( export class CpioArtifactReader implements ArtifactReader { readonly format = "file" as const; #source: ByteSource | undefined; + #signal: AbortSignal | undefined; #current: | { readonly key: string; @@ -276,9 +256,11 @@ export class CpioArtifactReader implements ArtifactReader { * siblings are still inventoried, instead of aborting the archive. */ private readonly integrity: "fail" | "record-and-continue" = "fail", + readonly decodedBudget: ArtifactDecodedBudget = new ArtifactDecodedBudget(), ) {} async *entries(signal?: AbortSignal): AsyncIterable { + this.#signal = signal; const compressed = await this.openCompressed(signal); const gunzip = createGunzip(); // `pipe()` does not forward source errors (cancellation, truncation); @@ -296,7 +278,11 @@ export class CpioArtifactReader implements ArtifactReader { // Prevent unhandled source errors after the decoder fails first. compressed.destroy(); }); - const budget = new DecodedBudget(); + const budget = new ArtifactBudgetTransform( + this.decodedBudget, + "gzip-cpio", + signal, + ); gunzip.on("error", (cause: unknown) => { budget.destroy( cause instanceof ArtifactReaderFailure @@ -308,6 +294,7 @@ export class CpioArtifactReader implements ArtifactReader { }); budget.on("error", () => { compressed.destroy(); + gunzip.destroy(); }); const source = new ByteSource(compressed.pipe(gunzip).pipe(budget)); this.#source = source; @@ -595,7 +582,9 @@ export class CpioArtifactReader implements ArtifactReader { path: string, ): AsyncGenerator { let sum = 0; + this.#cancelled(); for await (const chunk of source.take(header.fileSize, "member data")) { + this.#cancelled(); if (header.format === "crc") for (const byte of chunk) sum = (sum + byte) >>> 0; yield chunk; @@ -606,6 +595,7 @@ export class CpioArtifactReader implements ArtifactReader { await drain( source.take(padding(header.format, header.fileSize), "data padding"), ); + this.#cancelled(); if (header.format === "crc" && sum !== header.check) throw new ArtifactReaderFailure( "integrity", @@ -624,6 +614,14 @@ export class CpioArtifactReader implements ArtifactReader { return Buffer.concat(chunks); } + #cancelled(): void { + if (this.#signal?.aborted) + throw new ArtifactReaderFailure( + "cancelled", + "cpio expansion was cancelled", + ); + } + open(entry: ArtifactEntry): Promise { const alias = this.#aliases.get(entry.adapterKey); if (alias !== undefined) return Promise.resolve(Readable.from([alias])); diff --git a/src/artifacts/InstallerPackage.fixture.ts b/src/artifacts/InstallerPackage.fixture.ts index e44612cd5b..0c71f6037c 100644 --- a/src/artifacts/InstallerPackage.fixture.ts +++ b/src/artifacts/InstallerPackage.fixture.ts @@ -1,6 +1,6 @@ /** xar archives and cpio payloads built field by field for installer-package tests. */ import { createHash } from "node:crypto"; -import { deflateSync, gzipSync } from "node:zlib"; +import { deflateSync, gzipSync, inflateSync } from "node:zlib"; export interface XarFixtureMember { readonly name: string; @@ -147,3 +147,21 @@ export const MODE = { symlink: 0o120755, fifo: 0o010644, } as const; + +/** Mutate a fixture's XML while keeping its compressed TOC checksum/lengths consistent. */ +export const rewriteXarToc = ( + archive: Uint8Array, + rewrite: (xml: string) => string, +): Uint8Array => { + const input = Buffer.from(archive); + const oldLength = Number(input.readBigUInt64BE(8)); + const decoded = inflateSync(input.subarray(28, 28 + oldLength)); + const xml = Buffer.from(rewrite(decoded.toString("utf8"))); + const compressed = deflateSync(xml); + const header = Buffer.from(input.subarray(0, 28)); + header.writeBigUInt64BE(BigInt(compressed.length), 8); + header.writeBigUInt64BE(BigInt(xml.length), 16); + const heap = Buffer.from(input.subarray(28 + oldLength)); + createHash("sha1").update(compressed).digest().copy(heap, 0); + return Buffer.concat([header, compressed, heap]); +}; diff --git a/src/artifacts/InstallerPackageLimits.test.ts b/src/artifacts/InstallerPackageLimits.test.ts new file mode 100644 index 0000000000..25b9ca2434 --- /dev/null +++ b/src/artifacts/InstallerPackageLimits.test.ts @@ -0,0 +1,157 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { buffer } from "node:stream/consumers"; +import { Readable } from "node:stream"; +import { expect, it } from "vitest"; +import { ArtifactDecodedBudget } from "./ArtifactDecodedBudget.js"; +import { CpioArtifactReader } from "./CpioArtifactReader.js"; +import { + MODE, + gzipCpio, + xarArchive, + rewriteXarToc, + type XarFixtureMember, +} from "./InstallerPackage.fixture.js"; +import { XarArtifactReader } from "./XarArtifactReader.js"; + +const withReader = async ( + bytes: Uint8Array, + run: (reader: XarArtifactReader) => Promise, + decodedBudget?: ArtifactDecodedBudget, +) => { + const root = await mkdtemp(join(tmpdir(), "rea-pkg-limits-")); + const path = join(root, "fixture.pkg"); + await writeFile(path, bytes); + const reader = new XarArtifactReader( + path, + decodedBudget === undefined ? {} : { decodedBudget }, + ); + try { + await run(reader); + } finally { + await reader.close(); + await rm(root, { recursive: true, force: true }); + } +}; + +it("shares an inflation budget across separate zlib members", async () => { + const bytes = xarArchive([ + { name: "a", data: Buffer.alloc(40, 65), encoding: "zlib" }, + { name: "b", data: Buffer.alloc(40, 66), encoding: "zlib" }, + ]); + await withReader( + bytes, + async (reader) => { + let read = 0; + await expect( + (async () => { + for await (const entry of reader.entries()) { + await buffer(await reader.open(entry)); + read++; + } + })(), + ).rejects.toMatchObject({ + reason: "limit", + message: expect.stringContaining("Decoded archive budget"), + }); + expect(read).toBe(1); + }, + new ArtifactDecodedBudget(64), + ); +}); + +it.each(["", " ", "1e2", "0x10"])( + "rejects nondecimal/empty numeric TOC data: %j", + async (value) => { + const bytes = rewriteXarToc( + xarArchive([{ name: "a", data: Buffer.from("bytes") }]), + (xml) => + xml.replace(/\d+<\/length>/u, `${value}`), + ); + await withReader(bytes, async (reader) => { + await expect( + (async () => { + for await (const entry of reader.entries()) void entry; + })(), + ).rejects.toMatchObject({ + reason: "format", + message: "xar TOC has an invalid data length", + }); + }); + }, +); + +it("rejects unsafe absolute heap ranges with a tagged format failure", async () => { + const bytes = rewriteXarToc( + xarArchive([{ name: "Payload", data: Buffer.from("gzip") }]), + (xml) => + xml.replace( + /20<\/offset>/u, + `${Number.MAX_SAFE_INTEGER}`, + ), + ); + await withReader(bytes, async (reader) => { + await expect( + (async () => { + for await (const entry of reader.entries()) void entry; + })(), + ).rejects.toMatchObject({ + reason: "format", + message: expect.stringContaining("heap extent"), + }); + }); +}); + +it("bounds retained full paths from deeply nested TOCs", async () => { + let member: XarFixtureMember = { name: "leaf", data: Buffer.from("x") }; + for (let depth = 0; depth < 300; depth++) + member = { + name: "directory-segment", + type: "directory", + children: [member], + }; + await withReader(xarArchive([member]), async (reader) => { + await expect( + (async () => { + for await (const entry of reader.entries()) void entry; + })(), + ).rejects.toMatchObject({ + reason: "limit", + message: expect.stringContaining("path text"), + }); + }); +}); + +it("observes cancellation during internal symlink drains", async () => { + const controller = new AbortController(); + const archive = gzipCpio([ + { + name: "huge-link", + mode: MODE.symlink, + data: "x".repeat(4 * 1024 * 1024), + }, + ]); + let decoded = 0; + const budget = new (class extends ArtifactDecodedBudget { + override consume(bytes: number, path: string): void { + super.consume(bytes, path); + decoded += bytes; + if (decoded >= 64 * 1024) controller.abort(); + } + })(); + const reader = new CpioArtifactReader( + async () => Readable.from([archive]), + "fail", + budget, + ); + try { + await expect( + (async () => { + for await (const entry of reader.entries(controller.signal)) void entry; + })(), + ).rejects.toMatchObject({ reason: "cancelled" }); + } finally { + await reader.close(); + } +}); diff --git a/src/artifacts/XarArtifactReader.ts b/src/artifacts/XarArtifactReader.ts index fb3eb9d0e4..fb61da6a11 100644 --- a/src/artifacts/XarArtifactReader.ts +++ b/src/artifacts/XarArtifactReader.ts @@ -11,6 +11,12 @@ import { createInflate, inflateSync } from "node:zlib"; import { DOMParser, type Element, type Node } from "@xmldom/xmldom"; import type { ArtifactCommand } from "../domain/artifactGraph.js"; +import { ArtifactDecodedBudget } from "./ArtifactDecodedBudget.js"; +import { + xarInteger as integer, + xarHeapPosition, + XarPathBudget, +} from "./XarValidation.js"; import { ArtifactReaderFailure, type ArtifactEntry, @@ -85,16 +91,6 @@ const childElement = (element: Element, name: string): Element | undefined => const textOf = (element: Element, name: string): string | undefined => childElement(element, name)?.textContent ?? undefined; -const integer = (value: string | undefined, label: string): number => { - const parsed = value === undefined ? Number.NaN : Number(value.trim()); - if (!Number.isSafeInteger(parsed) || parsed < 0) - throw new ArtifactReaderFailure( - "format", - `xar TOC has an invalid ${label}`, - ); - return parsed; -}; - const parseChecksum = ( parent: Element, tag: string, @@ -200,6 +196,7 @@ const classifyMemberType = ( /** Walk nested `` elements in document order. */ const collectMembers = (toc: Element): XarMember[] => { const members: XarMember[] = []; + const paths = new XarPathBudget(); const pending: Array<{ readonly element: Element; readonly parent: string }> = childElements(toc, "file") .map((element) => ({ element, parent: "" })) @@ -216,7 +213,7 @@ const collectMembers = (toc: Element): XarMember[] => { name === ".." ) throw new ArtifactReaderFailure("path", "xar member has an unsafe name"); - const path = next.parent === "" ? name : `${next.parent}/${name}`; + const path = paths.join(next.parent, name); const type = textOf(next.element, "type")?.trim(); const modeText = textOf(next.element, "mode"); const mode = @@ -272,6 +269,8 @@ class SizeBound extends Transform { constructor( private readonly path: string, private readonly expected: number, + private readonly budget: ArtifactDecodedBudget, + private readonly signal?: AbortSignal, ) { super(); this.#cap = Math.min(expected, MAX_DECODED_MEMBER_BYTES); @@ -282,6 +281,21 @@ class SizeBound extends Transform { _encoding: BufferEncoding, done: TransformCallback, ): void { + if (this.signal?.aborted) { + done( + new ArtifactReaderFailure( + "cancelled", + `xar member decoding cancelled: ${this.path}`, + ), + ); + return; + } + try { + this.budget.consume(chunk.length, this.path); + } catch (cause: unknown) { + done(cause instanceof Error ? cause : new Error(String(cause))); + return; + } this.#seen += chunk.length; if (this.#seen > this.#cap) { done( @@ -372,6 +386,8 @@ export class XarArtifactReader implements ArtifactReader { readonly format = "pkg" as const; #handle: FileHandle | undefined; #heap = 0; + #size = 0; + readonly decodedBudget: ArtifactDecodedBudget; #members: readonly XarMember[] = []; readonly #integrity = new Map(); @@ -386,9 +402,13 @@ export class XarArtifactReader implements ArtifactReader { */ constructor( private readonly path: string, - options: { readonly verifyChecksums?: boolean } = {}, + options: { + readonly verifyChecksums?: boolean; + readonly decodedBudget?: ArtifactDecodedBudget; + } = {}, ) { this.#verifyChecksums = options.verifyChecksums ?? true; + this.decodedBudget = options.decodedBudget ?? new ArtifactDecodedBudget(); } async #load(signal?: AbortSignal): Promise { @@ -396,6 +416,7 @@ export class XarArtifactReader implements ArtifactReader { cancelled(signal); const handle = await open(this.path, "r"); this.#handle = handle; + this.#size = (await handle.stat()).size; const header = Buffer.alloc(28); const { bytesRead } = await handle.read(header, 0, 28, 0); if (bytesRead < 28 || header.readUInt32BE(0) !== XAR_MAGIC) @@ -536,18 +557,23 @@ export class XarArtifactReader implements ArtifactReader { const handle = this.#handle; if (handle === undefined) throw new ArtifactReaderFailure("unavailable", "xar archive is closed"); + const position = xarHeapPosition(this.#heap, offset, length, this.#size); const bytes = Buffer.alloc(length); - const { bytesRead } = await handle.read( - bytes, - 0, - length, - this.#heap + offset, - ); - if (bytesRead < length) - throw new ArtifactReaderFailure( - "format", - "xar member extends beyond the archive", + let done = 0; + while (done < length) { + const { bytesRead } = await handle.read( + bytes, + done, + length - done, + position + done, ); + if (bytesRead === 0) + throw new ArtifactReaderFailure( + "format", + "xar member extends beyond the archive", + ); + done += bytesRead; + } return bytes; } @@ -744,7 +770,12 @@ export class XarArtifactReader implements ArtifactReader { inflate.destroy(memberFailure(entry.path, cause)), ); const inflated = archivedChecked.pipe(inflate); - const bounded = new SizeBound(entry.path, data.size); + const bounded = new SizeBound( + entry.path, + data.size, + this.decodedBudget, + signal, + ); inflated.on("error", (cause: unknown) => bounded.destroy(memberFailure(entry.path, cause)), ); diff --git a/src/artifacts/XarValidation.ts b/src/artifacts/XarValidation.ts new file mode 100644 index 0000000000..472ed1b638 --- /dev/null +++ b/src/artifacts/XarValidation.ts @@ -0,0 +1,63 @@ +import { ArtifactReaderFailure } from "./ArtifactReader.js"; + +/** Required XAR integer syntax: nonempty unsigned decimal, exactly representable. */ +export const xarInteger = ( + value: string | undefined, + label: string, +): number => { + const text = value?.trim() ?? ""; + const parsed = /^\d+$/u.test(text) ? Number(text) : Number.NaN; + if (!Number.isSafeInteger(parsed)) + throw new ArtifactReaderFailure( + "format", + `xar TOC has an invalid ${label}`, + ); + return parsed; +}; + +/** Validate the entire absolute heap range before allocating or requesting it. */ +export const xarHeapPosition = ( + heap: number, + offset: number, + length: number, + size: number, +): number => { + const position = heap + offset; + const end = position + length; + if ( + ![position, end, offset, length].every(Number.isSafeInteger) || + offset < 0 || + length < 0 || + end > size + ) + throw new ArtifactReaderFailure( + "format", + "xar member has an invalid or out-of-file heap extent", + ); + return position; +}; + +/** Bound retained path text before constructing paths from deeply nested TOCs. */ +export class XarPathBudget { + #used = 0; + private readonly maximumPathBytes = 4096; + private readonly maximumTotalBytes = 16 * 1024 * 1024; + + /** Form the next path only when individual and cumulative budgets permit it. */ + join(parent: string, name: string): string { + const bytes = + Buffer.byteLength(parent) + + Buffer.byteLength(name) + + (parent.length > 0 ? 1 : 0); + if ( + bytes > this.maximumPathBytes || + bytes > this.maximumTotalBytes - this.#used + ) + throw new ArtifactReaderFailure( + "limit", + `xar TOC path text exceeds its ${this.maximumPathBytes}-byte path or ${this.maximumTotalBytes}-byte cumulative budget`, + ); + this.#used += bytes; + return parent === "" ? name : `${parent}/${name}`; + } +} diff --git a/src/artifacts/extraction/ArtifactExtraction.ts b/src/artifacts/extraction/ArtifactExtraction.ts index 38dc9568a9..fd330fb721 100644 --- a/src/artifacts/extraction/ArtifactExtraction.ts +++ b/src/artifacts/extraction/ArtifactExtraction.ts @@ -172,8 +172,10 @@ const materializeSelection = async ({ registry.add(path, expandable ? "directory" : entry.kind); if (expandable) { const parent = frame.reader; - const nested = new CpioArtifactReader((nestedSignal) => - parent.open(entry, nestedSignal), + const nested = new CpioArtifactReader( + (nestedSignal) => parent.open(entry, nestedSignal), + "fail", + parent.decodedBudget, ); stack.push({ reader: nested, diff --git a/src/artifacts/inventory/scanReader.ts b/src/artifacts/inventory/scanReader.ts index 378a56b3c5..0422b74097 100644 --- a/src/artifacts/inventory/scanReader.ts +++ b/src/artifacts/inventory/scanReader.ts @@ -182,6 +182,7 @@ const visitArtifactEntries = async ( ? new CpioArtifactReader( (signal) => parent.open(entry, signal), context.integrity.mode, + parent.decodedBudget, ) : new AsarArtifactReader(entry.adapterKey); // Only traversed containers can own members, not opaque archive-named files.