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

[Amos Jeffries]

On 11/08/2015 9:36 a.m., Alex Rousskov wrote:
> 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.

There is exactly 2 cases of benign malformation:

 * duplicate identical Content-Length
 * header is a list of identical values (ie "42, 42,42")

Erase and ignore is required for both cases.

The MUST requirement in section 3.3.2 paragraph 9 is an "either ... or
..." text requiring that we clean up the malformation when we can do so
without guessing the Content-Length's real intended value.

All other malformations are *malign*.

The case of Content-Length + Transfer-Encoding is *malign* malformation.
With many known abuses and failures resulting. Squid is part of the
problem here.

Content-Length on CONNECT or its 200 reponse is also malign
malformation. It is seen a lot in the wild but, ignoring causes worse
problems than 502.
- stripping the Content-Length causes browser/client cache poisoning.
- allowing it through leads to aborted tunnels with User-visible
mangling and malformations from incomplete content returned inside.


Note: high-speed routers that need to disable Content-Length headers do
so by mangling the name portion so it shows up as an unknown custom
header. There is never a need to send invalid value portions.


> 
> 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.
> 

Yes. C-L holds the message payload size, not the original object size.
 RFC 7233 section-4.1 examples show that.

Is that an IIS? Bing? or ICAP server bug?

Amos