Skip to content

Commit a8532d4

Browse files
mcollinarvagg
authored andcommitted
deps,http: http_parser set max header size to 8KB
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>
1 parent 38ca8ba commit a8532d4

File tree

5 files changed

+166
-11
lines changed

5 files changed

+166
-11
lines changed

deps/http_parser/http_parser.gyp

+2-2
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
'defines': [ 'HTTP_PARSER_STRICT=0' ],
5757
'include_dirs': [ '.' ],
5858
},
59-
'defines': [ 'HTTP_PARSER_STRICT=0' ],
59+
'defines': [ 'HTTP_MAX_HEADER_SIZE=8192', 'HTTP_PARSER_STRICT=0' ],
6060
'sources': [ './http_parser.c', ],
6161
'conditions': [
6262
['OS=="win"', {
@@ -79,7 +79,7 @@
7979
'defines': [ 'HTTP_PARSER_STRICT=1' ],
8080
'include_dirs': [ '.' ],
8181
},
82-
'defines': [ 'HTTP_PARSER_STRICT=1' ],
82+
'defines': [ 'HTTP_MAX_HEADER_SIZE=8192', 'HTTP_PARSER_STRICT=1' ],
8383
'sources': [ './http_parser.c', ],
8484
'conditions': [
8585
['OS=="win"', {

src/node_http_parser.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,8 @@ class Parser : public AsyncWrap, public StreamListener {
599599
size_t nparsed =
600600
http_parser_execute(&parser_, &settings, data, len);
601601

602+
enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
603+
602604
Save();
603605

604606
// Unassign the 'buffer_' variable
@@ -613,9 +615,7 @@ class Parser : public AsyncWrap, public StreamListener {
613615
Local<Integer> nparsed_obj = Integer::New(env()->isolate(), nparsed);
614616
// If there was a parse error in one of the callbacks
615617
// TODO(bnoordhuis) What if there is an error on EOF?
616-
if (!parser_.upgrade && nparsed != len) {
617-
enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
618-
618+
if ((!parser_.upgrade && nparsed != len) || err != HPE_OK) {
619619
Local<Value> e = Exception::Error(env()->parse_error_string());
620620
Local<Object> obj = e->ToObject(env()->isolate()->GetCurrentContext())
621621
.ToLocalChecked();

test/parallel/test-http-max-headers-count.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,14 @@ let requests = 0;
2828
let responses = 0;
2929

3030
const headers = {};
31-
const N = 2000;
31+
const N = 100;
3232
for (let i = 0; i < N; ++i) {
3333
headers[`key${i}`] = i;
3434
}
3535

3636
const maxAndExpected = [ // for server
3737
[50, 50],
38-
[1500, 1500],
38+
[1500, 102],
3939
[0, N + 2] // Host and Connection
4040
];
4141
let max = maxAndExpected[requests][0];
@@ -56,7 +56,7 @@ server.maxHeadersCount = max;
5656
server.listen(0, function() {
5757
const maxAndExpected = [ // for client
5858
[20, 20],
59-
[1200, 1200],
59+
[1200, 103],
6060
[0, N + 3] // Connection, Date and Transfer-Encoding
6161
];
6262
doRequest();

test/parallel/test-https-max-headers-count.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@ let requests = 0;
1717
let responses = 0;
1818

1919
const headers = {};
20-
const N = 2000;
20+
const N = 100;
2121
for (let i = 0; i < N; ++i) {
2222
headers[`key${i}`] = i;
2323
}
2424

2525
const maxAndExpected = [ // for server
2626
[50, 50],
27-
[1500, 1500],
27+
[1500, 102],
2828
[0, N + 2] // Host and Connection
2929
];
3030
let max = maxAndExpected[requests][0];
@@ -45,7 +45,7 @@ server.maxHeadersCount = max;
4545
server.listen(0, common.mustCall(() => {
4646
const maxAndExpected = [ // for client
4747
[20, 20],
48-
[1200, 1200],
48+
[1200, 103],
4949
[0, N + 3] // Connection, Date and Transfer-Encoding
5050
];
5151
const doRequest = common.mustCall(() => {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
'use strict';
2+
3+
const assert = require('assert');
4+
const common = require('../common');
5+
const http = require('http');
6+
const net = require('net');
7+
const MAX = 8 * 1024; // 8KB
8+
9+
// Verify that we cannot receive more than 8KB of headers.
10+
11+
function once(cb) {
12+
let called = false;
13+
return () => {
14+
if (!called) {
15+
called = true;
16+
cb();
17+
}
18+
};
19+
}
20+
21+
function finished(client, callback) {
22+
'abort error end'.split(' ').forEach((e) => {
23+
client.on(e, once(() => setImmediate(callback)));
24+
});
25+
}
26+
27+
function fillHeaders(headers, currentSize, valid = false) {
28+
headers += 'a'.repeat(MAX - headers.length - 3);
29+
// Generate valid headers
30+
if (valid) {
31+
// TODO(mcollina): understand why -9 is needed instead of -1
32+
headers = headers.slice(0, -9);
33+
}
34+
return headers + '\r\n\r\n';
35+
}
36+
37+
const timeout = common.platformTimeout(10);
38+
39+
function writeHeaders(socket, headers) {
40+
const array = [];
41+
42+
// this is off from 1024 so that \r\n does not get split
43+
const chunkSize = 1000;
44+
let last = 0;
45+
46+
for (let i = 0; i < headers.length / chunkSize; i++) {
47+
const current = (i + 1) * chunkSize;
48+
array.push(headers.slice(last, current));
49+
last = current;
50+
}
51+
52+
// safety check we are chunking correctly
53+
assert.strictEqual(array.join(''), headers);
54+
55+
next();
56+
57+
function next() {
58+
if (socket.write(array.shift())) {
59+
if (array.length === 0) {
60+
socket.end();
61+
} else {
62+
setTimeout(next, timeout);
63+
}
64+
} else {
65+
socket.once('drain', next);
66+
}
67+
}
68+
}
69+
70+
function test1() {
71+
let headers =
72+
'HTTP/1.1 200 OK\r\n' +
73+
'Content-Length: 0\r\n' +
74+
'X-CRASH: ';
75+
76+
// OK, Content-Length, 0, X-CRASH, aaa...
77+
const currentSize = 2 + 14 + 1 + 7;
78+
headers = fillHeaders(headers, currentSize);
79+
80+
const server = net.createServer((sock) => {
81+
sock.once('data', (chunk) => {
82+
writeHeaders(sock, headers);
83+
sock.resume();
84+
});
85+
});
86+
87+
server.listen(0, common.mustCall(() => {
88+
const port = server.address().port;
89+
const client = http.get({ port: port }, common.mustNotCall(() => {}));
90+
91+
client.on('error', common.mustCall((err) => {
92+
assert.strictEqual(err.code, 'HPE_HEADER_OVERFLOW');
93+
server.close();
94+
setImmediate(test2);
95+
}));
96+
}));
97+
}
98+
99+
const test2 = common.mustCall(() => {
100+
let headers =
101+
'GET / HTTP/1.1\r\n' +
102+
'Host: localhost\r\n' +
103+
'Agent: node\r\n' +
104+
'X-CRASH: ';
105+
106+
// /, Host, localhost, Agent, node, X-CRASH, a...
107+
const currentSize = 1 + 4 + 9 + 5 + 4 + 7;
108+
headers = fillHeaders(headers, currentSize);
109+
110+
const server = http.createServer(common.mustNotCall());
111+
112+
server.on('clientError', common.mustCall((err) => {
113+
assert.strictEqual(err.code, 'HPE_HEADER_OVERFLOW');
114+
}));
115+
116+
server.listen(0, common.mustCall(() => {
117+
const client = net.connect(server.address().port);
118+
client.on('connect', () => {
119+
writeHeaders(client, headers);
120+
client.resume();
121+
});
122+
123+
finished(client, common.mustCall((err) => {
124+
server.close();
125+
setImmediate(test3);
126+
}));
127+
}));
128+
});
129+
130+
const test3 = common.mustCall(() => {
131+
let headers =
132+
'GET / HTTP/1.1\r\n' +
133+
'Host: localhost\r\n' +
134+
'Agent: node\r\n' +
135+
'X-CRASH: ';
136+
137+
// /, Host, localhost, Agent, node, X-CRASH, a...
138+
const currentSize = 1 + 4 + 9 + 5 + 4 + 7;
139+
headers = fillHeaders(headers, currentSize, true);
140+
141+
const server = http.createServer(common.mustCall((req, res) => {
142+
res.end('hello world');
143+
setImmediate(server.close.bind(server));
144+
}));
145+
146+
server.listen(0, common.mustCall(() => {
147+
const client = net.connect(server.address().port);
148+
client.on('connect', () => {
149+
writeHeaders(client, headers);
150+
client.resume();
151+
});
152+
}));
153+
});
154+
155+
test1();

0 commit comments

Comments
 (0)