From 5c94624a25aad29520a27d6afe61e9698fe0295b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Feb 2016 23:19:34 -0800 Subject: [PATCH] http: strictly forbid invalid characters from headers PR-URL: https://github.com/nodejs/node-private/pull/22 --- doc/api/http.markdown | 8 +++ lib/http.js | 67 ++++++++++++++++++--- test/simple/test-http-response-splitting.js | 53 ++++++++++++++++ 3 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 test/simple/test-http-response-splitting.js diff --git a/doc/api/http.markdown b/doc/api/http.markdown index 7186a4b8d9..d6d3d0b5a6 100644 --- a/doc/api/http.markdown +++ b/doc/api/http.markdown @@ -278,6 +278,9 @@ should be used to determine the number of bytes in a given encoding. And Node 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. + ### response.setTimeout(msecs, callback) * `msecs` {Number} @@ -320,6 +323,9 @@ or response.setHeader("Set-Cookie", ["type=ninja", "language=javascript"]); +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. + ### response.headersSent Boolean (read-only). True if headers were sent, false otherwise. @@ -394,6 +400,8 @@ emit trailers, with a list of the header fields in its value. E.g., response.addTrailers({'Content-MD5': "7895bf4b8828b55ceaf47747b4bca667"}); response.end(); +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. ### response.end([data], [encoding]) diff --git a/lib/http.js b/lib/http.js index 8575781499..3175992fc4 100644 --- a/lib/http.js +++ b/lib/http.js @@ -30,6 +30,36 @@ var FreeList = require('freelist').FreeList; var HTTPParser = process.binding('http_parser').HTTPParser; var assert = require('assert').ok; +var lenientHttpHeaders = !!process.REVERT_CVE_2016_2216; + +function escapeHeaderValue(value) { + if (!lenientHttpHeaders) return 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; +} + +// Verifies that the given val is a valid HTTP token +// per the rules defined in RFC 7230 +var token = /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/; +function _checkIsHttpToken(val) { + return typeof val === 'string' && token.test(val); +} + +// 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++) { + var ch = val.charCodeAt(i); + if (ch === 9) continue; + if (ch <= 31 || ch > 255 || ch === 127) return true; + } + return false; +} + var debug; if (process.env.NODE_DEBUG && /http/.test(process.env.NODE_DEBUG)) { debug = function(x) { console.error('HTTP: %s', x); }; @@ -652,12 +682,16 @@ 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, ''); - - state.messageHeader += field + ': ' + value + CRLF; + if (!lenientHttpHeaders) { + if (!_checkIsHttpToken(field)) { + throw new TypeError( + 'Header name must be a valid HTTP Token ["' + field + '"]'); + } + if (_checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } + } + state.messageHeader += field + ': ' + escapeHeaderValue(value) + CRLF; if (connectionExpression.test(field)) { state.sentConnectionHeader = true; @@ -690,6 +724,16 @@ OutgoingMessage.prototype.setHeader = function(name, value) { throw new Error('Can\'t set headers after they are sent.'); } + if (!lenientHttpHeaders) { + if (!_checkIsHttpToken(name)) { + throw new TypeError( + 'Trailer name must be a valid HTTP Token ["' + name + '"]'); + } + if (_checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } + } + var key = name.toLowerCase(); this._headers = this._headers || {}; this._headerNames = this._headerNames || {}; @@ -902,7 +946,16 @@ OutgoingMessage.prototype.addTrailers = function(headers) { value = headers[key]; } - this._trailer += field + ': ' + value + CRLF; + if (!lenientHttpHeaders) { + if (!_checkIsHttpToken(field)) { + throw new TypeError( + 'Trailer name must be a valid HTTP Token ["' + field + '"]'); + } + if (_checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } + } + this._trailer += field + ': ' + escapeHeaderValue(value) + CRLF; } }; diff --git a/test/simple/test-http-response-splitting.js b/test/simple/test-http-response-splitting.js new file mode 100644 index 0000000000..bc9e31c054 --- /dev/null +++ b/test/simple/test-http-response-splitting.js @@ -0,0 +1,53 @@ +var common = require('../common'); +var http = require('http'); +var net = require('net'); +var url = require('url'); +var assert = require('assert'); + +// Response splitting example, credit: Amit Klein, Safebreach +var 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) +var x = 'fooഊSet-Cookie: foo=barഊഊ'; +var y = 'foo⠊Set-Cookie: foo=bar'; + +var count = 0; + +var server = http.createServer(function(req, res) { + switch (count++) { + case 0: + var loc = url.parse(req.url, true).query.lang; + assert.throws(common.mustCall(function() { + res.writeHead(302, {Location: '/foo?lang=' + loc}); + })); + break; + case 1: + assert.throws(common.mustCall(function() { + res.writeHead(200, {'foo' : x}); + })); + break; + case 2: + assert.throws(common.mustCall(function() { + 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, function() { + var end = 'HTTP/1.1\r\n\r\n'; + var client = net.connect({port: common.PORT}, function() { + client.write('GET ' + str + ' ' + end); + client.write('GET / ' + end); + client.write('GET / ' + end); + client.end(); + }); +}); \ No newline at end of file