From 6a67dfd927fbeae4b7eab32eb8bc065d1062b62b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Jan 2018 13:34:45 -0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/18050 PR-URL: https://github.com/nodejs/node/pull/17969 Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski Reviewed-By: Matteo Collina --- src/node_http2.cc | 40 ++++++++++++- test/common/http2.js | 10 ++++ .../test-http2-ping-unsolicited-ack.js | 43 ++++++++++++++ .../test-http2-settings-unsolicited-ack.js | 50 +++++++++++++++++ test/sequential/sequential.status | 2 + test/sequential/test-http2-ping-flood.js | 56 +++++++++++++++++++ test/sequential/test-http2-settings-flood.js | 53 ++++++++++++++++++ 7 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-ping-unsolicited-ack.js create mode 100644 test/parallel/test-http2-settings-unsolicited-ack.js create mode 100644 test/sequential/test-http2-ping-flood.js create mode 100644 test/sequential/test-http2-settings-flood.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 85bdde9b92160f..404606189cbbd6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1305,8 +1305,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 = env()->context(); + Context::Scope context_scope(context); + + Local argv[1] = { + Integer::New(isolate, NGHTTP2_ERR_PROTO), + }; + MakeCallback(env()->error_string(), arraysize(argv), argv); + } } } @@ -1317,8 +1333,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 = env()->context(); + Context::Scope context_scope(context); + + Local 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); diff --git a/test/common/http2.js b/test/common/http2.js index 44f5f9191e6e33..1d4c269fffd5b5 100644 --- a/test/common/http2.js +++ b/test/common/http2.js @@ -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 diff --git a/test/parallel/test-http2-ping-unsolicited-ack.js b/test/parallel/test-http2-ping-unsolicited-ack.js new file mode 100644 index 00000000000000..5a3a261cb098b1 --- /dev/null +++ b/test/parallel/test-http2-ping-unsolicited-ack.js @@ -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', () => {}); +})); diff --git a/test/parallel/test-http2-settings-unsolicited-ack.js b/test/parallel/test-http2-settings-unsolicited-ack.js new file mode 100644 index 00000000000000..fa63e9ee3f6425 --- /dev/null +++ b/test/parallel/test-http2-settings-unsolicited-ack.js @@ -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); + })); +})); diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index cf4763b00129cc..b95db2a111ea67 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -11,6 +11,8 @@ test-inspector-async-call-stack : PASS, FLAKY test-inspector-bindings : PASS, FLAKY test-inspector-debug-end : PASS, FLAKY test-inspector-async-hook-setup-at-signal: PASS, FLAKY +test-http2-ping-flood : PASS, FLAKY +test-http2-settings-flood : PASS, FLAKY [$system==linux] diff --git a/test/sequential/test-http2-ping-flood.js b/test/sequential/test-http2-ping-flood.js new file mode 100644 index 00000000000000..5b47d51be9c5a8 --- /dev/null +++ b/test/sequential/test-http2-ping-flood.js @@ -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', () => {}); +})); diff --git a/test/sequential/test-http2-settings-flood.js b/test/sequential/test-http2-settings-flood.js new file mode 100644 index 00000000000000..bad4cec9a8d509 --- /dev/null +++ b/test/sequential/test-http2-settings-flood.js @@ -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', () => {}); +}));