diff options
author | Daniel Bevenius <daniel.bevenius@gmail.com> | 2021-01-22 12:34:21 +0100 |
---|---|---|
committer | Richard Lau <rlau@redhat.com> | 2021-02-22 19:12:26 +0000 |
commit | 3f2e9dc40c9964965b075c00719829f9bb17e65f (patch) | |
tree | 7f0bd6c424b403b2b87484721e0f97da33c120ab | |
parent | d1cf6a9b0f74d587dea1d0f194d922ff94eddd06 (diff) | |
download | node-new-3f2e9dc40c9964965b075c00719829f9bb17e65f.tar.gz |
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.
CVE-ID: CVE-2021-22883
Refs: https://hackerone.com/reports/1043360
PR-URL: https://github.com/nodejs-private/node-private/pull/246
Backport PR-URL: https://github.com/nodejs-private/node-private/pull/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>
-rw-r--r-- | doc/api/http2.md | 25 | ||||
-rw-r--r-- | lib/internal/http2/core.js | 32 | ||||
-rw-r--r-- | test/parallel/test-http2-server-unknown-protocol.js | 33 |
3 files changed, 85 insertions, 5 deletions
diff --git a/doc/api/http2.md b/doc/api/http2.md index 640776f5e8..7a4fbf56e5 100644 --- 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 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`. * `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. * `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} @@ -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 index bb9e43ca8e..9c5d7376a4 100644 --- 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 new file mode 100644 index 0000000000..2c7aea54d5 --- /dev/null +++ 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'] + }); +}); |