Skip to content

Commit 6a67dfd

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 PR-URL: #17969 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent 43ac36c commit 6a67dfd

7 files changed

+252
-2
lines changed

‎src/node_http2.cc

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

@@ -1317,8 +1333,28 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
13171333
// If this is an acknowledgement, we should have an Http2Settings
13181334
// object for it.
13191335
Http2Settings* settings = PopSettings();
1320-
if (settings != nullptr)
1336+
if (settings != nullptr) {
13211337
settings->Done(true);
1338+
} else {
1339+
// SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2
1340+
// spec does not require this, but there is no legitimate reason to
1341+
// receive an unsolicited SETTINGS ack on a connection. Either the peer
1342+
// is buggy or malicious, and we're not going to tolerate such
1343+
// nonsense.
1344+
// Note that nghttp2 currently prevents this from happening for SETTINGS
1345+
// frames, so this block is purely defensive just in case that behavior
1346+
// changes. Specifically, unlike unsolicited PING acks, unsolicited
1347+
// SETTINGS acks should *never* make it this far.
1348+
Isolate* isolate = env()->isolate();
1349+
HandleScope scope(isolate);
1350+
Local<Context> context = env()->context();
1351+
Context::Scope context_scope(context);
1352+
1353+
Local<Value> argv[1] = {
1354+
Integer::New(isolate, NGHTTP2_ERR_PROTO),
1355+
};
1356+
MakeCallback(env()->error_string(), arraysize(argv), argv);
1357+
}
13221358
} else {
13231359
// Otherwise, notify the session about a new settings
13241360
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
@@ -11,6 +11,8 @@ test-inspector-async-call-stack : PASS, FLAKY
1111
test-inspector-bindings : PASS, FLAKY
1212
test-inspector-debug-end : PASS, FLAKY
1313
test-inspector-async-hook-setup-at-signal: PASS, FLAKY
14+
test-http2-ping-flood : PASS, FLAKY
15+
test-http2-settings-flood : PASS, FLAKY
1416

1517
[$system==linux]
1618

‎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)