Closed veshij closed 4 years ago
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.
I believe that there is an identical code path in http-parser with regards to handling this.
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.
😄
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.
I'm inclined to close this issue as it seems that the requirements of protocol specification are very clear on this.
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'.
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.
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.
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.
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
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.
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.
To summarize: the ask is to allow both Transfer-Encoding
and Content-Length
? Opt-in?
What are the exact semantics?
Content-Length
? Yes/no (currently no)Content-Length
if present twice? Yes/no (currently no and not because of 1.)parser->content_length
? Yes/no (see also 2. See also #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.
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.
Also as there is some active work on https://github.com/envoyproxy/envoy/issues/5155 - It'd be nice to have consistent change in llhttp library.
What do you think @bnoordhuis @indutny ?
How should the opt-in work?
will additional field in settings
work?
Can someone take a look at the PR, please?
Current implementation rejects requests with
Transfer-Encoding: chunked
andContent-Length
headers with HPE_UNEXPECTED_CONTENT_LENGTH. https://github.com/nodejs/http-parser/blob/master/http_parser.c#L1804-L1815But per https://tools.ietf.org/html/rfc7230#section-3.3.3 that's a valid http request,
Content-Length
must be ignored:llhttp parser has the correct behavior: https://github.com/nodejs/llhttp/blob/master/src/native/http.c#L50