[squid-dev] [PATCH] Handling syntactically valid requests with higher-than-supported HTTP versions

[Amos Jeffries]

On 7/09/2016 1:49 a.m., Amos Jeffries wrote:
> On 6/09/2016 9:06 a.m., Eduard Bagdasaryan wrote:
>> Hello,
>>
>> This patch fixes Squid to return 505 (Version Not Supported) error
>> code for HTTP/2+ versions.
>>
>> Before this change, when a syntactically valid HTTP/1 request indicated
>> HTTP major version 2, Squid mangled and forwarded the request as an
>> HTTP/1 message.
> 
> That is not true. The trunk code returns false the same as this patched
> version. Because the version field for HTTP/2 does not start with the
> static magic string "HTTP/1."
> 
> 
>> Since Squid does not and cannot correctly interpret an
>> HTTP/1 request using HTTP/2 rules, returning an error and closing the
>> connection appears to be the only correct action possible.
> 
> That is not correct either. The old logic requires method to be GET for
> full HTTP/0.9 handling. HTTP/2 pseudo-method is PRI. If anything other
> than PRI is seen it is not valid HTTP/2.
>  At most we should have logic passing HTTP/2 transparently through the
> proxy (configurable). Which is the relay behaviour of HTTP/0.9
> processing, but not its header mangling behaviour (because PRI, not GET
> method).
> 
> In the case where it is not HTTP/1.1, HTTP/1.0, HTTP/2, or GET without a
> version field. Then the version must start with "HTTP/" to be considered
> invalid HTTP version and the MUST clause applicable.
> 
> Otherwise it could be any protocol at all. HTTP/2, SMTP, Finger,
> Secure-HTTP, WebSocket, SPDY and ICAP can all be sent to this parse
> logic at various times.
> 
> Also, HTTP/2 does not obsolete HTTP/1.x RFC's, so there is an explicit
> possibility that HTTP/1.2+ will exist at some point. The IETF WG
> expectation is/was that 1.2+ (if any) would contain compatibility for
> HTTP/2+ features over HTTP/1.x syntax.
> 
>>
>> Also: since HTTPbis prohibits HTTP versions with multi-digits, treat
>> such requests as invalid.
>>
>> Attached Co-Advisor result pages for patched and unpatched Squid.
>>
> 
> For some reason I'm not sure of yet these changes result in unit test
> failure parsing a garbage line with various bad whitespace characters in it.
> 
> make check with testHttp1Parser debugging enabled produces:
> "
> TEST @1071, in="\r    \t   \n"
> 
> testHttp1Parser.cc:106:Assertion
> Test name: testHttp1Parser::testParseRequestLineProtocols
> equality assertion failed
> - Expected: 0
> - Actual  : 1
> "
> 

Just noticed that the unit test for "GET / HTTP/2.0" request-line is
also failing due to this parser change now accepting that invalid
version as valid.

I think this other fail is due to "2.0" meeting both the version layout
and the "multiDigits' conditions. You should use
tok.skip(Http1::MagicPrefix) instead of trying to count digits in the
major version. This is an HTTP/1 parser, non-1.x versions (except 0.9
special case) are all unsuccessful parse if they occur here.

Amos