Skip to content

Commit

Permalink
http2: verify flood error and unsolicited frames
Browse files Browse the repository at this point in the history
* verify protections against ping and settings flooding
* Strictly handle and verify handling of unsolicited ping and
  settings frame acks.

Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17969
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
jasnell authored and kjin committed Apr 30, 2018
1 parent 26c8b3c commit 0954785
Show file tree
Hide file tree
Showing 7 changed files with 252 additions and 2 deletions.
40 changes: 38 additions & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1306,8 +1306,24 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
if (ack) {
Http2Ping* ping = PopPing();
if (ping != nullptr)
if (ping != nullptr) {
ping->Done(true, frame->ping.opaque_data);
} else {
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
// spec does not require this, but there is no legitimate reason to
// receive an unsolicited PING ack on a connection. Either the peer
// is buggy or malicious, and we're not going to tolerate such
// nonsense.
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

Local<Value> argv[1] = {
Integer::New(isolate, NGHTTP2_ERR_PROTO),
};
MakeCallback(env()->error_string(), arraysize(argv), argv);
}
}
}

Expand All @@ -1318,8 +1334,28 @@ inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
// If this is an acknowledgement, we should have an Http2Settings
// object for it.
Http2Settings* settings = PopSettings();
if (settings != nullptr)
if (settings != nullptr) {
settings->Done(true);
} else {
// SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2
// spec does not require this, but there is no legitimate reason to
// receive an unsolicited SETTINGS ack on a connection. Either the peer
// is buggy or malicious, and we're not going to tolerate such
// nonsense.
// Note that nghttp2 currently prevents this from happening for SETTINGS
// frames, so this block is purely defensive just in case that behavior
// changes. Specifically, unlike unsolicited PING acks, unsolicited
// SETTINGS acks should *never* make it this far.
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);

Local<Value> argv[1] = {
Integer::New(isolate, NGHTTP2_ERR_PROTO),
};
MakeCallback(env()->error_string(), arraysize(argv), argv);
}
} else {
// Otherwise, notify the session about a new settings
MakeCallback(env()->onsettings_string(), 0, nullptr);
Expand Down
10 changes: 10 additions & 0 deletions test/common/http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,21 @@ class HeadersFrame extends Frame {
}
}

class PingFrame extends Frame {
constructor(ack = false) {
const buffers = [Buffer.alloc(8)];
super(8, 6, ack ? 1 : 0, 0);
buffers.unshift(this[kFrameData]);
this[kFrameData] = Buffer.concat(buffers);
}
}

module.exports = {
Frame,
DataFrame,
HeadersFrame,
SettingsFrame,
PingFrame,
kFakeRequestHeaders,
kFakeResponseHeaders,
kClientMagic
Expand Down
43 changes: 43 additions & 0 deletions test/parallel/test-http2-ping-unsolicited-ack.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const http2 = require('http2');
const net = require('net');
const http2util = require('../common/http2');

// Test that ping flooding causes the session to be torn down

const kSettings = new http2util.SettingsFrame();
const kPingAck = new http2util.PingFrame(true);

const server = http2.createServer();

server.on('stream', common.mustNotCall());
server.on('session', common.mustCall((session) => {
session.on('error', common.expectsError({
code: 'ERR_HTTP2_ERROR',
message: 'Protocol error'
}));
session.on('close', common.mustCall(() => server.close()));
}));

server.listen(0, common.mustCall(() => {
const client = net.connect(server.address().port);

client.on('connect', common.mustCall(() => {
client.write(http2util.kClientMagic, () => {
client.write(kSettings.data);
// Send an unsolicited ping ack
client.write(kPingAck.data);
});
}));

// An error event may or may not be emitted, depending on operating system
// and timing. We do not really care if one is emitted here or not, as the
// error on the server side is what we are testing for. Do not make this
// a common.mustCall() and there's no need to check the error details.
client.on('error', () => {});
}));
50 changes: 50 additions & 0 deletions test/parallel/test-http2-settings-unsolicited-ack.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const http2 = require('http2');
const net = require('net');
const http2util = require('../common/http2');
const Countdown = require('../common/countdown');

// Test that an unsolicited settings ack is ignored.

const kSettings = new http2util.SettingsFrame();
const kSettingsAck = new http2util.SettingsFrame(true);

const server = http2.createServer();
let client;

const countdown = new Countdown(3, () => {
client.destroy();
server.close();
});

server.on('stream', common.mustNotCall());
server.on('session', common.mustCall((session) => {
session.on('remoteSettings', common.mustCall(() => countdown.dec()));
}));

server.listen(0, common.mustCall(() => {
client = net.connect(server.address().port);

// Ensures that the clients settings frames are not sent until the
// servers are received, so that the first ack is actually expected.
client.once('data', (chunk) => {
// The very first chunk of data we get from the server should
// be a settings frame.
assert.deepStrictEqual(chunk.slice(0, 9), kSettings.data);
// The first ack is expected.
client.write(kSettingsAck.data, () => countdown.dec());
// The second one is not and will be ignored.
client.write(kSettingsAck.data, () => countdown.dec());
});

client.on('connect', common.mustCall(() => {
client.write(http2util.kClientMagic);
client.write(kSettings.data);
}));
}));
2 changes: 2 additions & 0 deletions test/sequential/sequential.status
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ test-inspector-bindings : PASS, FLAKY
test-inspector-debug-end : PASS, FLAKY
test-inspector-async-hook-setup-at-signal: PASS, FLAKY
test-inspector-stop-profile-after-done: PASS, FLAKY
test-http2-ping-flood : PASS, FLAKY
test-http2-settings-flood : PASS, FLAKY

[$system==linux]

Expand Down
56 changes: 56 additions & 0 deletions test/sequential/test-http2-ping-flood.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const http2 = require('http2');
const net = require('net');
const http2util = require('../common/http2');

// Test that ping flooding causes the session to be torn down

const kSettings = new http2util.SettingsFrame();
const kPing = new http2util.PingFrame();

const server = http2.createServer();

server.on('stream', common.mustNotCall());
server.on('session', common.mustCall((session) => {
session.on('error', common.expectsError({
code: 'ERR_HTTP2_ERROR',
message:
'Flooding was detected in this HTTP/2 session, and it must be closed'
}));
session.on('close', common.mustCall(() => {
server.close();
}));
}));

server.listen(0, common.mustCall(() => {
const client = net.connect(server.address().port);

// nghttp2 uses a limit of 10000 items in it's outbound queue.
// If this number is exceeded, a flooding error is raised. Set
// this lim higher to account for the ones that nghttp2 is
// successfully able to respond to.
// TODO(jasnell): Unfortunately, this test is inherently flaky because
// it is entirely dependent on how quickly the server is able to handle
// the inbound frames and whether those just happen to overflow nghttp2's
// outbound queue. The threshold at which the flood error occurs can vary
// from one system to another, and from one test run to another.
client.on('connect', common.mustCall(() => {
client.write(http2util.kClientMagic, () => {
client.write(kSettings.data, () => {
for (let n = 0; n < 35000; n++)
client.write(kPing.data);
});
});
}));

// An error event may or may not be emitted, depending on operating system
// and timing. We do not really care if one is emitted here or not, as the
// error on the server side is what we are testing for. Do not make this
// a common.mustCall() and there's no need to check the error details.
client.on('error', () => {});
}));
53 changes: 53 additions & 0 deletions test/sequential/test-http2-settings-flood.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const http2 = require('http2');
const net = require('net');
const http2util = require('../common/http2');

// Test that settings flooding causes the session to be torn down

const kSettings = new http2util.SettingsFrame();

const server = http2.createServer();

server.on('stream', common.mustNotCall());
server.on('session', common.mustCall((session) => {
session.on('error', common.expectsError({
code: 'ERR_HTTP2_ERROR',
message:
'Flooding was detected in this HTTP/2 session, and it must be closed'
}));
session.on('close', common.mustCall(() => {
server.close();
}));
}));

server.listen(0, common.mustCall(() => {
const client = net.connect(server.address().port);

// nghttp2 uses a limit of 10000 items in it's outbound queue.
// If this number is exceeded, a flooding error is raised. Set
// this lim higher to account for the ones that nghttp2 is
// successfully able to respond to.
// TODO(jasnell): Unfortunately, this test is inherently flaky because
// it is entirely dependent on how quickly the server is able to handle
// the inbound frames and whether those just happen to overflow nghttp2's
// outbound queue. The threshold at which the flood error occurs can vary
// from one system to another, and from one test run to another.
client.on('connect', common.mustCall(() => {
client.write(http2util.kClientMagic, () => {
for (let n = 0; n < 35000; n++)
client.write(kSettings.data);
});
}));

// An error event may or may not be emitted, depending on operating system
// and timing. We do not really care if one is emitted here or not, as the
// error on the server side is what we are testing for. Do not make this
// a common.mustCall() and there's no need to check the error details.
client.on('error', () => {});
}));

0 comments on commit 0954785

Please sign in to comment.