Re: Basic auth cache and SslBump

On 22/03/2014 7:46 a.m., Tsantilas Christos wrote:
>
> Hi all,
> a Measurement Factory customer reported the following bug:
>
> 1) A user sends a CONNECT request with valid credentials
> 2) Squid checks the credentials and adds the user to the user cache
> 3) The same user sends a CONNECT request with invalid credentials
> 4) Squid overwrites the entry in the user cache and denies the second
> CONNECT request
> 5) The user sends a GET request on the first SSL connection which is
> established by now
> 6) Squid knows that it does not need to check the credentials on the
> bumped connection but still somehow checks again whether the user is
> successfully authenticated
> 7) Due to the second CONNECT request the user is regarded as not
> successfully authenticated
> 8) Squid denies the GET request of the first SSL connection with 403
> ERR_CACHE_ACCESS_DENIED
>
> On proxies with Basic authentication and SSL bumping, this can be used
> to prevent a legitimate user from making any HTTPS requests
>
> I am attaching a patch which disables authentication for tunneled requests.

That is incorrect, however it is not what you have coded.

The correct action which your patch contains is to disables *re-*
authentication of bumped requests.

> Looks important bug and patch should applied.
>
> Looking in the authentication squid3 code I am seeing issues, related to
> authentication cache.
> For example
> - inside Auth::Basic::User::updateCached() we are replacing with
> (maybe wrong) password a valid cache entry. This is does not looks normal.

Yes that is a bit strange. To maintain old Basic::User state references
it should replace the entire cached User object if anything in the state
changes.

>
> - For digest authentication inside Auth::Digest::Config::decode method
> we are retrieving user from cache, and if we found it we are marking the
> cached user as Auth::Unchecked which means that the entry needs
> checking again.
> Looking in the code, in practice this is means, do not use cache, always
> query again.

Digest has quite a few bugs at present. I'm working on it. Thanks for
pointing out this one.

>
> Am I missing something?
>

To emulate bumped traffic transparently as if it were non-bumped the
right thing to do is to disable re-authentication.

But inside matchProxyAuth() is where the check for ssl-Bumped should be
happening I think. It already has access to Filled() state and can check
the flag and return 1 before doing anything else.
 If you just move the if-statement I think the updated patch can go
straight in without another review.

Amos
Received on Sat Mar 22 2014 - 03:55:09 MDT