http2: add unknownProtocol timeout

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
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>

doc/api/http2.md

@@ -2048,7 +2048,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
@@ -2120,6 +2122,9 @@ Throws `ERR_INVALID_ARG_TYPE` for invalid `settings` argument.
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/246
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2226,6 +2231,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}
@@ -2262,6 +2271,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/246
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2358,6 +2370,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}
 
@@ -2391,6 +2407,9 @@ server.listen(80);
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/246
+    description: Added `unknownProtocolTimeout` option with a default of 10000.
   - version:
      - v14.4.0
      - v12.18.0
@@ -2474,6 +2493,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}

lib/internal/http2/core.js

@@ -48,7 +48,7 @@ const { URL } = require('internal/url');
 const net = require('net');
 const { Duplex } = require('stream');
 const tls = require('tls');
-const { setImmediate } = require('timers');
+const { setImmediate, setTimeout, clearTimeout } = require('timers');
 
 const {
   kIncomingMessage,
@@ -2899,14 +2899,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
@@ -2966,6 +2966,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' +
@@ -3011,6 +3027,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;

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']
+  });
+});