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
