http: validate HTTP version

[ShogunPanda]

Fixes nodejs/node#43115 on Node 16+.

[ShogunPanda]

@indutny Any input on this? Does it look fine to you?

[pallas]

Why not check the major and minor after the parse is complete?

[ShogunPanda]

Well, I would say to allow to save some bytes to read and close the bad socket ASAP.

[pallas]

Yes, you can do that during on_status or on_status_complete. Maybe it's worth adding an on_version_complete callback? I just worry that someone is using a non-standard major/minor for something and this would break that.

[ShogunPanda]

Ok, let me think about it and eventually restructured this!

[ShogunPanda]

@pallas

I followed a slightly different approach without using callbacks. How does it look now?
I chose not to introduce a new callback as the version line might be invalid (let's say HTTP/1.1 THIS IS INVALID) and thus we could not really trigger a callback before the status is validated.

About people using invalid HTTP/1.1 version, I don't think we should support that. We are already lenient on many things but I don't think this really applies to anyone. Eventually they can complain and we will consider providing compatibility.

[pallas]

So this parser is used for protocols other than basic HTTP, which is why there are a set of methods for RTSP, ICE, &c.

If you really want to validate 0.9,1.0,1.1,1.2,2.0, could you please add a parser->lenient_flags so that the old path can be taken if it turns out to cause problems? Honestly though, I think this change is kind of an unnecessary constraint on users of the parser and any logic like this should be in the application instead.

[ShogunPanda]

Well, it was reported in Node for security issue.

Adding a new lenient flag will not be a problem, I'll have it ready on Monday!

[pallas]

Sure, maybe it is a security problem, but it's not a problem in the parser, it's a problem in the application calling the parser.

[ShogunPanda]

That's a fair point and I kinda agree.
But as we are stricter in some other places I think we should provide the same approach here.
Lenient flag is the way to go. I'll update this shortly.

[ShogunPanda]

Lenient flag added!

[ShogunPanda]

@indutny What do you think of the approach for checking the version? Is there any other way?

[indutny]

I feel like @pallas has a point. The parser supports multiple protocols and extensions and while we should make the core secure, the version checking sounds like it should be at the very least behind lenient flags.

[ShogunPanda]

It is under a flag now. Do you mean that by default we are lenient and the flag makes it stricter?

Nevermind. I misread.
Anyway, the parser now has a new lenient flag to restore current version parsing and it is now safer by default.
Are we good to land this?

[ShogunPanda]

@indutny @pallas Are we good about this? Can this land?

[ShogunPanda]

I have updated the PR to solve merge conflicts.
Are we ready to land this?

[mcollina]

lgtm

[ShogunPanda]

@indutny What is your take on this? Can we merge it?