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

Make HTTP_MAX_HEADER_SIZE configurable #24692

Closed
zauberpony opened this issue Nov 28, 2018 · 30 comments · Fixed by #24811

Comments

@zauberpony
Copy link

commented Nov 28, 2018

Is your feature request related to a problem? Please describe.

The recent limitation to HTTP_MAX_HEADER_SIZE (1860352) to mitigate CVE-2018-12121 is a problem for us.

We use headers internally to communicate the users' session, and sometimes (legitimate) requests from "outside" exceeed the 8 kb limit, too.
Given that JWT-strings easily exceed 1kb, I think the 8kb limit might be too little for others, too.
Or referrer-headers (especially in combination with payment-systems back-and-forth) tend to exceed 1kb, too.

Describe the solution you'd like
Have the possibility to configure the HTTP_MAX_HEADER_SIZE — at least via configuration-flag (at node-compile-time).
Setting this at run-time or at startup time would be nice, too.

Is setting this at compile time already possible? I couldn't find the option or best way to do it for node-gyp/gyp.

Describe alternatives you've considered

patching nodejs at compile time… not a good idea.

Reduce headers, yeah, would be nice, but that means completely changing parts of our architecture.

@mcjfunk

This comment has been minimized.

Copy link

commented Nov 28, 2018

Major +1 here. Our large enterprise is not in the position to reduce the possible headers that flow through our infrastructure to 8k. Agree that patching node at compile time is not a good solution for us.

@mpaw

This comment has been minimized.

Copy link

commented Nov 29, 2018

We are running into the same issue. The drop from 80kb to 8kb is forcing us to stick with the previous version of node until this is resolved.

This needs to be configurable either through a command line option, environment variable, or in code. A compile time flag is not a good solution for us (or probably 99% of other devs) either.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

Sorry if I missed the conversation somewhere, but do we have the option of updating http_parser (or floating a patch on it) to make the max header size configurable at the parser level?

I did a little experimenting (definitely not PR ready at this point) at cjihrig@e55765d, and was able to adjust the max header size. It breaks ABI, but I'm not sure if we worry about that with the http parser.

cc: @bnoordhuis

EDIT: I don't think I would attach the max header size to the parser, but maybe add a function that sets the global max header size. That shouldn't break ABI. (cjihrig@fb615c5)

@foo4u

This comment has been minimized.

Copy link

commented Nov 29, 2018

It's also a major issue that this went out as a patch level fix to LTS editions. This is a breaking change.

Ideally this 8K hard limit commit should be reverted and new LTS releases cut.

@brycesteinhoff

This comment has been minimized.

Copy link

commented Dec 1, 2018

This affects us also, as our headers are right around 8k usually, but sometimes more (we have JWT tokens which account for > half of this).

Our base Docker images were locked to a Node major version, so this breaking change appeared out of nowhere when the upstream image updated the Node minor version.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

It breaks ABI, but I'm not sure if we worry about that with the http parser.

http-parser does, as do distros that link node to http-parser dynamically.

A possible way forward: split p->nread into two, p->nread and p->nread_max, possibly with better names. :-)

Drawback: there are some downstream projects that read p->nread, even though it's marked as private.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2018

@bnoordhuis what about a function that would allow setting the global max header size? It's not necessarily the most elegant solution ever, but I don't think it would break anything.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

That could work. Two issues with the patch:

  • Don't use a k prefix if it's not actually constant.
  • Set the limit once, not every time a new parser is instantiated.
@cjihrig cjihrig referenced this issue Dec 3, 2018
4 of 4 tasks complete
@elad-yosifon

This comment has been minimized.

Copy link

commented Dec 4, 2018

A week of debugging finally brought me here....
could of been easily solved if there was a simple debug log line that indicates that the request was blocked

@siboulet

This comment has been minimized.

Copy link

commented Dec 19, 2018

The biggest problem with this 8KB header limit change in my opinion is that it also applies to outbound HTTP requests response header parsing. Without any sort of runtime maximum HTTP header size parameter that can be defined in http request, Node.js will start throwing HPE_HEADER_OVERFLOW exceptions when parsing responses from external HTTP API calls that have large response headers (which isn't so uncommon when you factor in JWT and CSP headers).

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

@mcollina Have you seen the above comment?

@mcollina

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Thanks @Fishrock123 for the ping! I didn't see the message.

The biggest problem with this 8KB header limit change in my opinion is that it also applies to outbound HTTP requests response header parsing. Without any sort of runtime maximum HTTP header size parameter that can be defined in http request, Node.js will start throwing HPE_HEADER_OVERFLOW exceptions when parsing responses from external HTTP API calls that have large response headers (which isn't so uncommon when you factor in JWT and CSP headers)

From a technical perspective, I think we could make this setting per-instance of the http parser once we switch to llhttp. I think should be our target.
Note that #24811 is going to make this configurable with a startup option, which is a step in the right direction.

@siboulet Regarding the default limit, I'm open to increase it to 10KB or 12KB if it's common to have more than 8KB of headers data. Our assumption was that 8KB was plenty. Would you mind opening a new issue about changing the default value, and making some examples of requests that will trigger this?

cjihrig added a commit to cjihrig/node-1 that referenced this issue Dec 20, 2018
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
cjihrig added a commit to cjihrig/node-1 that referenced this issue Dec 20, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
cjihrig added a commit to cjihrig/node-1 that referenced this issue Dec 20, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
cjihrig added a commit to cjihrig/node-1 that referenced this issue Dec 20, 2018
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
cjihrig added a commit to cjihrig/node-1 that referenced this issue Dec 20, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
cjihrig added a commit to cjihrig/http-parser that referenced this issue Dec 20, 2018
support overriding HTTP_MAX_HEADER_SIZE at runtime
This commit adds http_parser_set_max_header_size(), which can
override the compile time HTTP_MAX_HEADER_SIZE value.

Fixes: nodejs/node#24692
Refs: nodejs/node#24811
PR-URL: nodejs#453
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@1I1III1liL1

This comment has been minimized.

Copy link

commented Dec 20, 2018

could of been easily solved if there was a simple debug log line that indicates that the request was blocked

I think this is one of the biggest issues with introducing a breaking change without updating the major version. What was working suddenly isn't, and I've yet to see any log files that showed me why the request was being rejected. Sending a 400 with no details and no server-side log doesn't feel like the right way to go about it. Maybe I'm missing where that log would be generated?

MylesBorins added a commit that referenced this issue Dec 21, 2018
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins added a commit that referenced this issue Dec 21, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins added a commit to MylesBorins/node that referenced this issue Dec 21, 2018
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins added a commit that referenced this issue Dec 22, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25168
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins added a commit that referenced this issue Dec 25, 2018
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: #25168
PR-URL: #24811
Fixes: #24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins added a commit that referenced this issue Dec 25, 2018
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: #25168
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #24811
Fixes: #24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
refack added a commit to refack/node that referenced this issue Jan 14, 2019
deps: cherry-pick http_parser_set_max_header_size
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
refack added a commit to refack/node that referenced this issue Jan 14, 2019
cli: add --max-http-header-size flag
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@yonjah

This comment has been minimized.

Copy link

commented Mar 12, 2019

From a technical perspective, I think we could make this setting per-instance of the http parser once we switch to llhttp. I think should be our target.

@mcollina is this still a target ? is there any issue tracking it ?
Couldn't find any open issues for it.

Thanks

@mcollina

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

This was solved in cjihrig/http-parser@0ae8d93. Is there anything that is not clear?

@yonjah

This comment has been minimized.

Copy link

commented Mar 12, 2019

Sorry maybe I misunderstood your comment.

I thought it references an option to set it per instance of the http server. like-
http.createServer({maxHeaderSize: 8000}, (req, res) => {...})

So if I have two servers running in the same process I can have for example an internal one that has higher limit than the public facing one.

It's not a major issue, but I was mostly hoping I can enforce the size limit from code so I wont have to rely on some external variable which feels less reliable

@mcollina

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@yonjah

This comment has been minimized.

Copy link

commented Mar 12, 2019

Isn't it read only ? so I can see if it was set to a different value but I can't change it.

I don't mind an option to change it per process but I don't want to rely on the cli to set it

@tony-gutierrez

This comment has been minimized.

Copy link

commented Apr 9, 2019

We definitely need a way to configure this in code, for use on cloud hosts. Configuring via cli flag is not always possible.

@tony-gutierrez

This comment has been minimized.

Copy link

commented Apr 9, 2019

Even better a rever for LTS versions.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@tony-gutierrez you can set any of the command-line options via an environment variable NODE_OPTIONS is that sufficient? As this change came as a security fix we will not be reverting the change

@yonjah

This comment has been minimized.

Copy link

commented Apr 10, 2019

@MylesBorins at least in my use case there is no real than setting it via NODE_OPTIONS
especially since it will fail to run node if the option does not exist.
So running the app requires different command line/env variable depending on the node version it runs
Which can be a bit of a pain (not major).

Not sure about the internals but is there any technical limitation that this can't be set per HTTP server instance ?

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@yonjah this was backporting all the way back to 6.x... if the flag doesn't exist the version of node most likely does not have the patch to change header size.

Regarding the change itself... the header size is set in the C++ http parser... this is not something that can be changed on a per request basis and is something that needs to be set a single time for the runtime at startup. The variable that can be accessed on http.maxHeaderSize is sharing the value that has been set, but by the time it is accessed the C++ code has already been run and cannot be changed

@oshihirii

This comment has been minimized.

Copy link

commented Jul 22, 2019

can anyone please tell me what header and query string limits are in node version v10.16.0?

i am getting 400 errors on requests with long query strings (eg ~15200 bytes) and wondering if it could be caused by some sort of "max limit".

thank you.

@rosswilson

This comment has been minimized.

Copy link

commented Jul 22, 2019

@oshihirii

This comment has been minimized.

Copy link

commented Jul 22, 2019

thank you very much @rosswilson.

just to clarify - are query strings included in the 8kb header limit?

also, wondering why:

https://nodejs.org/api/http.html#http_http_maxheadersize

states:

http.maxHeaderSize was added in: v11.6.0

whereas the 10.15.0 changelog lists the addition of the maxHeaderSize property as a notable change.

just curious to know when the 80kb limit was decreased to 8kb.

thanks again.

@rosswilson

This comment has been minimized.

Copy link

commented Jul 22, 2019

@oshihirii I'm not sure, perhaps someone else here can help you with those questions.

@zauberpony

This comment has been minimized.

Copy link
Author

commented Jul 22, 2019

According to my research (i.e. looking at the code) when opening the issue, the request-path (and thus the query-string) does count as header for nodejs, although the RFC for HTTP states otherwise (i.e. path is not a header).
IIRC the Initial Header limit has been introduced with node 10.14.1.

@oshihirii

This comment has been minimized.

Copy link

commented Jul 22, 2019

thanks very much @zauberpony.

interesting, when i console.log(req.headers) it doesn't show the query string, but perhaps req.headers does not actually contain all the information included in the header. i tried logging req.header, but that seems to be a function and therefore not loggable :).

@srinath-ragh

This comment has been minimized.

Copy link

commented Jul 31, 2019

@oshihirii It does seem like query params are considered headers. Tried a long list of query params (about 10kb) on v10.12.0 vs v10.16.0 and 10.12 responds with a 200 while 10.16 gives a 400.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.