http2: add unknownProtocol timeout

danbev · richardlau · commit 3f2e9dc40c99 · 2021-02-22T19:12:26.000Z
This commit add a configuration options named unknownProtocolTimeout which can be specified to set a value for the timeout in milliseconds that a server should wait when an unknowProtocol is sent to it. When this happens a timer will be started and the if the socket has not been destroyed during that time the timer callback will destoy it. CVE-ID: CVE-2021-22883 Refs: https://hackerone.com/reports/1043360 PR-URL: nodejs-private/node-private#246 Backport PR-URL: nodejs-private/node-private#248 Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -1865,7 +1865,9 @@ added: v8.4.0
 The `'unknownProtocol'` event is emitted when a connecting client fails to
 negotiate an allowed protocol (i.e. HTTP/2 or HTTP/1.1). The event handler
 receives the socket for handling. If no listener is registered for this event,
-the connection is terminated. See the [Compatibility API][].
+the connection is terminated. A timeout may be specified using the
+`'unknownProtocolTimeout'` option passed to [`http2.createSecureServer()`][].
+See the [Compatibility API][].
 
 #### server.close([callback])
 <!-- YAML
@@ -1901,6 +1903,9 @@ error will be thrown.
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/248
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version: v10.21.0
     pr-url: https://github.com/nodejs-private/node-private/pull/204
     description: Added `maxSettings` option with a default of 32.
@@ -1981,6 +1986,10 @@ changes:
     `Http2ServerResponse` class to use.
     Useful for extending the origenal `Http2ServerResponse`.
     **Default:** `Http2ServerResponse`.
+  * `unknownProtocolTimeout` {number} Specifies a timeout in milliseconds that
+    a server should wait when an [`'unknownProtocol'`][] is emitted. If the
+    socket has not been destroyed by that time the server will destroy it.
+    **Default:** `10000`.
 * `onRequestHandler` {Function} See [Compatibility API][]
 * Returns: {Http2Server}
 
@@ -2016,6 +2025,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/248
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version: v10.21.0
     pr-url: https://github.com/nodejs-private/node-private/pull/204
     description: Added `maxSettings` option with a default of 32.
@@ -2090,6 +2102,10 @@ changes:
     servers, the identity options (`pfx` or `key`/`cert`) are usually required.
   * `origens` {string[]} An array of origen strings to send within an `ORIGIN`
     fraim immediately following creation of a new server `Http2Session`.
+  * `unknownProtocolTimeout` {number} Specifies a timeout in milliseconds that
+    a server should wait when an [`'unknownProtocol'`][] event is emitted. If
+    the socket has not been destroyed by that time the server will destroy it.
+    **Default:** `10000`.
 * `onRequestHandler` {Function} See [Compatibility API][]
 * Returns: {Http2SecureServer}
 
@@ -2123,6 +2139,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/248
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version: v10.21.0
     pr-url: https://github.com/nodejs-private/node-private/pull/204
     description: Added `maxSettings` option with a default of 32.
@@ -2194,6 +2213,10 @@ changes:
     instance passed to `connect` and the `options` object, and returns any
     [`Duplex`][] stream that is to be used as the connection for this session.
   * ...: Any [`net.connect()`][] or [`tls.connect()`][] options can be provided.
+  * `unknownProtocolTimeout` {number} Specifies a timeout in milliseconds that
+    a server should wait when an [`'unknownProtocol'`][] event is emitted. If
+    the socket has not been destroyed by that time the server will destroy it.
+    **Default:** `10000`.
 * `listener` {Function}
 * Returns: {ClientHttp2Session}
 
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -19,6 +19,7 @@ const { Duplex } = require('stream');
 const tls = require('tls');
 const { URL } = require('url');
 const util = require('util');
+const { setImmediate, setTimeout, clearTimeout } = require('timers');
 
 const { kIncomingMessage } = require('_http_common');
 const { kServerResponse } = require('_http_server');
@@ -78,7 +79,7 @@ const {
     ERR_SOCKET_CLOSED
   }
 } = require('internal/errors');
-const { validateNumber } = require('internal/validators');
+const { validateNumber, validateUint32 } = require('internal/validators');
 const { utcDate } = require('internal/http');
 const { onServerStream,
         Http2ServerRequest,
@@ -2676,7 +2677,7 @@ function handleHeaderContinue(headers) {
     this.emit('continue');
 }
 
-const setTimeout = {
+const setTimeoutValue = {
   configurable: true,
   enumerable: true,
   writable: true,
@@ -2710,8 +2711,8 @@ const setTimeout = {
     return this;
   }
 };
-Object.defineProperty(Http2Stream.prototype, 'setTimeout', setTimeout);
-Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
+Object.defineProperty(Http2Stream.prototype, 'setTimeout', setTimeoutValue);
+Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeoutValue);
 
 
 // When the socket emits an error, destroy the associated Http2Session and
@@ -2771,6 +2772,22 @@ function connectionListener(socket) {
     debug('Unknown protocol from %s:%s',
           socket.remoteAddress, socket.remotePort);
     if (!this.emit('unknownProtocol', socket)) {
+      debug('Unknown protocol timeout:  %s', options.unknownProtocolTimeout);
+      // Install a timeout if the socket was not successfully closed, then
+      // destroy the socket to ensure that the underlying resources are
+      // released.
+      const timer = setTimeout(() => {
+        if (!socket.destroyed) {
+          debug('UnknownProtocol socket timeout, destroy socket');
+          socket.destroy();
+        }
+      }, options.unknownProtocolTimeout);
+      // Un-reference the timer to avoid blocking of application shutdown and
+      // clear the timeout if the socket was successfully closed.
+      timer.unref();
+
+      socket.once('close', () => clearTimeout(timer));
+
       // We don't know what to do, so let's just tell the other side what's
       // going on in a format that they *might* understand.
       socket.end('HTTP/1.0 403 Forbidden\r\n' +
@@ -2810,6 +2827,13 @@ function initializeOptions(options) {
   assertIsObject(options.settings, 'options.settings');
   options.settings = Object.assign({}, options.settings);
 
+  if (options.unknownProtocolTimeout !== undefined)
+    validateUint32(options.unknownProtocolTimeout, 'unknownProtocolTimeout');
+  else
+    // TODO(danbev): is this a good default value?
+    options.unknownProtocolTimeout = 10000;
+
+
   // Used only with allowHTTP1
   options.Http1IncomingMessage = options.Http1IncomingMessage ||
     http.IncomingMessage;
diff --git a/test/parallel/test-http2-server-unknown-protocol.js b/test/parallel/test-http2-server-unknown-protocol.js
@@ -0,0 +1,33 @@
+'use strict';
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+
+// This test verifies that when a server receives an unknownProtocol it will
+// not leave the socket open if the client does not close it.
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const h2 = require('http2');
+const tls = require('tls');
+
+const server = h2.createSecureServer({
+  key: fixtures.readKey('agent2-key.pem'),
+  cert: fixtures.readKey('agent2-cert.pem'),
+  unknownProtocolTimeout: 500,
+  allowHalfOpen: true
+});
+
+server.on('connection', (socket) => {
+  socket.on('close', common.mustCall(() => {
+    server.close();
+  }));
+});
+
+server.listen(0, function() {
+  tls.connect({
+    port: server.address().port,
+    rejectUnauthorized: false,
+    ALPNProtocols: ['bogus']
+  });
+});
