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
Backport-PR-URL: https://github.com/nodejs/node/pull/20456
PR-URL: https://github.com/nodejs/node/pull/17969
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

7 files changed, 252 insertions, 2 deletions

diff --git a/src/node_http2.cc b/src/node_http2.cc
index 2ead727b11..844421fa35 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -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);
+    }
   }
 }
 
@@ -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);
diff --git a/test/common/http2.js b/test/common/http2.js
index 44f5f9191e..1d4c269fff 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 0000000000..5a3a261cb0
--- /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 0000000000..fa63e9ee3f
--- /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 656eb80c4d..5e39392e65 100644
--- a/test/sequential/sequential.status
+++ b/test/sequential/sequential.status
@@ -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]
 
diff --git a/test/sequential/test-http2-ping-flood.js b/test/sequential/test-http2-ping-flood.js
new file mode 100644
index 0000000000..5b47d51be9
--- /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 0000000000..bad4cec9a8
--- /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', () => {});
+}));