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

http: llhttp opt-in insecure HTTP header parsing #30567

Closed
wants to merge 2 commits into from

Conversation

@sam-github
Copy link
Member

sam-github commented Nov 20, 2019

WIP - depends on #30553

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

See:

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
@sam-github sam-github requested a review from indutny Nov 20, 2019
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

indutny left a comment

LGTM with a question.

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

doc/api/cli.md Outdated Show resolved Hide resolved
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 20, 2019

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

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Nov 20, 2019

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

Copy link
Member

addaleax left a comment

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

src/node_options.h Outdated Show resolved Hide resolved
@addaleax addaleax added the cli label Nov 20, 2019
@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member

addaleax commented Nov 20, 2019

@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

This comment has been minimized.

Copy link
Member

tniessen commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member

devsnek commented Nov 20, 2019

@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

This comment has been minimized.

Copy link
Member

tniessen commented Nov 20, 2019

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 added a commit to addaleax/node that referenced this pull request Nov 20, 2019
Make `maxHeaderSize` a.k.a. `--max-header-size` configurable now that
the legacy parser is gone (which only supported a single global value).

Refs: nodejs#30567
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 20, 2019

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

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 20, 2019

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

I'm also a fan of per-stream.

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

indutny commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

indutny commented Nov 21, 2019

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

Copy link
Member

mcollina left a comment

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

This comment has been minimized.

Copy link
Member

addaleax commented Nov 21, 2019

@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

This comment has been minimized.

Copy link
Member

indutny commented Nov 21, 2019

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

doc/api/cli.md Outdated Show resolved Hide resolved
sam-github added a commit to sam-github/node that referenced this pull request Jan 7, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567
@sam-github sam-github mentioned this pull request Jan 7, 2020
3 of 3 tasks complete
@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 7, 2020

In my attempts to get CI green, I didn't notice that I landed this without the test I'd intended to write. Sorry.

See #31253

sam-github added a commit to sam-github/node that referenced this pull request Jan 8, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567
sam-github added a commit to sam-github/node that referenced this pull request Jan 9, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567
sam-github added a commit that referenced this pull request Jan 9, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- #30567

PR-URL: #31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Jan 9, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jan 9, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jan 9, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567

PR-URL: nodejs#31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567

PR-URL: nodejs#31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567

PR-URL: nodejs#31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jan 10, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567

PR-URL: nodejs#31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos added a commit that referenced this pull request Jan 14, 2020
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- #30553
- #27711 (comment)
- #30515

PR-URL: #30567
Backport-PR-URL: #30473
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Jan 14, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- #30567

PR-URL: #31253
Backport-PR-URL: #30473
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this pull request Jan 16, 2020
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- #30567

PR-URL: #31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.