Skip to content

Commit 8ed8155

Browse files
elliott-with-the-longest-name-on-githubottomatedbenmccannhashcoko
authored
Merge commit from fork
* fix: Add length checks to remote forms * don't pre-instantiate a huge buffer * Update .changeset/thin-dodos-reply.md Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> * move errors to util function * test: avoid preallocating remote form buffers * test: make remote form allocation regression fail on old code --------- Co-authored-by: Ottomated <otto@ottomated.net> Co-authored-by: ottomated <31470743+ottomated@users.noreply.github.com> Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> Co-authored-by: Junha Heo <hashcoko@gmail.com>
1 parent d9ae9b0 commit 8ed8155

4 files changed

Lines changed: 223 additions & 23 deletions

File tree

‎.changeset/thin-dodos-reply.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@sveltejs/kit': patch
3+
---
4+
5+
fix: add length checks to remote forms

‎packages/kit/src/runtime/form-utils.js‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import { DEV } from 'esm-env';
66
import * as devalue from 'devalue';
77
import { text_decoder, text_encoder } from './utils.js';
8+
import { SvelteKitError } from '@sveltejs/kit/internal';
89

910
/**
1011
* Sets a value in a nested object using a path string, mutating the original object
@@ -64,7 +65,7 @@ export function convert_formdata(data) {
6465

6566
export const BINARY_FORM_CONTENT_TYPE = 'application/x-sveltekit-formdata';
6667
const BINARY_FORM_VERSION = 0;
67-
68+
const HEADER_BYTES = 1 + 4 + 2;
6869
/**
6970
* The binary format is as follows:
7071
* - 1 byte: Format version
@@ -144,7 +145,11 @@ export async function deserialize_binary_form(request) {
144145
return { data: convert_formdata(form_data), meta: {}, form_data };
145146
}
146147
if (!request.body) {
147-
throw new Error('Could not deserialize binary form: no body');
148+
throw deserialize_error('no body');
149+
}
150+
const content_length = parseInt(request.headers.get('content-length') ?? '');
151+
if (Number.isNaN(content_length)) {
152+
throw deserialize_error('invalid Content-Length header');
148153
}
149154

150155
const reader = request.body.getReader();
@@ -156,7 +161,7 @@ export async function deserialize_binary_form(request) {
156161
* @param {number} index
157162
* @returns {Promise<Uint8Array<ArrayBuffer> | undefined>}
158163
*/
159-
async function get_chunk(index) {
164+
function get_chunk(index) {
160165
if (index in chunks) return chunks[index];
161166

162167
let i = chunks.length;
@@ -195,8 +200,7 @@ export async function deserialize_binary_form(request) {
195200
return start_chunk.subarray(offset - chunk_start, offset + length - chunk_start);
196201
}
197202
// Otherwise, copy the data into a new buffer
198-
const buffer = new Uint8Array(length);
199-
buffer.set(start_chunk.subarray(offset - chunk_start));
203+
const chunks = [start_chunk.subarray(offset - chunk_start)];
200204
let cursor = start_chunk.byteLength - offset + chunk_start;
201205
while (cursor < length) {
202206
chunk_index++;
@@ -205,47 +209,62 @@ export async function deserialize_binary_form(request) {
205209
if (chunk.byteLength > length - cursor) {
206210
chunk = chunk.subarray(0, length - cursor);
207211
}
212+
chunks.push(chunk);
213+
cursor += chunk.byteLength;
214+
}
215+
const buffer = new Uint8Array(length);
216+
cursor = 0;
217+
for (const chunk of chunks) {
208218
buffer.set(chunk, cursor);
209219
cursor += chunk.byteLength;
210220
}
211221

212222
return buffer;
213223
}
214224

215-
const header = await get_buffer(0, 1 + 4 + 2);
216-
if (!header) throw new Error('Could not deserialize binary form: too short');
225+
const header = await get_buffer(0, HEADER_BYTES);
226+
if (!header) throw deserialize_error('too short');
217227

218228
if (header[0] !== BINARY_FORM_VERSION) {
219-
throw new Error(
220-
`Could not deserialize binary form: got version ${header[0]}, expected version ${BINARY_FORM_VERSION}`
221-
);
229+
throw deserialize_error(`got version ${header[0]}, expected version ${BINARY_FORM_VERSION}`);
222230
}
223231
const header_view = new DataView(header.buffer, header.byteOffset, header.byteLength);
224232
const data_length = header_view.getUint32(1, true);
233+
234+
if (HEADER_BYTES + data_length > content_length) {
235+
throw deserialize_error('data overflow');
236+
}
237+
225238
const file_offsets_length = header_view.getUint16(5, true);
226239

240+
if (HEADER_BYTES + data_length + file_offsets_length > content_length) {
241+
throw deserialize_error('file offset table overflow');
242+
}
243+
227244
// Read the form data
228-
const data_buffer = await get_buffer(1 + 4 + 2, data_length);
229-
if (!data_buffer) throw new Error('Could not deserialize binary form: data too short');
245+
const data_buffer = await get_buffer(HEADER_BYTES, data_length);
246+
if (!data_buffer) throw deserialize_error('data too short');
230247

231248
/** @type {Array<number>} */
232249
let file_offsets;
233250
/** @type {number} */
234251
let files_start_offset;
235252
if (file_offsets_length > 0) {
236253
// Read the file offset table
237-
const file_offsets_buffer = await get_buffer(1 + 4 + 2 + data_length, file_offsets_length);
238-
if (!file_offsets_buffer)
239-
throw new Error('Could not deserialize binary form: file offset table too short');
254+
const file_offsets_buffer = await get_buffer(HEADER_BYTES + data_length, file_offsets_length);
255+
if (!file_offsets_buffer) throw deserialize_error('file offset table too short');
240256

241257
file_offsets = /** @type {Array<number>} */ (
242258
JSON.parse(text_decoder.decode(file_offsets_buffer))
243259
);
244-
files_start_offset = 1 + 4 + 2 + data_length + file_offsets_length;
260+
files_start_offset = HEADER_BYTES + data_length + file_offsets_length;
245261
}
246262

247263
const [data, meta] = devalue.parse(text_decoder.decode(data_buffer), {
248264
File: ([name, type, size, last_modified, index]) => {
265+
if (files_start_offset + file_offsets[index] + size > content_length) {
266+
throw deserialize_error('file data overflow');
267+
}
249268
return new Proxy(
250269
new LazyFile(
251270
name,
@@ -276,6 +295,12 @@ export async function deserialize_binary_form(request) {
276295

277296
return { data, meta, form_data: null };
278297
}
298+
/**
299+
* @param {string} message
300+
*/
301+
function deserialize_error(message) {
302+
return new SvelteKitError(400, 'Bad Request', `Could not deserialize binary form: ${message}`);
303+
}
279304

280305
/** @implements {File} */
281306
class LazyFile {
@@ -380,7 +405,7 @@ class LazyFile {
380405
chunk_index++;
381406
let chunk = await this.#get_chunk(chunk_index);
382407
if (!chunk) {
383-
controller.error('Could not deserialize binary form: incomplete file data');
408+
controller.error('incomplete file data');
384409
controller.close();
385410
return;
386411
}

‎packages/kit/src/runtime/form-utils.spec.js‎

Lines changed: 175 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,8 @@ describe('binary form serializer', () => {
124124
method: 'POST',
125125
body: blob,
126126
headers: {
127-
'Content-Type': BINARY_FORM_CONTENT_TYPE
127+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
128+
'Content-Length': blob.size.toString()
128129
}
129130
})
130131
);
@@ -139,7 +140,8 @@ describe('binary form serializer', () => {
139140
large: new File([new Uint8Array(1024).fill('a'.charCodeAt(0))], 'large.txt', {
140141
type: 'text/plain',
141142
lastModified: 100
142-
})
143+
}),
144+
empty: new File([], 'empty.txt', { type: 'text/plain' })
143145
},
144146
{}
145147
);
@@ -160,11 +162,16 @@ describe('binary form serializer', () => {
160162
// @ts-expect-error duplex required in node
161163
duplex: 'half',
162164
headers: {
163-
'Content-Type': BINARY_FORM_CONTENT_TYPE
165+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
166+
'Content-Length': blob.size.toString()
164167
}
165168
})
166169
);
167-
const { small, large } = res.data;
170+
const { small, large, empty } = res.data;
171+
expect(empty.name).toBe('empty.txt');
172+
expect(empty.type).toBe('text/plain');
173+
expect(empty.size).toBe(0);
174+
expect(await empty.text()).toBe('');
168175
expect(small.name).toBe('a.txt');
169176
expect(small.type).toBe('text/plain');
170177
expect(small.size).toBe(1);
@@ -196,7 +203,8 @@ describe('binary form serializer', () => {
196203
method: 'POST',
197204
body: blob,
198205
headers: {
199-
'Content-Type': BINARY_FORM_CONTENT_TYPE
206+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
207+
'Content-Length': blob.size.toString()
200208
}
201209
})
202210
);
@@ -215,6 +223,166 @@ describe('binary form serializer', () => {
215223
expect(world_slice.type).toBe(file.type);
216224
});
217225

226+
test('throws when Content-Length is invalid', async () => {
227+
await expect(
228+
deserialize_binary_form(
229+
new Request('http://test', {
230+
method: 'POST',
231+
body: 'foo',
232+
headers: {
233+
'Content-Type': BINARY_FORM_CONTENT_TYPE
234+
}
235+
})
236+
)
237+
).rejects.toThrow('invalid Content-Length header');
238+
await expect(
239+
deserialize_binary_form(
240+
new Request('http://test', {
241+
method: 'POST',
242+
body: 'foo',
243+
headers: {
244+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
245+
'Content-Length': 'invalid'
246+
}
247+
})
248+
)
249+
).rejects.toThrow('invalid Content-Length header');
250+
});
251+
252+
test('data length check', async () => {
253+
const { blob } = serialize_binary_form(
254+
{
255+
foo: 'bar'
256+
},
257+
{}
258+
);
259+
await expect(
260+
deserialize_binary_form(
261+
new Request('http://test', {
262+
method: 'POST',
263+
body: blob,
264+
headers: {
265+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
266+
'Content-Length': (blob.size - 1).toString()
267+
}
268+
})
269+
)
270+
).rejects.toThrow('data overflow');
271+
});
272+
273+
test('file offset table length check', async () => {
274+
const { blob } = serialize_binary_form(
275+
{
276+
file: new File([''], 'a.txt')
277+
},
278+
{}
279+
);
280+
await expect(
281+
deserialize_binary_form(
282+
new Request('http://test', {
283+
method: 'POST',
284+
body: blob,
285+
headers: {
286+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
287+
'Content-Length': (blob.size - 1).toString()
288+
}
289+
})
290+
)
291+
).rejects.toThrow('file offset table overflow');
292+
});
293+
294+
test('file length check', async () => {
295+
const { blob } = serialize_binary_form(
296+
{
297+
file: new File(['a'], 'a.txt')
298+
},
299+
{}
300+
);
301+
await expect(
302+
deserialize_binary_form(
303+
new Request('http://test', {
304+
method: 'POST',
305+
body: blob,
306+
headers: {
307+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
308+
'Content-Length': (blob.size - 1).toString()
309+
}
310+
})
311+
)
312+
).rejects.toThrow('file data overflow');
313+
});
314+
315+
test('does not preallocate large buffers for incomplete bodies', async () => {
316+
const OriginalUint8Array = Uint8Array;
317+
const header_bytes = 1 + 4 + 2;
318+
const data_length = 32 * 1024 * 1024;
319+
320+
// This test should fail on the vulnerable implementation. To make the overallocation observable,
321+
// temporarily guard allocations of large Uint8Arrays — the fixed code only allocates after reading
322+
// the full range, so it should not trip this guard for an incomplete body.
323+
class GuardedUint8Array extends OriginalUint8Array {
324+
/** @param {...any} args */
325+
constructor(...args) {
326+
if (typeof args[0] === 'number' && args[0] === data_length) {
327+
throw new Error('EAGER_ALLOC');
328+
}
329+
330+
if (args.length === 0) {
331+
super();
332+
} else if (args.length === 1) {
333+
super(/** @type {any} */ (args[0]));
334+
} else if (args.length === 2) {
335+
super(/** @type {any} */ (args[0]), /** @type {any} */ (args[1]));
336+
} else {
337+
super(
338+
/** @type {any} */ (args[0]),
339+
/** @type {any} */ (args[1]),
340+
/** @type {any} */ (args[2])
341+
);
342+
}
343+
}
344+
}
345+
346+
/** @type {any} */ (globalThis).Uint8Array = GuardedUint8Array;
347+
try {
348+
// First chunk must include at least 1 byte past the header so that `get_buffer(header_bytes, data_length)`
349+
// takes the multi-chunk path.
350+
const first_chunk = new OriginalUint8Array(header_bytes + 1);
351+
first_chunk[0] = 0;
352+
const header_view = new DataView(
353+
first_chunk.buffer,
354+
first_chunk.byteOffset,
355+
first_chunk.byteLength
356+
);
357+
header_view.setUint32(1, data_length, true);
358+
header_view.setUint16(5, 0, true);
359+
360+
const stream = new ReadableStream({
361+
start(controller) {
362+
controller.enqueue(first_chunk);
363+
controller.close();
364+
}
365+
});
366+
367+
await expect(
368+
deserialize_binary_form(
369+
new Request('http://test', {
370+
method: 'POST',
371+
body: stream,
372+
// @ts-expect-error duplex required in node
373+
duplex: 'half',
374+
headers: {
375+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
376+
'Content-Length': (header_bytes + data_length).toString()
377+
}
378+
})
379+
)
380+
).rejects.toThrow('data too short');
381+
} finally {
382+
/** @type {any} */ (globalThis).Uint8Array = OriginalUint8Array;
383+
}
384+
});
385+
218386
// Regression test for https://github.com/sveltejs/kit/issues/14971
219387
test('DataView offset for shared memory', async () => {
220388
const { blob } = serialize_binary_form({ a: 1 }, {});
@@ -236,7 +404,8 @@ describe('binary form serializer', () => {
236404
// @ts-expect-error duplex required in node
237405
duplex: 'half',
238406
headers: {
239-
'Content-Type': BINARY_FORM_CONTENT_TYPE
407+
'Content-Type': BINARY_FORM_CONTENT_TYPE,
408+
'Content-Length': blob.size.toString()
240409
}
241410
})
242411
);

‎packages/kit/src/runtime/server/remote.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,7 @@ async function handle_remote_form_post_internal(event, state, manifest, id) {
294294
const fn = /** @type {RemoteInfo & { type: 'form' }} */ (/** @type {any} */ (form).__).fn;
295295

296296
const { data, meta, form_data } = await deserialize_binary_form(event.request);
297+
297298
if (action_id && !('id' in data)) {
298299
data.id = JSON.parse(decodeURIComponent(action_id));
299300
}

0 commit comments

Comments
 (0)