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 28, 2018
1 parent 3ec8576 commit 1860352
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();

6 comments on commit 1860352

@rvagg
Copy link
Member

@rvagg rvagg commented on 1860352 Nov 28, 2018

Choose a reason for hiding this comment

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

For the record, this commit on master (same on v11.x in 74e01d0) is incomplete thanks to a lot of rebasing and juggling that had to go on in the week before release thanks to some unexpected bugs found once we had full CI access.

Line 28 in test/sequential/test-http-max-http-headers.js should have the following:

headers += 'a'.repeat(MAX - headers.length - 3);

But it was pulled up in to a later commit, af8d9e3, where there's a conditional to make it work with llhttp. As it is in this commit, this test will not pass.

It's fine on the other release line branches, v10.x and below, because we didn't have to deal with the llhttp split.

Sorry @mcollina

@mcollina
Copy link
Member Author

Choose a reason for hiding this comment

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

@rvagg I noticed that when I LGTM the releases! Thanks for clarifying!

@nareshgnT
Copy link

@nareshgnT nareshgnT commented on 1860352 Dec 4, 2018

Choose a reason for hiding this comment

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

Is there way to override this 8K limit. Our application uses cookie from different apps which make header length more than 8K. With latest build our apps are blowing because of the change. @rvagg @mcollina

@mcollina
Copy link
Member Author

Choose a reason for hiding this comment

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

we are working on that, see #24811

@nareshgnT
Copy link

Choose a reason for hiding this comment

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

thanks @mcollina. what is tentative date of releasing this

@mcollina
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no tentative date, it'll be released when it's ready.

Please sign in to comment.