http: llhttp opt-in insecure HTTP header parsing

[sam-github]

WIP - depends on #30553

Allow insecure HTTP header parsing. Make clear it is insecure.

See:

• deps: update llhttp to 2.0.1 #30553
• HPE_INVALID_HEADER_TOKEN on http requests #27711 (comment)
• HPE_INVALID_HEADER_TOKEN on https requests #30515

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26795/

[indutny]

LGTM with a question.

It'd be great if @nodejs/security @nodejs/http and @jasnell could sign off on this.

[devsnek]

should the flag maybe be --insecure-http-parsing or --insecure-http-header-parsing?

[sam-github]

I think longer options are just more cumbersome. @devsnek are you concerned that some other kind of insecure HTTPness may need enabling, and that we'd need to have yet another option, and it wouldn't be clear?

I'm a bit of the YAGNI view on this, and I worry that "insecure-http-headers" might make people think that its just the headers are insecure, when its worse than that.

But other than that, no strong views, maybe a few other people can weigh in on what the option should be, and once there is is some agreement, I can change it.

Btw, this will need once it lands back on 12.x to be enhanced to update the http-parser, and to trigger it's insecure mode as well. I can do that. attn: @nodejs/lts

[addaleax]

I like the idea of a CLI flag, but I think making this programatically accessible as an option to the HTTP module should happen first or along with it

[sam-github]

I could move it around, but notice that its modelled after the other option to reduce HTTP security (--max-http-header-size): #24811

1. Its only a CLI option
2. Its in PerProcessOptions

aside: @devsnek which reinforces your comment about a different name, I'm open to suggestions.

Wrt to making it js configurable, neither --http-parser (which --insecure-http replaces) or --max-http-header can be changed via js, though I notice there is a unassignable http.maxHeaderSize.

You'd like something similar for this option? I'm not sure what purpose it would have.

[addaleax]

@sam-github My understanding is that both of these restrictions (being per-process + being only a CLI option) come from the inflexibilitiy in which the legacy HTTP parser provides the API, unlike llhttp. This should be changed now that the legacy parser is gone.

I do feel that it makes more sense to be able to provide this on a per-stream basis than setting it as a CLI flag; the issue here is that real-world HTTP servers might not always give technically valid responses, however, inside a single Node.js application I could see different parts that use HTTP differently coexisting. For example, an HTTP client and an HTTP server; the former might want to talk to be able to talk to those other real-world HTTP servers, whereas the server inside the application would probably want to keep being restrictive in the input that it accepts.

[tniessen]

It was probably in one of the other issues, but I remember seeing a comment that cited an RFC that specifies that "user agents" must be able to parse invalid headers. If that is true and my understanding correct, a Node.js application could legitimately make some HTTP requests as a "user agent" and some as a "non-user agent", which would not have to accept invalid headers. Based on that assumption, I agree with @addaleax that being able to enable it per-connection might make more sense. (But I might be completely wrong about it.)

[devsnek]

@tniessen its more about keeping the flag somewhat self documenting. just --insecure-http-parser makes it clear its to do with how http requests are parsed and not do with disabling tls or something.

[tniessen]

I am not talking about the option name, I am referring to @addaleax's suggestion:

I do feel that it makes more sense to be able to provide this on a per-stream basis than setting it as a CLI flag

[addaleax]

I’ve opened a PR for the header size flag, partially based on this PR, in #30570

[devsnek]

oops @tniessen i meant to ping @sam-github on that :)

I'm also a fan of per-stream.

[sam-github]

The impetus for me here is backwards compatibility and sec fixes on LTS. Its both to make a replacement for --http-parser=legacy, and to have a CLI (actually, NODE_OPTIONS) opt-out for apps that break when upgrading 8.x or 10.x to a release that includes the more strict http-parser 2.9.0.

Some users are staying on LTS or the legacy http-parser because their apps break on llhttp... they'd be unpleasantly surprised to find their apps break on LTS as well, which puts us in a hard place between insecure and backwards incompat. We have the theoretical policy that we can break backwards compat in a release line if we need to for security, but I think offering a reversion flag is user friendly, and both parsers have the capability to do this.

For this reason, I'd like a global flag, something that is roughly a feature replacement to --http-parser=legacy, and that requires no src code changes to use. I'll commit to the work of pushing the change down through the LTS branches, adding http-parser support to it in the branches that still have it, and later removing llhttp support in the older branches that don't have llhttp.

I also like the idea of fine-grained js APIs, but could they be addded later, or even concurrently, by someone else? I don't think they would conflict with this much, its only a couple lines of js and c++, its mostly docs and dire warnings.

Probably the setting can be per http.request() setting on the client side, but I'm not sure of http parser lifetime, and since requests all go through the Agent, it could be that request() ends up being insecure for some requests, but not others on the same cached client, and the insecurity could leak between requests. That would be bad, some careful looking around would be needed there.

On the server side, I assume per-stream is not a goal, it would be set on a particular HTTP server and apply to all its clients, though I can't reall whether buggy clients are as common as buggy servers (ahem, Incapsula).

[indutny]

Looks like I’ve missed an edge case in llhttp. The lenient parsing flag resets after a single request/response. Please do not land this until the fix will be complete.

[indutny]

Here is a pull request to address this: nodejs/llhttp#34

[mcollina]

Good work! Would you mind to add an option to http request to enable this for very specific http? I can see some good reasons why somebody would like to make calls with these checks, and avoid them in others.

It seems llhttp support this already, so it's a matter of exposing this option.

[addaleax]

@sam-github I’m good with adding per-stream/per-server support in a follow-up along the lines of #30570 that doesn’t land on v12.x, and I can do that myself if you like.

[indutny]

Force-pushed the branch: #30553 It is ready now 😉 Thanks!