diff --git a/src/common/formData.js b/src/common/formData.js index 9fa4510..57b362f 100644 --- a/src/common/formData.js +++ b/src/common/formData.js @@ -51,15 +51,28 @@ const isFormData = (obj) => (obj != null // neither null nor undefined const getFooter = (boundary) => `--${boundary}--\r\n\r\n`; +// Escape field names and file names per the WHATWG multipart/form-data +// serialization algorithm, preventing CR/LF (and `"`) injection into the +// multipart body. See https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart-form-data +const escapeName = (str) => String(str) + .replace(/\r/g, '%0D') + .replace(/\n/g, '%0A') + .replace(/"/g, '%22'); + +// Strip CR/LF from a part's Content-Type. Unlike a spec-compliant Blob, the +// duck-typed blob check accepts an arbitrary `type` string, so sanitize it to +// avoid injecting additional part headers/body into the multipart payload. +const sanitizeContentType = (str) => String(str).replace(/[\r\n]/g, ''); + const getHeader = (boundary, name, field) => { let header = ''; header += `--${boundary}\r\n`; - header += `Content-Disposition: form-data; name="${name}"`; + header += `Content-Disposition: form-data; name="${escapeName(name)}"`; if (isBlob(field)) { - header += `; filename="${field.name}"\r\n`; - header += `Content-Type: ${field.type || 'application/octet-stream'}`; + header += `; filename="${escapeName(field.name)}"\r\n`; + header += `Content-Type: ${sanitizeContentType(field.type || 'application/octet-stream')}`; } return `${header}\r\n\r\n`; diff --git a/test/common/formData.test.js b/test/common/formData.test.js index 2f94e9b..a0fea97 100644 --- a/test/common/formData.test.js +++ b/test/common/formData.test.js @@ -15,6 +15,7 @@ import assert from 'assert'; import { fileURLToPath } from 'url'; +import { Readable } from 'stream'; import { FormData, File, Blob } from 'formdata-node'; // eslint-disable-next-line import/no-unresolved @@ -57,4 +58,47 @@ describe('FormData Helpers Test', () => { assert.strictEqual(fds.length(), buf.length); assert(fds.contentType().startsWith('multipart/form-data; boundary=')); }); + + it('FormDataSerializer escapes CRLF in field names, file names and blob type', async () => { + // A CRLF-laden blob-like value. We drive the serializer with a hand-rolled + // form iterable because both a spec Blob and formdata-node normalize/strip + // these values before our serializer would ever see them; the duck-typed + // isBlob() check, however, accepts an arbitrary blob-like object. + const blob = { + [Symbol.toStringTag]: 'Blob', + name: 'evil\r\nX-Injected: filename.txt', + type: 'text/plain\r\n\r\ninjected part', + size: 4, + arrayBuffer: async () => new ArrayBuffer(4), + stream: () => Readable.from('data'), + text: async () => 'data', + slice: () => {}, + }; + const form = { + * [Symbol.iterator]() { + yield ['field\r\nX-Injected: name', 'value']; + yield ['blob', blob]; + }, + }; + + const fds = new FormDataSerializer(form); + const buf = await streamToBuffer(fds.stream()); + const boundary = fds.contentType().slice('multipart/form-data; boundary='.length); + const body = buf.toString(); + + // The tainted values must not introduce raw CRLF into the body: a real + // "\r\nX-Injected" / "\r\n\r\ninjected part" sequence would break out of the + // intended part header / body framing. + assert(!body.includes('\r\nX-Injected')); + assert(!body.includes('\r\n\r\ninjected part')); + // instead the CR/LF must be percent-escaped (names) or stripped (type) + assert(body.includes('name="field%0D%0AX-Injected: name"')); + assert(body.includes('filename="evil%0D%0AX-Injected: filename.txt"')); + assert(body.includes('Content-Type: text/plaininjected part')); + + // sanity: the declared length still matches the produced body + assert.strictEqual(fds.length(), buf.length); + // and no rogue boundary was injected + assert.strictEqual(body.split(`--${boundary}`).length - 1, 2 /* parts */ + 1 /* footer */); + }); });