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: #2945
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>

Author: bnoordhuis

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;
   }
 };
 

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();
     }));