diff options
author | Matteo Collina <hello@matteocollina.com> | 2018-08-23 16:46:07 +0200 |
---|---|---|
committer | Rod Vagg <rod@vagg.org> | 2018-11-28 11:36:34 +1100 |
commit | ee618a7ab239c98d945c723a4e225bc409151736 (patch) | |
tree | b70be2ea28bb3773d6c455a61a273cf8c5edbfb8 | |
parent | 7bfcfc2ffe4940898cf7b70890a55eb91cbdd112 (diff) | |
download | android-node-v8-ee618a7ab239c98d945c723a4e225bc409151736.tar.gz android-node-v8-ee618a7ab239c98d945c723a4e225bc409151736.tar.bz2 android-node-v8-ee618a7ab239c98d945c723a4e225bc409151736.zip |
http,https: protect against slow headers attack
CVE-2018-12122
An attacker can send a char/s within headers and exahust the resources
(file descriptors) of a system even with a tight max header length
protection. This PR destroys a socket if it has not received the headers
in 40s.
PR-URL: https://github.com/nodejs-private/node-private/pull/144
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
-rw-r--r-- | doc/api/http.md | 20 | ||||
-rw-r--r-- | doc/api/https.md | 7 | ||||
-rw-r--r-- | lib/_http_server.js | 22 | ||||
-rw-r--r-- | lib/https.js | 1 | ||||
-rw-r--r-- | lib/internal/http.js | 27 | ||||
-rw-r--r-- | test/async-hooks/test-graph.http.js | 2 | ||||
-rw-r--r-- | test/parallel/test-http-slow-headers.js | 50 | ||||
-rw-r--r-- | test/parallel/test-https-slow-headers.js | 63 |
8 files changed, 182 insertions, 10 deletions
diff --git a/doc/api/http.md b/doc/api/http.md index 1c6b5717e0..13373debb4 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -958,6 +958,26 @@ added: v0.7.0 Limits maximum incoming headers count. If set to 0, no limit will be applied. +### server.headersTimeout +<!-- YAML +added: REPLACEME +--> + +* {number} **Default:** `40000` + +Limit the amount of time the parser will wait to receive the complete HTTP +headers. + +In case of inactivity, the rules defined in [server.timeout][] apply. However, +that inactivity based timeout would still allow the connection to be kept open +if the headers are being sent very slowly (by default, up to a byte per 2 +minutes). In order to prevent this, whenever header data arrives an additional +check is made that more than `server.headersTimeout` milliseconds has not +passed since the connection was established. If the check fails, a `'timeout'` +event is emitted on the server object, and (by default) the socket is destroyed. +See [server.timeout][] for more information on how timeout behaviour can be +customised. + ### server.setTimeout([msecs][, callback]) <!-- YAML added: v0.9.12 diff --git a/doc/api/https.md b/doc/api/https.md index 777fbab741..81a5bcce93 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -44,6 +44,12 @@ This method is identical to [`server.listen()`][] from [`net.Server`][]. See [`http.Server#maxHeadersCount`][]. +### server.headersTimeout + +- {number} **Default:** `40000` + +See [`http.Server#headersTimeout`][]. + ### server.setTimeout([msecs][, callback]) <!-- YAML added: v0.11.2 @@ -363,6 +369,7 @@ headers: max-age=0; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; p [`http.Agent`]: http.html#http_class_http_agent [`http.Server#keepAliveTimeout`]: http.html#http_server_keepalivetimeout [`http.Server#maxHeadersCount`]: http.html#http_server_maxheaderscount +[`http.Server#headersTimeout`]: http.html#http_server_headerstimeout [`http.Server#setTimeout()`]: http.html#http_server_settimeout_msecs_callback [`http.Server#timeout`]: http.html#http_server_timeout [`http.Server`]: http.html#http_class_http_server diff --git a/lib/_http_server.js b/lib/_http_server.js index 96f05f5819..c171b1d3e7 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -37,7 +37,7 @@ const { _checkInvalidHeaderChar: checkInvalidHeaderChar } = require('_http_common'); const { OutgoingMessage } = require('_http_outgoing'); -const { outHeadersKey, ondrain } = require('internal/http'); +const { outHeadersKey, ondrain, nowDate } = require('internal/http'); const { defaultTriggerAsyncIdScope, getOrSetAsyncId @@ -306,6 +306,7 @@ function Server(options, requestListener) { this.keepAliveTimeout = 5000; this._pendingResponseData = 0; this.maxHeadersCount = null; + this.headersTimeout = 40 * 1000; // 40 seconds } util.inherits(Server, net.Server); @@ -344,6 +345,9 @@ function connectionListenerInternal(server, socket) { var parser = parsers.alloc(); parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]); parser.socket = socket; + + // We are starting to wait for our headers. + parser.parsingHeadersStart = nowDate(); socket.parser = parser; // Propagate headers limit from server instance to parser @@ -481,7 +485,20 @@ function socketOnData(server, socket, parser, state, d) { function onParserExecute(server, socket, parser, state, ret) { socket._unrefTimer(); + const start = parser.parsingHeadersStart; debug('SERVER socketOnParserExecute %d', ret); + + // If we have not parsed the headers, destroy the socket + // after server.headersTimeout to protect from DoS attacks. + // start === 0 means that we have parsed headers. + if (start !== 0 && nowDate() - start > server.headersTimeout) { + const serverTimeout = server.emit('timeout', socket); + + if (!serverTimeout) + socket.destroy(); + return; + } + onParserExecuteCommon(server, socket, parser, state, ret, undefined); } @@ -598,6 +615,9 @@ function emitCloseNT(self) { function parserOnIncoming(server, socket, state, req, keepAlive) { resetSocketTimeout(server, socket, state); + // Set to zero to communicate that we have finished parsing. + socket.parser.parsingHeadersStart = 0; + if (req.upgrade) { req.upgrade = req.method === 'CONNECT' || server.listenerCount('upgrade') > 0; diff --git a/lib/https.js b/lib/https.js index 66e76c1f05..0854c3d440 100644 --- a/lib/https.js +++ b/lib/https.js @@ -74,6 +74,7 @@ function Server(opts, requestListener) { this.timeout = 2 * 60 * 1000; this.keepAliveTimeout = 5000; this.maxHeadersCount = null; + this.headersTimeout = 40 * 1000; // 40 seconds } inherits(Server, tls.Server); diff --git a/lib/internal/http.js b/lib/internal/http.js index 2b9c948aee..47a51fb739 100644 --- a/lib/internal/http.js +++ b/lib/internal/http.js @@ -2,19 +2,29 @@ const { setUnrefTimeout } = require('internal/timers'); -var dateCache; +var nowCache; +var utcCache; + +function nowDate() { + if (!nowCache) cache(); + return nowCache; +} + function utcDate() { - if (!dateCache) { - const d = new Date(); - dateCache = d.toUTCString(); + if (!utcCache) cache(); + return utcCache; +} - setUnrefTimeout(resetCache, 1000 - d.getMilliseconds()); - } - return dateCache; +function cache() { + const d = new Date(); + nowCache = d.valueOf(); + utcCache = d.toUTCString(); + setUnrefTimeout(resetCache, 1000 - d.getMilliseconds()); } function resetCache() { - dateCache = undefined; + nowCache = undefined; + utcCache = undefined; } function ondrain() { @@ -24,5 +34,6 @@ function ondrain() { module.exports = { outHeadersKey: Symbol('outHeadersKey'), ondrain, + nowDate, utcDate }; diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index 414ebabeee..3461974ef9 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -45,7 +45,7 @@ process.on('exit', function() { triggerAsyncId: 'tcp:2' }, { type: 'Timeout', id: 'timeout:2', - triggerAsyncId: 'httpparser:2' }, + triggerAsyncId: 'tcp:2' }, { type: 'SHUTDOWNWRAP', id: 'shutdown:1', triggerAsyncId: 'tcp:2' } ] diff --git a/test/parallel/test-http-slow-headers.js b/test/parallel/test-http-slow-headers.js new file mode 100644 index 0000000000..6da11465da --- /dev/null +++ b/test/parallel/test-http-slow-headers.js @@ -0,0 +1,50 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); +const { finished } = require('stream'); + +// This test validates that the 'timeout' event fires +// after server.headersTimeout. + +const headers = + 'GET / HTTP/1.1\r\n' + + 'Host: localhost\r\n' + + 'Agent: node\r\n'; + +const server = createServer(common.mustNotCall()); +let sendCharEvery = 1000; + +// 40 seconds is the default +assert.strictEqual(server.headersTimeout, 40 * 1000); + +// Pass a REAL env variable to shortening up the default +// value which is 40s otherwise this is useful for manual +// testing +if (!process.env.REAL) { + sendCharEvery = common.platformTimeout(10); + server.headersTimeout = 2 * sendCharEvery; +} + +server.once('timeout', common.mustCall((socket) => { + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = connect(server.address().port); + client.write(headers); + client.write('X-CRASH: '); + + const interval = setInterval(() => { + client.write('a'); + }, sendCharEvery); + + client.resume(); + + finished(client, common.mustCall((err) => { + clearInterval(interval); + server.close(); + })); +})); diff --git a/test/parallel/test-https-slow-headers.js b/test/parallel/test-https-slow-headers.js new file mode 100644 index 0000000000..2a55850b5d --- /dev/null +++ b/test/parallel/test-https-slow-headers.js @@ -0,0 +1,63 @@ +'use strict'; + +const common = require('../common'); +const { readKey } = require('../common/fixtures'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { createServer } = require('https'); +const { connect } = require('tls'); +const { finished } = require('stream'); + +// This test validates that the 'timeout' event fires +// after server.headersTimeout. + +const headers = + 'GET / HTTP/1.1\r\n' + + 'Host: localhost\r\n' + + 'Agent: node\r\n'; + +const server = createServer({ + key: readKey('agent1-key.pem'), + cert: readKey('agent1-cert.pem'), + ca: readKey('ca1-cert.pem'), +}, common.mustNotCall()); + +let sendCharEvery = 1000; + +// 40 seconds is the default +assert.strictEqual(server.headersTimeout, 40 * 1000); + +// pass a REAL env variable to shortening up the default +// value which is 40s otherwise +// this is useful for manual testing +if (!process.env.REAL) { + sendCharEvery = common.platformTimeout(10); + server.headersTimeout = 2 * sendCharEvery; +} + +server.once('timeout', common.mustCall((socket) => { + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = connect({ + port: server.address().port, + rejectUnauthorized: false + }); + client.write(headers); + client.write('X-CRASH: '); + + const interval = setInterval(() => { + client.write('a'); + }, sendCharEvery); + + client.resume(); + + finished(client, common.mustCall((err) => { + clearInterval(interval); + server.close(); + })); +})); |