Skip to content

Commit ccc2ec9

Browse files
authored
fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion (#6166)
* 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. * fix(file-parsers): verify actual inflation, not just declared ZIP sizes 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. * fix(file-parsers): require central and local ZIP headers to agree 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. * fix(file-parsers): charge hidden central-directory entries against the cap 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.
1 parent 5686b7b commit ccc2ec9

5 files changed

Lines changed: 540 additions & 37 deletions

File tree

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import JSZip from 'jszip'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const { mockParseOfficeAsync, mockExtractRawText } = vi.hoisted(() => ({
8+
mockParseOfficeAsync: vi.fn(),
9+
mockExtractRawText: vi.fn(),
10+
}))
11+
12+
vi.mock('officeparser', () => ({ parseOfficeAsync: mockParseOfficeAsync }))
13+
vi.mock('mammoth', () => ({
14+
default: { extractRawText: mockExtractRawText },
15+
extractRawText: mockExtractRawText,
16+
}))
17+
18+
import { DocParser } from '@/lib/file-parsers/doc-parser'
19+
20+
const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50
21+
22+
/**
23+
* Build a small OOXML-shaped archive whose central directory *declares* a huge
24+
* uncompressed size. The guard reads declared sizes without inflating anything,
25+
* so this reproduces a zip bomb's central directory at a few hundred bytes.
26+
*/
27+
async function buildDeclaredOversizeArchive(declaredUncompressedBytes: number): Promise<Buffer> {
28+
const zip = new JSZip()
29+
zip.file('word/document.xml', '<w:document><w:body>A</w:body></w:document>')
30+
const buffer = await zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' })
31+
32+
for (let offset = 0; offset + 28 <= buffer.length; offset++) {
33+
if (buffer.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE) {
34+
buffer.writeUInt32LE(declaredUncompressedBytes, offset + 24)
35+
return buffer
36+
}
37+
}
38+
throw new Error('No central directory header found in generated archive')
39+
}
40+
41+
/** A legacy OLE compound-file `.doc` — not a ZIP, so the guard must no-op. */
42+
function buildLegacyOleDoc(): Buffer {
43+
const buffer = Buffer.alloc(512)
44+
Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(buffer, 0)
45+
return buffer
46+
}
47+
48+
describe('DocParser.parseBuffer', () => {
49+
beforeEach(() => {
50+
vi.clearAllMocks()
51+
})
52+
53+
it('rejects a ZIP-shaped .doc whose declared expanded size exceeds the cap', async () => {
54+
const bomb = await buildDeclaredOversizeArchive(2 * 1024 * 1024 * 1024)
55+
56+
await expect(new DocParser().parseBuffer(bomb)).rejects.toThrow(/exceeds the maximum allowed/)
57+
})
58+
59+
it('rejects the bomb before either decompression library sees the buffer', async () => {
60+
const bomb = await buildDeclaredOversizeArchive(2 * 1024 * 1024 * 1024)
61+
62+
await expect(new DocParser().parseBuffer(bomb)).rejects.toThrow()
63+
expect(mockParseOfficeAsync).not.toHaveBeenCalled()
64+
expect(mockExtractRawText).not.toHaveBeenCalled()
65+
})
66+
67+
it('rejects a .doc that under-declares its uncompressed size', async () => {
68+
// Declared sizes alone put this under every limit; officeparser and mammoth
69+
// only notice the mismatch after inflating the entry in full, so the guard
70+
// has to catch it before either library sees the buffer.
71+
const zip = new JSZip()
72+
zip.file('word/document.xml', 'A'.repeat(4 * 1024 * 1024))
73+
const honest = (await zip.generateAsync({
74+
type: 'nodebuffer',
75+
compression: 'DEFLATE',
76+
})) as Buffer
77+
78+
const lying = Buffer.from(honest)
79+
for (let offset = 0; offset + 30 <= lying.length; offset++) {
80+
const signature = lying.readUInt32LE(offset)
81+
if (signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE) {
82+
lying.writeUInt32LE(1000, offset + 24)
83+
} else if (signature === 0x04034b50) {
84+
lying.writeUInt32LE(1000, offset + 22)
85+
}
86+
}
87+
88+
await expect(new DocParser().parseBuffer(lying)).rejects.toThrow(/do not match declared sizes/)
89+
expect(mockParseOfficeAsync).not.toHaveBeenCalled()
90+
expect(mockExtractRawText).not.toHaveBeenCalled()
91+
})
92+
93+
it('rejects a ZIP-shaped .doc whose central directory cannot be parsed', async () => {
94+
const buffer = Buffer.alloc(64)
95+
buffer.writeUInt32LE(0x04034b50, 0)
96+
97+
await expect(new DocParser().parseBuffer(buffer)).rejects.toThrow(
98+
/refusing to parse an unverifiable ZIP-shaped archive/
99+
)
100+
expect(mockParseOfficeAsync).not.toHaveBeenCalled()
101+
})
102+
103+
it('still parses a well-formed OOXML archive renamed to .doc', async () => {
104+
const zip = new JSZip()
105+
zip.file('word/document.xml', '<w:document><w:body>hello</w:body></w:document>')
106+
const buffer = await zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' })
107+
mockParseOfficeAsync.mockResolvedValue('hello')
108+
109+
const result = await new DocParser().parseBuffer(buffer)
110+
111+
expect(result.content).toBe('hello')
112+
expect(result.metadata.extractionMethod).toBe('officeparser')
113+
})
114+
115+
it('no-ops the guard for a legacy OLE .doc and parses it', async () => {
116+
mockParseOfficeAsync.mockResolvedValue('legacy doc text')
117+
118+
const result = await new DocParser().parseBuffer(buildLegacyOleDoc())
119+
120+
expect(mockParseOfficeAsync).toHaveBeenCalledOnce()
121+
expect(result.content).toBe('legacy doc text')
122+
})
123+
124+
it('rejects an empty buffer', async () => {
125+
await expect(new DocParser().parseBuffer(Buffer.alloc(0))).rejects.toThrow('Empty buffer')
126+
})
127+
})

apps/sim/lib/file-parsers/doc-parser.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { readFile } from 'fs/promises'
33
import { createLogger } from '@sim/logger'
44
import type { FileParseResult, FileParser } from '@/lib/file-parsers/types'
55
import { sanitizeTextForUTF8 } from '@/lib/file-parsers/utils'
6+
import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard'
67

78
const logger = createLogger('DocParser')
89

@@ -25,12 +26,20 @@ export class DocParser implements FileParser {
2526
}
2627
}
2728

29+
/**
30+
* A `.doc` upload is only routed here by extension — `officeparser` and
31+
* `mammoth` both accept an OOXML/ZIP container regardless of its name, so the
32+
* zip-bomb guard must run here exactly as it does in the docx/pptx/xlsx
33+
* parsers. It no-ops for genuine legacy OLE `.doc` buffers.
34+
*/
2835
async parseBuffer(buffer: Buffer): Promise<FileParseResult> {
2936
try {
3037
if (!buffer || buffer.length === 0) {
3138
throw new Error('Empty buffer provided')
3239
}
3340

41+
assertOoxmlArchiveWithinLimits(buffer)
42+
3443
try {
3544
const officeParser = await import('officeparser')
3645
const result = await officeParser.parseOfficeAsync(buffer)

apps/sim/lib/file-parsers/index.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { existsSync } from 'fs'
22
import path from 'path'
33
import { createLogger } from '@sim/logger'
44
import type { FileParseResult, FileParser, SupportedFileType } from '@/lib/file-parsers/types'
5+
import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard'
56

67
const logger = createLogger('FileParser')
78

@@ -168,6 +169,11 @@ export async function parseFile(filePath: string): Promise<FileParseResult> {
168169
* @param buffer Buffer containing the file data
169170
* @param extension File extension without the dot (e.g., 'pdf', 'csv')
170171
* @returns Parsed content and metadata
172+
*
173+
* The zip-bomb guard runs here for every extension, not just the OOXML ones:
174+
* the extension is an attacker-controlled routing hint, and the guard no-ops
175+
* for buffers that are not ZIP archives. Individual parsers still call it so a
176+
* direct `parser.parseBuffer` caller is covered too.
171177
*/
172178
export async function parseBuffer(buffer: Buffer, extension: string): Promise<FileParseResult> {
173179
try {
@@ -179,6 +185,8 @@ export async function parseBuffer(buffer: Buffer, extension: string): Promise<Fi
179185
throw new Error('No file extension provided')
180186
}
181187

188+
assertOoxmlArchiveWithinLimits(buffer)
189+
182190
const normalizedExtension = extension.toLowerCase()
183191
logger.info('Attempting to parse buffer with extension:', normalizedExtension)
184192

apps/sim/lib/file-parsers/zip-guard.test.ts

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,63 @@ async function buildZip(
3030
})
3131
}
3232

33+
const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50
34+
const LOCAL_FILE_HEADER_SIGNATURE = 0x04034b50
35+
36+
/**
37+
* Rewrite every declared uncompressed size — in both the central directory and
38+
* the local file headers — so the archive under-reports how much it expands to.
39+
* This is the bypass a declared-size-only check cannot see. Zero-length records
40+
* (JSZip emits a stored directory entry per folder) are left alone so the
41+
* archive stays well-formed apart from the lie under test.
42+
*/
43+
function underDeclareSizes(source: Buffer, declared: number): Buffer {
44+
const buffer = Buffer.from(source)
45+
for (let offset = 0; offset + 30 <= buffer.length; offset++) {
46+
const signature = buffer.readUInt32LE(offset)
47+
if (signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE) {
48+
if (buffer.readUInt32LE(offset + 24) !== 0) {
49+
buffer.writeUInt32LE(declared, offset + 24)
50+
}
51+
} else if (signature === LOCAL_FILE_HEADER_SIGNATURE) {
52+
if (buffer.readUInt32LE(offset + 22) !== 0) {
53+
buffer.writeUInt32LE(declared, offset + 22)
54+
}
55+
}
56+
}
57+
return buffer
58+
}
59+
60+
/**
61+
* Overwrite the compression method on every non-empty record. `where` selects
62+
* which header is rewritten, so a test can make the two disagree — JSZip trusts
63+
* the central method while SheetJS switches on the local one.
64+
*/
65+
function setCompressionMethod(
66+
source: Buffer,
67+
method: number,
68+
where: 'central' | 'local' | 'both' = 'both'
69+
): Buffer {
70+
const buffer = Buffer.from(source)
71+
for (let offset = 0; offset + 46 <= buffer.length; offset++) {
72+
const signature = buffer.readUInt32LE(offset)
73+
if (
74+
signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE &&
75+
buffer.readUInt32LE(offset + 24) !== 0 &&
76+
where !== 'local'
77+
) {
78+
buffer.writeUInt16LE(method, offset + 10)
79+
} else if (
80+
signature === LOCAL_FILE_HEADER_SIGNATURE &&
81+
buffer.readUInt32LE(offset + 22) !== 0 &&
82+
where !== 'central'
83+
) {
84+
buffer.writeUInt16LE(method, offset + 8)
85+
}
86+
}
87+
return buffer
88+
}
89+
3390
describe('assertOoxmlArchiveWithinLimits', () => {
3491
it('accepts a well-formed archive within limits', async () => {
3592
const buffer = await buildZip({ 'word/document.xml': '<xml>hello world</xml>' })
@@ -108,6 +165,105 @@ describe('assertOoxmlArchiveWithinLimits', () => {
108165
expect(() => assertOoxmlArchiveWithinLimits(tampered)).toThrow(ZipBombError)
109166
})
110167

168+
it('rejects an archive that under-declares its uncompressed size', async () => {
169+
// The declared sizes put this archive far under both limits, so only
170+
// inflating it reveals that it actually expands ~200x further.
171+
const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) })
172+
const lying = underDeclareSizes(honest, 1000)
173+
174+
expect(() => assertOoxmlArchiveWithinLimits(lying, HIGH_LIMITS)).toThrow(ZipBombError)
175+
expect(() => assertOoxmlArchiveWithinLimits(lying, HIGH_LIMITS)).toThrow(
176+
/inflates beyond the 1000 bytes it declares/
177+
)
178+
})
179+
180+
it('still accepts the same archive when its declared sizes are honest', async () => {
181+
const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) })
182+
expect(() => assertOoxmlArchiveWithinLimits(honest, HIGH_LIMITS)).not.toThrow()
183+
})
184+
185+
it('rejects a stored entry whose declared size does not match its payload', async () => {
186+
const zip = new JSZip()
187+
zip.file('document.xml', 'A'.repeat(50_000))
188+
const stored = (await zip.generateAsync({
189+
type: 'nodebuffer',
190+
compression: 'STORE',
191+
})) as Buffer
192+
193+
expect(() =>
194+
assertOoxmlArchiveWithinLimits(underDeclareSizes(stored, 10), HIGH_LIMITS)
195+
).toThrow(/stored entry declares 10 bytes but holds 50000/)
196+
})
197+
198+
it('rejects an entry using a compression method the parsers cannot read', async () => {
199+
const buffer = await buildZip({ 'word/document.xml': '<xml>hello</xml>' })
200+
expect(() =>
201+
assertOoxmlArchiveWithinLimits(setCompressionMethod(buffer, 12), HIGH_LIMITS)
202+
).toThrow(/unsupported compression method 12/)
203+
})
204+
205+
it('rejects an entry whose central and local compression methods disagree', async () => {
206+
// Claiming STORED centrally skips the bounded inflation, while SheetJS
207+
// switches on the local header and would inflate the payload anyway.
208+
const honest = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) })
209+
const split = setCompressionMethod(honest, 0, 'central')
210+
211+
expect(() => assertOoxmlArchiveWithinLimits(split, HIGH_LIMITS)).toThrow(ZipBombError)
212+
expect(() => assertOoxmlArchiveWithinLimits(split, HIGH_LIMITS)).toThrow(
213+
/compression method 0 centrally but 8 locally/
214+
)
215+
})
216+
217+
it('rejects an entry whose central and local declared sizes disagree', async () => {
218+
const honest = await buildZip({ 'word/document.xml': 'A'.repeat(200_000) })
219+
const buffer = Buffer.from(honest)
220+
for (let offset = 0; offset + 30 <= buffer.length; offset++) {
221+
if (
222+
buffer.readUInt32LE(offset) === LOCAL_FILE_HEADER_SIGNATURE &&
223+
buffer.readUInt32LE(offset + 22) !== 0
224+
) {
225+
buffer.writeUInt32LE(64, offset + 22)
226+
}
227+
}
228+
229+
expect(() => assertOoxmlArchiveWithinLimits(buffer, HIGH_LIMITS)).toThrow(
230+
/200000 bytes centrally but .* locally/
231+
)
232+
})
233+
234+
it('charges entries hidden behind an under-reported EOCD count against the cap', async () => {
235+
// JSZip's readCentralDir loops on the record signature and keeps every
236+
// entry it finds — a count mismatch is explicitly not an error there — so
237+
// entries past the declared count must still be charged against the cap.
238+
const buffer = await buildZip({
239+
'a.xml': 'A'.repeat(60_000),
240+
'b.xml': 'B'.repeat(60_000),
241+
'c.xml': 'C'.repeat(60_000),
242+
})
243+
const eocdOffset = buffer.length - 22
244+
expect(buffer.readUInt32LE(eocdOffset)).toBe(0x06054b50)
245+
buffer.writeUInt16LE(1, eocdOffset + 8) // entries on this disk
246+
buffer.writeUInt16LE(1, eocdOffset + 10) // total entries
247+
248+
expect(() =>
249+
assertOoxmlArchiveWithinLimits(buffer, {
250+
maxTotalUncompressedBytes: 100_000,
251+
maxCompressionRatio: 10_000,
252+
ratioCheckFloorBytes: 1024 * 1024 * 1024,
253+
})
254+
).toThrow(/exceeds the maximum allowed/)
255+
})
256+
257+
it('accepts a multi-entry archive whose entries all inflate to what they declare', async () => {
258+
const buffer = await buildZip({
259+
'[Content_Types].xml': '<?xml version="1.0"?><Types/>',
260+
'_rels/.rels': '<?xml version="1.0"?><Relationships/>',
261+
'word/document.xml': `<w:document>${'text '.repeat(5000)}</w:document>`,
262+
'word/styles.xml': `<w:styles>${'style '.repeat(2000)}</w:styles>`,
263+
})
264+
expect(() => assertOoxmlArchiveWithinLimits(buffer, HIGH_LIMITS)).not.toThrow()
265+
})
266+
111267
it('no-ops for buffers that are not ZIP archives', () => {
112268
const plaintext = Buffer.from('this is just plain text, not a zip archive at all')
113269
expect(() => assertOoxmlArchiveWithinLimits(plaintext)).not.toThrow()

0 commit comments

Comments
 (0)