Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase HTTP_MAX_HEADER_SIZE to 16kb #27645

Closed
southpolesteve opened this issue May 10, 2019 · 21 comments
Closed

Increase HTTP_MAX_HEADER_SIZE to 16kb #27645

southpolesteve opened this issue May 10, 2019 · 21 comments
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. memory Issues and PRs related to the memory management or memory footprint. security Issues and PRs related to security.

Comments

@southpolesteve
Copy link

southpolesteve commented May 10, 2019

Is your feature request related to a problem? Please describe.
Node recently introduced a security fix that decreased the max header size to 8kb 1860352. This has caused problems for users who are using APIs that pass large amounts of data via headers. One example: Azure/azure-cosmos-js#221. Node users running in managed environments like FaaS cloud services are unable to set the --max-http-header-size CLI flag. More examples in #24692

Describe the solution you'd like
Increase the max header size to 16kb. This StackOverflow issue outlines the max header size for various web servers and 16kb is the maximum for IIS which is used in many of our APIs. It seems to me that 16kb is a more reasonable default with the widespread usage of IIS. Assuming of course that 16kb is still secure given the vulnerability present when the default value was 80kb.

Describe alternatives you've considered
In our case, I am also working to solve this issue on the API side, but in some cases, for legacy production APIs it is just not possible to decrease our header usage.

@mcollina @bnb

@Trott
Copy link
Member

Trott commented May 10, 2019

@nodejs/http @nodejs/security @nodejs/security-wg

@sam-github
Copy link
Contributor

@southpolesteve

I fixed a typo in the CLI flag name in the description.

Wrt.

Node users running in managed environments like FaaS cloud services are unable to set the --max-http-header-size CLI flag.
Many PAAS allow setting env vars even if the CLI is hard to access, have you considered setting NODE_OPTIONS=--max-http-header-size=16000?

It is hard to find a reasonable default, some kind of argument best on common secure practice would be convincing for me.

I notice NGINX isn't in the list you linked to, would be interesting to know what its limit is. Also, its conclusion was:

To be accepted by all web servers above, a request’s request line plus header fields should not exceed 8190 Bytes.

Of course, that's the send limit, it doesn't mean that should be the receive limit.

@sam-github
Copy link
Contributor

nginx: 8K -- http://nginx.org/en/docs/http/ngx_http_core_module.html#large_client_header_buffers

anyone have the energy to look up the PAAS provider's defaults? :-)

@mcollina
Copy link
Member

The argument is convincing for me, and we picked NGINX default.

I'll test a value of 16KB with the actual exploit for this DoS attack. Based on my previous analysis, I think it's not going to be problematic, but it's better to check.

@ChALkeR ChALkeR added http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. security Issues and PRs related to security. labels May 12, 2019
@Antonius-S
Copy link

According to this, IIS only occupies 8.5% of the market.

@mcollina
Copy link
Member

I'm sorry it took a long time for doing the due diligence on my side.

8 KB -> peak RSS is 400MB
16 KB -> peak RSS is 800MB
32KB -> peak RSS is 1.4GB

The server does not crash for any of those values.

Keeping 8 KB is safer, however 16KB might provide a good default as well.

cc @nodejs/tsc.

@sam-github
Copy link
Contributor

I would be OK with moving to 16K as the default.

Safest is to explicitly set the limit to your application's needs, which we support.

For default, don't we need safest, just reasonable. There is a wide range of reasonable, 8K is reasonable, but bumping up to IIS's default seems like it will reduce pain, with not so much extra exposure to DoS.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 13, 2019

According to this, IIS only occupies 8.5% of the market.

And Apache and Nginx both have the default maximum at 8k, and they occupy 85% of the market, according to the link above.

I don't think that we should increase the default over 8k, though I am not going to block it.
The memory effect seems significant.

@southpolesteve Am I correct that the problem is caused by inability to configure the default parameter? Do those systems allow passing in NODE_OPTIONS, as mentioned above? If no -- is there no way to affect that, e.g. by setting process.env.NODE_OPTIONS and launching node via fork? If that is also not possible (why?) would a runtime API for setting that option help?

@ChALkeR ChALkeR added the memory Issues and PRs related to the memory management or memory footprint. label Jun 13, 2019
@southpolesteve
Copy link
Author

The 8.5% number linked applies only to websites. I don't think it should have much weight here. This issue is more likely to be encountered accessing an API that passes lots of data via headers. IIS fronts a significant portion of Microsoft and Azure APIs, those of our customers, and transitively our customer's customers.

@southpolesteve Am I correct that the problem is caused by inability to configure the default parameter?

Inability, yes. In Functions-as-a-service environments, this is sometimes not possible. Our customers have figured out some workarounds for Azure App Service (linked in the issue body above), but I am unsure if those also work for Azure Functions.

A runtime API is interesting. I could increase it in our SDK, but that would bring new security issues? A package could set it arbitrarily high and expose users to the CVE. I'm not sure if that is in scope for the node.js project. Certainly possible to do plenty of nasty things now in npm packages.

Unwillingness to set the flag is also an issue. I have a major customer who has the technical ability to set --max-http-header-size but is unwilling to due to the linked CVE. I can't go into much detail, but you can imagine that setting a flag associated with a CVE is not an easy sell to an enterprise security team. To be clear, they bear some responsibility for that situation! But it would be nice if node helped by changing the default.

@Antonius-S
Copy link

Unwillingness to set the flag is also an issue. I have a major customer who has the technical ability to set --max-http-header-size but is unwilling to due to the linked CVE. I can't go into much detail, but you can imagine that setting a flag associated with a CVE is not an easy sell to an enterprise security team. To be clear, they bear some responsibility for that situation! But it would be nice if node helped by changing the default.

Ehm... I can't get your point. You say that one doesn't want to set an obvious run-time parameter to some value but will accept if default setting would be changed to that value?

@sam-github
Copy link
Contributor

@Antonius-S Your summary makes sense to me! If we are comfortable with a value being the default, they are comfortable, but they don't want to override our opinion and be responsible for their decision to override our opinion.

@Antonius-S
Copy link

@sam-github sounds reasonable. And there's really no way to conveniently redefine the limit in runtime. Probably adding appropriate parameter to process.argv or process.env will do the trick?

@southpolesteve
Copy link
Author

@Antonius-S @sam-github you have it correct. Node blessing a default has a significant benefit over a user override of the same value. I've opened a PR over in http_parser as it looks like this default is set there.

@mcollina
Copy link
Member

mcollina commented Jul 4, 2019

It's possible to provide a way to change this value via an API in Node 12: as you can see in

if (header_nread_ >= per_process::cli_options->max_http_header_size) {
, the max amount of headers to read is read from the options. In Node < 12, this is set globally at startup, and it's way harder to change.

The best solution should be to add an option to set this behavior for each connection/server via an option in the server and client. @southpolesteve would you like to give that a shot?

@fresheneesz
Copy link

Yes, please let us programatically configure the header size! Node.js's runtime should not be imposing arbitrary limits that aren't able to be changed by configuration options. Its really annoying to have to pass in a special flag in order to make my program work. I'm not able to control the header size on my end.

@addaleax
Copy link
Member

I think the solution suggested by @mcollina should be quite a bit easier to implement now that we’ve removed the legacy HTTP parser in Node.js 13 :)

@shastriUF
Copy link

shastriUF commented Nov 11, 2019

@addaleax I found this issue from the Node Todo page, is this something I could help with? My understanding is that the solution is to not change the default in node_options.cc, but instead pass in a 16kb value to --max-http-header-size from the client/server definition, is that correct? But it's not clear to me where the "server" and "client" code @mcollina mentions are located? I'm new to this codebase so any help is appreciated!

Edit:
Thinking about this more, do you mean we should add a new CLI option "--use_extended_max_http_header_size" (or something like that) and update the validation code in node_http_parser::TrackHeader() to also check for this new CLI option before setting the error?

@adarshsaraogi
Copy link

@southpolesteve I will like to work on this issue can you please mentor or else give me some other issue to kick start my contribution in NODE core.

@shastriUF
Copy link

@addaleax @southpolesteve Well that's time I wasted I won't be getting back... Would have appreciated the feedback that all you wanted to change was the default, and not extend the CLI option.

@addaleax
Copy link
Member

@shastriUF I’m sorry nobody replied to your comment (neither me nor somebody else), in a project as big as Node.js sometimes these things simply get lost. Pings and bumping a thread are appreciated in those cases.

That being said, the issue description mentions that using CLI options is part of the issue here.

@shastriUF
Copy link

@addaleax Thanks for the explanation, I'll try to follow up more frequently in the future.

addaleax pushed a commit that referenced this issue Mar 30, 2020
Fixes: #27645

PR-URL: #32520
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. memory Issues and PRs related to the memory management or memory footprint. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants