Re: Basic auth cache and SslBump

On 03/22/2014 05:54 AM, Amos Jeffries wrote:
> 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.

I did not express my self well.
What I mean is that we should authenticate only the first CONNECT
request. We must not authenticate the HTTP requests which tunneled
through the CONNECT tunnel.

>
>> 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.

Yep.

>
>>
>> - 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.

ok

>
>>
>> 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.

Applied with this fix.

>
> Amos
>
Received on Sun Mar 23 2014 - 10:30:09 MDT