[squid-dev] [PATCH] Reject responses with conflicting Content-Length

[Alex Rousskov]

On 08/07/2015 10:48 PM, Amos Jeffries wrote:
> On 8/08/2015 8:54 a.m., Alex Rousskov wrote:
>>     Squid trusts and forwards the largest Content-Length header. This
>> behavior violates an RFC 7230 MUST in Section 3.3.3 item #4. It also
>> confuses some ICAP services and probably some HTTP clients. With the
>> proposed changes, Squid refuses to forward the message to the ICAP
>> service and HTTP client, responding with an HTTP 502 error instead.
>>
>> This is a quick-and-dirty implementation. A polished version should
>> reject responses with invalid Content-Length values as well (per RFC
>> 7230 MUST), should return 502 even with a strict parser (this is not a
>> header parsing issue), and should probably not warn the admin when all
>> values actually match.


> Please do eitehr XXX or implement the clause about 502 on invalid
> Content-Length header. The unable to parse cases when comparing l1 and
> l2 at chunk @548.

XXX added:

> // XXX: RFC 7230 Section 3.3.3 item #4 requires sending a 502 error in 
> // several cases that we do not yet cover. TODO: Rewrite to cover more.
> if (e->id == Http::HdrType::CONTENT_LENGTH && (e2 = ...


I am reluctant to make invalid Content-Length handling compliant during
this project because

(a) the right fix will probably require a serious code rewrite; and

(b) I do not have a good way to test how damaging that compliance will
be to real traffic. There might be enough malformed benign responses out
there to necessitate a USE_HTTP_VIOLATIONS exception when there is no
Transfer-Encoding, there is only one Content-Length header field, and
that single header field is malformed.


While writing that XXX, I realized that Squid has a similar bug with
request processing, and that the proposed change affects but is not
enough to fix the request processing path. To fix that, I added the
following check to the existing clientIsContentLengthValid() in
client_side.cc:


> +// checks body length of non-chunked requests
>  static int
>  clientIsContentLengthValid(HttpRequest * r)
>  {
> +    // No Content-Length means this request just has no body, but conflicting
> +    // Content-Lengths mean a message framing error (RFC 7230 Section 3.3.3 #4).
> +    if (r->header.conflictingContentLength())
> +        return 0;
> +
>      switch (r->method.id()) {


Just like with response handling, the above code is not enough to cover
all bad request cases, but it improves coverage. Addressing the new XXX
comment will cover more HTTP Client and Server cases.

Finally, while testing request path changes, I noticed that the patch
left multiple same-value Content-Length fields in the header. The
committed version leaves just one of them, as the unpatched Squid code did.


> +1 as a temporary workaround. Looks like it catches some important cases
> correctly.

Committed to trunk as r14215. Please let me know if you see any problems
with the added clientIsContentLengthValid() code.


For the record, here is a Bing server response that prompted this change:

> HTTP/1.1 206 Partial Content
> Cache-Control: public, max-age=15552000
> Content-Length: 14543
> Content-Type: application/x-javascript; charset=utf-8
> Last-Modified: Tue, 14 Jul 2015 01:35:04 GMT
> Vary: Accept-Encoding
> Server: Microsoft-IIS/8.5
> Date: Wed, 05 Aug 2015 15:42:26 GMT
> Content-Length: 300
> Content-Range: bytes 14243-14542/14543
> Accept-Ranges: bytes

As you can see, the addition of an extra Content-Length field was
probably triggered by the partial response to the Range request.


HTH,

Alex.