http: strictly forbid invalid characters from headers

PR-URL: https://github.com/nodejs/node-private/pull/26
Reviewed-By: Rod Vagg <r@va.gg>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

doc/api/http.markdown

@@ -649,8 +649,8 @@ response.addTrailers({'Content-MD5': '7895bf4b8828b55ceaf47747b4bca667'});
 response.end();
 ```
 
-Attempting to set a trailer field name that contains invalid characters will
+Attempting to set a header field name or value that contains invalid characters
-result in a [`TypeError`][] being thrown.
+will result in a [`TypeError`][] being thrown.
 
 ### response.end([data][, encoding][, callback])
 
@@ -721,8 +721,8 @@ or
 response.setHeader('Set-Cookie', ['type=ninja', 'language=javascript']);
 ```
 
-Attempting to set a header field name that contains invalid characters will
+Attempting to set a header field name or value that contains invalid characters
-result in a [`TypeError`][] being thrown.
+will result in a [`TypeError`][] being thrown.
 
 When headers have been set with [`response.setHeader()`][], they will be merged with
 any headers passed to [`response.writeHead()`][], with the headers passed to
@@ -836,8 +836,8 @@ response.writeHead(200, {
 This method must only be called once on a message and it must
 be called before [`response.end()`][] is called.
 
-If you call [`response.write()`][] or [`response.end()`][] before calling this, the
+If you call [`response.write()`][] or [`response.end()`][] before calling this,
-implicit/mutable headers will be calculated and call this function for you.
+the implicit/mutable headers will be calculated and call this function for you.
 
 When headers have been set with [`response.setHeader()`][], they will be merged with
 any headers passed to [`response.writeHead()`][], with the headers passed to
@@ -860,6 +860,9 @@ should be used to determine the number of bytes in a given encoding.
 And Node.js does not check whether Content-Length and the length of the body
 which has been transmitted are equal or not.
 
+Attempting to set a header field name or value that contains invalid characters
+will result in a [`TypeError`][] being thrown.
+
 ## Class: http.IncomingMessage
 
 An `IncomingMessage` object is created by [`http.Server`][] or

lib/_http_common.js

@@ -231,3 +231,20 @@ function checkIsHttpToken(val) {
   return typeof val === 'string' && token.test(val);
 }
 exports._checkIsHttpToken = checkIsHttpToken;
+
+/**
+ * True if val contains an invalid field-vchar
+ *  field-value    = *( field-content / obs-fold )
+ *  field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+ *  field-vchar    = VCHAR / obs-text
+ **/
+function checkInvalidHeaderChar(val) {
+  val = '' + val;
+  for (var i = 0; i < val.length; i++) {
+    const ch = val.charCodeAt(i);
+    if (ch === 9) continue;
+    if (ch <= 31 || ch > 255 || ch === 127) return true;
+  }
+  return false;
+}
+exports._checkInvalidHeaderChar = checkInvalidHeaderChar;

lib/_http_outgoing.js

@@ -305,8 +305,10 @@ function storeHeader(self, state, field, value) {
     throw new TypeError(
       'Header name must be a valid HTTP Token ["' + field + '"]');
   }
-  value = escapeHeaderValue(value);
+  if (common._checkInvalidHeaderChar(value) === true) {
-  state.messageHeader += field + ': ' + value + CRLF;
+    throw new TypeError('The header content contains invalid characters');
+  }
+  state.messageHeader += field + ': ' + escapeHeaderValue(value) + CRLF;
 
   if (connectionExpression.test(field)) {
     state.sentConnectionHeader = true;
@@ -341,8 +343,10 @@ OutgoingMessage.prototype.setHeader = function(name, value) {
   if (value === undefined)
     throw new Error('"value" required in setHeader("' + name + '", value)');
   if (this._header)
-    throw new Error('Can\'t set headers after they are sent');
+    throw new Error('Can\'t set headers after they are sent.');
-
+  if (common._checkInvalidHeaderChar(value) === true) {
+    throw new TypeError('The header content contains invalid characters');
+  }
   if (this._headers === null)
     this._headers = {};
 
@@ -515,6 +519,9 @@ OutgoingMessage.prototype.addTrailers = function(headers) {
       throw new TypeError(
         'Trailer name must be a valid HTTP Token ["' + field + '"]');
     }
+    if (common._checkInvalidHeaderChar(value) === true) {
+      throw new TypeError('The header content contains invalid characters');
+    }
     this._trailer += field + ': ' + escapeHeaderValue(value) + CRLF;
   }
 };

test/parallel/test-http-header-response-splitting.js

@@ -1,87 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-const http = require('http');
-
-var testIndex = 0;
-const testCount = 2 * 4 * 6;
-const responseBody = 'Hi mars!';
-
-var server = http.createServer(function(req, res) {
-  function reply(header) {
-    switch (testIndex % 4) {
-      case 0:
-        res.writeHead(200, { a: header, b: header });
-        break;
-      case 1:
-        res.setHeader('a', header);
-        res.setHeader('b', header);
-        res.writeHead(200);
-        break;
-      case 2:
-        res.setHeader('a', header);
-        res.writeHead(200, { b: header });
-        break;
-      case 3:
-        res.setHeader('a', [header]);
-        res.writeHead(200, { b: header });
-        break;
-      default:
-        assert.fail(null, null, 'unreachable');
-    }
-    res.write(responseBody);
-    if (testIndex % 8 < 4) {
-      res.addTrailers({ ta: header, tb: header });
-    } else {
-      res.addTrailers([['ta', header], ['tb', header]]);
-    }
-    res.end();
-  }
-  switch ((testIndex / 8) | 0) {
-    case 0:
-      reply('foo \r\ninvalid: bar');
-      break;
-    case 1:
-      reply('foo \ninvalid: bar');
-      break;
-    case 2:
-      reply('foo \rinvalid: bar');
-      break;
-    case 3:
-      reply('foo \n\n\ninvalid: bar');
-      break;
-    case 4:
-      reply('foo \r\n \r\n \r\ninvalid: bar');
-      break;
-    case 5:
-      reply('foo \r \n \r \n \r \ninvalid: bar');
-      break;
-    default:
-      assert(false);
-  }
-  if (++testIndex === testCount) {
-    server.close();
-  }
-});
-
-server.listen(common.PORT, common.mustCall(function() {
-  for (var i = 0; i < testCount; i++) {
-    http.get({ port: common.PORT, path: '/' }, common.mustCall(function(res) {
-      assert.strictEqual(res.headers.a, 'foo invalid: bar');
-      assert.strictEqual(res.headers.b, 'foo invalid: bar');
-      assert.strictEqual(res.headers.foo, undefined);
-      assert.strictEqual(res.headers.invalid, undefined);
-      var data = '';
-      res.setEncoding('utf8');
-      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();
-    }));
-  }
-}));

test/parallel/test-http-response-splitting.js

@@ -0,0 +1,55 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+const net = require('net');
+const url = require('url');
+const assert = require('assert');
+
+// Response splitting example, credit: Amit Klein, Safebreach
+const str = '/welcome?lang=bar%c4%8d%c4%8aContent­Length:%200%c4%8d%c4%8a%c' +
+            '4%8d%c4%8aHTTP/1.1%20200%20OK%c4%8d%c4%8aContent­Length:%202' +
+            '0%c4%8d%c4%8aLast­Modified:%20Mon,%2027%20Oct%202003%2014:50:18' +
+            '%20GMT%c4%8d%c4%8aContent­Type:%20text/html%c4%8d%c4%8a%c4%8' +
+            'd%c4%8a%3chtml%3eGotcha!%3c/html%3e';
+
+// Response splitting example, credit: Сковорода Никита Андреевич (@ChALkeR)
+const x = 'fooഊSet-Cookie: foo=barഊഊ<script>alert("Hi!")</script>';
+const y = 'foo⠊Set-Cookie: foo=bar';
+
+var count = 0;
+
+const server = http.createServer((req, res) => {
+  switch (count++) {
+    case 0:
+      const loc = url.parse(req.url, true).query.lang;
+      assert.throws(common.mustCall(() => {
+        res.writeHead(302, {Location: `/foo?lang=${loc}`});
+      }));
+      break;
+    case 1:
+      assert.throws(common.mustCall(() => {
+        res.writeHead(200, {'foo' : x});
+      }));
+      break;
+    case 2:
+      assert.throws(common.mustCall(() => {
+        res.writeHead(200, {'foo' : y});
+      }));
+      break;
+    default:
+      assert.fail(null, null, 'should not get to here.');
+  }
+  if (count === 3)
+    server.close();
+  res.end('ok');
+});
+server.listen(common.PORT, () => {
+  const end = 'HTTP/1.1\r\n\r\n';
+  const client = net.connect({port: common.PORT}, () => {
+    client.write(`GET ${str} ${end}`);
+    client.write(`GET / ${end}`);
+    client.write(`GET / ${end}`);
+    client.end();
+  });
+});