Improve Transfer-Encoding handling

[yadij]

Reject messages containing Transfer-Encoding header with coding other
than chunked or identity. Squid does not support other codings.

For simplicity and security sake, also reject messages where
Transfer-Encoding contains unnecessary complex values that are
technically equivalent to "chunked" or "identity" (e.g., ",,chunked" or
"identity, chunked").

RFC 7230 formally deprecated and removed identity coding, but it is
still used by some agents.

[rousskov]

Found one bug and two serious concerns. The rest is simple polishing.

[rousskov]

Thank you for addressing my previous concerns. I have only two minor nits left for now. Will wait for PR 701 going in so that its XXX can be addressed as well.

[rousskov]

I have only two minor nits left for now.

And an HttpHeader::chunked() change.

[kinkie]

Latest jenkins build failed on commit hash not being found; next commit will fix it

[rousskov]

Latest jenkins build failed on commit hash not being found; next commit will fix it

I pushed a trivial change (commit 7439beb) in hope to fix Jenkins.

Thank you for addressing my primary concerns.

[rousskov]

FTR, Squid's strict handling of Transfer-Encoding values rejects the following (slightly abridged) server responses containing two Transfer-Encoding: chunked header fields:

HTTP/1.1 200 OK
Date: Thu, 24 Sep 2020 11:56:13 GMT
Server: JBoss-EAP/7
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Cache-Control: no-cache
Cache-Control: no-store
X-Powered-By: Undertow/1
X-Powered-By: JSP/2.3
Pragma: no-cache
Transfer-Encoding: chunked
Content-Type: text/html;charset=UTF-8
Vary: Accept-Encoding
Connection: close
Cache-Control: no-cache
Content-Encoding: gzip
Transfer-Encoding: chunked

The above response header can be interpreted as a violation of the following RFC 7230 MUST-level rquirement: A sender MUST NOT apply chunked more than once to a message body (i.e., chunking an already chunked message is not allowed). The message body itself is probably not double-chunked, but I did not check, and it should not really matter.

55,2| HttpHeader.cc(489) parse: WARNING: unsupported Transfer-Encoding used by client: chunked, chunked

FWIW, at this time, Factory does not plan to change Squid to treat such messages specially -- they will be rejected to mitigate HTTP framing-related attacks. However, if this and similar problems become too frequent, we will need to do more to improve compatibility. For now, let's just collect information about these use cases.

[yadij]

The above response header can be interpreted as a violation of the following RFC 7230 MUST-level rquirement: A sender MUST NOT apply chunked more than once to a message body (i.e., chunking an already chunked message is not allowed). The message body itself is probably not double-chunked, but I did not check, and it should not really matter.

It is definitively a violation. If chunked was applied twice that is forbidden. The header is a list header - this input means explicitly that chunked was applied twice. Whether or not it was actually applied twice the sender is not performing HTTP sufficiently well to trust its message framing.

[rousskov]

FTR, I am updating the list of known real-world benign cases that Squid (correctly!) rejects as of this PR:

1. WARNING: unsupported Transfer-Encoding used by client: chunked, chunked.
2. WARNING: unsupported Transfer-Encoding used by client: gzip, chunked.
   Note: Only chunked transfer encoding was applied, no compression.
3. WARNING: unsupported Transfer-Encoding used by client: Binary.
   Note: There was no actual transfer encoding applied. Known Content-Length.
4. WARNING: unsupported Transfer-Encoding used by client:
   Note: There was no actual transfer encoding applied. The header value was empty.
5. WARNING: Leading garbage or empty value in Content-Length[4]=null

The above were seen on various secondary legitimate servers, including servers that belong to well-known well-resourced companies, organizations, and governments around the world. The apparent "resilience" of these problems is consistent with 2014 client-side observations by Daniel Stenberg.

I may update this comment as Squid admins continue to complain about these cases.