Skip to content

Commit d71466d

Browse files
committed
fix(copilot): guard document-style extraction against zip bombs
extractDocumentStyle handed an attacker-controlled archive straight to JSZip and inflated named parts (word/theme/theme1.xml, word/styles.xml, ppt/presentation.xml, ppt/slideMasters/slideMaster1.xml) with no size bound, reachable from GET /api/workspaces/[id]/files/[fileId]/style and from the workspace VFS. Reading only a few entries is no protection: the bomb just has to live at one of those paths. It now calls assertOoxmlArchiveWithinLimits, the same guard the document parsers use, and the hand-rolled ZIP_MAGIC check is replaced by isZipShaped from that module — zip-guard is already shared this way by lib/uploads/archive.ts and lib/copilot/tools/handlers/upload-file-reader.ts. Checking each entry's size through JSZip instead would have been cheaper, since only a handful of parts are read, but JSZip reports the size the archive declares — the same attacker-controlled field a bomb lies about — so it would need to re-derive the guard's verification to be sound. The guard sits inside the existing try, so a rejection logs and returns null: the route already answers 422 and the VFS already returns null when no summary can be produced, and neither caller changes.
1 parent ccc2ec9 commit d71466d

2 files changed

Lines changed: 171 additions & 7 deletions

File tree

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { deflateRawSync } from 'zlib'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const { mockLoadAsync } = vi.hoisted(() => ({ mockLoadAsync: vi.fn() }))
8+
9+
vi.mock('jszip', () => ({ default: { loadAsync: mockLoadAsync } }))
10+
11+
import { extractDocumentStyle } from '@/lib/copilot/vfs/document-style'
12+
13+
const THEME_XML = `<?xml version="1.0"?>
14+
<a:theme xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main">
15+
<a:themeElements>
16+
<a:clrScheme name="Office">
17+
<a:dk1><a:sysClr val="windowText" lastClr="000000"/></a:dk1>
18+
<a:lt1><a:sysClr val="window" lastClr="FFFFFF"/></a:lt1>
19+
<a:accent1><a:srgbClr val="4472C4"/></a:accent1>
20+
</a:clrScheme>
21+
<a:fontScheme name="Office">
22+
<a:majorFont><a:latin typeface="Calibri Light"/></a:majorFont>
23+
<a:minorFont><a:latin typeface="Calibri"/></a:minorFont>
24+
</a:fontScheme>
25+
</a:themeElements>
26+
</a:theme>`
27+
28+
interface ZipEntryInput {
29+
name: string
30+
content: string
31+
/** Overrides the uncompressed size written to both headers, to model a lying archive. */
32+
declaredUncompressedSize?: number
33+
}
34+
35+
/**
36+
* Emit a ZIP archive byte by byte. Generating one with a library and patching it
37+
* afterwards cannot express a declared size that never matched the payload, and
38+
* that divergence is the whole subject of these tests.
39+
*/
40+
function buildZip(entries: ZipEntryInput[]): Buffer {
41+
const locals: Buffer[] = []
42+
const centrals: Buffer[] = []
43+
let offset = 0
44+
45+
for (const entry of entries) {
46+
const name = Buffer.from(entry.name, 'utf8')
47+
const raw = Buffer.from(entry.content, 'utf8')
48+
const deflated = deflateRawSync(raw)
49+
const declared = entry.declaredUncompressedSize ?? raw.length
50+
51+
const local = Buffer.alloc(30 + name.length)
52+
local.writeUInt32LE(0x04034b50, 0)
53+
local.writeUInt16LE(20, 4)
54+
local.writeUInt16LE(8, 8) // deflate
55+
local.writeUInt32LE(0, 14) // crc32 — nothing under test validates it
56+
local.writeUInt32LE(deflated.length, 18)
57+
local.writeUInt32LE(declared, 22)
58+
local.writeUInt16LE(name.length, 26)
59+
name.copy(local, 30)
60+
locals.push(local, deflated)
61+
62+
const central = Buffer.alloc(46 + name.length)
63+
central.writeUInt32LE(0x02014b50, 0)
64+
central.writeUInt16LE(20, 4)
65+
central.writeUInt16LE(20, 6)
66+
central.writeUInt16LE(8, 10) // deflate
67+
central.writeUInt32LE(0, 16)
68+
central.writeUInt32LE(deflated.length, 20)
69+
central.writeUInt32LE(declared, 24)
70+
central.writeUInt16LE(name.length, 28)
71+
central.writeUInt32LE(offset, 42)
72+
name.copy(central, 46)
73+
centrals.push(central)
74+
75+
offset += local.length + deflated.length
76+
}
77+
78+
const centralDirectory = Buffer.concat(centrals)
79+
const eocd = Buffer.alloc(22)
80+
eocd.writeUInt32LE(0x06054b50, 0)
81+
eocd.writeUInt16LE(entries.length, 8)
82+
eocd.writeUInt16LE(entries.length, 10)
83+
eocd.writeUInt32LE(centralDirectory.length, 12)
84+
eocd.writeUInt32LE(offset, 16)
85+
86+
return Buffer.concat([...locals, centralDirectory, eocd])
87+
}
88+
89+
/** Stand-in for a loaded JSZip archive, serving the parts the extractor asks for. */
90+
function fakeArchive(files: Record<string, string>) {
91+
return {
92+
file: (path: string) => (files[path] ? { async: async () => files[path] } : null),
93+
}
94+
}
95+
96+
describe('extractDocumentStyle', () => {
97+
beforeEach(() => {
98+
vi.clearAllMocks()
99+
})
100+
101+
it('extracts theme colors and fonts from a well-formed docx', async () => {
102+
mockLoadAsync.mockResolvedValue(fakeArchive({ 'word/theme/theme1.xml': THEME_XML }))
103+
104+
const summary = await extractDocumentStyle(
105+
buildZip([{ name: 'word/theme/theme1.xml', content: THEME_XML }]),
106+
'docx'
107+
)
108+
109+
expect(mockLoadAsync).toHaveBeenCalledOnce()
110+
expect(summary?.theme?.colors).toMatchObject({
111+
dk1: '000000',
112+
lt1: 'FFFFFF',
113+
accent1: '4472C4',
114+
})
115+
expect(summary?.theme?.fonts).toEqual({ major: 'Calibri Light', minor: 'Calibri' })
116+
})
117+
118+
it('extracts theme data from a well-formed pptx', async () => {
119+
mockLoadAsync.mockResolvedValue(fakeArchive({ 'ppt/theme/theme1.xml': THEME_XML }))
120+
121+
const summary = await extractDocumentStyle(
122+
buildZip([{ name: 'ppt/theme/theme1.xml', content: THEME_XML }]),
123+
'pptx'
124+
)
125+
126+
expect(summary?.theme?.fonts.minor).toBe('Calibri')
127+
})
128+
129+
it('never hands JSZip an archive declaring more expansion than the guard allows', async () => {
130+
// JSZip would also fail this archive — but only after inflating the entry,
131+
// which is the memory cost the guard exists to avoid. Asserting on the
132+
// return value alone cannot tell the two apart, so assert JSZip is never
133+
// reached.
134+
const bomb = buildZip([
135+
{
136+
name: 'word/theme/theme1.xml',
137+
content: THEME_XML,
138+
declaredUncompressedSize: 2 * 1024 * 1024 * 1024,
139+
},
140+
])
141+
142+
expect(await extractDocumentStyle(bomb, 'docx')).toBeNull()
143+
expect(mockLoadAsync).not.toHaveBeenCalled()
144+
})
145+
146+
it('never hands JSZip an archive with an implausible compression ratio', async () => {
147+
const bomb = buildZip([
148+
{
149+
name: 'word/theme/theme1.xml',
150+
content: 'A'.repeat(400 * 1024 * 1024),
151+
},
152+
])
153+
154+
expect(await extractDocumentStyle(bomb, 'docx')).toBeNull()
155+
expect(mockLoadAsync).not.toHaveBeenCalled()
156+
})
157+
158+
it('returns null for a buffer that is not a ZIP archive', async () => {
159+
expect(await extractDocumentStyle(Buffer.from('not an archive at all'), 'docx')).toBeNull()
160+
expect(await extractDocumentStyle(Buffer.alloc(2), 'docx')).toBeNull()
161+
expect(mockLoadAsync).not.toHaveBeenCalled()
162+
})
163+
})

apps/sim/lib/copilot/vfs/document-style.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
import { createLogger } from '@sim/logger'
22
import { toError } from '@sim/utils/errors'
3+
import { assertOoxmlArchiveWithinLimits, isZipShaped } from '@/lib/file-parsers/zip-guard'
34

45
const logger = createLogger('DocumentStyle')
56

6-
// ZIP magic bytes: PK\x03\x04
7-
const ZIP_MAGIC = [0x50, 0x4b, 0x03, 0x04]
8-
97
interface ThemeColors {
108
dk1: string
119
lt1: string
@@ -389,12 +387,15 @@ export async function extractDocumentStyle(
389387
return extractPdfStyle(buffer)
390388
}
391389

392-
if (buffer.length < 4) return null
393-
for (let i = 0; i < 4; i++) {
394-
if (buffer[i] !== ZIP_MAGIC[i]) return null
395-
}
390+
if (!isZipShaped(buffer)) return null
396391

397392
try {
393+
// Reading a handful of named parts still means handing an attacker-controlled
394+
// archive to JSZip, which inflates whatever those entries hold. Bound it with
395+
// the same guard the document parsers use rather than trusting the entry
396+
// sizes JSZip reports, which come from the archive itself.
397+
assertOoxmlArchiveWithinLimits(buffer)
398+
398399
const JSZip = (await import('jszip')).default
399400
const zip = await JSZip.loadAsync(buffer)
400401

0 commit comments

Comments
 (0)