http: create an option for setting a maximum size for uri parsing #26553

caiolrm · 2019-03-09T19:36:30Z

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: #26296
Refs: expressjs/express#3898

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/http.md

bnoordhuis

If --max-http-uri-size > --http-max-header-size you'll never get a 414.

That's probably going to be surprising to some. I'm slightly -1 on adding a separate switch.

doc/api/http.md

src/node_http_parser_impl.h

lpinca · 2019-03-10T13:55:01Z

I think making it configurable makes sense. Apache uses the LimitRequestLine Directive for this.

lpinca · 2019-03-10T13:59:24Z

Yes, I agree on that. This is what this PR is addressing no?

lpinca

I'm not sure about the default value. Other than that LGTM.

test/parallel/test-http-uri-overflow.js

lpinca · 2019-03-10T15:45:34Z

@BridgeAR your comment is actually correct as it seems the same counter is used for both the request line and the rest of the headers. I think @caiolrm is working on that.

lpinca · 2019-03-10T16:53:25Z

Added semver-major because #25605 was also tagged as semver-major.

caiolrm · 2019-03-10T19:27:21Z

I splitted the counting in two different functions, now the uri length isn't being considered with the headers, also had to make some changes to tests. @BridgeAR @lpinca

test/sequential/test-http-max-http-headers.js

lpinca · 2019-03-10T20:06:47Z

Thank you. Would you be so kind to remove the merge commit?

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: nodejs#26296 Refs: expressjs/express#3898

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

mcollina · 2019-03-15T17:41:14Z

This is a shot in the dark but Is there anything wrong on picking something a lot higher like for example 500k as a default and letting the end user customize via flag or similar? My thought process: picking up 8k might break some people but definitely not 500k and if you feel it as a security threat... just reduce it.

Unfortunately no. We had 80KB before and it was exploitable as a DoS vector https://nodejs.org/en/blog/vulnerability/november-2018-security-releases/#denial-of-service-with-large-http-headers-cve-2018-12121. I prefer a safe default.

There is little difference in safety between 8KB and 16KB.

Trott · 2019-03-15T19:20:26Z

I know URI !== URL but simplicity for the developer probably takes precedence over terminology precision. Since we use url elsewhere in the module (message.url as well as the name of the first argument for http.get() and http.request()), I wonder if this should be maxHttpUrlSize instead of maxHttpUriSize? (Same for the CLI flag.)

richardlau · 2019-03-15T19:47:00Z

I know URI !== URL but simplicity for the developer probably takes precedence over terminology precision. Since we use url elsewhere in the module (message.url as well as the name of the first argument for http.get() and http.request()), I wonder if this should be maxHttpUrlSize instead of maxHttpUriSize? (Same for the CLI flag.)

But the error that ends up being written is 414 URI Too Long.

Trott · 2019-03-16T02:32:38Z

But the error that ends up being written is 414 URI Too Long.

Not a bad point, but anything coming from the spec is going to say "URI" as the string "URL" does not appear at all in RFC 7231. But we're not going to change "url" to "uri" in our code base for any other options. So probably best to stick to one term when referring to the same thing.

All that said, I'm fine if we decide to use "uri" here. Let's just make sure it's a considered decision and not an accidental inconsistency.

Counting headers, URL and start lines separately because it was counting everything as headers before the headers_done state, throwing HPE_HEADER_OVERFLOW errors even when what really caused the overflow was the request URL being too long Creating an HPE_URL_OVERFLOW err to make sure embedders still have a request line limit, but now differentiating what went over such limit. Refs: nodejs/node#26553

caiolrm · 2019-03-16T20:55:29Z

@bnoordhuis that would render the "--http-max-header-size" option unable to limit the actual header size when using the legacy parser. Could you take a look at nodejs/http-parser#463? I splitted the counting there too, maybe that's better, considering we would only need to do something like this:

  const uint32_t max_http_uri_size =
      per_process::cli_options->max_http_uri_size;
  http_parser_set_max_uri_size(max_http_uri_size);

and the options would work for both parsers after that

@mcollina just to clear it up, are you talking about 16KB for headers and 8KB for url?

@Trott I agree with you, there seems to be a preference for url over uri everywhere else in the code.

vsemozhetbyt · 2019-03-17T21:14:06Z

doc/api/http.md

+
+* {number}
+
+Read-only property specifying the maximum allowed size of HTTP request uri


Suggested change

Read-only property specifying the maximum allowed size of HTTP request uri

Read-only property specifying the maximum allowed size of HTTP request URI

vsemozhetbyt · 2019-03-17T21:16:57Z

doc/api/http.md

+
+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.


Missing bottom references. Will this option be documented later?

mcollina · 2019-03-18T09:33:47Z

@mcollina just to clear it up, are you talking about 16KB for headers and 8KB for url?

I'm talking about 16KB total. I'd leave the division of those an implementation detail. If possible, I would limit the url to 7KB. I would also let headers use whatever is left in the "budget".

mcollina

This PR has changes specific to llhttp and http_parser. Can you add a test that exercise both paths?

See https://github.com/nodejs/node/blob/93e384a75bd5ff8b528437e9c8a937575d27bb59/test/sequential/test-set-http-max-http-headers.js.

mcollina · 2019-03-18T09:35:22Z

test/sequential/test-http-max-http-headers.js

-  // /, Host, localhost, Agent, node, X-CRASH, a...
-  const currentSize = 1 + 4 + 9 + 5 + 4 + 7;
+  // Host, localhost, Agent, node, X-CRASH, a...
+  const currentSize = 4 + 9 + 5 + 4 + 7;


Can you please explain this change?

currentSize is being used by fillHeaders to subtract from the max header size and generate a header value with a length that makes the total header size hit the limit. Since the implementation at node_http_parser_impl.h when using llhttp doesn't count the url size towards the header size after the changes, there's no point considering the "/" size when using fillHeaders anymore.

mcollina · 2019-03-18T09:36:46Z

test/parallel/test-http-uri-overflow.js

+  let received = '';
+
+  c.on('connect', mustCall(() => {
+    c.write(PAYLOAD);


It is highly possible that the payload comes out of the other side of the socket as a single Buffer. Can you please add a test where the URL is split into two or more chunks?

mcollina · 2019-03-18T09:37:07Z

test/parallel/test-http-header-overflow.js

+    return maxHeaderSize - HEADER_NAME.length - HEADER_SEPARATOR -
+      (2 * CRLF.length) + 1;
+  }
+};


Can you add some comments in this function?

lirantal · 2019-03-22T16:37:21Z

I'm talking about 16KB total. I'd leave the division of those an implementation detail. If possible, I would limit the url to 7KB. I would also let headers use whatever is left in the "budget".

@mcollina if we're updating the total request size to 16KB, perhaps we should conform with rfc7230 recommendation of request-line lengths of 8000 octets ?

sam-github · 2019-03-22T17:44:13Z

Reference:

Various ad hoc limitations on request-line length are found in
practice. It is RECOMMENDED that all HTTP senders and recipients
support, at a minimum, request-line lengths of 8000 octets.

https://tools.ietf.org/html/rfc7230#section-3.1.1

I looked, but not equivalent recommendation exists for header section as a whole, not that I found:

HTTP does not place a predefined limit on the length of each header
field or on the length of the header section as a whole, as described
in Section 2.5. Various ad hoc limitations on individual header
field length are found in practice, often depending on the specific
field semantics.

A server that receives a request header field, or set of fields,
larger than it wishes to process MUST respond with an appropriate 4xx
(Client Error) status code.

https://tools.ietf.org/html/rfc7230#section-3.2

fhinkel · 2019-10-28T23:43:14Z

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

Joshsteverson · 2019-10-29T14:33:46Z

I have been waiting on this PR to be merged. The --max-http-header-size flag does not seem to account for large uri strings. What is the suggested path forward for URI (with query parameters) that exceeds 8K? I am supporting a legacy system that sends very large form data URI parameters.

sam-github · 2019-10-29T14:53:56Z

@nodejs/http could use more reviews

addaleax

This seems mostly fine to me, but it definitely needs a rebase now that the legacy parser has been removed…

addaleax · 2019-10-29T15:11:46Z

src/node_options.cc

+  AddOption("--max-http-uri-size",
+            "set the maximum size of HTTP request uri (default: 7KB)",
+            &PerProcessOptions::max_http_uri_size,
+            kAllowedInEnvironment);


Fwiw, this should not be a per-process but rather a per-Environment option.

addaleax · 2019-10-29T15:14:59Z

@Joshsteverson If you are invested in this PR, I think you can feel free to pick it up and open a new PR based on the code in this one. Usually, all that a PR needs to eventually land is an active author.

jasnell · 2020-06-25T14:06:22Z

Looks like this stalled out and would need to be updated in order for it to move forward. Closing but it can be reopened if someone wants to pick it back up.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 9, 2019

BridgeAR reviewed Mar 9, 2019

View reviewed changes

doc/api/http.md Show resolved Hide resolved

bnoordhuis reviewed Mar 10, 2019

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

src/node_http_parser_impl.h Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

BridgeAR requested review from lpinca and mcollina March 10, 2019 13:28

This comment has been minimized.

Sign in to view

lpinca approved these changes Mar 10, 2019

View reviewed changes

test/parallel/test-http-uri-overflow.js Outdated Show resolved Hide resolved

lpinca reviewed Mar 10, 2019

View reviewed changes

test/parallel/test-http-uri-overflow.js Outdated Show resolved Hide resolved

lpinca added http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 10, 2019

lpinca added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 10, 2019

lpinca reviewed Mar 10, 2019

View reviewed changes

test/sequential/test-http-max-http-headers.js Outdated Show resolved Hide resolved

caiolrm added 7 commits March 10, 2019 17:52

http: fix style and naming

3218828

http: fix doc added version to replaceme

197d264

http: fixing comment because lint

e7ade8b

http: requiring common first

78dfcf7

http: splitting the limit check of header and uri

710e830

caiolrm force-pushed the creating-http-max-uri-limit branch from ba4861e to 38307dc Compare March 10, 2019 20:55

http: fix the comment to reflect the actual currentSize composition

93e384a

jasnell approved these changes Mar 17, 2019

View reviewed changes

vsemozhetbyt reviewed Mar 17, 2019

View reviewed changes

mcollina reviewed Mar 18, 2019

View reviewed changes

BridgeAR self-requested a review April 3, 2019 02:23

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

fhinkel closed this Oct 28, 2019

sam-github reopened this Oct 29, 2019

addaleax reviewed Oct 29, 2019

View reviewed changes

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

jonchurch mentioned this pull request Jan 23, 2020

ERR_EMPTY_RESPONSE when GET URL is too long expressjs/express#3322

Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

jasnell closed this Jun 25, 2020

nflaig mentioned this pull request Jun 26, 2023

fix: return 431 status code on HTTP header overflow error fastify/fastify#4856

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: create an option for setting a maximum size for uri parsing #26553

http: create an option for setting a maximum size for uri parsing #26553

caiolrm commented Mar 9, 2019 •

edited

Loading

bnoordhuis left a comment

This comment has been minimized.

lpinca commented Mar 10, 2019

This comment has been minimized.

lpinca commented Mar 10, 2019 •

edited

Loading

lpinca left a comment

lpinca commented Mar 10, 2019

lpinca commented Mar 10, 2019 •

edited

Loading

caiolrm commented Mar 10, 2019

lpinca commented Mar 10, 2019

mcollina commented Mar 15, 2019

Trott commented Mar 15, 2019

richardlau commented Mar 15, 2019

Trott commented Mar 16, 2019

caiolrm commented Mar 16, 2019

vsemozhetbyt Mar 17, 2019

vsemozhetbyt Mar 17, 2019

mcollina commented Mar 18, 2019 •

edited

Loading

mcollina left a comment

mcollina Mar 18, 2019

caiolrm Mar 21, 2019 •

edited

Loading

mcollina Mar 18, 2019

mcollina Mar 18, 2019

lirantal commented Mar 22, 2019

sam-github commented Mar 22, 2019

fhinkel commented Oct 28, 2019

Joshsteverson commented Oct 29, 2019

sam-github commented Oct 29, 2019

addaleax left a comment

addaleax Oct 29, 2019

addaleax commented Oct 29, 2019

jasnell commented Jun 25, 2020


		* {number}

		Read-only property specifying the maximum allowed size of HTTP request uri

http: create an option for setting a maximum size for uri parsing #26553

http: create an option for setting a maximum size for uri parsing #26553

Conversation

caiolrm commented Mar 9, 2019 • edited Loading

Checklist

bnoordhuis left a comment

Choose a reason for hiding this comment

This comment has been minimized.

lpinca commented Mar 10, 2019

This comment has been minimized.

lpinca commented Mar 10, 2019 • edited Loading

lpinca left a comment

Choose a reason for hiding this comment

lpinca commented Mar 10, 2019

lpinca commented Mar 10, 2019 • edited Loading

caiolrm commented Mar 10, 2019

lpinca commented Mar 10, 2019

mcollina commented Mar 15, 2019

Trott commented Mar 15, 2019

richardlau commented Mar 15, 2019

Trott commented Mar 16, 2019

caiolrm commented Mar 16, 2019

vsemozhetbyt Mar 17, 2019

Choose a reason for hiding this comment

vsemozhetbyt Mar 17, 2019

Choose a reason for hiding this comment

mcollina commented Mar 18, 2019 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina Mar 18, 2019

Choose a reason for hiding this comment

caiolrm Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

mcollina Mar 18, 2019

Choose a reason for hiding this comment

mcollina Mar 18, 2019

Choose a reason for hiding this comment

lirantal commented Mar 22, 2019

sam-github commented Mar 22, 2019

fhinkel commented Oct 28, 2019

Joshsteverson commented Oct 29, 2019

sam-github commented Oct 29, 2019

addaleax left a comment

Choose a reason for hiding this comment

addaleax Oct 29, 2019

Choose a reason for hiding this comment

addaleax commented Oct 29, 2019

jasnell commented Jun 25, 2020

caiolrm commented Mar 9, 2019 •

edited

Loading

lpinca commented Mar 10, 2019 •

edited

Loading

lpinca commented Mar 10, 2019 •

edited

Loading

mcollina commented Mar 18, 2019 •

edited

Loading

caiolrm Mar 21, 2019 •

edited

Loading