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 >
