[squid-dev] checklist->conn assertion in DestinationIp.cc

[Amos Jeffries]

On 18/07/2015 6:09 a.m., Alex Rousskov wrote:
> Hello,
> 
>     Intercepting Squids sometimes fail with the following assertion in
> ACLDestinationIP::match():
> 
>>     // Bug 3243: CVE 2009-0801
>>     // Bypass of browser same-origin access control in intercepted communication
>>     // To resolve this we will force DIRECT and only to the original client destination.
>>     // In which case, we also need this ACL to accurately match the destination
>>     if (Config.onoff.client_dst_passthru && ... intercepted ...) {
>>         assert(checklist->conn() && checklist->conn()->clientConnection != NULL);
>>         return ACLIP::match(checklist->conn()->clientConnection->local);
>>     }
> 
> There are several reports about these failures on squid-users, including
> http://lists.squid-cache.org/pipermail/squid-users/2015-May/003562.html
> 
> The assertion makes no sense to me -- why would an ACL assert that a
> connection is valid? A lot of things can happen between the time the ACL
> checklist was formed and the time the ACL got evaluated. This is true
> for all ACLs, but should be especially obvious for slow/asynchronous
> ACLs such as "dst".

What is a transaction without any existing state objects?

The connection may be closed (definition of 'invalid'?) and still pass
this test, so long as the state actually exists.


If the ConnStateData and ConnStateData::clientConnection are previously
deleted and things are still starting ACL tests something very bad has
happened. Probably broken refcount/cbdata locking on the Job-like object
holding the Checklist.


> 
> Is suggest replacing the assert with an if-statement to return -1
> (matching failure) when the connection is gone. Rationale: With the
> connection gone, the matching result probably does not matter anymore so
> there is little incentive for us to use alternative (and insecure!)
> sources of destination information.
> 
> Any better ideas?

That would seem to work for this problem. But be careful about other
hidden side effects of trying to complete delivery of a client response
without any existing client state object(s).

CbcPointer use for FilledChecklist::conn_ should also get a look-in to
see if its possible. The hitch there IIRC, was that CbcPointer get()
still produces NULL if the object has been 'deleted' but locks are still
existing for the Checklist to be destructed.

Amos