Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
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>
  • Loading branch information
danbev authored and BethGriggs committed Feb 22, 2021
1 parent 1ca3f5a commit afea10b
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 5 deletions.
25 changes: 24 additions & 1 deletion doc/api/http2.md
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down
31 changes: 27 additions & 4 deletions lib/internal/http2/core.js
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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' +
Expand Down Expand Up @@ -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;
Expand Down
33 changes: 33 additions & 0 deletions 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']
});
});

0 comments on commit afea10b

Please sign in to comment.