From 91029143f5b641cd9be08636a49c42bae012a131 Mon Sep 17 00:00:00 2001 From: Florian Hoffarth <223641703+FHoffarth@users.noreply.github.com> Date: Sun, 6 Sep 2026 18:37:53 +0200 Subject: [PATCH] fix: cap cumulative PNG metadata decompression Text metadata was budgeted at 8 MiB per file and ICC profiles at 16 MiB, but nothing counted the two together. A 23 KiB PNG carrying a 15 MiB profile and 7.5 MiB of compressed text therefore unpacked to about 22.5 MiB, was accepted by inspection, and cleaned and verified without complaint. Reproduced against 71f320b before touching anything. One DecompressionBudget per file now carries both: 16 MiB in total, the 8 MiB text budget and the 1 MiB single-chunk ceiling unchanged inside it, the profile ceiling unchanged at 16 MiB but no longer additive. Bytes are charged as the reader hands them over, so a stream that dies late has still spent what it delivered, and the reader is cancelled when the budget refuses. Text-only bombs keep the sentence they had; a combined overflow says metadata, because saying text would be untrue. The chunk that breaks a per-item ceiling is deliberately not charged, which is only sound while every caller turns an overflow into a refusal. That invariant is now written down where it has to hold. Original fix by Antigravity. Independently reviewed here: the budget itself stands, the profile budget became a required argument rather than an optional one, and the tests were rewritten - nine of the ten proposed cases passed against the unfixed code, and the reported attack was not among them. Co-Authored-By: Claude Opus 5 --- audit/shared-resource-budget.test.ts | 247 +++++++++++++++++++++++++++ src/core/png.ts | 69 +++++--- 2 files changed, 292 insertions(+), 24 deletions(-) create mode 100644 audit/shared-resource-budget.test.ts diff --git a/audit/shared-resource-budget.test.ts b/audit/shared-resource-budget.test.ts new file mode 100644 index 0000000..d3ecbff --- /dev/null +++ b/audit/shared-resource-budget.test.ts @@ -0,0 +1,247 @@ +import { describe, expect, it } from 'vitest'; +import zlib from 'node:zlib'; +import { cleanFile, inspectFile } from '../src/core/pipeline'; +import { readChunks } from '../src/core/png'; +import { MalformedFileError } from '../src/core/types'; +import { fixture, runBytes } from './harness'; + +const crc = (buf: Buffer): number => { + let c = ~0; + for (const b of buf) { + c ^= b; + for (let k = 0; k < 8; k++) c = (c >>> 1) ^ (0xedb88320 & -(c & 1)); + } + return (~c) >>> 0; +}; + +const makeChunk = (type: string, payload: Buffer): Buffer => { + const body = Buffer.concat([Buffer.from(type, 'latin1'), payload]); + const out = Buffer.alloc(8 + payload.length + 4); + out.writeUInt32BE(payload.length, 0); + body.copy(out, 4); + out.writeUInt32BE(crc(body), 8 + payload.length); + return out; +}; + +const makeIccChunk = (decompressedSize: number, name = 'custom'): Buffer => { + const iccBuf = Buffer.alloc(decompressedSize, 0); + iccBuf.writeUInt32BE(decompressedSize, 0); + iccBuf.set(Buffer.from('acsp', 'latin1'), 36); + + const compressedIcc = zlib.deflateSync(iccBuf); + return makeChunk('iCCP', Buffer.concat([ + Buffer.from(`${name}\0`, 'latin1'), + Buffer.from([0]), + compressedIcc, + ])); +}; + +const makeZtxtChunk = (decompressedSize: number, keyword = 'comment'): Buffer => { + const textBuf = Buffer.alloc(decompressedSize, 0x41); + const compressedText = zlib.deflateSync(textBuf); + return makeChunk('zTXt', Buffer.concat([ + Buffer.from(`${keyword}\0\0`, 'latin1'), + compressedText, + ])); +}; + +const insertChunks = (basePng: Uint8Array, chunks: Buffer[]): Uint8Array => { + const source = Buffer.from(basePng); + const idat = readChunks(new Uint8Array(source)).find((c) => c.type === 'IDAT')!; + return new Uint8Array(Buffer.concat([ + source.subarray(0, idat.start), + ...chunks, + source.subarray(idat.start), + ])); +}; + +describe('shared decompression resource budget', () => { + const base = fixture('clean.png'); + + it('text alone under limit (8 MiB ok)', async () => { + // 8 chunks of 1 MiB each = 8 MiB text + const chunks = Array.from({ length: 8 }, (_, i) => makeZtxtChunk(1024 * 1024, `txt${i}`)); + const png = insertChunks(base, chunks); + const report = await inspectFile(png); + expect(report.format).toBe('png'); + expect(report.findings.filter((f) => f.container === 'zTXt')).toHaveLength(8); + }, 30000); + + it('ICC alone under limit (16 MiB ok)', async () => { + // 1 ICC profile of 16 MiB + const icc = makeIccChunk(16 * 1024 * 1024); + const png = insertChunks(base, [icc]); + const report = await inspectFile(png); + expect(report.format).toBe('png'); + expect(report.findings.some((f) => f.container === 'iCCP-profile')).toBe(true); + }, 30000); + + it('text + ICC together under total limit (8 MiB text + 8 MiB ICC = 16 MiB ok)', async () => { + const icc = makeIccChunk(8 * 1024 * 1024); + const textChunks = Array.from({ length: 8 }, (_, i) => makeZtxtChunk(1024 * 1024, `txt${i}`)); + const png = insertChunks(base, [icc, ...textChunks]); + const report = await inspectFile(png); + expect(report.format).toBe('png'); + expect(report.findings.filter((f) => f.container === 'zTXt')).toHaveLength(8); + expect(report.findings.some((f) => f.container === 'iCCP-profile')).toBe(true); + }, 30000); + + it('text + ICC together over total limit (8 MiB text + 8.1 MiB ICC refused with MalformedFileError)', async () => { + // 8 MiB text + 8.1 MiB ICC = 16.1 MiB (> 16 MiB total) + const icc = makeIccChunk(8 * 1024 * 1024 + 100 * 1024); + const textChunks = Array.from({ length: 8 }, (_, i) => makeZtxtChunk(1024 * 1024, `txt${i}`)); + const png = insertChunks(base, [icc, ...textChunks]); + + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + await expect(inspectFile(png)).rejects.toThrow( + 'The metadata in this image unpacks to more than FilePass will read, so it will not vouch for it.' + ); + }, 30000); + + it('text over 8 MiB alone refused (even without ICC)', async () => { + // 9 chunks of 1 MiB = 9 MiB (> 8 MiB text budget) + const textChunks = Array.from({ length: 9 }, (_, i) => makeZtxtChunk(1024 * 1024, `txt${i}`)); + const png = insertChunks(base, textChunks); + + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + await expect(inspectFile(png)).rejects.toThrow( + 'The text in this image unpacks to more than FilePass will read, so it will not vouch for it.' + ); + }, 30000); + + it('text chunk over 1 MiB refused', async () => { + // Single chunk unpacking to 1.1 MiB (> 1 MiB single chunk ceiling) + const largeChunk = makeZtxtChunk(1024 * 1024 + 100 * 1024, 'large'); + const png = insertChunks(base, [largeChunk]); + + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + await expect(inspectFile(png)).rejects.toThrow( + 'The text in this image unpacks to more than FilePass will read, so it will not vouch for it.' + ); + }, 30000); + + it('multiple chunks cumulative refusal', async () => { + // 17 chunks of 500 KB = 8.5 MiB (> 8 MiB text budget) + const textChunks = Array.from({ length: 17 }, (_, i) => makeZtxtChunk(500 * 1024, `txt${i}`)); + const png = insertChunks(base, textChunks); + + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + await expect(inspectFile(png)).rejects.toThrow( + 'The text in this image unpacks to more than FilePass will read, so it will not vouch for it.' + ); + }, 30000); + + it('corrupt/broken streams inside budget handled gracefully', async () => { + // Truncated deflate stream that unpacks 50 KB then ends abruptly + const textBuf = Buffer.alloc(50 * 1024, 0x42); + const compressed = zlib.deflateSync(textBuf); + const truncatedCompressed = compressed.subarray(0, compressed.length - 10); + const brokenChunk = makeChunk('zTXt', Buffer.concat([ + Buffer.from('broken\0\0', 'latin1'), + truncatedCompressed, + ])); + + const png = insertChunks(base, [brokenChunk]); + const report = await inspectFile(png); + expect(report.format).toBe('png'); + const brokenFinding = report.findings.find((f) => f.container === 'zTXt'); + expect(brokenFinding).toBeDefined(); + expect(brokenFinding?.value).toBe('compressed text FilePass can remove but did not decode'); + }); + + it('failed decompression after partial consumption charges the budget', async () => { + // 8 broken zTXt chunks that each produce ~950 KB before truncating + // Total partially decompressed ~ 7.6 MiB + const brokenChunks = Array.from({ length: 8 }, (_, i) => { + const textBuf = Buffer.alloc(950 * 1024, 0x43); + const compressed = zlib.deflateSync(textBuf); + // Truncate last 15 bytes to force decompression error at end of stream + const truncated = compressed.subarray(0, compressed.length - 15); + return makeChunk('zTXt', Buffer.concat([ + Buffer.from(`broken${i}\0\0`, 'latin1'), + truncated, + ])); + }); + + // 9th valid chunk of 900 KB. + // If broken chunks were free, 0 + 900 KB <= 8 MiB (would pass). + // Because broken chunks charged ~7.6 MiB, 7.6 MiB + 900 KB > 8 MiB (refused). + const finalValidChunk = makeZtxtChunk(900 * 1024, 'final'); + + const png = insertChunks(base, [...brokenChunks, finalValidChunk]); + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + await expect(inspectFile(png)).rejects.toThrow( + 'The text in this image unpacks to more than FilePass will read, so it will not vouch for it.' + ); + }, 30000); + + /** + * The case this fix was written for, kept as a regression: against 71f320b a 23 KiB PNG + * unpacked to about 22.5 MiB of metadata and was accepted, and the clean copy verified. + */ + it('the reported adversarial file - 15 MiB ICC plus 7.5 MiB text - is refused end to end', async () => { + const png = insertChunks(base, [ + makeIccChunk(15 * 1024 * 1024), + ...Array.from({ length: 8 }, (_, i) => makeZtxtChunk(960 * 1024, `txt${i}`)), + ]); + expect(png.length, 'a small file asking for a lot').toBeLessThan(64 * 1024); + + await expect(inspectFile(png)).rejects.toThrow( + 'The metadata in this image unpacks to more than FilePass will read, so it will not vouch for it.', + ); + + // and through the product path, where the refusal has to end in no verdict and no download + const outcome = await runBytes(png, 'metadata-bomb.png'); + expect(outcome.verdict).toBeUndefined(); + expect(outcome.downloadable).toBe(false); + expect(outcome.error).toMatch(/unpacks to more than FilePass will read/); + }, 60000); + + it('the shared budget does not depend on which of the two comes first', async () => { + const text = () => Array.from({ length: 8 }, (_, i) => makeZtxtChunk(1024 * 1024, `txt${i}`)); + // 8 MiB text + 8.5 MiB ICC = 16.5 MiB either way round + const iccFirst = insertChunks(base, [makeIccChunk(8 * 1024 * 1024 + 512 * 1024), ...text()]); + const textFirst = insertChunks(base, [...text(), makeIccChunk(8 * 1024 * 1024 + 512 * 1024)]); + + for (const [name, png] of [['ICC first', iccFirst], ['text first', textFirst]] as const) { + await expect(inspectFile(png), name).rejects.toThrow( + 'The metadata in this image unpacks to more than FilePass will read, so it will not vouch for it.', + ); + } + }, 60000); + + it('an ICC that fills the whole budget leaves no room for even a small text chunk', async () => { + // Neither ceiling is breached on its own: the profile is under 16 MiB and the text is + // far under 8 MiB. Only the shared total refuses this. + const png = insertChunks(base, [makeIccChunk(16 * 1024 * 1024 - 4096), makeZtxtChunk(64 * 1024, 'small')]); + await expect(inspectFile(png)).rejects.toThrow( + 'The metadata in this image unpacks to more than FilePass will read, so it will not vouch for it.', + ); + }, 60000); + + /** + * What the cleaner can and cannot do here. It never inflates text - it drops those chunks + * whole - so the shared total cannot bind on this path, and the budget threaded into + * clean() is future-proofing rather than a second enforced gate. What it does enforce is + * the profile ceiling: a colour profile FilePass cannot vouch for is never rewritten. + */ + it('the cleaner still refuses a profile it cannot vouch for, so cleaning is not a way round', async () => { + const png = insertChunks(base, [makeIccChunk(16 * 1024 * 1024 + 100 * 1024)]); + const report = { format: 'png' as const, byteLength: png.length, findings: [], notes: [] }; + await expect(cleanFile(png, report)).rejects.toThrow(MalformedFileError); + }, 60000); + + it('the cleaner does not decompress text at all, which is why the total cannot bind there', async () => { + // 8 MiB of text plus a profile that on its own is within every ceiling. Inspection + // refuses it on the shared total; the cleaner, asked directly, only reads the profile. + const png = insertChunks(base, [ + makeIccChunk(15 * 1024 * 1024), + ...Array.from({ length: 8 }, (_, i) => makeZtxtChunk(960 * 1024, `txt${i}`)), + ]); + await expect(inspectFile(png)).rejects.toThrow(MalformedFileError); + + const report = { format: 'png' as const, byteLength: png.length, findings: [], notes: [] }; + await expect(cleanFile(png, report), 'documented, not desired: the cleaner sees only the profile') + .resolves.toBeTruthy(); + }, 60000); +}); diff --git a/src/core/png.ts b/src/core/png.ts index 504a6d6..03f5bf2 100644 --- a/src/core/png.ts +++ b/src/core/png.ts @@ -67,7 +67,11 @@ type Inflated = | { status: 'overflow' } | { status: 'unreadable' }; -async function inflateBounded(data: Uint8Array, limit: number, budget?: TextBudget): Promise { +async function inflateBounded( + data: Uint8Array, + limit: number, + onBytes?: (chunkLength: number) => void, +): Promise { if (typeof DecompressionStream === 'undefined') return { status: 'unreadable' }; try { const stream = new Blob([data as BlobPart]).stream().pipeThrough(new DecompressionStream('deflate')); @@ -78,11 +82,15 @@ async function inflateBounded(data: Uint8Array, limit: number, budget?: TextBudg const { done, value } = await reader.read(); if (done) break; total += value.length; - // Charged as it arrives. A stream that hands over most of its bytes and then dies has - // still spent them, and counting only streams that finish would let a broken one work - // for free. - budget?.spend(value.length); + // The chunk that breaks the ceiling is not charged, which is only safe because every + // caller turns 'overflow' into a refusal. If one ever tolerates it, charge first. if (total > limit) { await reader.cancel(); return { status: 'overflow' }; } + try { + onBytes?.(value.length); + } catch (error) { + await reader.cancel(); + throw error; + } parts.push(value); } return { status: 'ok', bytes: concat(parts) }; @@ -93,27 +101,42 @@ async function inflateBounded(data: Uint8Array, limit: number, budget?: TextBudg } /** - * What FilePass is willing to unpack from one text chunk, and from a whole file. Compressed - * text metadata is small in every real image; these are product limits chosen so a file - * cannot make FilePass spend unbounded memory, not a rule the PNG format states. + * What FilePass is willing to unpack from text chunks, ICC profiles, and a whole file. + * Compressed metadata is small in every real image; these are product limits chosen so a file + * cannot make FilePass spend unbounded memory, not rules the PNG format states. */ const MAX_TEXT_BYTES = 1024 * 1024; const MAX_TEXT_BUDGET = 8 * 1024 * 1024; +const MAX_PROFILE_BYTES = 16 * 1024 * 1024; +const MAX_TOTAL_DECOMPRESSION_BUDGET = 16 * 1024 * 1024; /** Raised when a file asks FilePass to unpack more than it is willing to. */ -class TextBudget { - private spent = 0; +class DecompressionBudget { + private totalSpent = 0; + private textSpent = 0; - spend(amount: number): void { - this.spent += amount; - if (this.spent > MAX_TEXT_BUDGET) { + spendText(amount: number): void { + this.textSpent += amount; + if (this.textSpent > MAX_TEXT_BUDGET) { throw new MalformedFileError('The text in this image unpacks to more than FilePass will read, so it will not vouch for it.'); } + this.spendTotal(amount); + } + + spendIcc(amount: number): void { + this.spendTotal(amount); + } + + private spendTotal(amount: number): void { + this.totalSpent += amount; + if (this.totalSpent > MAX_TOTAL_DECOMPRESSION_BUDGET) { + throw new MalformedFileError('The metadata in this image unpacks to more than FilePass will read, so it will not vouch for it.'); + } } } -async function inflate(data: Uint8Array, budget: TextBudget): Promise { - const unpacked = await inflateBounded(data, MAX_TEXT_BYTES, budget); +async function inflate(data: Uint8Array, budget: DecompressionBudget): Promise { + const unpacked = await inflateBounded(data, MAX_TEXT_BYTES, (n) => budget.spendText(n)); if (unpacked.status === 'overflow') { throw new MalformedFileError('The text in this image unpacks to more than FilePass will read, so it will not vouch for it.'); } @@ -122,7 +145,7 @@ async function inflate(data: Uint8Array, budget: TextBudget): Promise { +async function readTextChunk(chunk: Chunk, budget: DecompressionBudget): Promise<{ keyword: string; text: string }> { const nul = chunk.data.indexOf(0); const keyword = utf8(chunk.data.subarray(0, nul < 0 ? chunk.data.length : nul)); const rest = chunk.data.subarray(nul + 1); @@ -146,9 +169,6 @@ const clip = (s: string) => (s.length > 160 ? `${s.slice(0, 157)}...` : s); /** The label FilePass writes in place of whatever text a profile was carrying. */ const PROFILE_LABEL = 'ICC profile'; -/** No real profile comes close to this, and it stops a crafted chunk from expanding forever. */ -const MAX_PROFILE_BYTES = 16 * 1024 * 1024; - const CRC_TABLE = (() => { const table = new Uint32Array(256); for (let n = 0; n < 256; n++) { @@ -315,8 +335,8 @@ function assertChunkCardinality(chunks: Chunk[]): void { * be large enough to hold an ICC header, agree with its own declared size, and carry the * ICC signature. This is a validity check for the claim FilePass makes, not colour management. */ -async function profileBytes(rest: Uint8Array): Promise { - const unpacked = await inflateBounded(rest.subarray(1), MAX_PROFILE_BYTES); +async function profileBytes(rest: Uint8Array, budget: DecompressionBudget): Promise { + const unpacked = await inflateBounded(rest.subarray(1), MAX_PROFILE_BYTES, (n) => budget.spendIcc(n)); const profile = unpacked.status === 'ok' ? unpacked.bytes : undefined; if (!profile) { throw new MalformedFileError('The colour profile in this image could not be unpacked, so FilePass will not vouch for it.'); @@ -361,7 +381,7 @@ export async function inspect(bytes: Uint8Array): Promise { assertRetainedChunkShapes(chunks); const findings: Finding[] = []; const notes: string[] = []; - const budget = new TextBudget(); + const budget = new DecompressionBudget(); for (const chunk of chunks) { if (chunk.type === 'iCCP') { @@ -369,7 +389,7 @@ export async function inspect(bytes: Uint8Array): Promise { // ride along undisclosed. Its name is free text with no effect on rendering, so that // part is replaced with a plain label. const { name, rest } = readProfileChunk(chunk); - const profile = await profileBytes(rest); + const profile = await profileBytes(rest, budget); if (name && name !== PROFILE_LABEL) { findings.push({ id: 'iCCP#name', @@ -468,11 +488,12 @@ export async function clean(bytes: Uint8Array, report: InspectionReport): Promis assertRetainedChunkShapes(chunks); const parts: Uint8Array[] = [bytes.subarray(0, 8)]; const removedContainers = new Set(); + const budget = new DecompressionBudget(); for (const chunk of chunks) { if (chunk.type === 'iCCP') { const { name, rest } = readProfileChunk(chunk); - await profileBytes(rest); // never rewrite a chunk whose profile FilePass cannot vouch for + await profileBytes(rest, budget); // never rewrite a chunk whose profile FilePass cannot vouch for if (name && name !== PROFILE_LABEL) { const label = Uint8Array.from(PROFILE_LABEL, (ch) => ch.charCodeAt(0)); parts.push(buildChunk('iCCP', concat([label, Uint8Array.of(0), rest])));