Skip to content

Commit fac1c7c

Browse files
committed
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.
1 parent 07e2eea commit fac1c7c

2 files changed

Lines changed: 83 additions & 4 deletions

File tree

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

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,31 @@ function underDeclareSizes(source: Buffer, declared: number): Buffer {
5757
return buffer
5858
}
5959

60-
/** Overwrite the compression method on every non-empty central-directory record. */
61-
function setCompressionMethod(source: Buffer, method: number): Buffer {
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 {
6270
const buffer = Buffer.from(source)
6371
for (let offset = 0; offset + 46 <= buffer.length; offset++) {
72+
const signature = buffer.readUInt32LE(offset)
6473
if (
65-
buffer.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE &&
66-
buffer.readUInt32LE(offset + 24) !== 0
74+
signature === CENTRAL_DIRECTORY_HEADER_SIGNATURE &&
75+
buffer.readUInt32LE(offset + 24) !== 0 &&
76+
where !== 'local'
6777
) {
6878
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)
6985
}
7086
}
7187
return buffer
@@ -186,6 +202,35 @@ describe('assertOoxmlArchiveWithinLimits', () => {
186202
).toThrow(/unsupported compression method 12/)
187203
})
188204

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+
189234
it('accepts a multi-entry archive whose entries all inflate to what they declare', async () => {
190235
const buffer = await buildZip({
191236
'[Content_Types].xml': '<?xml version="1.0"?><Types/>',

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ const UINT16_SENTINEL = 0xffff
3434
const COMPRESSION_METHOD_STORED = 0
3535
const COMPRESSION_METHOD_DEFLATE = 8
3636

37+
/** General-purpose bit 3: sizes live in a trailing data descriptor, not the local header. */
38+
const DATA_DESCRIPTOR_FLAG = 0x0008
39+
3740
export interface OoxmlSizeLimits {
3841
/** Hard ceiling on the summed declared uncompressed size of all entries. */
3942
maxTotalUncompressedBytes: number
@@ -274,6 +277,12 @@ function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): n
274277
* to exactly what it declared and passes, having already been bounded by
275278
* {@link sumDeclaredUncompressedSize}.
276279
*
280+
* The central and local headers must also agree on the compression method and
281+
* sizes, because the parsers disagree about which one to trust — JSZip reads
282+
* the central directory, SheetJS switches on the local header — and a record
283+
* that reads as STORED here but DEFLATE downstream would skip inflation
284+
* verification entirely.
285+
*
277286
* Returns an error message when the archive is lying or is shaped in a way that
278287
* cannot be verified, and `null` when every entry checks out.
279288
*/
@@ -310,6 +319,31 @@ function findInflationMismatch(buffer: Buffer, location: CentralDirectoryLocatio
310319
return 'entry data starts outside the archive'
311320
}
312321

322+
// The two headers must agree on how the payload is encoded. JSZip trusts
323+
// the central directory while SheetJS switches on the local header's
324+
// method, so a record that claims STORED centrally and DEFLATE locally
325+
// would skip verification here and still be inflated downstream.
326+
const localFlags = buffer.readUInt16LE(localHeaderOffset + 6)
327+
const localMethod = buffer.readUInt16LE(localHeaderOffset + 8)
328+
if (localMethod !== compressionMethod) {
329+
return `entry declares compression method ${compressionMethod} centrally but ${localMethod} locally`
330+
}
331+
332+
// Sizes must agree too, for the same reason. They are legitimately absent
333+
// from the local header when the data-descriptor flag is set, and are
334+
// sentinels under ZIP64, so only compare when both are actually present.
335+
const hasDataDescriptor = (localFlags & DATA_DESCRIPTOR_FLAG) !== 0
336+
const localCompressedSize = buffer.readUInt32LE(localHeaderOffset + 18)
337+
const localUncompressedSize = buffer.readUInt32LE(localHeaderOffset + 22)
338+
if (
339+
!hasDataDescriptor &&
340+
localCompressedSize !== UINT32_SENTINEL &&
341+
localUncompressedSize !== UINT32_SENTINEL &&
342+
(localCompressedSize !== compressedSize || localUncompressedSize !== uncompressedSize)
343+
) {
344+
return `entry declares ${compressedSize}/${uncompressedSize} bytes centrally but ${localCompressedSize}/${localUncompressedSize} locally`
345+
}
346+
313347
if (compressionMethod === COMPRESSION_METHOD_STORED) {
314348
// A stored entry is its own payload, so any divergence is a lie outright.
315349
if (uncompressedSize !== compressedSize) {

0 commit comments

Comments
 (0)