Skip to content

Commit

Permalink
deps,http: http_parser set max header size to 8KB
Browse files Browse the repository at this point in the history
CVE-2018-12121

PR-URL: nodejs-private/node-private#143
Ref: nodejs-private/security#139
Ref: nodejs-private/http-parser-private#2
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
mcollina authored and rvagg committed Nov 27, 2018
1 parent 8f191f3 commit 74e01d0
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 8 deletions.
4 changes: 2 additions & 2 deletions deps/http_parser/http_parser.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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"', {
Expand All @@ -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"', {
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-http-max-headers-count.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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();
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-https-max-headers-count.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,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];
Expand All @@ -45,7 +45,7 @@ server.maxHeadersCount = max;
server.listen(0, common.mustCall(() => {
const maxAndExpected = [ // for client
[20, 20],
[1200, 1200],
[1200, 103],
[0, N + 3] // Connection, Date and Transfer-Encoding
];
const doRequest = common.mustCall(() => {
Expand Down
154 changes: 154 additions & 0 deletions test/sequential/test-http-max-http-headers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
'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) {
// 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();

1 comment on commit 74e01d0

@syrnick
Copy link

@syrnick syrnick commented on 74e01d0 Dec 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, --max-http-header-size is already added: 693e362175

The comment here https://github.com/nodejs/node/blob/master/deps/http_parser/http_parser.c#L141 indicates that it's the total size of the headers including status line that is limited to 8K. The maximum of 8K for all the headers seems unreasonably low.

This adversely affects systems with upstream load balancers and web servers before node that have higher limits. E.g. AWS ALB has 64K limit for all headers https://docs.aws.amazon.com/elasticloadbalancing/latest/userguide/how-elastic-load-balancing-works.html and 16K for a single line.

Please consider allowing to opt-out of this limit or implementing a more granular check (e.g. no more than 1000 concurrent requests over 8k limit are allowed).

Please sign in to comment.