http2: add unknownProtocol timeout

danbev · BethGriggs · commit afea10b09785 · 2021-02-22T18:25:10.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. Refs: https://hackerone.com/reports/1043360 CVE-ID: CVE-2021-22883 PR-URL: nodejs-private/node-private#246 Backport-PR-URL: nodejs-private/node-private#250 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
@@ -1993,7 +1993,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
@@ -2052,6 +2054,9 @@ value only affects new connections to the server, not any existing connections.
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/250
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2158,6 +2163,10 @@ changes:
     `Http2ServerResponse` class to use.
     Useful for extending the original `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`.
   * ...: Any [`net.createServer()`][] option can be provided.
 * `onRequestHandler` {Function} See [Compatibility API][]
 * Returns: {Http2Server}
@@ -2194,6 +2203,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/250
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2290,6 +2302,10 @@ changes:
     servers, the identity options (`pfx` or `key`/`cert`) are usually required.
   * `origins` {string[]} An array of origin strings to send within an `ORIGIN`
     frame 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}
 
@@ -2323,6 +2339,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/250
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2406,6 +2425,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} Will be registered as a one-time listener of the
   [`'connect'`][] event.
 * Returns: {ClientHttp2Session}
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -35,7 +35,7 @@ const net = require('net');
 const { Duplex } = require('stream');
 const tls = require('tls');
 const { URL } = require('url');
-const { setImmediate } = require('timers');
+const { setImmediate, setTimeout, clearTimeout } = require('timers');
 
 const { kIncomingMessage } = require('_http_common');
 const { kServerResponse } = require('_http_server');
@@ -2810,14 +2810,14 @@ function handleHeaderContinue(headers) {
     this.emit('continue');
 }
 
-const setTimeout = {
+const setTimeoutValue = {
   configurable: true,
   enumerable: true,
   writable: true,
   value: setStreamTimeout
 };
-ObjectDefineProperty(Http2Stream.prototype, 'setTimeout', setTimeout);
-ObjectDefineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
+ObjectDefineProperty(Http2Stream.prototype, 'setTimeout', setTimeoutValue);
+ObjectDefineProperty(Http2Session.prototype, 'setTimeout', setTimeoutValue);
 
 
 // When the socket emits an error, destroy the associated Http2Session and
@@ -2877,6 +2877,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' +
@@ -2922,6 +2938,13 @@ function initializeOptions(options) {
     );
   }
 
+  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('rsa_private.pem'),
+  cert: fixtures.readKey('rsa_cert.crt'),
+  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']
+  });
+});
