http: guard against response splitting in trailers

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>

diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index c9b0a87..0522ecb 100644 (file)
--- 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 c89497f..ad8cc9c 100644 (file)
--- 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();
     }));