From 0eae95eae322ba06aaec77227a02927ba3a90ea9 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Feb 2016 19:22:12 -0800 Subject: [PATCH] deps: update http-parser to version 2.5.1 includes parsing improvements to ensure closer HTTP spec conformance PR-URL: https://github.com/nodejs/node-private/pull/20 --- deps/http_parser/Makefile | 2 +- deps/http_parser/http_parser.c | 31 +++- deps/http_parser/http_parser.h | 12 +- deps/http_parser/test.c | 161 +++++++++++++++++++++ src/node_http_parser.cc | 3 + src/node_revert.h | 4 +- ...tp-client-reject-chunked-with-content-length.js | 28 ++++ test/parallel/test-http-client-reject-cr-no-lf.js | 27 ++++ test/parallel/test-http-double-content-length.js | 33 +++++ .../test-http-header-response-splitting.js | 1 + .../test-http-response-multi-content-length.js | 52 +++++++ test/parallel/test-http-server-multiheaders2.js | 6 +- ...tp-server-reject-chunked-with-content-length.js | 31 ++++ test/parallel/test-http-server-reject-cr-no-lf.js | 33 +++++ 14 files changed, 411 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-http-client-reject-chunked-with-content-length.js create mode 100644 test/parallel/test-http-client-reject-cr-no-lf.js create mode 100644 test/parallel/test-http-double-content-length.js create mode 100644 test/parallel/test-http-response-multi-content-length.js create mode 100644 test/parallel/test-http-server-reject-chunked-with-content-length.js create mode 100644 test/parallel/test-http-server-reject-cr-no-lf.js diff --git a/deps/http_parser/Makefile b/deps/http_parser/Makefile index 373709c..9ad5fc6 100644 --- a/deps/http_parser/Makefile +++ b/deps/http_parser/Makefile @@ -19,7 +19,7 @@ # IN THE SOFTWARE. PLATFORM ?= $(shell sh -c 'uname -s | tr "[A-Z]" "[a-z]"') -SONAME ?= libhttp_parser.so.2.5.0 +SONAME ?= libhttp_parser.so.2.5.1 CC?=gcc AR?=ar diff --git a/deps/http_parser/http_parser.c b/deps/http_parser/http_parser.c index 0fa1c36..e1d4a73 100644 --- a/deps/http_parser/http_parser.c +++ b/deps/http_parser/http_parser.c @@ -433,6 +433,12 @@ enum http_host_state (IS_ALPHANUM(c) || (c) == '.' || (c) == '-' || (c) == '_') #endif +/** + * Verify that a char is a valid visible (printable) US-ASCII + * character or %x80-FF + **/ +#define IS_HEADER_CHAR(ch) \ + (ch == CR || ch == LF || ch == 9 || (ch > 31 && ch != 127)) #define start_state (parser->type == HTTP_REQUEST ? s_start_req : s_start_res) @@ -638,6 +644,8 @@ size_t http_parser_execute (http_parser *parser, const char *status_mark = 0; enum state p_state = (enum state) parser->state; + const unsigned int lenient = parser->lenient_http_headers; + /* We're in an error state. Don't bother doing anything. */ if (HTTP_PARSER_ERRNO(parser) != HPE_OK) { return 0; @@ -1384,7 +1392,12 @@ reexecute: || c != CONTENT_LENGTH[parser->index]) { parser->header_state = h_general; } else if (parser->index == sizeof(CONTENT_LENGTH)-2) { + if (parser->flags & F_CONTENTLENGTH) { + SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH); + goto error; + } parser->header_state = h_content_length; + parser->flags |= F_CONTENTLENGTH; } break; @@ -1536,6 +1549,11 @@ reexecute: REEXECUTE(); } + if (!lenient && !IS_HEADER_CHAR(ch)) { + SET_ERRNO(HPE_INVALID_HEADER_TOKEN); + goto error; + } + c = LOWER(ch); switch (h_state) { @@ -1703,7 +1721,10 @@ reexecute: case s_header_almost_done: { - STRICT_CHECK(ch != LF); + if (UNLIKELY(ch != LF)) { + SET_ERRNO(HPE_LF_EXPECTED); + goto error; + } UPDATE_STATE(s_header_value_lws); break; @@ -1787,6 +1808,14 @@ reexecute: REEXECUTE(); } + /* Cannot use chunked encoding and a content-length header together + per the HTTP specification. */ + if ((parser->flags & F_CHUNKED) && + (parser->flags & F_CONTENTLENGTH)) { + SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH); + goto error; + } + UPDATE_STATE(s_headers_done); /* Set this here so that on_headers_complete() callbacks can see it */ diff --git a/deps/http_parser/http_parser.h b/deps/http_parser/http_parser.h index eb71bf9..999b3d5 100644 --- a/deps/http_parser/http_parser.h +++ b/deps/http_parser/http_parser.h @@ -27,7 +27,7 @@ extern "C" { /* Also update SONAME in the Makefile whenever you change these. */ #define HTTP_PARSER_VERSION_MAJOR 2 #define HTTP_PARSER_VERSION_MINOR 5 -#define HTTP_PARSER_VERSION_PATCH 0 +#define HTTP_PARSER_VERSION_PATCH 1 #include #if defined(_WIN32) && !defined(__MINGW32__) && (!defined(_MSC_VER) || _MSC_VER<1600) @@ -140,6 +140,7 @@ enum flags , F_TRAILING = 1 << 4 , F_UPGRADE = 1 << 5 , F_SKIPBODY = 1 << 6 + , F_CONTENTLENGTH = 1 << 7 }; @@ -182,6 +183,8 @@ enum flags XX(INVALID_HEADER_TOKEN, "invalid character in header") \ XX(INVALID_CONTENT_LENGTH, \ "invalid character in content-length header") \ + XX(UNEXPECTED_CONTENT_LENGTH, \ + "unexpected content-length header") \ XX(INVALID_CHUNK_SIZE, \ "invalid character in chunk size header") \ XX(INVALID_CONSTANT, "invalid constant string") \ @@ -206,10 +209,11 @@ enum http_errno { struct http_parser { /** PRIVATE **/ unsigned int type : 2; /* enum http_parser_type */ - unsigned int flags : 7; /* F_* values from 'flags' enum; semi-public */ + unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */ unsigned int state : 7; /* enum state from http_parser.c */ - unsigned int header_state : 8; /* enum header_state from http_parser.c */ - unsigned int index : 8; /* index into current matcher */ + unsigned int header_state : 7; /* enum header_state from http_parser.c */ + unsigned int index : 7; /* index into current matcher */ + unsigned int lenient_http_headers : 1; uint32_t nread; /* # bytes read in various scenarios */ uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */ diff --git a/deps/http_parser/test.c b/deps/http_parser/test.c index 4c00571..885d89e 100644 --- a/deps/http_parser/test.c +++ b/deps/http_parser/test.c @@ -3166,6 +3166,155 @@ test_simple (const char *buf, enum http_errno err_expected) } void +test_invalid_header_content (int req, const char* str) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = str; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in invalid header content test ***\n"); + abort(); +} + +void +test_invalid_header_field_content_error (int req) +{ + test_invalid_header_content(req, "Foo: F\01ailure"); + test_invalid_header_content(req, "Foo: B\02ar"); +} + +void +test_invalid_header_field (int req, const char* str) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = str; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in invalid header token test ***\n"); + abort(); +} + +void +test_invalid_header_field_token_error (int req) +{ + test_invalid_header_field(req, "Fo@: Failure"); + test_invalid_header_field(req, "Foo\01\test: Bar"); +} + +void +test_double_content_length_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Content-Length: 0\r\nContent-Length: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_MULTIPLE_CONTENT_LENGTH); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in double content-length test ***\n"); + abort(); +} + +void +test_chunked_content_length_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Transfer-Encoding: chunked\r\nContent-Length: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_CHUNKED_WITH_CONTENT_LENGTH); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in chunked content-length test ***\n"); + abort(); +} + +void +test_header_cr_no_lf_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Foo: 1\rBar: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_LF_EXPECTED); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in header whitespace test ***\n"); + abort(); +} + +void test_header_overflow_error (int req) { http_parser parser; @@ -3591,6 +3740,18 @@ main (void) test_header_content_length_overflow_error(); test_chunk_content_length_overflow_error(); + //// HEADER FIELD CONDITIONS + test_double_content_length_error(HTTP_REQUEST); + test_chunked_content_length_error(HTTP_REQUEST); + test_header_cr_no_lf_error(HTTP_REQUEST); + test_invalid_header_field_token_error(HTTP_REQUEST); + test_invalid_header_field_content_error(HTTP_REQUEST); + test_double_content_length_error(HTTP_RESPONSE); + test_chunked_content_length_error(HTTP_RESPONSE); + test_header_cr_no_lf_error(HTTP_RESPONSE); + test_invalid_header_field_token_error(HTTP_RESPONSE); + test_invalid_header_field_content_error(HTTP_RESPONSE); + //// RESPONSES for (i = 0; i < response_count; i++) { diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index ff3dfb2..9225915 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -1,6 +1,7 @@ #include "node.h" #include "node_buffer.h" #include "node_http_parser.h" +#include "node_revert.h" #include "base-object.h" #include "base-object-inl.h" @@ -670,6 +671,8 @@ class Parser : public BaseObject { void Init(enum http_parser_type type) { http_parser_init(&parser_, type); + /* Allow the strict http header parsing to be reverted */ + parser_.lenient_http_headers = IsReverted(REVERT_CVE_2016_2216) ? 1 : 0; url_.Reset(); status_message_.Reset(); num_fields_ = 0; diff --git a/src/node_revert.h b/src/node_revert.h index 7a779ff..b5435e7 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -12,8 +12,8 @@ * For *master* this list should always be empty! * **/ -#define REVERSIONS(XX) -// XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") +#define REVERSIONS(XX) \ + XX(CVE_2016_2216, "CVE-2016-2216", "Strict HTTP Header Parsing") namespace node { diff --git a/test/parallel/test-http-client-reject-chunked-with-content-length.js b/test/parallel/test-http-client-reject-chunked-with-content-length.js new file mode 100644 index 0000000..a6639c9 --- /dev/null +++ b/test/parallel/test-http-client-reject-chunked-with-content-length.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'HTTP/1.1 200 OK\r\n' + + 'Content-Length: 1\r\n' + + 'Transfer-Encoding: chunked\r\n\r\n'; + +const server = net.createServer((socket) => { + socket.write(reqstr); +}); + +server.listen(common.PORT, () => { + // The callback should not be called because the server is sending + // both a Content-Length header and a Transfer-Encoding: chunked + // header, which is a violation of the HTTP spec. + const req = http.get({port:common.PORT}, (res) => { + assert.fail(null, null, 'callback should not be called'); + }); + req.on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + server.close(); + })); +}); diff --git a/test/parallel/test-http-client-reject-cr-no-lf.js b/test/parallel/test-http-client-reject-cr-no-lf.js new file mode 100644 index 0000000..b60220c --- /dev/null +++ b/test/parallel/test-http-client-reject-cr-no-lf.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'HTTP/1.1 200 OK\r\n' + + 'Foo: Bar\r' + + 'Content-Length: 1\r\n\r\n'; + +const server = net.createServer((socket) => { + socket.write(reqstr); +}); + +server.listen(common.PORT, () => { + // The callback should not be called because the server is sending a + // header field that ends only in \r with no following \n + const req = http.get({port:common.PORT}, (res) => { + assert.fail(null, null, 'callback should not be called'); + }); + req.on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_LF_EXPECTED'); + server.close(); + })); +}); diff --git a/test/parallel/test-http-double-content-length.js b/test/parallel/test-http-double-content-length.js new file mode 100644 index 0000000..c35f183 --- /dev/null +++ b/test/parallel/test-http-double-content-length.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +// The callback should never be invoked because the server +// should respond with a 400 Client Error when a double +// Content-Length header is received. +const server = http.createServer((req, res) => { + assert(false, 'callback should not have been invoked'); + res.end(); +}); +server.on('clientError', common.mustCall((err, socket) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + socket.destroy(); +})); + +server.listen(common.PORT, () => { + const req = http.get({ + port: common.PORT, + // Send two content-length header values. + headers: {'Content-Length': [1, 2]}}, + (res) => { + assert.fail(null, null, 'an error should have occurred'); + server.close(); + } + ); + req.on('error', common.mustCall(() => { + server.close(); + })); +}); diff --git a/test/parallel/test-http-header-response-splitting.js b/test/parallel/test-http-header-response-splitting.js index 437793a..41233ae 100644 --- a/test/parallel/test-http-header-response-splitting.js +++ b/test/parallel/test-http-header-response-splitting.js @@ -1,3 +1,4 @@ +// Flags: --security-revert=CVE-2016-2216 'use strict'; var common = require('../common'), assert = require('assert'), diff --git a/test/parallel/test-http-response-multi-content-length.js b/test/parallel/test-http-response-multi-content-length.js new file mode 100644 index 0000000..4b0f2c1 --- /dev/null +++ b/test/parallel/test-http-response-multi-content-length.js @@ -0,0 +1,52 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +const MAX_COUNT = 2; + +const server = http.createServer((req, res) => { + const num = req.headers['x-num']; + // TODO(@jasnell) At some point this should be refactored as the API + // should not be allowing users to set multiple content-length values + // in the first place. + switch (num) { + case '1': + res.setHeader('content-length', [2, 1]); + break; + case '2': + res.writeHead(200, {'content-length': [1, 2]}); + break; + default: + assert.fail(null, null, 'should never get here'); + } + res.end('ok'); +}); + +var count = 0; + +server.listen(common.PORT, common.mustCall(() => { + for (let n = 1; n <= MAX_COUNT ; n++) { + // This runs twice, the first time, the server will use + // setHeader, the second time it uses writeHead. In either + // case, the error handler must be called because the client + // is not allowed to accept multiple content-length headers. + http.get( + {port:common.PORT, headers:{'x-num': n}}, + (res) => { + assert(false, 'client allowed multiple content-length headers.'); + } + ).on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + count++; + if (count === MAX_COUNT) + server.close(); + })); + } +})); + +process.on('exit', () => { + assert.equal(count, MAX_COUNT); +}); diff --git a/test/parallel/test-http-server-multiheaders2.js b/test/parallel/test-http-server-multiheaders2.js index 438e8ba..bf54af3 100644 --- a/test/parallel/test-http-server-multiheaders2.js +++ b/test/parallel/test-http-server-multiheaders2.js @@ -57,7 +57,6 @@ var srv = http.createServer(function(req, res) { assert.equal(req.headers[header.toLowerCase()], 'foo, bar', 'header parsed incorrectly: ' + header); }); - assert.equal(req.headers['content-length'], 0); res.writeHead(200, {'Content-Type' : 'text/plain'}); res.end('EOF'); @@ -75,10 +74,7 @@ var headers = [] .concat(multipleAllowed.map(makeHeader('foo'))) .concat(multipleForbidden.map(makeHeader('foo'))) .concat(multipleAllowed.map(makeHeader('bar'))) - .concat(multipleForbidden.map(makeHeader('bar'))) - // content-length is a special case since node.js - // is dropping connetions with non-numeric headers - .concat([['content-length', 0], ['content-length', 123]]); + .concat(multipleForbidden.map(makeHeader('bar'))); srv.listen(common.PORT, function() { http.get({ diff --git a/test/parallel/test-http-server-reject-chunked-with-content-length.js b/test/parallel/test-http-server-reject-chunked-with-content-length.js new file mode 100644 index 0000000..42cc1e8 --- /dev/null +++ b/test/parallel/test-http-server-reject-chunked-with-content-length.js @@ -0,0 +1,31 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'POST / HTTP/1.1\r\n' + + 'Content-Length: 1\r\n' + + 'Transfer-Encoding: chunked\r\n\r\n'; + +const server = http.createServer((req, res) => { + assert.fail(null, null, 'callback should not be invoked'); +}); +server.on('clientError', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + server.close(); +})); +server.listen(common.PORT, () => { + const client = net.connect({port: common.PORT}, () => { + client.write(reqstr); + client.end(); + }); + client.on('data', (data) => { + // Should not get to this point because the server should simply + // close the connection without returning any data. + assert.fail(null, null, 'no data should be returned by the server'); + }); + client.on('end', common.mustCall(() => {})); +}); diff --git a/test/parallel/test-http-server-reject-cr-no-lf.js b/test/parallel/test-http-server-reject-cr-no-lf.js new file mode 100644 index 0000000..fbb89f0 --- /dev/null +++ b/test/parallel/test-http-server-reject-cr-no-lf.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const net = require('net'); +const http = require('http'); +const assert = require('assert'); + +const str = 'GET / HTTP/1.1\r\n' + + 'Dummy: Header\r' + + 'Content-Length: 1\r\n' + + '\r\n'; + + +const server = http.createServer((req, res) => { + assert.fail(null, null, 'this should not be called'); +}); +server.on('clientError', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_LF_EXPECTED'); + server.close(); +})); +server.listen(common.PORT, () => { + const client = net.connect({port:common.PORT}, () => { + client.on('data', (chunk) => { + assert.fail(null, null, 'this should not be called'); + }); + client.on('end', common.mustCall(() => { + server.close(); + })); + client.write(str); + client.end(); + }); +}); -- 2.7.4