From f542e74c93c66c203628bb4c53bc527949203377 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 c9b0a87b3dbd0c..0522ecb2a276f2 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 c89497f5896c4e..ad8cc9c5ba8e8f 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();
     }));