From 45fa65249ed59c41eb3717762c1108b65ab78d95 Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sat, 9 Mar 2019 16:28:41 -0300 Subject: [PATCH 1/8] http: create an option for setting a maximum size for uri parsing The http server wasn't able to tell exactly what caused an HPE_HEADER_OVERFLOW, meaning it would yield a 431 error even if what caused it was the request URI being too long. This adds a limit to the URI sizes through a new option called max-http-uri-size, which will be checked against the actual URIs after on_url callback at the node_http_parser_impl file. Fixes: https://github.com/nodejs/node/issues/26296 Refs: https://github.com/expressjs/express/issues/3898 --- doc/api/http.md | 11 ++++++ lib/_http_server.js | 17 +++++++-- lib/http.js | 14 ++++++++ src/node_http_parser_impl.h | 17 ++++++--- src/node_options.cc | 4 +++ src/node_options.h | 1 + test/parallel/test-http-uri-overflow.js | 46 +++++++++++++++++++++++++ 7 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-http-uri-overflow.js diff --git a/doc/api/http.md b/doc/api/http.md index b06a28e3319572..0f03408da00b73 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -1959,6 +1959,17 @@ added: v11.6.0 Read-only property specifying the maximum allowed size of HTTP headers in bytes. Defaults to 8KB. Configurable using the [`--max-http-header-size`][] CLI option. +## http.maxUriSize + + +* {number} + +Read-only property specifying the maximum allowed size of HTTP request uri +in bytes. Defaults to 7KB. Configurable using the +[`--max-http-uri-size`][] CLI option. + ## http.request(options[, callback]) ## http.request(url[, options][, callback]) * {number} From e7ade8bdc3a00fa65f53b100af4a216d70dc0bea Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sun, 10 Mar 2019 11:38:03 -0300 Subject: [PATCH 4/8] http: fixing comment because lint --- test/parallel/test-http-uri-overflow.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-uri-overflow.js b/test/parallel/test-http-uri-overflow.js index 9ef8ed3e7652a2..671fccf626324c 100644 --- a/test/parallel/test-http-uri-overflow.js +++ b/test/parallel/test-http-uri-overflow.js @@ -8,7 +8,7 @@ const CRLF = '\r\n'; const REQUEST_METHOD_AND_SPACE = 'GET '; const DUMMY_URI = '/' + 'a'.repeat( maxUriSize -); // the slash makes it just 1 byte too long +); // The slash makes it just 1 byte too long. const PAYLOAD = REQUEST_METHOD_AND_SPACE + DUMMY_URI + CRLF; From 78dfcf720f28585ab86fec6d9aaf2cd23d8e0033 Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sun, 10 Mar 2019 11:50:28 -0300 Subject: [PATCH 5/8] http: requiring common first --- test/parallel/test-http-uri-overflow.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-uri-overflow.js b/test/parallel/test-http-uri-overflow.js index 671fccf626324c..c4345bdaeaca9a 100644 --- a/test/parallel/test-http-uri-overflow.js +++ b/test/parallel/test-http-uri-overflow.js @@ -1,8 +1,8 @@ 'use strict'; +const { expectsError, mustCall } = require('../common'); const assert = require('assert'); const { createServer, maxUriSize } = require('http'); const { createConnection } = require('net'); -const { expectsError, mustCall } = require('../common'); const CRLF = '\r\n'; const REQUEST_METHOD_AND_SPACE = 'GET '; From 710e83086c73cc13193c38301964a27024facaf4 Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sun, 10 Mar 2019 16:12:55 -0300 Subject: [PATCH 6/8] http: splitting the limit check of header and uri --- src/node_http_parser_impl.h | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index e1daf0b49672de..576d35d3fedd2f 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -180,7 +180,7 @@ class Parser : public AsyncWrap, public StreamListener { int on_url(const char* at, size_t length) { - int rv = TrackHeader(length, kBeforeHeaders); + int rv = TrackURI(length); if (rv != 0) { return rv; } @@ -251,6 +251,7 @@ class Parser : public AsyncWrap, public StreamListener { int on_headers_complete() { #ifdef NODE_EXPERIMENTAL_HTTP + uri_nread_ = 0; header_nread_ = 0; #endif /* NODE_EXPERIMENTAL_HTTP */ @@ -813,6 +814,7 @@ class Parser : public AsyncWrap, public StreamListener { void Init(parser_type_t type) { #ifdef NODE_EXPERIMENTAL_HTTP llhttp_init(&parser_, type, &settings); + uri_nread_ = 0; header_nread_ = 0; #else /* !NODE_EXPERIMENTAL_HTTP */ http_parser_init(&parser_, type); @@ -825,21 +827,22 @@ class Parser : public AsyncWrap, public StreamListener { got_exception_ = false; } - enum HeaderTrackState { - kBeforeHeaders, - kAfterRequestLine - }; - - int TrackHeader(size_t len, enum HeaderTrackState pos = kAfterRequestLine) { + int TrackURI(size_t len) { #ifdef NODE_EXPERIMENTAL_HTTP - header_nread_ += len; - if (pos == kBeforeHeaders && - header_nread_ >= per_process::cli_options->max_http_uri_size) { + uri_nread_ += len; + if (uri_nread_ >= per_process::cli_options->max_http_uri_size) { llhttp_set_error_reason(&parser_, "HPE_URI_OVERFLOW:URI overflow"); return HPE_USER; - } else if (pos == kAfterRequestLine && - header_nread_ >= per_process::cli_options->max_http_header_size) { + } +#endif /* NODE_EXPERIMENTAL_HTTP */ + return 0; + } + + int TrackHeader(size_t len) { +#ifdef NODE_EXPERIMENTAL_HTTP + header_nread_ += len; + if (header_nread_ >= per_process::cli_options->max_http_header_size) { llhttp_set_error_reason(&parser_, "HPE_HEADER_OVERFLOW:Header overflow"); return HPE_USER; } @@ -879,6 +882,7 @@ class Parser : public AsyncWrap, public StreamListener { #ifdef NODE_EXPERIMENTAL_HTTP unsigned int execute_depth_ = 0; bool pending_pause_ = false; + uint64_t uri_nread_ = 0; uint64_t header_nread_ = 0; #endif /* NODE_EXPERIMENTAL_HTTP */ From 38307dcf8f57db08364da37e4ed00bd430a40ed9 Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sun, 10 Mar 2019 16:20:12 -0300 Subject: [PATCH 7/8] http: fixing tests test-http-header-overflow.js: the llhttp doesn't count space and separator, so to generate a breaking number of chars, just add +1 to the maxHeaderSize test-http-max-http-headers.js: remove the +1 that was referring to the slash of the uri, because it's not being counted anymore --- test/parallel/test-http-header-overflow.js | 38 +++++++++++++++---- test/sequential/test-http-max-http-headers.js | 2 +- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/test/parallel/test-http-header-overflow.js b/test/parallel/test-http-header-overflow.js index a539a69c030ae8..e2fa3c16e23805 100644 --- a/test/parallel/test-http-header-overflow.js +++ b/test/parallel/test-http-header-overflow.js @@ -1,18 +1,34 @@ +// Flags: --expose-internals 'use strict'; +const { expectsError, mustCall } = require('../common'); const assert = require('assert'); const { createServer, maxHeaderSize } = require('http'); const { createConnection } = require('net'); -const { expectsError, mustCall } = require('../common'); + +const { getOptionValue } = require('internal/options'); + +const currentParser = getOptionValue('--http-parser'); + +const sumOfLengths = (strings) => strings + .map((string) => string.length) + .reduce((a, b) => a + b); + +const getBreakingLength = () => { + if (currentParser === 'llhttp') { + return maxHeaderSize - HEADER_NAME.length + 1; + } else { + return maxHeaderSize - HEADER_NAME.length - HEADER_SEPARATOR - + (2 * CRLF.length) + 1; + } +}; const CRLF = '\r\n'; -const DUMMY_HEADER_NAME = 'Cookie: '; -const DUMMY_HEADER_VALUE = 'a'.repeat( - // Plus one is to make it 1 byte too big - maxHeaderSize - DUMMY_HEADER_NAME.length - (2 * CRLF.length) + 1 -); +const HEADER_NAME = 'Cookie'; +const HEADER_SEPARATOR = ': '; +const HEADER_VALUE = 'a'.repeat(getBreakingLength()); const PAYLOAD_GET = 'GET /blah HTTP/1.1'; const PAYLOAD = PAYLOAD_GET + CRLF + - DUMMY_HEADER_NAME + DUMMY_HEADER_VALUE + CRLF.repeat(2); + HEADER_NAME + HEADER_SEPARATOR + HEADER_VALUE + CRLF.repeat(2); const server = createServer(); @@ -21,7 +37,13 @@ server.on('connection', mustCall((socket) => { type: Error, message: 'Parse Error', code: 'HPE_HEADER_OVERFLOW', - bytesParsed: maxHeaderSize + PAYLOAD_GET.length, + bytesParsed: sumOfLengths([ + PAYLOAD_GET, + CRLF, + HEADER_NAME, + HEADER_SEPARATOR, + HEADER_VALUE + ]) + 1, rawPacket: Buffer.from(PAYLOAD) })); })); diff --git a/test/sequential/test-http-max-http-headers.js b/test/sequential/test-http-max-http-headers.js index 16704fd9124c3e..89407dc1dc1fc4 100644 --- a/test/sequential/test-http-max-http-headers.js +++ b/test/sequential/test-http-max-http-headers.js @@ -123,7 +123,7 @@ const test2 = common.mustCall(() => { 'X-CRASH: '; // /, Host, localhost, Agent, node, X-CRASH, a... - const currentSize = 1 + 4 + 9 + 5 + 4 + 7; + const currentSize = 4 + 9 + 5 + 4 + 7; headers = fillHeaders(headers, currentSize); const server = http.createServer(common.mustNotCall()); From 93e384a75bd5ff8b528437e9c8a937575d27bb59 Mon Sep 17 00:00:00 2001 From: Caio Rodrigues Date: Sun, 10 Mar 2019 17:57:32 -0300 Subject: [PATCH 8/8] http: fix the comment to reflect the actual currentSize composition --- test/sequential/test-http-max-http-headers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-http-max-http-headers.js b/test/sequential/test-http-max-http-headers.js index 89407dc1dc1fc4..cc01235a370fc5 100644 --- a/test/sequential/test-http-max-http-headers.js +++ b/test/sequential/test-http-max-http-headers.js @@ -122,7 +122,7 @@ const test2 = common.mustCall(() => { 'Agent: nod2\r\n' + 'X-CRASH: '; - // /, Host, localhost, Agent, node, X-CRASH, a... + // Host, localhost, Agent, node, X-CRASH, a... const currentSize = 4 + 9 + 5 + 4 + 7; headers = fillHeaders(headers, currentSize);