From 6a006c466fbd74a60ff965757d2a2a548b1a70c6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 11:50:35 -0700 Subject: [PATCH 1/4] fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion DocParser handed the raw upload straight to officeparser and then mammoth, both of which inflate every ZIP entry into memory before any app-level size cap applies. The extension is only a routing hint, so a bomb-bearing OOXML archive renamed to .doc selected the one parser that skipped the guard its docx/pptx/xlsx siblings all call. Adds assertOoxmlArchiveWithinLimits to DocParser.parseBuffer, and centrally in file-parsers parseBuffer so a future parser cannot silently opt out. The guard reads the central directory's declared sizes without decompressing, and no-ops for non-ZIP buffers, so legacy OLE .doc files are unaffected. --- apps/sim/lib/file-parsers/doc-parser.test.ts | 101 +++++++++++++++++++ apps/sim/lib/file-parsers/doc-parser.ts | 9 ++ apps/sim/lib/file-parsers/index.ts | 8 ++ 3 files changed, 118 insertions(+) create mode 100644 apps/sim/lib/file-parsers/doc-parser.test.ts diff --git a/apps/sim/lib/file-parsers/doc-parser.test.ts b/apps/sim/lib/file-parsers/doc-parser.test.ts new file mode 100644 index 00000000000..7a88dbbe8f1 --- /dev/null +++ b/apps/sim/lib/file-parsers/doc-parser.test.ts @@ -0,0 +1,101 @@ +/** + * @vitest-environment node + */ +import JSZip from 'jszip' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockParseOfficeAsync, mockExtractRawText } = vi.hoisted(() => ({ + mockParseOfficeAsync: vi.fn(), + mockExtractRawText: vi.fn(), +})) + +vi.mock('officeparser', () => ({ parseOfficeAsync: mockParseOfficeAsync })) +vi.mock('mammoth', () => ({ + default: { extractRawText: mockExtractRawText }, + extractRawText: mockExtractRawText, +})) + +import { DocParser } from '@/lib/file-parsers/doc-parser' + +const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 + +/** + * Build a small OOXML-shaped archive whose central directory *declares* a huge + * uncompressed size. The guard reads declared sizes without inflating anything, + * so this reproduces a zip bomb's central directory at a few hundred bytes. + */ +async function buildDeclaredOversizeArchive(declaredUncompressedBytes: number): Promise { + const zip = new JSZip() + zip.file('word/document.xml', 'A') + const buffer = await zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }) + + for (let offset = 0; offset + 28 <= buffer.length; offset++) { + if (buffer.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + buffer.writeUInt32LE(declaredUncompressedBytes, offset + 24) + return buffer + } + } + throw new Error('No central directory header found in generated archive') +} + +/** A legacy OLE compound-file `.doc` — not a ZIP, so the guard must no-op. */ +function buildLegacyOleDoc(): Buffer { + const buffer = Buffer.alloc(512) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(buffer, 0) + return buffer +} + +describe('DocParser.parseBuffer', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('rejects a ZIP-shaped .doc whose declared expanded size exceeds the cap', async () => { + const bomb = await buildDeclaredOversizeArchive(2 * 1024 * 1024 * 1024) + + await expect(new DocParser().parseBuffer(bomb)).rejects.toThrow(/exceeds the maximum allowed/) + }) + + it('rejects the bomb before either decompression library sees the buffer', async () => { + const bomb = await buildDeclaredOversizeArchive(2 * 1024 * 1024 * 1024) + + await expect(new DocParser().parseBuffer(bomb)).rejects.toThrow() + expect(mockParseOfficeAsync).not.toHaveBeenCalled() + expect(mockExtractRawText).not.toHaveBeenCalled() + }) + + it('rejects a ZIP-shaped .doc whose central directory cannot be parsed', async () => { + const buffer = Buffer.alloc(64) + buffer.writeUInt32LE(0x04034b50, 0) + + await expect(new DocParser().parseBuffer(buffer)).rejects.toThrow( + /refusing to parse an unverifiable ZIP-shaped archive/ + ) + expect(mockParseOfficeAsync).not.toHaveBeenCalled() + }) + + it('still parses a well-formed OOXML archive renamed to .doc', async () => { + const zip = new JSZip() + zip.file('word/document.xml', 'hello') + const buffer = await zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }) + mockParseOfficeAsync.mockResolvedValue('hello') + + const result = await new DocParser().parseBuffer(buffer) + + expect(result.content).toBe('hello') + expect(result.metadata.extractionMethod).toBe('officeparser') + }) + + it('no-ops the guard for a legacy OLE .doc and parses it', async () => { + mockParseOfficeAsync.mockResolvedValue('legacy doc text') + + const result = await new DocParser().parseBuffer(buildLegacyOleDoc()) + + expect(mockParseOfficeAsync).toHaveBeenCalledOnce() + expect(result.content).toBe('legacy doc text') + }) + + it('rejects an empty buffer', async () => { + await expect(new DocParser().parseBuffer(Buffer.alloc(0))).rejects.toThrow('Empty buffer') + }) +}) diff --git a/apps/sim/lib/file-parsers/doc-parser.ts b/apps/sim/lib/file-parsers/doc-parser.ts index 0d7379721f9..f03d3a45955 100644 --- a/apps/sim/lib/file-parsers/doc-parser.ts +++ b/apps/sim/lib/file-parsers/doc-parser.ts @@ -3,6 +3,7 @@ import { readFile } from 'fs/promises' import { createLogger } from '@sim/logger' import type { FileParseResult, FileParser } from '@/lib/file-parsers/types' import { sanitizeTextForUTF8 } from '@/lib/file-parsers/utils' +import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard' const logger = createLogger('DocParser') @@ -25,12 +26,20 @@ export class DocParser implements FileParser { } } + /** + * A `.doc` upload is only routed here by extension — `officeparser` and + * `mammoth` both accept an OOXML/ZIP container regardless of its name, so the + * zip-bomb guard must run here exactly as it does in the docx/pptx/xlsx + * parsers. It no-ops for genuine legacy OLE `.doc` buffers. + */ async parseBuffer(buffer: Buffer): Promise { try { if (!buffer || buffer.length === 0) { throw new Error('Empty buffer provided') } + assertOoxmlArchiveWithinLimits(buffer) + try { const officeParser = await import('officeparser') const result = await officeParser.parseOfficeAsync(buffer) diff --git a/apps/sim/lib/file-parsers/index.ts b/apps/sim/lib/file-parsers/index.ts index 793dd0ea530..d53298cea0a 100644 --- a/apps/sim/lib/file-parsers/index.ts +++ b/apps/sim/lib/file-parsers/index.ts @@ -2,6 +2,7 @@ import { existsSync } from 'fs' import path from 'path' import { createLogger } from '@sim/logger' import type { FileParseResult, FileParser, SupportedFileType } from '@/lib/file-parsers/types' +import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard' const logger = createLogger('FileParser') @@ -168,6 +169,11 @@ export async function parseFile(filePath: string): Promise { * @param buffer Buffer containing the file data * @param extension File extension without the dot (e.g., 'pdf', 'csv') * @returns Parsed content and metadata + * + * The zip-bomb guard runs here for every extension, not just the OOXML ones: + * the extension is an attacker-controlled routing hint, and the guard no-ops + * for buffers that are not ZIP archives. Individual parsers still call it so a + * direct `parser.parseBuffer` caller is covered too. */ export async function parseBuffer(buffer: Buffer, extension: string): Promise { try { @@ -179,6 +185,8 @@ export async function parseBuffer(buffer: Buffer, extension: string): Promise Date: Sat, 1 Aug 2026 16:43:38 -0700 Subject: [PATCH 2/4] fix(file-parsers): verify actual inflation, not just declared ZIP sizes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The declared uncompressed sizes in a ZIP central directory are attacker- controlled, so a bomb can under-report them and pass the size and ratio checks untouched. officeparser and mammoth only detect the mismatch after inflating the entry in full: a 498 KB archive declaring 1000 bytes per entry drove 559 MB resident through the .doc parser and 538 MB through .docx, then failed. SheetJS and officeparser reject the container earlier, so xlsx/pptx were not affected, but doc and docx both were. Each entry is now inflated during verification under a maxOutputLength bound equal to the size it declared. Node's zlib aborts the moment output would exceed that bound, so a lying entry costs only its declared size and the inflated bytes are discarded immediately; both bomb variants now reject at +0 MB across every extension. Stored entries are checked against their own compressed size, and unsupported compression methods fail closed. Verification walks the contiguous run of central-directory records rather than the EOCD's declared entry count, since that run is what a decompression library allocates per entry — a lied-down count must not hide an entry from verification. Cost is ~0.45 ms per MB of uncompressed content (22 ms for a 50 MB archive), against parse times an order of magnitude larger. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. --- apps/sim/lib/file-parsers/doc-parser.test.ts | 26 +++ apps/sim/lib/file-parsers/zip-guard.test.ts | 88 ++++++++ apps/sim/lib/file-parsers/zip-guard.ts | 216 ++++++++++++++++--- 3 files changed, 301 insertions(+), 29 deletions(-) diff --git a/apps/sim/lib/file-parsers/doc-parser.test.ts b/apps/sim/lib/file-parsers/doc-parser.test.ts index 7a88dbbe8f1..cf65c95cda0 100644 --- a/apps/sim/lib/file-parsers/doc-parser.test.ts +++ b/apps/sim/lib/file-parsers/doc-parser.test.ts @@ -64,6 +64,32 @@ describe('DocParser.parseBuffer', () => { expect(mockExtractRawText).not.toHaveBeenCalled() }) + it('rejects a .doc that under-declares its uncompressed size', async () => { + // Declared sizes alone put this under every limit; officeparser and mammoth + // only notice the mismatch after inflating the entry in full, so the guard + // has to catch it before either library sees the buffer. + const zip = new JSZip() + zip.file('word/document.xml', 'A'.repeat(4 * 1024 * 1024)) + const honest = (await zip.generateAsync({ + type: 'nodebuffer', + compression: 'DEFLATE', + })) as Buffer + + const lying = Buffer.from(honest) + for (let offset = 0; offset + 30 <= lying.length; offset++) { + const signature = lying.readUInt32LE(offset) + if (signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + lying.writeUInt32LE(1000, offset + 24) + } else if (signature === 0x04034b50) { + lying.writeUInt32LE(1000, offset + 22) + } + } + + await expect(new DocParser().parseBuffer(lying)).rejects.toThrow(/do not match declared sizes/) + expect(mockParseOfficeAsync).not.toHaveBeenCalled() + expect(mockExtractRawText).not.toHaveBeenCalled() + }) + it('rejects a ZIP-shaped .doc whose central directory cannot be parsed', async () => { const buffer = Buffer.alloc(64) buffer.writeUInt32LE(0x04034b50, 0) diff --git a/apps/sim/lib/file-parsers/zip-guard.test.ts b/apps/sim/lib/file-parsers/zip-guard.test.ts index e4aaca3f480..91ef85ce241 100644 --- a/apps/sim/lib/file-parsers/zip-guard.test.ts +++ b/apps/sim/lib/file-parsers/zip-guard.test.ts @@ -30,6 +30,47 @@ async function buildZip( }) } +const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 +const LOCAL_FILE_HEADER_SIGNATURE = 0x04034b50 + +/** + * Rewrite every declared uncompressed size — in both the central directory and + * the local file headers — so the archive under-reports how much it expands to. + * This is the bypass a declared-size-only check cannot see. Zero-length records + * (JSZip emits a stored directory entry per folder) are left alone so the + * archive stays well-formed apart from the lie under test. + */ +function underDeclareSizes(source: Buffer, declared: number): Buffer { + const buffer = Buffer.from(source) + for (let offset = 0; offset + 30 <= buffer.length; offset++) { + const signature = buffer.readUInt32LE(offset) + if (signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + if (buffer.readUInt32LE(offset + 24) !== 0) { + buffer.writeUInt32LE(declared, offset + 24) + } + } else if (signature === LOCAL_FILE_HEADER_SIGNATURE) { + if (buffer.readUInt32LE(offset + 22) !== 0) { + buffer.writeUInt32LE(declared, offset + 22) + } + } + } + return buffer +} + +/** Overwrite the compression method on every non-empty central-directory record. */ +function setCompressionMethod(source: Buffer, method: number): Buffer { + const buffer = Buffer.from(source) + for (let offset = 0; offset + 46 <= buffer.length; offset++) { + if ( + buffer.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE && + buffer.readUInt32LE(offset + 24) !== 0 + ) { + buffer.writeUInt16LE(method, offset + 10) + } + } + return buffer +} + describe('assertOoxmlArchiveWithinLimits', () => { it('accepts a well-formed archive within limits', async () => { const buffer = await buildZip({ 'word/document.xml': 'hello world' }) @@ -108,6 +149,53 @@ describe('assertOoxmlArchiveWithinLimits', () => { expect(() => assertOoxmlArchiveWithinLimits(tampered)).toThrow(ZipBombError) }) + it('rejects an archive that under-declares its uncompressed size', async () => { + // The declared sizes put this archive far under both limits, so only + // inflating it reveals that it actually expands ~200x further. + const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) }) + const lying = underDeclareSizes(honest, 1000) + + expect(() => assertOoxmlArchiveWithinLimits(lying, HIGH_LIMITS)).toThrow(ZipBombError) + expect(() => assertOoxmlArchiveWithinLimits(lying, HIGH_LIMITS)).toThrow( + /inflates beyond the 1000 bytes it declares/ + ) + }) + + it('still accepts the same archive when its declared sizes are honest', async () => { + const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) }) + expect(() => assertOoxmlArchiveWithinLimits(honest, HIGH_LIMITS)).not.toThrow() + }) + + it('rejects a stored entry whose declared size does not match its payload', async () => { + const zip = new JSZip() + zip.file('document.xml', 'A'.repeat(50_000)) + const stored = (await zip.generateAsync({ + type: 'nodebuffer', + compression: 'STORE', + })) as Buffer + + expect(() => + assertOoxmlArchiveWithinLimits(underDeclareSizes(stored, 10), HIGH_LIMITS) + ).toThrow(/stored entry declares 10 bytes but holds 50000/) + }) + + it('rejects an entry using a compression method the parsers cannot read', async () => { + const buffer = await buildZip({ 'word/document.xml': 'hello' }) + expect(() => + assertOoxmlArchiveWithinLimits(setCompressionMethod(buffer, 12), HIGH_LIMITS) + ).toThrow(/unsupported compression method 12/) + }) + + it('accepts a multi-entry archive whose entries all inflate to what they declare', async () => { + const buffer = await buildZip({ + '[Content_Types].xml': '', + '_rels/.rels': '', + 'word/document.xml': `${'text '.repeat(5000)}`, + 'word/styles.xml': `${'style '.repeat(2000)}`, + }) + expect(() => assertOoxmlArchiveWithinLimits(buffer, HIGH_LIMITS)).not.toThrow() + }) + it('no-ops for buffers that are not ZIP archives', () => { const plaintext = Buffer.from('this is just plain text, not a zip archive at all') expect(() => assertOoxmlArchiveWithinLimits(plaintext)).not.toThrow() diff --git a/apps/sim/lib/file-parsers/zip-guard.ts b/apps/sim/lib/file-parsers/zip-guard.ts index 07642b3740c..f9ce50ea8d2 100644 --- a/apps/sim/lib/file-parsers/zip-guard.ts +++ b/apps/sim/lib/file-parsers/zip-guard.ts @@ -1,3 +1,4 @@ +import { inflateRawSync } from 'zlib' import { createLogger } from '@sim/logger' const logger = createLogger('ZipBombGuard') @@ -25,10 +26,14 @@ const ZIP64_EXTRA_FIELD_ID = 0x0001 const EOCD_MIN_SIZE = 22 const ZIP64_EOCD_LOCATOR_SIZE = 20 const CENTRAL_DIRECTORY_HEADER_MIN_SIZE = 46 +const LOCAL_FILE_HEADER_MIN_SIZE = 30 const MAX_EOCD_COMMENT_SIZE = 0xffff const UINT32_SENTINEL = 0xffffffff const UINT16_SENTINEL = 0xffff +const COMPRESSION_METHOD_STORED = 0 +const COMPRESSION_METHOD_DEFLATE = 8 + export interface OoxmlSizeLimits { /** Hard ceiling on the summed declared uncompressed size of all entries. */ maxTotalUncompressedBytes: number @@ -138,37 +143,70 @@ function locateCentralDirectory( return { offset: cdOffset, entryCount } } +interface CentralDirectoryEntry { + compressionMethod: number + compressedSize: number + uncompressedSize: number + localHeaderOffset: number +} + /** - * Read an entry's declared uncompressed size, preferring the ZIP64 extra field - * when the 32-bit central-directory field is saturated. The saturated 64-bit - * values appear in the extra field in a fixed order with the uncompressed size - * first, so it is always the leading 8 bytes of the ZIP64 field. + * Read an entry's sizes, method, and local-header offset, preferring the ZIP64 + * extra field for whichever 32-bit fields are saturated. The saturated 64-bit + * values appear in the extra field in a fixed order — uncompressed size, + * compressed size, local-header offset — and only the saturated ones are + * present, so they must be consumed positionally rather than at fixed offsets. */ -function readUncompressedSize( +function readCentralDirectoryEntry( buffer: Buffer, headerOffset: number, fileNameLength: number, extraFieldLength: number -): number { - const uncompressedSize = buffer.readUInt32LE(headerOffset + 24) - if (uncompressedSize !== UINT32_SENTINEL) { - return uncompressedSize - } +): CentralDirectoryEntry { + const compressionMethod = buffer.readUInt16LE(headerOffset + 10) + let compressedSize = buffer.readUInt32LE(headerOffset + 20) + let uncompressedSize = buffer.readUInt32LE(headerOffset + 24) + let localHeaderOffset = buffer.readUInt32LE(headerOffset + 42) + + const needsZip64 = + uncompressedSize === UINT32_SENTINEL || + compressedSize === UINT32_SENTINEL || + localHeaderOffset === UINT32_SENTINEL - const extraStart = headerOffset + CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength - const extraEnd = extraStart + extraFieldLength - let cursor = extraStart - while (cursor + 4 <= extraEnd) { - const fieldId = buffer.readUInt16LE(cursor) - const fieldSize = buffer.readUInt16LE(cursor + 2) - const dataStart = cursor + 4 - if (fieldId === ZIP64_EXTRA_FIELD_ID && dataStart + 8 <= extraEnd) { - return Number(buffer.readBigUInt64LE(dataStart)) + if (needsZip64) { + const extraStart = headerOffset + CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength + const extraEnd = extraStart + extraFieldLength + let cursor = extraStart + while (cursor + 4 <= extraEnd) { + const fieldId = buffer.readUInt16LE(cursor) + const fieldSize = buffer.readUInt16LE(cursor + 2) + const dataStart = cursor + 4 + if (fieldId === ZIP64_EXTRA_FIELD_ID) { + let zip64Cursor = dataStart + const readNext = (): number | null => { + if (zip64Cursor + 8 > Math.min(extraEnd, dataStart + fieldSize)) { + return null + } + const value = Number(buffer.readBigUInt64LE(zip64Cursor)) + zip64Cursor += 8 + return value + } + if (uncompressedSize === UINT32_SENTINEL) { + uncompressedSize = readNext() ?? uncompressedSize + } + if (compressedSize === UINT32_SENTINEL) { + compressedSize = readNext() ?? compressedSize + } + if (localHeaderOffset === UINT32_SENTINEL) { + localHeaderOffset = readNext() ?? localHeaderOffset + } + break + } + cursor = dataStart + fieldSize } - cursor = dataStart + fieldSize } - return uncompressedSize + return { compressionMethod, compressedSize, uncompressedSize, localHeaderOffset } } /** @@ -206,7 +244,12 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n const extraFieldLength = buffer.readUInt16LE(cursor + 30) const commentLength = buffer.readUInt16LE(cursor + 32) - total += readUncompressedSize(buffer, cursor, fileNameLength, extraFieldLength) + total += readCentralDirectoryEntry( + buffer, + cursor, + fileNameLength, + extraFieldLength + ).uncompressedSize if (total > abortAboveBytes) { return total } @@ -217,6 +260,93 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n return total } +/** + * A declared-size check alone is not sufficient: the central directory is + * attacker-controlled, so a bomb can under-report each entry's uncompressed + * size and sail through. The decompression libraries only notice the mismatch + * *after* inflating the entry in full — measured at ~560 MB resident for a + * 498 KB input — so the lie must be caught before they see the buffer. + * + * Each entry is therefore inflated here with `maxOutputLength` set to the size + * it declared. Node's zlib aborts the moment output would exceed that bound, so + * a lying entry costs only its declared size in memory (~0 for the bomb above) + * and the inflated bytes are discarded immediately. An honest archive inflates + * to exactly what it declared and passes, having already been bounded by + * {@link sumDeclaredUncompressedSize}. + * + * Returns an error message when the archive is lying or is shaped in a way that + * cannot be verified, and `null` when every entry checks out. + */ +function findInflationMismatch(buffer: Buffer, location: CentralDirectoryLocation): string | null { + let cursor = location.offset + let verified = 0 + + // Walk the contiguous run of records rather than the EOCD's declared count — + // the run is what a decompression library actually allocates an entry per, so + // a lied-down count must not be able to hide an entry from verification. + while ( + cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE <= buffer.length && + buffer.readUInt32LE(cursor) === CENTRAL_DIRECTORY_HEADER_SIGNATURE + ) { + const fileNameLength = buffer.readUInt16LE(cursor + 28) + const extraFieldLength = buffer.readUInt16LE(cursor + 30) + const commentLength = buffer.readUInt16LE(cursor + 32) + const { compressionMethod, compressedSize, uncompressedSize, localHeaderOffset } = + readCentralDirectoryEntry(buffer, cursor, fileNameLength, extraFieldLength) + + if (localHeaderOffset + LOCAL_FILE_HEADER_MIN_SIZE > buffer.length) { + return 'entry points outside the archive' + } + if (buffer.readUInt32LE(localHeaderOffset) !== LOCAL_FILE_HEADER_SIGNATURE) { + return 'entry has an invalid local file header' + } + + const dataStart = + localHeaderOffset + + LOCAL_FILE_HEADER_MIN_SIZE + + buffer.readUInt16LE(localHeaderOffset + 26) + + buffer.readUInt16LE(localHeaderOffset + 28) + if (dataStart > buffer.length) { + return 'entry data starts outside the archive' + } + + if (compressionMethod === COMPRESSION_METHOD_STORED) { + // A stored entry is its own payload, so any divergence is a lie outright. + if (uncompressedSize !== compressedSize) { + return `stored entry declares ${uncompressedSize} bytes but holds ${compressedSize}` + } + } else if (compressionMethod === COMPRESSION_METHOD_DEFLATE) { + // The declared compressed size is untrusted too; clamp it to the buffer + // and let the deflate stream's own end marker terminate the read. + const dataEnd = Math.min(dataStart + compressedSize, buffer.length) + try { + inflateRawSync(buffer.subarray(dataStart, dataEnd), { + maxOutputLength: Math.max(uncompressedSize, 1), + }) + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code === 'ERR_BUFFER_TOO_LARGE') { + return `entry inflates beyond the ${uncompressedSize} bytes it declares` + } + // A stream the guard cannot inflate is one the parser cannot read + // either; treat it as unverifiable rather than assuming it is safe. + return `entry could not be inflated for verification (${code ?? 'unknown error'})` + } + } else { + return `entry uses unsupported compression method ${compressionMethod}` + } + + verified += 1 + cursor += CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength + extraFieldLength + commentLength + } + + if (verified < location.entryCount) { + return `central directory declares ${location.entryCount} entries but only ${verified} could be verified` + } + + return null +} + /** Parse-time shape of a ZIP central directory, read without decompressing anything. */ export interface ZipCentralDirectoryStats { /** Records in the contiguous central-directory run — what a per-signature parser allocates. */ @@ -276,14 +406,21 @@ export function readZipCentralDirectoryStats(buffer: Buffer): ZipCentralDirector } /** - * Reject an OOXML archive whose declared expanded size or compression ratio - * exceeds safe bounds, before any decompression library materializes it. + * Reject an OOXML archive whose expanded size or compression ratio exceeds safe + * bounds, before any decompression library materializes it. + * + * Runs in two stages. The declared sizes in the central directory are checked + * first, which costs nothing and rejects a straightforward bomb outright. Those + * sizes are attacker-controlled, so every entry is then inflated under a + * `maxOutputLength` bound equal to what it declared — see + * {@link findInflationMismatch} — which catches an archive that under-reports + * its way past the first stage. * - * Fails closed: a ZIP-shaped buffer whose central directory cannot be parsed is - * rejected rather than passed through, so a malformed archive that a downstream - * library still inflates cannot bypass the guard. Genuinely non-ZIP inputs - * (legacy OLE `.xls`/`.doc`, misidentified plaintext) no-op and defer to the - * downstream parser's own validation and fallbacks. + * Fails closed: a ZIP-shaped buffer whose central directory cannot be parsed, + * or whose entries cannot be verified, is rejected rather than passed through, + * so a malformed archive that a downstream library still inflates cannot bypass + * the guard. Genuinely non-ZIP inputs (legacy OLE `.xls`/`.doc`, misidentified + * plaintext) no-op and defer to the downstream parser's own validation. */ export function assertOoxmlArchiveWithinLimits( buffer: Buffer, @@ -325,4 +462,25 @@ export function assertOoxmlArchiveWithinLimits( `Compression ratio (${ratio.toFixed(1)}x) exceeds the maximum allowed ${limits.maxCompressionRatio}x` ) } + + const eocdOffset = findEocdOffset(buffer) + const location = eocdOffset < 0 ? null : locateCentralDirectory(buffer, eocdOffset) + if (!location) { + logger.warn('Rejected ZIP-shaped archive: central directory could not be re-read', { + compressedBytes: buffer.length, + }) + throw new ZipBombError( + 'Unable to inspect ZIP central directory; refusing to parse an unverifiable ZIP-shaped archive' + ) + } + + const mismatch = findInflationMismatch(buffer, location) + if (mismatch) { + logger.warn('Rejected OOXML archive: declared sizes do not match actual contents', { + reason: mismatch, + declaredTotalUncompressed: totalUncompressed, + compressedBytes: buffer.length, + }) + throw new ZipBombError(`Archive contents do not match declared sizes: ${mismatch}`) + } } From fac1c7cda08531db44cc1db2ac3b25b554ab0166 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:54:22 -0700 Subject: [PATCH 3/4] fix(file-parsers): require central and local ZIP headers to agree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parsers disagree about which header to trust. JSZip skips the local header outright and decompresses using the central directory's method, while SheetJS's parse_local_file switches on the local header's method and inflates from there. An entry claiming STORED centrally and DEFLATE locally therefore took the guard's stored branch, skipping bounded inflation, and was still expanded downstream — a 398 KB archive hiding a 400 MB deflate payload. Verification now rejects any entry whose two headers disagree on compression method, and on declared sizes when the local header carries them (the data-descriptor flag and ZIP64 sentinels legitimately omit them, and those entries stay covered by the bounded inflate). Caught by Greptile review. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted. --- apps/sim/lib/file-parsers/zip-guard.test.ts | 53 +++++++++++++++++++-- apps/sim/lib/file-parsers/zip-guard.ts | 34 +++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/file-parsers/zip-guard.test.ts b/apps/sim/lib/file-parsers/zip-guard.test.ts index 91ef85ce241..1bba53ce82e 100644 --- a/apps/sim/lib/file-parsers/zip-guard.test.ts +++ b/apps/sim/lib/file-parsers/zip-guard.test.ts @@ -57,15 +57,31 @@ function underDeclareSizes(source: Buffer, declared: number): Buffer { return buffer } -/** Overwrite the compression method on every non-empty central-directory record. */ -function setCompressionMethod(source: Buffer, method: number): Buffer { +/** + * Overwrite the compression method on every non-empty record. `where` selects + * which header is rewritten, so a test can make the two disagree — JSZip trusts + * the central method while SheetJS switches on the local one. + */ +function setCompressionMethod( + source: Buffer, + method: number, + where: 'central' | 'local' | 'both' = 'both' +): Buffer { const buffer = Buffer.from(source) for (let offset = 0; offset + 46 <= buffer.length; offset++) { + const signature = buffer.readUInt32LE(offset) if ( - buffer.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE && - buffer.readUInt32LE(offset + 24) !== 0 + signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE && + buffer.readUInt32LE(offset + 24) !== 0 && + where !== 'local' ) { buffer.writeUInt16LE(method, offset + 10) + } else if ( + signature === LOCAL_FILE_HEADER_SIGNATURE && + buffer.readUInt32LE(offset + 22) !== 0 && + where !== 'central' + ) { + buffer.writeUInt16LE(method, offset + 8) } } return buffer @@ -186,6 +202,35 @@ describe('assertOoxmlArchiveWithinLimits', () => { ).toThrow(/unsupported compression method 12/) }) + it('rejects an entry whose central and local compression methods disagree', async () => { + // Claiming STORED centrally skips the bounded inflation, while SheetJS + // switches on the local header and would inflate the payload anyway. + const honest = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) }) + const split = setCompressionMethod(honest, 0, 'central') + + expect(() => assertOoxmlArchiveWithinLimits(split, HIGH_LIMITS)).toThrow(ZipBombError) + expect(() => assertOoxmlArchiveWithinLimits(split, HIGH_LIMITS)).toThrow( + /compression method 0 centrally but 8 locally/ + ) + }) + + it('rejects an entry whose central and local declared sizes disagree', async () => { + const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) }) + const buffer = Buffer.from(honest) + for (let offset = 0; offset + 30 <= buffer.length; offset++) { + if ( + buffer.readUInt32LE(offset) === LOCAL_FILE_HEADER_SIGNATURE && + buffer.readUInt32LE(offset + 22) !== 0 + ) { + buffer.writeUInt32LE(64, offset + 22) + } + } + + expect(() => assertOoxmlArchiveWithinLimits(buffer, HIGH_LIMITS)).toThrow( + /200000 bytes centrally but .* locally/ + ) + }) + it('accepts a multi-entry archive whose entries all inflate to what they declare', async () => { const buffer = await buildZip({ '[Content_Types].xml': '', diff --git a/apps/sim/lib/file-parsers/zip-guard.ts b/apps/sim/lib/file-parsers/zip-guard.ts index f9ce50ea8d2..a71f71c8c74 100644 --- a/apps/sim/lib/file-parsers/zip-guard.ts +++ b/apps/sim/lib/file-parsers/zip-guard.ts @@ -34,6 +34,9 @@ const UINT16_SENTINEL = 0xffff const COMPRESSION_METHOD_STORED = 0 const COMPRESSION_METHOD_DEFLATE = 8 +/** General-purpose bit 3: sizes live in a trailing data descriptor, not the local header. */ +const DATA_DESCRIPTOR_FLAG = 0x0008 + export interface OoxmlSizeLimits { /** Hard ceiling on the summed declared uncompressed size of all entries. */ maxTotalUncompressedBytes: number @@ -274,6 +277,12 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n * to exactly what it declared and passes, having already been bounded by * {@link sumDeclaredUncompressedSize}. * + * The central and local headers must also agree on the compression method and + * sizes, because the parsers disagree about which one to trust — JSZip reads + * the central directory, SheetJS switches on the local header — and a record + * that reads as STORED here but DEFLATE downstream would skip inflation + * verification entirely. + * * Returns an error message when the archive is lying or is shaped in a way that * cannot be verified, and `null` when every entry checks out. */ @@ -310,6 +319,31 @@ function findInflationMismatch(buffer: Buffer, location: CentralDirectoryLocatio return 'entry data starts outside the archive' } + // The two headers must agree on how the payload is encoded. JSZip trusts + // the central directory while SheetJS switches on the local header's + // method, so a record that claims STORED centrally and DEFLATE locally + // would skip verification here and still be inflated downstream. + const localFlags = buffer.readUInt16LE(localHeaderOffset + 6) + const localMethod = buffer.readUInt16LE(localHeaderOffset + 8) + if (localMethod !== compressionMethod) { + return `entry declares compression method ${compressionMethod} centrally but ${localMethod} locally` + } + + // Sizes must agree too, for the same reason. They are legitimately absent + // from the local header when the data-descriptor flag is set, and are + // sentinels under ZIP64, so only compare when both are actually present. + const hasDataDescriptor = (localFlags & DATA_DESCRIPTOR_FLAG) !== 0 + const localCompressedSize = buffer.readUInt32LE(localHeaderOffset + 18) + const localUncompressedSize = buffer.readUInt32LE(localHeaderOffset + 22) + if ( + !hasDataDescriptor && + localCompressedSize !== UINT32_SENTINEL && + localUncompressedSize !== UINT32_SENTINEL && + (localCompressedSize !== compressedSize || localUncompressedSize !== uncompressedSize) + ) { + return `entry declares ${compressedSize}/${uncompressedSize} bytes centrally but ${localCompressedSize}/${localUncompressedSize} locally` + } + if (compressionMethod === COMPRESSION_METHOD_STORED) { // A stored entry is its own payload, so any divergence is a lie outright. if (uncompressedSize !== compressedSize) { From bea4e75ecb73000c9bd33bfadc7d3d0b7996b055 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:56:23 -0700 Subject: [PATCH 4/4] fix(file-parsers): charge hidden central-directory entries against the cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sumDeclaredUncompressedSize walked only the entry count the EOCD declares, while verification walks the contiguous run of records. JSZip's readCentralDir loops on the record signature and keeps every entry it finds — a count mismatch is explicitly not an error there — so an archive that under-reported its count could hide honestly-large entries from the total-size cap and still have the parser expand them. The sum now walks the same contiguous run as the verification pass and readZipCentralDirectoryStats, and fails closed when the run is shorter than the declared count. Caught by Cursor Bugbot review. --- apps/sim/lib/file-parsers/zip-guard.test.ts | 23 ++++++++++++++++++ apps/sim/lib/file-parsers/zip-guard.ts | 27 +++++++++++++++------ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/apps/sim/lib/file-parsers/zip-guard.test.ts b/apps/sim/lib/file-parsers/zip-guard.test.ts index 1bba53ce82e..9c4db20609d 100644 --- a/apps/sim/lib/file-parsers/zip-guard.test.ts +++ b/apps/sim/lib/file-parsers/zip-guard.test.ts @@ -231,6 +231,29 @@ describe('assertOoxmlArchiveWithinLimits', () => { ) }) + it('charges entries hidden behind an under-reported EOCD count against the cap', async () => { + // JSZip's readCentralDir loops on the record signature and keeps every + // entry it finds — a count mismatch is explicitly not an error there — so + // entries past the declared count must still be charged against the cap. + const buffer = await buildZip({ + 'a.xml': 'A'.repeat(60_000), + 'b.xml': 'B'.repeat(60_000), + 'c.xml': 'C'.repeat(60_000), + }) + const eocdOffset = buffer.length - 22 + expect(buffer.readUInt32LE(eocdOffset)).toBe(0x06054b50) + buffer.writeUInt16LE(1, eocdOffset + 8) // entries on this disk + buffer.writeUInt16LE(1, eocdOffset + 10) // total entries + + expect(() => + assertOoxmlArchiveWithinLimits(buffer, { + maxTotalUncompressedBytes: 100_000, + maxCompressionRatio: 10_000, + ratioCheckFloorBytes: 1024 * 1024 * 1024, + }) + ).toThrow(/exceeds the maximum allowed/) + }) + it('accepts a multi-entry archive whose entries all inflate to what they declare', async () => { const buffer = await buildZip({ '[Content_Types].xml': '', diff --git a/apps/sim/lib/file-parsers/zip-guard.ts b/apps/sim/lib/file-parsers/zip-guard.ts index a71f71c8c74..b2acf322cb6 100644 --- a/apps/sim/lib/file-parsers/zip-guard.ts +++ b/apps/sim/lib/file-parsers/zip-guard.ts @@ -217,6 +217,13 @@ function readCentralDirectoryEntry( * `null` when the buffer is not a parseable ZIP archive (e.g. legacy binary * `.xls`/`.doc`, or a misidentified plaintext file) so the caller can defer to * the downstream parser. Stops early once the running total exceeds the limit. + * + * Like {@link readZipCentralDirectoryStats}, this charges the CONTIGUOUS run of + * records rather than the EOCD's declared count. JSZip's `readCentralDir` loops + * on the record signature and keeps every entry it finds — a count mismatch is + * explicitly not an error there — so an archive that under-reports its count + * would otherwise hide honestly-large entries from this cap while the parser + * still expanded them. */ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): number | null { if (buffer.length < EOCD_MIN_SIZE) { @@ -234,15 +241,12 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n } let total = 0 + let counted = 0 let cursor = location.offset - for (let entry = 0; entry < location.entryCount; entry++) { - if (cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE > buffer.length) { - return null - } - if (buffer.readUInt32LE(cursor) !== CENTRAL_DIRECTORY_HEADER_SIGNATURE) { - return null - } - + while ( + cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE <= buffer.length && + buffer.readUInt32LE(cursor) === CENTRAL_DIRECTORY_HEADER_SIGNATURE + ) { const fileNameLength = buffer.readUInt16LE(cursor + 28) const extraFieldLength = buffer.readUInt16LE(cursor + 30) const commentLength = buffer.readUInt16LE(cursor + 32) @@ -257,9 +261,16 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n return total } + counted += 1 cursor += CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength + extraFieldLength + commentLength } + // Fewer records than the archive claims means the directory is malformed; + // fail closed rather than charging a partial total against the cap. + if (counted < location.entryCount) { + return null + } + return total }