diff options
author | Matteo Collina <hello@matteocollina.com> | 2018-08-23 17:46:07 +0300 |
---|---|---|
committer | Rod Vagg <rod@vagg.org> | 2018-11-27 07:07:09 +0300 |
commit | 696f063c5e9157fd10859515da00fd8bd190d76d (patch) | |
tree | fa1d77499696773138310b3bbf6c93065e38534d | |
parent | 93dba83fb0fb46ee2ea87163f435392490b4d59b (diff) |
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/151
Ref: 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 | 2 | ||||
-rw-r--r-- | lib/internal/http.js | 33 | ||||
-rw-r--r-- | test/async-hooks/test-graph.http.js | 2 | ||||
-rw-r--r-- | test/parallel/test-http-slow-headers.js | 56 | ||||
-rw-r--r-- | test/parallel/test-https-slow-headers.js | 69 |
8 files changed, 199 insertions, 12 deletions
diff --git a/doc/api/http.md b/doc/api/http.md index 9b0be0b61bc..9dc46b13f34 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -882,6 +882,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 ee023810f80..f6c0da5cbb0 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -36,6 +36,12 @@ See [`server.close()`][`http.close()`] from the HTTP module for details. Starts the HTTPS server listening for encrypted connections. This method is identical to [`server.listen()`][] from [`net.Server`][]. +### server.headersTimeout + +- {number} **Default:** `40000` + +See [`http.Server#headersTimeout`][]. + ### server.setTimeout([msecs][, callback]) <!-- YAML added: v0.11.2 @@ -253,6 +259,7 @@ const req = https.request(options, (res) => { [`URL`]: url.html#url_the_whatwg_url_api [`http.Agent`]: http.html#http_class_http_agent [`http.Server#keepAliveTimeout`]: http.html#http_server_keepalivetimeout +[`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 b95736d1ea6..a015213c76b 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 @@ -296,6 +296,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); @@ -334,6 +335,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; parser.incoming = null; @@ -475,7 +479,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); } @@ -579,6 +596,9 @@ function resOnFinish(req, res, socket, state, server) { 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; + state.incoming.push(req); // If the writable end isn't consuming, then stop reading diff --git a/lib/https.js b/lib/https.js index a2aea08ac9c..04710a07bdf 100644 --- a/lib/https.js +++ b/lib/https.js @@ -70,6 +70,8 @@ function Server(opts, requestListener) { this.timeout = 2 * 60 * 1000; this.keepAliveTimeout = 5000; + + this.headersTimeout = 40 * 1000; // 40 seconds } inherits(Server, tls.Server); exports.Server = Server; diff --git a/lib/internal/http.js b/lib/internal/http.js index 71e32498f35..f45a79343f8 100644 --- a/lib/internal/http.js +++ b/lib/internal/http.js @@ -2,18 +2,30 @@ const timers = require('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(); - timers.enroll(utcDate, 1000 - d.getMilliseconds()); - timers._unrefActive(utcDate); - } - return dateCache; + if (!utcCache) cache(); + return utcCache; } -utcDate._onTimeout = function() { - dateCache = undefined; + +function cache() { + const d = new Date(); + nowCache = d.valueOf(); + utcCache = d.toUTCString(); + timers.enroll(cache, 1000 - d.getMilliseconds()); + timers._unrefActive(cache); +} + +cache._onTimeout = function() { + nowCache = undefined; + utcCache = undefined; }; function ondrain() { @@ -23,5 +35,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 4f787d43e82..48b3ef0be4c 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -46,7 +46,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 00000000000..54c0310e91f --- /dev/null +++ b/test/parallel/test-http-slow-headers.js @@ -0,0 +1,56 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { createServer } = require('http'); +const { connect } = require('net'); + +// 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(); + + const onClose = common.mustCall(() => { + client.removeListener('close', onClose); + client.removeListener('error', onClose); + client.removeListener('end', onClose); + clearInterval(interval); + server.close(); + }); + + client.on('error', onClose); + client.on('close', onClose); + client.on('end', onClose); +})); diff --git a/test/parallel/test-https-slow-headers.js b/test/parallel/test-https-slow-headers.js new file mode 100644 index 00000000000..83e42a94907 --- /dev/null +++ b/test/parallel/test-https-slow-headers.js @@ -0,0 +1,69 @@ +'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'); + +// 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(); + + const onClose = common.mustCall(() => { + client.removeListener('close', onClose); + client.removeListener('error', onClose); + client.removeListener('end', onClose); + clearInterval(interval); + server.close(); + }); + + client.on('error', onClose); + client.on('close', onClose); + client.on('end', onClose); +})); |