From eb43bc04b1390ce2506144b46d081e63f7a7d5b7 Mon Sep 17 00:00:00 2001
From: Matteo Collina <hello@matteocollina.com>
Date: Thu, 23 Aug 2018 16:46:07 +0200
Subject: [PATCH] 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/150
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>
---
 doc/api/http.md                          | 20 ++++++++
 doc/api/https.md                         |  7 +++
 lib/_http_server.js                      | 22 ++++++++-
 lib/https.js                             |  1 +
 lib/internal/http.js                     | 27 +++++++---
 test/async-hooks/test-graph.http.js      |  4 +-
 test/parallel/test-http-slow-headers.js  | 50 +++++++++++++++++++
 test/parallel/test-https-slow-headers.js | 63 ++++++++++++++++++++++++
 8 files changed, 183 insertions(+), 11 deletions(-)
 create mode 100644 test/parallel/test-http-slow-headers.js
 create mode 100644 test/parallel/test-https-slow-headers.js

diff --git a/doc/api/http.md b/doc/api/http.md
index c34e6cded4d5a7..a06d0e44bf9e83 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -941,6 +941,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 e01606042ee557..21317f83a4486a 100644
--- a/doc/api/https.md
+++ b/doc/api/https.md
@@ -43,6 +43,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
@@ -360,6 +366,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 8e0cbfbf266878..db12980697b1dd 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
@@ -303,6 +303,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);
 
@@ -341,6 +342,9 @@ function connectionListenerInternal(server, socket) {
   var parser = parsers.alloc();
   parser.reinitialize(HTTPParser.REQUEST);
   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
@@ -478,7 +482,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);
 }
 
@@ -589,6 +606,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;
+
   if (req.upgrade) {
     req.upgrade = req.method === 'CONNECT' ||
                   server.listenerCount('upgrade') > 0;
diff --git a/lib/https.js b/lib/https.js
index 15970c182ea56f..c1053da1a13eb6 100644
--- a/lib/https.js
+++ b/lib/https.js
@@ -75,6 +75,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 2b9c948aeefb30..47a51fb7394947 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 eea72ca3bac72c..05f585832f5d11 100644
--- a/test/async-hooks/test-graph.http.js
+++ b/test/async-hooks/test-graph.http.js
@@ -52,10 +52,10 @@ process.on('exit', function() {
         triggerAsyncId: 'tcp:2' },
       { type: 'Timeout',
         id: 'timeout:2',
-        triggerAsyncId: 'httpparser:4' },
+        triggerAsyncId: 'tcp:2' },
       { type: 'TIMERWRAP',
         id: 'timer:2',
-        triggerAsyncId: 'httpparser:4' },
+        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 00000000000000..6da11465dac17b
--- /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 00000000000000..2a55850b5de847
--- /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();
+  }));
+}));