Jetty.project: Duplicate Content-Length header not handled correctly

[sbordet]

The relevant section of the specification is: https://tools.ietf.org/html/rfc7230#section-3.3.2.

POST / HTTP/1.1
Host: localhost
Content-Length: 1
Content-Length: 2

The request should be rejected.

[sbordet]

Fixed as required by the spec, rejecting the request.

[djscholl]

@sbordet do you have any suggestions for options on undoing this change without forking Jetty? We'd like to upgrade from 9.2 to 9.4, but we know we have existing clients that sometimes provide duplicate content-length headers. We don't want to suddenly start giving them 400s, especially when both content-length headers contain the same value. It's certainly understandable to return a 400 when the values don't match.

[joakime]

What @djscholl is talking about is in https://tools.ietf.org/html/rfc7230#section-3.3.2

   If a message is received that has multiple Content-Length header
   fields with field-values consisting of the same decimal value, or a
   single Content-Length header field with a field value containing a
   list of identical decimal values (e.g., "Content-Length: 42, 42"),
   indicating that duplicate Content-Length header fields have been
   generated or combined by an upstream message processor, then the
   recipient MUST either reject the message as invalid or replace the
   duplicated field-values with a single valid Content-Length field
   containing that decimal value prior to determining the message body
   length or forwarding the message.

[joakime]

This should probably be configurable via a new HttpCompliance option on the parser.

[joakime]

@djscholl frankly, I'm surprised that those problematic clients haven't been impacted by all of the various intermediaries that also reject that kind of request.

[djscholl]

Thanks for the response @joakime. It's likely that these problematic clients do run into other problems. We found at least one existing problematic test client in our testing of the upgrade. The main concern for us is the unknown factor of how often these sorts of requests end up getting all the way to our server. We could add some logging to start detecting how often that happens, but we don't have the historical data to give us any confidence that it never happens.

[djscholl]

@joakime or anyone else - I assume this was done as part of a general cleanup of "invalid" requests. Is there any documented list of all the related changes that were made? We also found that GET requests with non-zero content lengths had their handling changed. The new handling makes sense (Jetty waits for the data and the request eventually times out if there is none). But since we've found two new behaviors, we'd like to try and understand any others that are yet to be discovered. I appreciate any help anyone can give! We're hoping to not have to crawl all of the git commits to find these answers.

[joakime]

Bad HTTP clients that rely on bad spec interpretation or rely quirks in server behavior are always subject to being broken when the various intermediaries and servers tighten up their behaviors.
If the HTTP client follows the specs, then breakage is truly rare beast. (And if discovered cause a quick fix on our side!)

As for a GET request with a payload, this is problematic, as this concept no spec defined behavior.

The RFC7231: Section 4.3.1 states:

   A payload within a GET request message has no defined semantics;
   sending a payload body on a GET request might cause some existing
   implementations to reject the request.

Jetty has traditionally supported GET requests with payload, with minor differences on how HttpServletRequest behaves based on the method (GET vs POST/PUT) and how parameter/part parsing works.

It's worth noting that many intermediaries can also make a judgement call on allowing or rejecting GET with request payloads as well (firewalls, load balancers, mobile network http caches, mobile OS http optimization, etc).

As for the list of changes, the VERSION.txt is the list of changes in high level form.
The individual issues / commits / test cases document the behavior listed in the VERSION.txt for each release.

Just to repeat, we have put many of these more problematic changes behind various HttpCompliance configurables in the existing codebase.

See ...

• org/eclipse/jetty/http/HttpCompliance - for high level grouping of various compliance behaviors
• org/eclipse/jetty/http/HttpComplianceSection - for the specific behaviors in parsing / generating that have a switch in behavior.
• org/eclipse/jetty/http/CookieCompliance - even Cookie parsing/generating behavior has been modified over time to behave to the newer, stricter, Cookie standards that nearly all modern browsers now require.

[cy6erGn0m]

Isn't it related to https://security-tracker.debian.org/tracker/CVE-2017-7657 ?

[joakime]

@cy6erGn0m that CVE was fixed with commit a285deea42fcab60d9edcf994e458c238a348b55 (Issue #2529)
See June 2018 announcement for all recent CVEs (including the one you linked): https://dev.eclipse.org/mhonarc/lists/jetty-announce/msg00123.html

[joakime]

This is currently implemented in branch jetty-9.4.x and has been in place since commit a285deea42fcab60d9edcf994e458c238a348b55
Available in Jetty 9.4.11.v20180605 and newer.