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

[veshij]

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]

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]

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

[euroelessar]

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]

😄

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]

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

[veshij]

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]

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]

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]

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]

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]

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]

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.

[bnoordhuis]

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]

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]

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

[veshij]

What do you think @bnoordhuis @indutny ?

[veshij]

How should the opt-in work?

will additional field in settings work?

[veshij]

Can someone take a look at the PR, please?