From e68a119c180679631f140c3c9db9e9ff15750ca9 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Fri, 18 Sep 2015 05:06:35 +0200
Subject: [PATCH] http: guard against response splitting in trailers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 3c293ba ("http: protect against response splitting attacks")
filters out newline characters from HTTP headers but forgot to apply
the same logic to trailing HTTP headers, i.e., headers that come after
the response body.  This commit rectifies that.

The expected security impact is low because approximately no one uses
trailing headers.  Some HTTP clients can't even parse them.

PR-URL: https://github.com/nodejs/node/pull/2945
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Rod Vagg <r@va.gg>
---
 lib/_http_outgoing.js                            | 15 +++++++++------
 .../test-http-header-response-splitting.js       | 16 +++++++++++++---
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index c9b0a87b3d..0522ecb2a2 100644
--- a/lib/_http_outgoing.js
+++ b/lib/_http_outgoing.js
@@ -295,11 +295,7 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) {
 };
 
 function storeHeader(self, state, field, value) {
-  // Protect against response splitting. The if statement is there to
-  // minimize the performance impact in the common case.
-  if (/[\r\n]/.test(value))
-    value = value.replace(/[\r\n]+[ \t]*/g, '');
-
+  value = escapeHeaderValue(value);
   state.messageHeader += field + ': ' + value + CRLF;
 
   if (connectionExpression.test(field)) {
@@ -481,6 +477,13 @@ function connectionCorkNT(conn) {
 }
 
 
+function escapeHeaderValue(value) {
+  // Protect against response splitting. The regex test is there to
+  // minimize the performance impact in the common case.
+  return /[\r\n]/.test(value) ? value.replace(/[\r\n]+[ \t]*/g, '') : value;
+}
+
+
 OutgoingMessage.prototype.addTrailers = function(headers) {
   this._trailer = '';
   var keys = Object.keys(headers);
@@ -496,7 +499,7 @@ OutgoingMessage.prototype.addTrailers = function(headers) {
       value = headers[key];
     }
 
-    this._trailer += field + ': ' + value + CRLF;
+    this._trailer += field + ': ' + escapeHeaderValue(value) + CRLF;
   }
 };
 
diff --git a/test/parallel/test-http-header-response-splitting.js b/test/parallel/test-http-header-response-splitting.js
index c89497f589..ad8cc9c5ba 100644
--- a/test/parallel/test-http-header-response-splitting.js
+++ b/test/parallel/test-http-header-response-splitting.js
@@ -4,7 +4,7 @@ var common = require('../common'),
     http = require('http');
 
 var testIndex = 0;
-const testCount = 4 * 6;
+const testCount = 2 * 4 * 6;
 const responseBody = 'Hi mars!';
 
 var server = http.createServer(function(req, res) {
@@ -29,9 +29,15 @@ var server = http.createServer(function(req, res) {
     default:
       assert.fail('unreachable');
     }
-    res.end(responseBody);
+    res.write(responseBody);
+    if (testIndex % 8 < 4) {
+      res.addTrailers({ ta: header, tb: header });
+    } else {
+      res.addTrailers([['ta', header], ['tb', header]]);
+    }
+    res.end();
   }
-  switch ((testIndex / 4) | 0) {
+  switch ((testIndex / 8) | 0) {
     case 0:
       reply('foo \r\ninvalid: bar');
       break;
@@ -70,6 +76,10 @@ server.listen(common.PORT, common.mustCall(function() {
       res.on('data', function(s) { data += s; });
       res.on('end', common.mustCall(function() {
         assert.equal(data, responseBody);
+        assert.strictEqual(res.trailers.ta, 'foo invalid: bar');
+        assert.strictEqual(res.trailers.tb, 'foo invalid: bar');
+        assert.strictEqual(res.trailers.foo, undefined);
+        assert.strictEqual(res.trailers.invalid, undefined);
       }));
       res.resume();
     }));