From 25b6897e8a1072bd38b7d12d3ec6920bfe1c8de3 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 10 Jan 2020 15:00:11 -0800 Subject: [PATCH] http: strip trailing OWS from header values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTP header values can have trailing OWS, but it should be stripped. It is not semantically part of the header's value, and if treated as part of the value, it can cause spurious inequality between expected and actual header values. Note that a single SPC of leading OWS is common before the field-value, and it is already handled by the HTTP parser by stripping all leading OWS. It is only the trailing OWS that must be stripped by the parser user. header-field = field-name ":" OWS field-value OWS ; https://tools.ietf.org/html/rfc7230#section-3.2 OWS = *( SP / HTAB ) ; https://tools.ietf.org/html/rfc7230#section-3.2.3 Fixes: https://hackerone.com/reports/730779 PR-URL: https://github.com/nodejs-private/node-private/pull/189 Reviewed-By: Matteo Collina  Reviewed-By: Fedor Indutny  Reviewed-By: Ben Noordhuis  Reviewed-By: James M Snell  --- benchmark/http/incoming_headers.js | 15 ++++--- src/node_http_parser.cc | 17 +++++++- test/parallel/test-http-header-owstext.js | 49 +++++++++++++++++++++++ 3 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-http-header-owstext.js diff --git a/benchmark/http/incoming_headers.js b/benchmark/http/incoming_headers.js index f10178c1891805..810c92687bd981 100644 --- a/benchmark/http/incoming_headers.js +++ b/benchmark/http/incoming_headers.js @@ -3,12 +3,12 @@ const common = require('../common.js'); const http = require('http'); const bench = common.createBenchmark(main, { - // Unicode confuses ab on os x. - c: [50, 500], - n: [0, 5, 20] + c: [50], // Concurrent connections + n: [20], // Number of header lines to append after the common headers + w: [0, 6], // Amount of trailing whitespace }); -function main({ c, n }) { +function main({ c, n, w }) { const server = http.createServer((req, res) => { res.end(); }); @@ -22,7 +22,12 @@ function main({ c, n }) { 'Cache-Control': 'no-cache' }; for (let i = 0; i < n; i++) { - headers[`foo${i}`] = `some header value ${i}`; + // Note: + // - autocannon does not send header values with OWS + // - wrk can only send trailing OWS. This is a side-effect of wrk + // processing requests with http-parser before sending them, causing + // leading OWS to be stripped. + headers[`foo${i}`] = `some header value ${i}${' \t'.repeat(w / 2)}`; } bench.http({ path: '/', diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 5e1da912e0c31e..a8c48999c57901 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -77,6 +77,10 @@ const uint32_t kOnExecute = 4; // Any more fields than this will be flushed into JS const size_t kMaxHeaderFieldsCount = 32; +inline bool IsOWS(char c) { + return c == ' ' || c == '\t'; +} + // helper class for the Parser struct StringPtr { StringPtr() { @@ -136,13 +140,22 @@ struct StringPtr { Local ToString(Environment* env) const { - if (str_) + if (size_ != 0) return OneByteString(env->isolate(), str_, size_); else return String::Empty(env->isolate()); } + // Strip trailing OWS (SPC or HTAB) from string. + Local ToTrimmedString(Environment* env) { + while (size_ > 0 && IsOWS(str_[size_ - 1])) { + size_--; + } + return ToString(env); + } + + const char* str_; bool on_heap_; size_t size_; @@ -730,7 +743,7 @@ class Parser : public AsyncWrap, public StreamListener { for (size_t i = 0; i < num_values_; ++i) { headers_v[i * 2] = fields_[i].ToString(env()); - headers_v[i * 2 + 1] = values_[i].ToString(env()); + headers_v[i * 2 + 1] = values_[i].ToTrimmedString(env()); } return Array::New(env()->isolate(), headers_v, num_values_ * 2); diff --git a/test/parallel/test-http-header-owstext.js b/test/parallel/test-http-header-owstext.js new file mode 100644 index 00000000000000..bc094137a29e5e --- /dev/null +++ b/test/parallel/test-http-header-owstext.js @@ -0,0 +1,49 @@ +'use strict'; +const common = require('../common'); + +// This test ensures that the http-parser strips leading and trailing OWS from +// header values. It sends the header values in chunks to force the parser to +// build the string up through multiple calls to on_header_value(). + +const assert = require('assert'); +const http = require('http'); +const net = require('net'); + +function check(hdr, snd, rcv) { + const server = http.createServer(common.mustCall((req, res) => { + assert.strictEqual(req.headers[hdr], rcv); + req.pipe(res); + })); + + server.listen(0, common.mustCall(function() { + const client = net.connect(this.address().port, start); + function start() { + client.write('GET / HTTP/1.1\r\n' + hdr + ':', drain); + } + + function drain() { + if (snd.length === 0) { + return client.write('\r\nConnection: close\r\n\r\n'); + } + client.write(snd.shift(), drain); + } + + const bufs = []; + client.on('data', function(chunk) { + bufs.push(chunk); + }); + client.on('end', common.mustCall(function() { + const head = Buffer.concat(bufs) + .toString('latin1') + .split('\r\n')[0]; + assert.strictEqual(head, 'HTTP/1.1 200 OK'); + server.close(); + })); + })); +} + +check('host', [' \t foo.com\t'], 'foo.com'); +check('host', [' \t foo\tcom\t'], 'foo\tcom'); +check('host', [' \t', ' ', ' foo.com\t', '\t '], 'foo.com'); +check('host', [' \t', ' \t'.repeat(100), '\t '], ''); +check('host', [' \t', ' - - - - ', '\t '], '- - - -');