For record:

With this little change the problem is gone at least with none_max_count x

 158 UserRequest.cc

 -        auth_user->credentials(Auth::Handshake);
 +        auth_user->credentials(Auth::Pending);


With none_max_count 10, after 10 requests squid makes a ldap request without any popup for the user if there is nothing new in hash -> Ok seems a good behavior, squid check the hash and if there is a change somewhere the user should fix his login/password. 

For nonce_max_duration I think the problem is in auth_digest.cc
 
Line 542

stale = digest_user->credentials() == Auth::Handshake;

This stale Auth::Handshake seem never checked in UserRequest.cc

Line 183 
    case Auth::Handshake:
        debugs(1, 1, HERE << "fred"); -> I'm seeing "fred" only after 10 requests with "none_max_count 10"  - without the previous change of course (I mean change Auth::Handshake to Auth::Pending) -

I hope this can help

I forgot 

> stale = digest_user->credentials() == Auth::Handshake;
> 

With stale = digest_user->credentials() == Auth::Failed;

In this case the popup appear and the browser must replay the login/password, but the problem remains a user deleted or with an old password can pass ...

Created attachment 3058 [details]
Fix bug 4066

This patch adds a ldap request when:

nonce_max_duration is reached
nonce_max_count is reached 

When a nonce is expired, squid check the HASH, makes a LDAP request, create a new nonce, without any pop-up if there is no change.

But if something is changed in LDAP (login, password), the user is denied when the nonce is expired, a pop-up appear to takes the new informations.

Tested on Squid 3.4.5

This bug still present in 3.4.7,and I agree that there is a safety issue.
For me (squid 3.4.7) the patch fixes all my problems - users with wrong password, or worst with login disabled ... -, could it be included in 3.4.8 ?

(In reply to comment #4)
> This bug still present in 3.4.7,and I agree that there is a safety issue.
> For me (squid 3.4.7) the patch fixes all my problems - users with wrong
> password, or worst with login disabled ... -, could it be included in 3.4.8 ?

Except the patch there is another way, you can also restart Squid each day for example at midnight or during logrotate. 
Restart purge the "Ghosts in the machine", only once by day but it's better than wait a hypothetical restart of browsers (sometimes after some weeks ...) and even in this case I think that the user can use his previous account because in squid the storage hash table still the same.

authDigestNonceIsValid() checks authDigestNonceIsStale() as one of its actions.

Does this patch work just as well if you remove the call to authDigestNonceIsStale() from the UserRequest.cc chunk ?

Hi Amos,

I was testing your advice and run without authDigestNonceIsStale() and I found a new bug with Squid 3.5, to be sure I started some new tests.
Now, I'm speaking without any patch and exactly the same configuration for the both Squid:

Squid 3.5 makes a request for each GET/POST requests (So my patch seem unnecessary). The problem is that with many users the CPU increases and the ldap is slow, Squid becomes unusable.

I checked and this problem doesn't appear with Squid 3.4.5. 
Not tested with BASIC and others, so I don't know if this is a general problem

auth_param digest check_nonce_count off
auth_param digest nonce_max_duration 3600 minutes
auth_param digest nonce_max_count 5000
auth_param digest nonce_garbage_interval 2 minutes
auth_param digest nonce_strictness off 

I made my test with tcpdump to port 389 and also with password change - Squid 3.5 show a pop-up immediately, squid 3.4 never (And ok with my patch when nonce is stalled/expired)

------------

About Squid 3.4 you are right, without authDigestNonceIsStale(nonce) my patch works perfectly

(!authDigestNonceIsValid(digest_request->nonce, digest_request->nc) && (user()->credentials() != Auth::Pending )) is enough to check stalled/expired - a new nonce = Ldap request -

So It's confirm that authDigestNonceIsStale() from auth_digest.cc was also unnecessary

Thanks

(In reply to comment #7)

> Squid 3.5 makes a request for each GET/POST requests (So my patch seem
> unnecessary). The problem is that with many users the CPU increases and the
> ldap is slow, Squid becomes unusable.
> 

I forgot, it doesn't generates nonce at each request, it makes only Ldap check

Maybe this should help:

Debug mode:

Squid 3.5 -> For each request

2014/12/22 12:11:54.568 kid1| helper.cc(1309) helperDispatch: helperDispatch: Request sent to digestauthenticator #Hlpr1, 21 bytes
2014/12/22 12:11:54.960 kid1| helper.cc(1309) helperDispatch: helperDispatch: Request sent to digestauthenticator #Hlpr1, 21 bytes
2014/12/22 12:11:55.003 kid1| helper.cc(1309) helperDispatch: helperDispatch: Request sent to digestauthenticator #Hlpr1, 21 bytes
2014/12/22 12:11:55.508 kid1| helper.cc(1309) helperDispatch: helperDispatch: Request sent to digestauthenticator #Hlpr1, 21 bytes

And a lot of 

2014/12/22 12:18:19.694 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:20.394 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:20.487 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:21.696 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:22.487 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:22.659 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:23.492 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:23.583 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:23.614 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'
2014/12/22 12:18:23.720 kid1| Config.cc(1047) decode: Creating new digest user 'fbourgeois'

But my nonce never changes in my browser

Squid 3.4 -> just one "Creating new digest" and "helperDispatch: helperDispatch"

2014/12/22 12:12:36.566 kid1| helper.cc(1350) helperDispatch: helperDispatch: Request sent to digestauthenticator #Hlpr0, 21 bytes

After investigations, I guess the bug is in the new line 1045 in Config.cc:

1045     if (key.isEmpty() || (auth_user = findUserInCache(key.c_str(), Auth::AUTH_DIGEST)) == NULL) 

If I switch to

if ((auth_user = findUserInCache(username, Auth::AUTH_DIGEST)) == NULL) (from squid 3.4) 
It's works, except that my patch is needed for ldap requests of course

Amos, Should I open a separate bug about this problem - Squid 3.5 only - like this you can close this one ?

In comment 7 I confirm that the patch works good with your modification. 

(!authDigestNonceIsValid(digest_request->nonce, digest_request->nc) &&
(user()->credentials() != Auth::Pending )) is enough in UserRequest.cc

The new bug is here 

findUserInCache(key.c_str(), Auth::AUTH_DIGEST); is always NULL because key.c_str() value is "fbourgeois:(null)" 

The value needed is "fbourgeois", so it's works if I switch key.c_str() by username

Like this 

if (key.isEmpty() || (auth_user = findUserInCache(username, Auth::AUTH_DIGEST)) == NULL) {

Just to clarify the final situation;

Squid-3.4: I have now applied your patch with my small modification.

Squid-3.5: does not have this bug? no patch applied.


A new bug in the hash keys inverts the behaviour to be too-many helper queries instead of not-enough. Tracking that in bug 4176.

(In reply to comment #12)
> Just to clarify the final situation;
> 
> Squid-3.4: I have now applied your patch with my small modification.
> 
> Squid-3.5: does not have this bug? no patch applied.
> 


Yes there is the same bug, tested, the patch is also needed 

The second bug is not related at all, just discovered during my tests.

findUserInCache(key.c_str(), Auth::AUTH_DIGEST); is always NULL because
key.c_str() value is "fbourgeois:(null)" 

So squid creates (maybe partially ?) new users at each request

Thank you.

the patch for this bug is now applied to Squid-3, 3.5 and 3.4.

Since this has been identified as a security flaw, is there going to be a security advisory ? (with CVE id of-course)

CVE-2014-9749 has been issued for the bug described in comment 1

(In reply to Amos Jeffries from comment #16)
> CVE-2014-9749 has been issued for the bug described in comment 1

oops, that should be "comment 0".

Hi,

we were testing squid 3.1.23 with applied patch as well as squid 3.4.14 (patch is included in source code by default), but security flaw is still present there. 

Any of these events
1) nonce_max_duration timeout reached
2) nonce_garbage_interval reached
3) nonce_max_count reached (with "check_nonce_count on")

prevent from accessing squid with outdated credentials.

(In reply to Luboš Uhliarik from comment #18)
> Hi,
> 
> we were testing squid 3.1.23 with applied patch as well as squid 3.4.14
> (patch is included in source code by default), but security flaw is still
> present there. 
> 
> Any of these events
> 1) nonce_max_duration timeout reached
> 2) nonce_garbage_interval reached
> 3) nonce_max_count reached (with "check_nonce_count on")
> 
> prevent from accessing squid with outdated credentials.


The problem is fixed for me, but your problem seems different
I was talking about LDAP (password changed or account removed)

With your three events, the nonce is just changed as expected

(In reply to Frederic Bourgeois from comment #19)
> (In reply to Luboš Uhliarik from comment #18)
> > Hi,
> > 
> > we were testing squid 3.1.23 with applied patch as well as squid 3.4.14
> > (patch is included in source code by default), but security flaw is still
> > present there. 
> > 
> > Any of these events
> > 1) nonce_max_duration timeout reached
> > 2) nonce_garbage_interval reached
> > 3) nonce_max_count reached (with "check_nonce_count on")
> > 
> > prevent from accessing squid with outdated credentials.
> 
> 
> The problem is fixed for me, but your problem seems different
> I was talking about LDAP (password changed or account removed)
> 
> With your three events, the nonce is just changed as expected

Yes, we were not testing LDAP, but passwords/users were stored in file. Anyway, after removing user or changing password, we are still able to access using old credentials.

(In rents, the nonce is just changed as expected
> 
> Yes, we were not testing LDAP, but passwords/users were stored in file.
> Anyway, after removing user or changing password, we are still able to
> access using old credentials.

I never tried like this, but I guess this is not "dynamic" in this case 
and Squid should be restarted or at least reloaded.

I think this is a normal behaviour you should see the same thing with basic auth, no ?

We are adding users and passwords to file by following function:

pw_hash=`echo -n "user:$REALM:password"|md5sum|cut -d ' ' -f 1`
echo 'user:$REALM:$pw_hash' >> /etc/squid/users

It works fine. 

We are checking access to proxy by following command:

curl -s --proxy-digest -x http://localhost:3128/ -D header.log -U user:password http://localhost/page.html -o /dev/null

which is also working fine.

The testing workflow is following:

1. create user "nonce_max_count" with password "password"
2. try to access localhost/page.html with valid credetials, we are getting 200 OK
3. delete all users in /etc/squid/users
4. add new user "nonce_max_count" with password "new_password"
5. try to access 3 TIMES page localhost/page.html with user "nonce_max_count" and password "password", which is OLD password - still getting 200 OK, which is correct
6. now, trying to access page localhost/page.html for 4th time, user = "nonce_max_count" and password = "password", receiving again 200 OK, which is INCORRECT
7. now, try to access page localhost/page.html with user="nonce_max_count" and password = "new_password", getting 200 OK - correct behavior
8. now, try to access page with OLD credentials again (user="nonce_max_count" and password = "password"), now got 407 Proxy Authentication Required - which is also correct behavior

So until you will not try to access squid with different password, than OLD password, OLD password will be still valid. It seems to me, old password is cached somewhere.


Squid.conf is following:

debug_options ALL,0 11,2 29,9
authenticate_ttl 10 seconds
auth_param digest program <DIGEST_PROGRAM> -c /etc/squid/users
auth_param digest children 1
auth_param digest realm Squid_realm

auth_param digest nonce_max_duration 5 seconds
auth_param digest nonce_garbage_interval 1 seconds
auth_param digest check_nonce_count on
auth_param digest nonce_max_count 4
auth_param digest nonce_strictness off

#acl manager proto cache_object
#acl localhost src 127.0.0.1/32 ::1

acl secret proxy_auth REQUIRED

http_access allow secret
http_access allow localhost manager
http_access deny all

http_port 3128
visible_hostname localhost

(In reply to Frederic Bourgeois from comment #21)
> (In rents, the nonce is just changed as expected
> > 
> > Yes, we were not testing LDAP, but passwords/users were stored in file.
> > Anyway, after removing user or changing password, we are still able to
> > access using old credentials.
> 
> I never tried like this, but I guess this is not "dynamic" in this case 
> and Squid should be restarted or at least reloaded.
> 
> I think this is a normal behaviour you should see the same thing with basic
> auth, no ?

Yeah, I think, with basic auth is the same problem, but Im not 100% sure now. 

So do you think, this behavior is correct? I don't think, it is correct, if you remove user from auth database, and you have to restart whole Squid because of that. 

Imagine situation, where users are added/removed by some script and you would have to restart squid after each removal. I think, this is security flaw.