Node: Increase HTTP_MAX_HEADER_SIZE to 16kb

[southpolesteve]

Is your feature request related to a problem? Please describe.
Node recently introduced a security fix that decreased the max header size to 8kb https://github.com/nodejs/node/commit/186035243fad247e3955fa0c202987cae99e82db. This has caused problems for users who are using APIs that pass large amounts of data via headers. One example: https://github.com/Azure/azure-cosmos-js/issues/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 https://github.com/nodejs/node/issues/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]

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

[sam-github]

@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]

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]

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.

[Antonius-S]

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

[mcollina]

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]

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]

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?

[southpolesteve]

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]

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]

@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]

@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]

@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]

It's possible to provide a way to change this value via an API in Node 12: as you can see in https://github.com/nodejs/node/blob/39a935883df8cc87e1bd6364df38a61131e1e4ba/src/node_http_parser_impl.h#L821, 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]

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]

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]

@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]

@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]

@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]

@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]

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