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

[Amos Jeffries]

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
"


Amos