Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Request with Transfer-Encoding: chunked and Content-Length is valid per RFC, but rejected with HPE_UNEXPECTED_CONTENT_LENGTH #517

Closed
veshij opened this issue May 21, 2020 · 18 comments

Comments

@veshij
Copy link
Contributor

veshij commented May 21, 2020

Current implementation rejects requests with Transfer-Encoding: chunked and Content-Length headers with HPE_UNEXPECTED_CONTENT_LENGTH.
https://github.com/nodejs/http-parser/blob/master/http_parser.c#L1804-L1815

But per https://tools.ietf.org/html/rfc7230#section-3.3.3 that's a valid http request, Content-Length must be ignored:

       If a message is received with both a Transfer-Encoding and a
       Content-Length header field, the Transfer-Encoding overrides the
       Content-Length. 

llhttp parser has the correct behavior: https://github.com/nodejs/llhttp/blob/master/src/native/http.c#L50

@indutny
Copy link
Member

indutny commented May 21, 2020

There is one more sentence right after your quote:

Such a message might indicate an attempt to
perform request smuggling (Section 9.5) or response splitting
(Section 9.4) and ought to be handled as an error.

@indutny
Copy link
Member

indutny commented May 21, 2020

I believe that there is an identical code path in http-parser with regards to handling this.

@euroelessar
Copy link

One more sentence later it states how this requests must be handled by proxies, which indicates that they should be processed, not outright rejected:

A sender MUST
remove the received Content-Length field prior to forwarding such
a message downstream.

@indutny
Copy link
Member

indutny commented May 21, 2020

😄

ought / ɔt /

   (used to express duty or moral obligation): Every citizen ought to help.

So "ought to be handled as an error" at best translates as "SHOULD be handled as an error".


There is a paragraph in RFC 6919 as well (even though it is not referenced in RFC 7230):

 The phrase "OUGHT TO" conveys an optimistic assertion of an
   implementation behavior that is clearly morally right, and thus does
   not require substantiation.

@indutny
Copy link
Member

indutny commented May 21, 2020

I'm inclined to close this issue as it seems that the requirements of protocol specification are very clear on this.

@indutny indutny closed this as completed May 21, 2020
@veshij
Copy link
Contributor Author

veshij commented May 21, 2020

Also nginx's header parser: https://github.com/nginx/nginx/blob/76ac67b36f2db9acb2aeb4672f0aeaa8008e7d93/src/http/ngx_http_request.c#L1954-L1962

I'm inclined to close this issue as it seems that the requirements of protocol specification are very clear on this.

Can't agree with that. The whole paragraph is about how to handle this situation instead of 'server MUST treat it as protocol error'.

@SaveTheRbtz
Copy link

FWIW i would agree with @veshij that in RFC terms "MUST" beats both "ought to" and "SHOULD". Also smuggling is not an issue if "Content-Length" is removed.

@indutny
Copy link
Member

indutny commented May 21, 2020

MUST obviously applies only when the request gets "proxied"/forwarded to downstream:

A sender MUST remove the received Content-Length field prior to forwarding such a message downstream.

If there is an error during the parsing step and it doesn't get forwarded at all - the above statement has no effect.

@veshij
Copy link
Contributor Author

veshij commented May 21, 2020

Is there any downsides which I might be missing in trying to serve traffic with removed Content-Length header as RFC allows such behaviour? As @SaveTheRbtz mentioned - smuggling won't be an issue.

There are some amount of clients in wild which behave in such way (both headers set) and they are successfully served if there is a proxy such an nginx in front of application using http-parser, but it's not possible to use http-parser in a proxy itself.

@indutny
Copy link
Member

indutny commented May 21, 2020

This is a good question. It is not convincing that removing Content-Length necessarily prevents smuggling attacks. My understanding is that smuggling implies inconsistencies in the handling of the request when proxying it. What if there is one more proxy layer in front of http-parser based proxy? What if that layer used Content-Length instead of Transfer-Encoding for determining body length? There seem to be dangerous scenarios that won't be ruled out if http-parser would be more lenient.

With this in mind, let's see if other maintainers have different opinion on this: @bnoordhuis @mscdex @nodejs/http

@jasnell
Copy link
Member

jasnell commented May 21, 2020

Just for historical context...The behavior for proxies removing content-length exists specifically because attackers were leveraging differences in how various implementations handled the presence of the content-length header. Specifically, some would give the chunked encoding precedence while others would give content length precedence. This difference could be used by attackers to effectively smuggle multiple requests through a proxy that implemented one behavior to an origin server implementing the other.

Node.js is not designed as a proxy (even if it can be used as such) so the must requirement that the spec assigns to proxies does not apply here. And our strict handling of the header when using chunked falls is compliant with the spec which recommends that it be treated as an error. That said, the spec also gives us room to be lenient and simply ignore the header, in which case we should not pass it on to application code. I would not want to change the default strict behavior, but adding an option to allow more lenient handling would be fine I think so long as the presence of the header is completely ignored.

@jasnell
Copy link
Member

jasnell commented May 21, 2020

Heh... I just saw that this issue is in the http-parser repo and not the node repo... Same answer tho ;) providing an option for lenient handling should be fine.

@indutny indutny reopened this May 21, 2020
@bnoordhuis
Copy link
Member

To summarize: the ask is to allow both Transfer-Encoding and Content-Length? Opt-in?

What are the exact semantics?

  1. Ignore Content-Length? Yes/no (currently no)
  2. Ignore Content-Length if present twice? Yes/no (currently no and not because of 1.)
  3. Set parser->content_length? Yes/no (see also 2. See also Consolidate duplicate content-length response headers #435)

How should the opt-in work? There's not much wiggle room in struct http_parser, global flags are icky, and stretching the definition of parser->lenient_http_headers further might be dangerous.

@veshij
Copy link
Contributor Author

veshij commented May 28, 2020

My personal opinion based on reading RFC:

To summarize: the ask is to allow both Transfer-Encoding and Content-Length? Opt-in?

Consider requests with both Transfer-Encoding: chunked and Content-Length set as valid and ignore CL header during parsing. Opt-in or default - both options work for us, I can cut a change for envoy to support opt-in.

Ignore Content-Length? Yes/no (currently no)

Ignore Content-Length if Transfer-Encoding is chunked

Ignore Content-Length if present twice? Yes/no (currently no and not because of 1.)

Same as above, ignore Content-Length if Transfer-Encoding is chunked

Set parser->content_length? Yes/no (see also 2. See also #435)

Same as above, ignore Content-Length if Transfer-Encoding is chunked, don't set parser->content_length.

@veshij
Copy link
Contributor Author

veshij commented May 28, 2020

Also as there is some active work on envoyproxy/envoy#5155 - It'd be nice to have consistent change in llhttp library.

@veshij
Copy link
Contributor Author

veshij commented May 29, 2020

What do you think @bnoordhuis @indutny ?

@veshij
Copy link
Contributor Author

veshij commented Jun 3, 2020

How should the opt-in work?

will additional field in settings work?

@veshij
Copy link
Contributor Author

veshij commented Jun 9, 2020

Can someone take a look at the PR, please?

bnoordhuis pushed a commit to bnoordhuis/http-parser that referenced this issue Jul 10, 2020
Fixes: nodejs#517
PR-URL: nodejs#518
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Pierce Lopez <pierce.lopez@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants