[squid-dev] on_unsupported_protocol doesn't work for bumped https connecttions

[Alex Rousskov]

On 11/20/2015 06:55 AM, Tarik Demirci wrote:
> On Wed, Nov 18, 2015 at 5:55 PM, Alex Rousskov wrote:
>> On 11/18/2015 12:53 AM, Tarik Demirci wrote:
>>
>>> I did more detailed tests for this case. Constructing a tcp-in-https
>>> connection results with error ERR_PROTOCOL_UNKNOWN in spite of
>>> "on_unsupported_protocol tunnel all" conf directive. Is this a Squid
>>> bug? Doc for on_unsupported_protocol says it works for bumped tunnels
>>> but I can't confirm this in any way.
>>>
>>> I debugged the code and it fails in a check in clientTunnelOnError
>>> function. By the time Squid understands it's not http inside https,
>>> conn->nrequests value is 2. So conn->nrequests <= 1 check fails.


>> AFAICT, the intended goal of the nrequests check is to prevent switching
>> to tunnel mode after the tunnel has already been proven to carry a
>> "supported" protocol (i.e., HTTPS or HTTP).
>>
>> I do not think that nrequests check is correct: The nrequests member is
>> incremented on every request, so it may be very large if a browser
>> switches to a tunnel after sending many regular requests:
>>
>>   GET
>>   GET
>>   GET
>>   CONNECT


> In my situation it's something like this because of ssl bump:
> 
> CONNECT
> GET (or unknown protocol)


>> I also suspect the check is difficult to get right because fake CONNECTs
>> on intercepted connections and real CONNECTs on forwarded connections
>> might be counted differently. I did not verify that, but it may explain
>> why you are hitting this bug -- the code may have been tested with
>> intercepted connections only and just "assumed" to work for CONNECT
>> tunnels as well.
> 
> Does connect TUNNEL mean explicit proxy by client (client is proxy
> aware)? 

In this context, yes.


> If yes, this is not my case. I test this situation for
> intercepted connections on 443 (using iptables REDIRECT).

Noted.


>> I recommend replacing nrequests check with a check based on a new
>> tooLateToTunnel boolean data member. That member can be initialized to
>> false and set to true after receiving valid HTTP request headers inside
>> an inspected connection (at least).


> I implemented your suggestion but failed. During ssl bump,
> httpsSslBumpAccessCheckDone fires a fakeAConnectRequest. This CONNECT
> request has an 'Host:' header. Therefore it makes tooLateToTunnel
> value true which causes bypass to fail again.

The last part does not compute for me: Why would the Host header
presence be relevant to tooLateToTunnel? The code should not make
tooLateToTunnel true until it is really too late to tunnel. Dealing with
a [fake] CONNECT request does not mean it is too late to tunnel,
regardless of the Host header presence.

One of the difficulties you may face is that, on one hand,
tooLateToTunnel should remain false before the first CONNECT (fake or
real). The member should be set to true after receiving valid HTTP
request headers _inside_ an inspected connection. On the other hand, we
should _not_ start tunneling in this http_port situation either:

  GET
  GET
  GET
  non-HTTP-traffic

That means we may need to reset tooLateTooTunnel back to false when
dealing with a CONNECT request. The new member may need to be called
prohibitTunneling:


For http_port connection:

  prohibitTunneling = false
  accept
  successfully parsed first HTTP request
  prohibitTunneling = the request was not a CONNECT request
  ...
  real CONNECT
  bumping
  successfully parsed first inspected HTTP request
  prohibitTunneling = true


For intercepted https_port connections:

    prohibitTunneling = false
    accept
    fake CONNECT
    bumping
    successfully parsed first inspected HTTP request
    prohibitTunneling = true

The above sketch may not cover all situations, of course, but your code
should.


HTH,

Alex.