From 93dba83fb0fb46ee2ea87163f435392490b4d59b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 21 Aug 2018 17:26:51 +0200 Subject: [PATCH] deps,http: http_parser set max header size to 8KB CVE-2018-12121 PR-URL: https://github.com/nodejs-private/node-private/pull/143 Ref: https://github.com/nodejs-private/security/issues/139 Ref: https://github.com/nodejs-private/http-parser-private/pull/2 Reviewed-By: Anatoli Papirovski Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Rod Vagg Reviewed-By: Anna Henningsen --- deps/http_parser/http_parser.gyp | 4 +- src/node_http_parser.cc | 6 +- test/parallel/test-http-max-headers-count.js | 6 +- test/sequential/test-http-max-http-headers.js | 155 ++++++++++++++++++ 4 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 test/sequential/test-http-max-http-headers.js diff --git a/deps/http_parser/http_parser.gyp b/deps/http_parser/http_parser.gyp index ef34ecaeaeab45..4364f73d1f4548 100644 --- a/deps/http_parser/http_parser.gyp +++ b/deps/http_parser/http_parser.gyp @@ -56,7 +56,7 @@ 'defines': [ 'HTTP_PARSER_STRICT=0' ], 'include_dirs': [ '.' ], }, - 'defines': [ 'HTTP_PARSER_STRICT=0' ], + 'defines': [ 'HTTP_MAX_HEADER_SIZE=8192', 'HTTP_PARSER_STRICT=0' ], 'sources': [ './http_parser.c', ], 'conditions': [ ['OS=="win"', { @@ -79,7 +79,7 @@ 'defines': [ 'HTTP_PARSER_STRICT=1' ], 'include_dirs': [ '.' ], }, - 'defines': [ 'HTTP_PARSER_STRICT=1' ], + 'defines': [ 'HTTP_MAX_HEADER_SIZE=8192', 'HTTP_PARSER_STRICT=1' ], 'sources': [ './http_parser.c', ], 'conditions': [ ['OS=="win"', { diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 94abe9e390857b..9854b51c06db5d 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -621,6 +621,8 @@ class Parser : public AsyncWrap { size_t nparsed = http_parser_execute(&parser_, &settings, data, len); + enum http_errno err = HTTP_PARSER_ERRNO(&parser_); + Save(); // Unassign the 'buffer_' variable @@ -635,9 +637,7 @@ class Parser : public AsyncWrap { Local nparsed_obj = Integer::New(env()->isolate(), nparsed); // If there was a parse error in one of the callbacks // TODO(bnoordhuis) What if there is an error on EOF? - if (!parser_.upgrade && nparsed != len) { - enum http_errno err = HTTP_PARSER_ERRNO(&parser_); - + if ((!parser_.upgrade && nparsed != len) || err != HPE_OK) { Local e = Exception::Error(env()->parse_error_string()); Local obj = e->ToObject(env()->isolate()); obj->Set(env()->bytes_parsed_string(), nparsed_obj); diff --git a/test/parallel/test-http-max-headers-count.js b/test/parallel/test-http-max-headers-count.js index 05f4f774c2c929..9fcfe316e392ff 100644 --- a/test/parallel/test-http-max-headers-count.js +++ b/test/parallel/test-http-max-headers-count.js @@ -28,14 +28,14 @@ let requests = 0; let responses = 0; const headers = {}; -const N = 2000; +const N = 100; for (let i = 0; i < N; ++i) { headers[`key${i}`] = i; } const maxAndExpected = [ // for server [50, 50], - [1500, 1500], + [1500, 102], [0, N + 2] // Host and Connection ]; let max = maxAndExpected[requests][0]; @@ -56,7 +56,7 @@ server.maxHeadersCount = max; server.listen(0, function() { const maxAndExpected = [ // for client [20, 20], - [1200, 1200], + [1200, 103], [0, N + 3] // Connection, Date and Transfer-Encoding ]; doRequest(); diff --git a/test/sequential/test-http-max-http-headers.js b/test/sequential/test-http-max-http-headers.js new file mode 100644 index 00000000000000..ae76142a4fd077 --- /dev/null +++ b/test/sequential/test-http-max-http-headers.js @@ -0,0 +1,155 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const MAX = 8 * 1024; // 8KB + +// Verify that we cannot receive more than 8KB of headers. + +function once(cb) { + let called = false; + return () => { + if (!called) { + called = true; + cb(); + } + }; +} + +function finished(client, callback) { + 'abort error end'.split(' ').forEach((e) => { + client.on(e, once(() => setImmediate(callback))); + }); +} + +function fillHeaders(headers, currentSize, valid = false) { + headers += 'a'.repeat(MAX - headers.length - 3); + // Generate valid headers + if (valid) { + // TODO(mcollina): understand why -9 is needed instead of -1 + headers = headers.slice(0, -9); + } + return headers + '\r\n\r\n'; +} + +const timeout = common.platformTimeout(10); + +function writeHeaders(socket, headers) { + const array = []; + + // this is off from 1024 so that \r\n does not get split + const chunkSize = 1000; + let last = 0; + + for (let i = 0; i < headers.length / chunkSize; i++) { + const current = (i + 1) * chunkSize; + array.push(headers.slice(last, current)); + last = current; + } + + // safety check we are chunking correctly + assert.strictEqual(array.join(''), headers); + + next(); + + function next() { + if (socket.write(array.shift())) { + if (array.length === 0) { + socket.end(); + } else { + setTimeout(next, timeout); + } + } else { + socket.once('drain', next); + } + } +} + +function test1() { + let headers = + 'HTTP/1.1 200 OK\r\n' + + 'Content-Length: 0\r\n' + + 'X-CRASH: '; + + // OK, Content-Length, 0, X-CRASH, aaa... + const currentSize = 2 + 14 + 1 + 7; + headers = fillHeaders(headers, currentSize); + + const server = net.createServer((sock) => { + sock.once('data', (chunk) => { + writeHeaders(sock, headers); + sock.resume(); + }); + }); + + server.listen(0, common.mustCall(() => { + const port = server.address().port; + const client = http.get({ port: port }, common.mustNotCall(() => {})); + + client.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'HPE_HEADER_OVERFLOW'); + server.close(); + setImmediate(test2); + })); + })); +} + +const test2 = common.mustCall(() => { + let headers = + 'GET / HTTP/1.1\r\n' + + 'Host: localhost\r\n' + + 'Agent: node\r\n' + + 'X-CRASH: '; + + // /, Host, localhost, Agent, node, X-CRASH, a... + const currentSize = 1 + 4 + 9 + 5 + 4 + 7; + headers = fillHeaders(headers, currentSize); + + const server = http.createServer(common.mustNotCall()); + + server.on('clientError', common.mustCall((err) => { + assert.strictEqual(err.code, 'HPE_HEADER_OVERFLOW'); + })); + + server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + client.on('connect', () => { + writeHeaders(client, headers); + client.resume(); + }); + + finished(client, common.mustCall((err) => { + server.close(); + setImmediate(test3); + })); + })); +}); + +const test3 = common.mustCall(() => { + let headers = + 'GET / HTTP/1.1\r\n' + + 'Host: localhost\r\n' + + 'Agent: node\r\n' + + 'X-CRASH: '; + + // /, Host, localhost, Agent, node, X-CRASH, a... + const currentSize = 1 + 4 + 9 + 5 + 4 + 7; + headers = fillHeaders(headers, currentSize, true); + + const server = http.createServer(common.mustCall((req, res) => { + res.end('hello world'); + setImmediate(server.close.bind(server)); + })); + + server.listen(0, common.mustCall(() => { + const client = net.connect(server.address().port); + client.on('connect', () => { + writeHeaders(client, headers); + client.resume(); + }); + })); +}); + +test1();