Skip to content

Commit 46d1b33

Browse files
jasnellMylesBorins
jasnell
authored and
MylesBorins
committed
http2: verify flood error and unsolicited frames
* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 591f78b commit 46d1b33

7 files changed

+252
-2
lines changed

‎src/node_http2.cc

+38-2
Original file line numberDiff line numberDiff line change
@@ -1306,8 +1306,24 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
13061306
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
13071307
if (ack) {
13081308
Http2Ping* ping = PopPing();
1309-
if (ping != nullptr)
1309+
if (ping != nullptr) {
13101310
ping->Done(true, frame->ping.opaque_data);
1311+
} else {
1312+
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
1313+
// spec does not require this, but there is no legitimate reason to
1314+
// receive an unsolicited PING ack on a connection. Either the peer
1315+
// is buggy or malicious, and we're not going to tolerate such
1316+
// nonsense.
1317+
Isolate* isolate = env()->isolate();
1318+
HandleScope scope(isolate);
1319+
Local<Context> context = env()->context();
1320+
Context::Scope context_scope(context);
1321+
1322+
Local<Value> argv[1] = {
1323+
Integer::New(isolate, NGHTTP2_ERR_PROTO),
1324+
};
1325+
MakeCallback(env()->error_string(), arraysize(argv), argv);
1326+
}
13111327
}
13121328
}
13131329

@@ -1318,8 +1334,28 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
13181334
// If this is an acknowledgement, we should have an Http2Settings
13191335
// object for it.
13201336
Http2Settings* settings = PopSettings();
1321-
if (settings != nullptr)
1337+
if (settings != nullptr) {
13221338
settings->Done(true);
1339+
} else {
1340+
// SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2
1341+
// spec does not require this, but there is no legitimate reason to
1342+
// receive an unsolicited SETTINGS ack on a connection. Either the peer
1343+
// is buggy or malicious, and we're not going to tolerate such
1344+
// nonsense.
1345+
// Note that nghttp2 currently prevents this from happening for SETTINGS
1346+
// frames, so this block is purely defensive just in case that behavior
1347+
// changes. Specifically, unlike unsolicited PING acks, unsolicited
1348+
// SETTINGS acks should *never* make it this far.
1349+
Isolate* isolate = env()->isolate();
1350+
HandleScope scope(isolate);
1351+
Local<Context> context = env()->context();
1352+
Context::Scope context_scope(context);
1353+
1354+
Local<Value> argv[1] = {
1355+
Integer::New(isolate, NGHTTP2_ERR_PROTO),
1356+
};
1357+
MakeCallback(env()->error_string(), arraysize(argv), argv);
1358+
}
13231359
} else {
13241360
// Otherwise, notify the session about a new settings
13251361
MakeCallback(env()->onsettings_string(), 0, nullptr);

‎test/common/http2.js

+10
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,21 @@ class HeadersFrame extends Frame {
118118
}
119119
}
120120

121+
class PingFrame extends Frame {
122+
constructor(ack = false) {
123+
const buffers = [Buffer.alloc(8)];
124+
super(8, 6, ack ? 1 : 0, 0);
125+
buffers.unshift(this[kFrameData]);
126+
this[kFrameData] = Buffer.concat(buffers);
127+
}
128+
}
129+
121130
module.exports = {
122131
Frame,
123132
DataFrame,
124133
HeadersFrame,
125134
SettingsFrame,
135+
PingFrame,
126136
kFakeRequestHeaders,
127137
kFakeResponseHeaders,
128138
kClientMagic

‎test/parallel/test-http2-ping-unsolicited-ack.js

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const http2 = require('http2');
8+
const net = require('net');
9+
const http2util = require('../common/http2');
10+
11+
// Test that ping flooding causes the session to be torn down
12+
13+
const kSettings = new http2util.SettingsFrame();
14+
const kPingAck = new http2util.PingFrame(true);
15+
16+
const server = http2.createServer();
17+
18+
server.on('stream', common.mustNotCall());
19+
server.on('session', common.mustCall((session) => {
20+
session.on('error', common.expectsError({
21+
code: 'ERR_HTTP2_ERROR',
22+
message: 'Protocol error'
23+
}));
24+
session.on('close', common.mustCall(() => server.close()));
25+
}));
26+
27+
server.listen(0, common.mustCall(() => {
28+
const client = net.connect(server.address().port);
29+
30+
client.on('connect', common.mustCall(() => {
31+
client.write(http2util.kClientMagic, () => {
32+
client.write(kSettings.data);
33+
// Send an unsolicited ping ack
34+
client.write(kPingAck.data);
35+
});
36+
}));
37+
38+
// An error event may or may not be emitted, depending on operating system
39+
// and timing. We do not really care if one is emitted here or not, as the
40+
// error on the server side is what we are testing for. Do not make this
41+
// a common.mustCall() and there's no need to check the error details.
42+
client.on('error', () => {});
43+
}));

‎test/parallel/test-http2-settings-unsolicited-ack.js

+50
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const http2 = require('http2');
9+
const net = require('net');
10+
const http2util = require('../common/http2');
11+
const Countdown = require('../common/countdown');
12+
13+
// Test that an unsolicited settings ack is ignored.
14+
15+
const kSettings = new http2util.SettingsFrame();
16+
const kSettingsAck = new http2util.SettingsFrame(true);
17+
18+
const server = http2.createServer();
19+
let client;
20+
21+
const countdown = new Countdown(3, () => {
22+
client.destroy();
23+
server.close();
24+
});
25+
26+
server.on('stream', common.mustNotCall());
27+
server.on('session', common.mustCall((session) => {
28+
session.on('remoteSettings', common.mustCall(() => countdown.dec()));
29+
}));
30+
31+
server.listen(0, common.mustCall(() => {
32+
client = net.connect(server.address().port);
33+
34+
// Ensures that the clients settings frames are not sent until the
35+
// servers are received, so that the first ack is actually expected.
36+
client.once('data', (chunk) => {
37+
// The very first chunk of data we get from the server should
38+
// be a settings frame.
39+
assert.deepStrictEqual(chunk.slice(0, 9), kSettings.data);
40+
// The first ack is expected.
41+
client.write(kSettingsAck.data, () => countdown.dec());
42+
// The second one is not and will be ignored.
43+
client.write(kSettingsAck.data, () => countdown.dec());
44+
});
45+
46+
client.on('connect', common.mustCall(() => {
47+
client.write(http2util.kClientMagic);
48+
client.write(kSettings.data);
49+
}));
50+
}));

‎test/sequential/sequential.status

+2
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ test-inspector-bindings : PASS, FLAKY
1212
test-inspector-debug-end : PASS, FLAKY
1313
test-inspector-async-hook-setup-at-signal: PASS, FLAKY
1414
test-inspector-stop-profile-after-done: PASS, FLAKY
15+
test-http2-ping-flood : PASS, FLAKY
16+
test-http2-settings-flood : PASS, FLAKY
1517

1618
[$system==linux]
1719

‎test/sequential/test-http2-ping-flood.js

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const http2 = require('http2');
8+
const net = require('net');
9+
const http2util = require('../common/http2');
10+
11+
// Test that ping flooding causes the session to be torn down
12+
13+
const kSettings = new http2util.SettingsFrame();
14+
const kPing = new http2util.PingFrame();
15+
16+
const server = http2.createServer();
17+
18+
server.on('stream', common.mustNotCall());
19+
server.on('session', common.mustCall((session) => {
20+
session.on('error', common.expectsError({
21+
code: 'ERR_HTTP2_ERROR',
22+
message:
23+
'Flooding was detected in this HTTP/2 session, and it must be closed'
24+
}));
25+
session.on('close', common.mustCall(() => {
26+
server.close();
27+
}));
28+
}));
29+
30+
server.listen(0, common.mustCall(() => {
31+
const client = net.connect(server.address().port);
32+
33+
// nghttp2 uses a limit of 10000 items in it's outbound queue.
34+
// If this number is exceeded, a flooding error is raised. Set
35+
// this lim higher to account for the ones that nghttp2 is
36+
// successfully able to respond to.
37+
// TODO(jasnell): Unfortunately, this test is inherently flaky because
38+
// it is entirely dependent on how quickly the server is able to handle
39+
// the inbound frames and whether those just happen to overflow nghttp2's
40+
// outbound queue. The threshold at which the flood error occurs can vary
41+
// from one system to another, and from one test run to another.
42+
client.on('connect', common.mustCall(() => {
43+
client.write(http2util.kClientMagic, () => {
44+
client.write(kSettings.data, () => {
45+
for (let n = 0; n < 35000; n++)
46+
client.write(kPing.data);
47+
});
48+
});
49+
}));
50+
51+
// An error event may or may not be emitted, depending on operating system
52+
// and timing. We do not really care if one is emitted here or not, as the
53+
// error on the server side is what we are testing for. Do not make this
54+
// a common.mustCall() and there's no need to check the error details.
55+
client.on('error', () => {});
56+
}));

‎test/sequential/test-http2-settings-flood.js

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const http2 = require('http2');
8+
const net = require('net');
9+
const http2util = require('../common/http2');
10+
11+
// Test that settings flooding causes the session to be torn down
12+
13+
const kSettings = new http2util.SettingsFrame();
14+
15+
const server = http2.createServer();
16+
17+
server.on('stream', common.mustNotCall());
18+
server.on('session', common.mustCall((session) => {
19+
session.on('error', common.expectsError({
20+
code: 'ERR_HTTP2_ERROR',
21+
message:
22+
'Flooding was detected in this HTTP/2 session, and it must be closed'
23+
}));
24+
session.on('close', common.mustCall(() => {
25+
server.close();
26+
}));
27+
}));
28+
29+
server.listen(0, common.mustCall(() => {
30+
const client = net.connect(server.address().port);
31+
32+
// nghttp2 uses a limit of 10000 items in it's outbound queue.
33+
// If this number is exceeded, a flooding error is raised. Set
34+
// this lim higher to account for the ones that nghttp2 is
35+
// successfully able to respond to.
36+
// TODO(jasnell): Unfortunately, this test is inherently flaky because
37+
// it is entirely dependent on how quickly the server is able to handle
38+
// the inbound frames and whether those just happen to overflow nghttp2's
39+
// outbound queue. The threshold at which the flood error occurs can vary
40+
// from one system to another, and from one test run to another.
41+
client.on('connect', common.mustCall(() => {
42+
client.write(http2util.kClientMagic, () => {
43+
for (let n = 0; n < 35000; n++)
44+
client.write(kSettings.data);
45+
});
46+
}));
47+
48+
// An error event may or may not be emitted, depending on operating system
49+
// and timing. We do not really care if one is emitted here or not, as the
50+
// error on the server side is what we are testing for. Do not make this
51+
// a common.mustCall() and there's no need to check the error details.
52+
client.on('error', () => {});
53+
}));

0 commit comments

Comments
 (0)