Skip to content

Commit 1260c38

Browse files
UlisesGasconblue2catbjohansebas
committed
fix: prevent process crash from trailing bytes after a compressed body
Ref: GHSA-27r3-jxmh-p828 Co-authored-by: Gavin Stuart <46635587+blue2cat@users.noreply.github.com> Co-authored-by: Sebastian Beltran <bjohansebas@gmail.com>
1 parent b4c2112 commit 1260c38

2 files changed

Lines changed: 227 additions & 8 deletions

File tree

‎lib/read.js‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@ function read (req, res, next, parse, debug, options) {
5151
: null
5252
var verify = opts.verify
5353

54+
// assert charset is supported before creating the decompression stream, so a
55+
// 415 cannot leave a decoder piped with no error listener
56+
if (verify && encoding !== null && !iconv.encodingExists(encoding)) {
57+
return next(createError(415, 'unsupported charset "' + encoding.toUpperCase() + '"', {
58+
charset: encoding.toLowerCase(),
59+
type: 'charset.unsupported'
60+
}))
61+
}
62+
5463
try {
5564
// get the content stream
5665
stream = contentstream(req, debug, opts.inflate)
@@ -66,14 +75,6 @@ function read (req, res, next, parse, debug, options) {
6675
? null
6776
: encoding
6877

69-
// assert charset is supported
70-
if (opts.encoding === null && encoding !== null && !iconv.encodingExists(encoding)) {
71-
return next(createError(415, 'unsupported charset "' + encoding.toUpperCase() + '"', {
72-
charset: encoding.toLowerCase(),
73-
type: 'charset.unsupported'
74-
}))
75-
}
76-
7778
// read body
7879
debug('read body')
7980
getBody(stream, opts, function (error, body) {
@@ -104,6 +105,23 @@ function read (req, res, next, parse, debug, options) {
104105
return
105106
}
106107

108+
// detach and destroy the decompression stream to avoid
109+
// decompression errors on the finished stream
110+
if (stream !== req) {
111+
// only detach the decompression stream, keep other consumers piped
112+
if (typeof req.unpipe === 'function') {
113+
req.unpipe(stream)
114+
} else {
115+
// Node.js 0.8 streams have no unpipe(dest)
116+
unpipe(req)
117+
}
118+
stream.on('error', function () {})
119+
destroy(stream, true)
120+
121+
// consume any remaining bytes so the HTTP request can finish
122+
req.resume()
123+
}
124+
107125
// verify
108126
if (verify) {
109127
try {

‎test/json.js‎

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
var assert = require('assert')
33
var asyncHooks = tryRequire('async_hooks')
44
var Buffer = require('safe-buffer').Buffer
5+
var crypto = require('crypto')
56
var http = require('http')
7+
var onFinished = require('on-finished')
68
var request = require('supertest')
79

810
var bodyParser = require('..')
@@ -11,6 +13,18 @@ var describeAsyncHooks = typeof asyncHooks.AsyncLocalStorage === 'function'
1113
? describe
1214
: describe.skip
1315

16+
// Since Node.js 11 zlib ends the readable side of a decompression stream as
17+
// soon as the compressed frame is complete (Z_STREAM_END). Older versions keep
18+
// the decoder open until the request ends, so the parser can never finish
19+
// before the request does and trailing bytes are reported as a decode error.
20+
var zlibEndsAtStreamEnd = parseInt(process.versions.node.split('.')[0], 10) >= 11
21+
var itZlibEndsAtStreamEnd = zlibEndsAtStreamEnd ? it : it.skip
22+
23+
// zlib.deflateSync('{"a":1}') and zlib.gzipSync('{"a":1}'), precomputed because
24+
// the sync zlib API only exists since Node.js 0.12
25+
var DEFLATED_JSON = Buffer.from('789cab564a54b232ac0500082a0209', 'hex')
26+
var GZIPPED_JSON = Buffer.from('1f8b0800000000000003ab564a54b232ac0500afac1b5607000000', 'hex')
27+
1428
describe('bodyParser.json()', function () {
1529
it('should parse JSON', function (done) {
1630
request(createServer())
@@ -234,6 +248,193 @@ describe('bodyParser.json()', function () {
234248
})
235249
})
236250

251+
// @see https://github.com/expressjs/body-parser/security/advisories/GHSA-27r3-jxmh-p828
252+
// Uses a raw socket (not supertest) so the trailing bytes reach the decoder
253+
// after it has finished the valid frame, which is what triggers the crash.
254+
describe('with trailing bytes after a compressed body', function () {
255+
itZlibEndsAtStreamEnd('should finish the request when the decompressor finishes early', function (done) {
256+
var jsonParser = bodyParser.json()
257+
var client
258+
var server = createServer(function (req, res, next) {
259+
jsonParser(req, res, function (err) {
260+
if (err) return next(err)
261+
// downstream middleware waits for the entire HTTP request to finish
262+
onFinished(req, next)
263+
// ensure the last bytes arrive after the decompressor has finished
264+
client.end(trailing)
265+
})
266+
})
267+
var first = Buffer.concat([
268+
DEFLATED_JSON,
269+
Buffer.from([0x00])
270+
])
271+
var trailing = Buffer.alloc(1024)
272+
273+
server.listen(0, '127.0.0.1', function () {
274+
client = http.request({
275+
host: '127.0.0.1',
276+
port: server.address().port,
277+
method: 'POST',
278+
path: '/',
279+
agent: false,
280+
headers: {
281+
'Content-Type': 'application/json',
282+
'Content-Encoding': 'deflate',
283+
'Content-Length': first.length + trailing.length
284+
}
285+
}, function (res) {
286+
var data = ''
287+
res.setEncoding('utf8')
288+
res.on('data', function (c) { data += c })
289+
res.on('end', function () {
290+
server.close()
291+
assert.strictEqual(res.statusCode, 200)
292+
assert.strictEqual(data, '{"a":1}')
293+
done()
294+
})
295+
})
296+
client.on('error', done)
297+
client.setTimeout(1000, function () {
298+
client.destroy(new Error('Timed out waiting for the HTTP request to finish'))
299+
})
300+
client.write(first)
301+
})
302+
})
303+
304+
itZlibEndsAtStreamEnd('should preserve other consumers of the request body', function (done) {
305+
var jsonParser = bodyParser.json()
306+
var client
307+
var server = createServer(function (req, res, next) {
308+
// a second consumer hashes the raw request body, e.g. for an audit log
309+
var hash = crypto.createHash('sha256')
310+
req.pipe(hash)
311+
312+
jsonParser(req, res, function (err) {
313+
if (err) return next(err)
314+
hash.once('finish', function () {
315+
res.setHeader('X-Body-SHA256', hash.read().toString('hex'))
316+
next()
317+
})
318+
// ensure the last bytes arrive after the decompressor has finished
319+
client.end(trailing)
320+
})
321+
})
322+
var first = Buffer.concat([
323+
DEFLATED_JSON,
324+
Buffer.from([0x00])
325+
])
326+
var trailing = Buffer.alloc(1024)
327+
var expectedHash = crypto.createHash('sha256').update(first).update(trailing).digest('hex')
328+
329+
server.listen(0, '127.0.0.1', function () {
330+
client = http.request({
331+
host: '127.0.0.1',
332+
port: server.address().port,
333+
method: 'POST',
334+
path: '/',
335+
agent: false,
336+
headers: {
337+
'Content-Type': 'application/json',
338+
'Content-Encoding': 'deflate',
339+
'Content-Length': first.length + trailing.length
340+
}
341+
}, function (res) {
342+
var data = ''
343+
res.setEncoding('utf8')
344+
res.on('data', function (c) { data += c })
345+
res.on('end', function () {
346+
server.close()
347+
assert.strictEqual(res.statusCode, 200)
348+
assert.strictEqual(data, '{"a":1}')
349+
assert.strictEqual(res.headers['x-body-sha256'], expectedHash)
350+
done()
351+
})
352+
})
353+
client.on('error', done)
354+
client.setTimeout(1000, function () {
355+
client.destroy(new Error('Timed out waiting for the request body hash'))
356+
})
357+
client.write(first)
358+
})
359+
})
360+
361+
it('should not crash on trailing bytes after a gzip body', function (done) {
362+
var server = createServer()
363+
server.listen(0, '127.0.0.1', function () {
364+
var port = server.address().port
365+
var body = Buffer.concat([
366+
GZIPPED_JSON,
367+
Buffer.from([0x00]),
368+
Buffer.alloc(100 * 1024, 0x58)
369+
])
370+
var req = http.request({
371+
host: '127.0.0.1',
372+
port: port,
373+
method: 'POST',
374+
path: '/',
375+
headers: {
376+
'Content-Type': 'application/json',
377+
'Content-Encoding': 'gzip',
378+
'Content-Length': body.length
379+
}
380+
}, function (res) {
381+
var data = ''
382+
res.on('data', function (c) { data += c })
383+
res.on('end', function () {
384+
setTimeout(function () {
385+
server.close()
386+
if (zlibEndsAtStreamEnd) {
387+
assert.strictEqual(res.statusCode, 200)
388+
assert.strictEqual(data, '{"a":1}')
389+
} else {
390+
// older zlib keeps decoding the trailing bytes: depending on the
391+
// version they are ignored (200) or reported as invalid (400)
392+
assert.ok(res.statusCode === 200 || res.statusCode === 400, 'unexpected status ' + res.statusCode)
393+
}
394+
done()
395+
}, 200)
396+
})
397+
})
398+
req.on('error', done)
399+
req.end(body)
400+
})
401+
})
402+
403+
it('should not crash when an unsupported charset rejects a compressed body', function (done) {
404+
var server = createServer({ verify: function () {} })
405+
server.listen(0, '127.0.0.1', function () {
406+
var port = server.address().port
407+
var body = Buffer.concat([
408+
GZIPPED_JSON,
409+
Buffer.from([0x00]),
410+
Buffer.alloc(100 * 1024, 0x58)
411+
])
412+
var req = http.request({
413+
host: '127.0.0.1',
414+
port: port,
415+
method: 'POST',
416+
path: '/',
417+
headers: {
418+
'Content-Type': 'application/json; charset=utf-999',
419+
'Content-Encoding': 'gzip',
420+
'Content-Length': body.length
421+
}
422+
}, function (res) {
423+
res.on('data', function () {})
424+
res.on('end', function () {
425+
setTimeout(function () {
426+
server.close()
427+
assert.strictEqual(res.statusCode, 415)
428+
done()
429+
}, 200)
430+
})
431+
})
432+
req.on('error', done)
433+
req.end(body)
434+
})
435+
})
436+
})
437+
237438
describe('with inflate option', function () {
238439
describe('when false', function () {
239440
before(function () {

0 commit comments

Comments
 (0)