From cf2b714b029db496512521d4f9f02c51d86c001b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Feb 2016 17:32:23 -0800 Subject: [PATCH] http: strictly forbid invalid characters from headers PR-URL: https://github.com/nodejs/node-private/pull/20 --- doc/api/http.markdown | 15 +++-- lib/_http_common.js | 27 +++++++++ lib/_http_outgoing.js | 34 ++++++++++-- test/parallel/test-http-response-splitting.js | 55 +++++++++++++++++++ 4 files changed, 121 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-http-response-splitting.js diff --git a/doc/api/http.markdown b/doc/api/http.markdown index 56d3595ed12ca3..5ff823e630abf0 100644 --- a/doc/api/http.markdown +++ b/doc/api/http.markdown @@ -614,8 +614,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 trailer field name that contains invalid characters will -result in a [`TypeError`][] being thrown. +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. ### response.end([data][, encoding][, callback]) @@ -678,8 +678,8 @@ or response.setHeader('Set-Cookie', ['type=ninja', 'language=javascript']); -Attempting to set a header field name that contains invalid characters will -result in a [`TypeError`][] being thrown. +Attempting to set a header field name or value that contains invalid characters +will result in a [`TypeError`][] being thrown. ### response.setTimeout(msecs, callback) @@ -773,8 +773,8 @@ Example: 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 -implicit/mutable headers will be calculated and call this function for you. +If you call [`response.write()`][] or [`response.end()`][] before calling this, +the implicit/mutable headers will be calculated and call this function for you. Note that Content-Length is given in bytes not characters. The above example works because the string `'hello world'` contains only single byte characters. @@ -783,6 +783,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 diff --git a/lib/_http_common.js b/lib/_http_common.js index 4b57460981617f..5c24180371e4fb 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -199,3 +199,30 @@ function httpSocketSetup(socket) { socket.on('drain', ondrain); } exports.httpSocketSetup = httpSocketSetup; + +/** + * Verifies that the given val is a valid HTTP token + * per the rules defined in RFC 7230 + **/ +const token = /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/; +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; diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 8a6f76a357d40d..47c6f93e99b277 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -19,6 +19,7 @@ const contentLengthExpression = /^Content-Length$/i; const dateExpression = /^Date$/i; const expectExpression = /^Expect$/i; const trailerExpression = /^Trailer$/i; +const lenientHttpHeaders = !!process.REVERT_CVE_2016_2216; const automaticHeaders = { connection: true, @@ -299,8 +300,16 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) { }; function storeHeader(self, state, field, value) { - value = escapeHeaderValue(value); - state.messageHeader += field + ': ' + value + CRLF; + if (!lenientHttpHeaders) { + if (!common._checkIsHttpToken(field)) { + throw new TypeError( + 'Header name must be a valid HTTP Token ["' + field + '"]'); + } + if (common._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; @@ -333,7 +342,15 @@ OutgoingMessage.prototype.setHeader = function(name, value) { throw new Error('`value` required in setHeader("' + name + '", value).'); if (this._header) throw new Error('Can\'t set headers after they are sent.'); - + if (!lenientHttpHeaders) { + if (!common._checkIsHttpToken(name)) { + throw new TypeError( + 'Trailer name must be a valid HTTP Token ["' + name + '"]'); + } + if (common._checkInvalidHeaderChar(value) === true) { + throw new TypeError('The header content contains invalid characters'); + } + } if (this._headers === null) this._headers = {}; @@ -482,6 +499,7 @@ function connectionCorkNT(conn) { 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; @@ -502,7 +520,15 @@ OutgoingMessage.prototype.addTrailers = function(headers) { field = key; value = headers[key]; } - + if (!lenientHttpHeaders) { + if (!common._checkIsHttpToken(field)) { + 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; } }; diff --git a/test/parallel/test-http-response-splitting.js b/test/parallel/test-http-response-splitting.js new file mode 100644 index 00000000000000..4c954bf90acc7e --- /dev/null +++ b/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ഊഊ'; +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(); + }); +});