2003-05-18 23:49 hno

2003-05-24 13:08  robertc

Bug #630: digest authentication and buggy browsers

This patch does four things:

* correct signalling of stale digest nonces

* auth_param digest check-nonce-count off option to completely
disable the nonce count check. Needed to work around Konqueror and
certain other broken digest qop implementations.

* auth_param digest post_workaround on to work around certain
broken browsers sending POST requests with a GET digest response

* changes the default of nonce_strictness to off. It is to early to
use strict nonce count increase by one checks, and even in the long
run this is probably not wanted. Enforcing increase by one does not
really add to security, only complexity.

diff --git a/src/auth/digest/auth_digest.cc b/src/auth/digest/auth_digest.cc
index 1e0b1229fca4718189065d8a731f9473341633cd..71382805b66abf65607f8d71a8ffdfefb736fa77 100644 (file)
--- a/src/auth/digest/auth_digest.cc
+++ b/src/auth/digest/auth_digest.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: auth_digest.cc,v 1.22 2003/02/26 06:11:41 robertc Exp $
+ * $Id: auth_digest.cc,v 1.23 2003/06/19 18:57:00 hno Exp $
  *
  * DEBUG: section 29    Authenticator
  * AUTHOR: Robert Collins
@@ -364,6 +364,18 @@ authDigestNonceIsValid(digest_nonce_h * nonce, char nc[9])
 
     intnc = strtol(nc, NULL, 16);
 
+    /* has it already been invalidated ? */
+    if (!nonce->flags.valid) {
+        debug(29, 4) ("authDigestNonceIsValid: Nonce already invalidated\n");
+        return 0;
+    }
+
+    /* is the nonce-count ok ? */
+    if (!digestConfig->CheckNonceCount) {
+        nonce->nc++;
+        return -1;              /* forced OK by configuration */
+    }
+
     if ((digestConfig->NonceStrictness && intnc != nonce->nc + 1) ||
             intnc < nonce->nc + 1) {
         debug(29, 4) ("authDigestNonceIsValid: Nonce count doesn't match\n");
@@ -371,12 +383,6 @@ authDigestNonceIsValid(digest_nonce_h * nonce, char nc[9])
         return 0;
     }
 
-    /* has it already been invalidated ? */
-    if (!nonce->flags.valid) {
-        debug(29, 4) ("authDigestNonceIsValid: Nonce already invalidated\n");
-        return 0;
-    }
-
     /* seems ok */
     /* increment the nonce count - we've already checked that intnc is a
      *  valid representation for us, so we don't need the test here.
@@ -686,11 +692,47 @@ digest_request_h::authenticate(request_t * request, ConnStateData * conn, http_h
                   "squid is = '%s'\n", digest_request->response, Response);
 
     if (strcasecmp(digest_request->response, Response)) {
-        credentials(Failed);
-        return;
+        if (digestConfig->PostWorkaround && request->method != METHOD_GET) {
+            /* Ugly workaround for certain very broken browsers using the
+             * wrong method to calculate the request-digest on POST request.
+             * This should be deleted once Digest authentication becomes more
+             * widespread and such broken browsers no longer are commonly
+             * used.
+             */
+            DigestCalcResponse(SESSIONKEY, authenticateDigestNonceNonceb64(digest_request->nonce),
+                               digest_request->nc, digest_request->cnonce, digest_request->qop,
+                               RequestMethodStr[METHOD_GET], digest_request->uri, HA2, Response);
+
+            if (strcasecmp(digest_request->response, Response)) {
+                credentials(Failed);
+                return;
+            } else {
+                const char *useragent = httpHeaderGetStr(&request->header, HDR_USER_AGENT);
+
+                static struct in_addr last_broken_addr = {0};
+
+                if (memcmp(&last_broken_addr, &request->client_addr, sizeof(last_broken_addr)) != 0) {
+                    debug(29, 1) ("\nDigest POST bug detected from %s using '%s'. Please upgrade browser. See Bug #630 for details.\n", inet_ntoa(request->client_addr), useragent ? useragent : "-");
+                    last_broken_addr = request->client_addr;
+                }
+            }
+        } else {
+            credentials(Failed);
+            return;
+        }
+
+        /* check for stale nonce */
+        if (!authDigestNonceIsValid(digest_request->nonce, digest_request->nc)) {
+            debug(29, 3) ("authenticateDigestAuthenticateuser: user '%s' validated OK but nonce stale\n",
+                          digest_user->username);
+            digest_request->flags.nonce_stale = 1;
+            credentials(Failed);
+            return;
+        }
     }
 
     credentials(Ok);
+
     /* password was checked and did match */
     debug(29, 4) ("authenticateDigestAuthenticateuser: user '%s' validated OK\n",
                   digest_user->username);
@@ -711,16 +753,17 @@ digest_request_h::direction()
 
     case Ok:
 
-        if (authDigestNonceIsStale(nonce))
-            /* send stale response to the client agent */
-            return -2;
-
         return 0;
 
     case Pending:
         return -1;
 
     case Failed:
+
+        if (digest_request->flags.nonce_stale)
+            /* nonce is stale, send new challenge */
+            return 1;
+
         return -2;
     }
 
@@ -801,11 +844,7 @@ authenticateDigestFixHeader(auth_user_request_t * auth_user_request, HttpReply *
         digest_request = dynamic_cast < digest_request_h * >(auth_user_request->state());
         assert (digest_request);
 
-        if (digest_request->authenticated())
-            /* stale indicates that the old nonce can't be used
-             * and we are providing a new one.
-             */
-            stale = authDigestNonceIsStale(digest_request->nonce);
+        stale = digest_request->flags.nonce_stale;
     }
 
     /* on a 407 or 401 we always use a new nonce */
@@ -954,8 +993,10 @@ authDigestParse(authScheme * scheme, int n_configured, char *param_str)
         digestConfig->noncemaxduration = 30 * 60;
         /* 50 requests */
         digestConfig->noncemaxuses = 50;
-        /* strict nonce count behaviour */
-        digestConfig->NonceStrictness = 1;
+        /* Not strict nonce count behaviour */
+        digestConfig->NonceStrictness = 0;
+        /* Verify nonce count */
+        digestConfig->CheckNonceCount = 1;
     }
 
     digestConfig = static_cast < auth_digest_config * >(scheme->scheme_data);
@@ -979,6 +1020,10 @@ authDigestParse(authScheme * scheme, int n_configured, char *param_str)
         parse_int((int *) &digestConfig->noncemaxuses);
     } else if (strcasecmp(param_str, "nonce_strictness") == 0) {
         parse_onoff(&digestConfig->NonceStrictness);
+    } else if (strcasecmp(param_str, "check_nonce_count") == 0) {
+        parse_onoff(&digestConfig->CheckNonceCount);
+    } else if (strcasecmp(param_str, "post_workaround") == 0) {
+        parse_onoff(&digestConfig->PostWorkaround);
     } else {
         debug(28, 0) ("unrecognised digest auth scheme parameter '%s'\n", param_str);
     }
@@ -1288,7 +1333,7 @@ authenticateDigestDecodeAuth(auth_user_request_t * auth_user_request, const char
     /* now the nonce */
     nonce = authenticateDigestNonceFindNonce(digest_request->nonceb64);
 
-    if ((nonce == NULL) || !(authDigestNonceIsValid(nonce, digest_request->nc))) {
+    if (!nonce) {
         /* we couldn't find a matching nonce! */
         debug(29, 4) ("authenticateDigestDecode: Unexpected or invalid nonce recieved\n");
         authDigestLogUsername(auth_user_request, username);

diff --git a/src/auth/digest/auth_digest.h b/src/auth/digest/auth_digest.h
index abb3d753dc5b8c1c71966675e7b23dd30d005957..415eb5884cc4ddce82a9c18bbc97ecf74ed4ad33 100644 (file)
--- a/src/auth/digest/auth_digest.h
+++ b/src/auth/digest/auth_digest.h
@@ -86,6 +86,9 @@ public:
 
 unsigned int authinfo_sent:
         1;
+
+unsigned int nonce_stale:
+        1;
     }
 
     flags;
@@ -144,6 +147,8 @@ struct _auth_digest_config
     time_t noncemaxduration;
     unsigned int noncemaxuses;
     int NonceStrictness;
+    int CheckNonceCount;
+    int PostWorkaround;
 };
 
 typedef struct _auth_digest_config auth_digest_config;

diff --git a/src/authenticate.cc b/src/authenticate.cc
index a644ccca512b095a7e99d09a6dd61771648ba32e..89d37f623ced364fd76931fa2c0bd71b91c8c6a1 100644 (file)
--- a/src/authenticate.cc
+++ b/src/authenticate.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: authenticate.cc,v 1.57 2003/06/19 18:20:51 hno Exp $
+ * $Id: authenticate.cc,v 1.58 2003/06/19 18:56:59 hno Exp $
  *
  * DEBUG: section 29    Authenticator
  * AUTHOR:  Robert Collins
@@ -722,10 +722,22 @@ AuthUserRequest::authenticate(auth_user_request_t ** auth_user_request, http_hdr
 
         case 1:
 
+            if (!request->auth_user_request) {
+
+                (*auth_user_request)->lock()
+
+                ;
+                request->auth_user_request = *auth_user_request;
+            }
+
+            /* fallthrough to -2 */
+
         case -2:
             /* this ACL check is finished. Unlock. */
             (*auth_user_request)->unlock();
+
             *auth_user_request = NULL;
+
             return AUTH_ACL_CHALLENGE;
 
         case -1:

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 1fb63d208f06d8aa7b773e54d723b6644410caec..82938f996d86cd4ee5346be25f1ef009413ba019 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1,6 +1,6 @@
 
 #
-# $Id: cf.data.pre,v 1.322 2003/06/19 17:34:10 hno Exp $
+# $Id: cf.data.pre,v 1.323 2003/06/19 18:56:59 hno Exp $
 #
 #
 # SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -1636,10 +1636,22 @@ DOC_START
        used.
 
        "nonce_strictness" on|off
-       Determines if squid requires increment-by-1 behaviour for
-       nonce counts (on - the default), or strictly incrementing
-       (off - for use when useragents generate nonce counts that
-       occasionally miss 1 (ie, 1,2,4,6)).
+       Determines if squid requires strict increment-by-1 behaviour
+       for nonce counts, or just incrementing (off - for use when
+       useragents generate nonce counts that occasionally miss 1
+       (ie, 1,2,4,6)). Default off.
+
+       "check_nonce_count" on|off
+       This directive if set to off can disable the nonce count check
+       completely to work around buggy digest qop implementations in
+       certain mainstream browser versions. Default on to check the
+       nonce count to protect from authentication replay attacks.
+
+       "post_workaround" on|off
+       This is a workaround to certain buggy browsers who sends
+       an incorrect request digest in POST requests when reusing
+       the same nonce as aquired earlier on a GET request.
+
 
        === NTLM scheme options follow ===
 

diff --git a/src/client_side.cc b/src/client_side.cc
index bd5ef6fc87174a9b12693ffdf3707f4ad4516bf6..7ac80e0b4fccc0d53af4fd47ecc0e4d55bc156c8 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side.cc,v 1.639 2003/06/19 18:18:46 hno Exp $
+ * $Id: client_side.cc,v 1.640 2003/06/19 18:56:59 hno Exp $
  *
  * DEBUG: section 33    Client-side Routines
  * AUTHOR: Duane Wessels
@@ -487,10 +487,12 @@ clientPrepareLogWithRequestDetails(request_t * request, AccessLogEntry * aLogEnt
     aLogEntry->hier = request->hier;
 
     if (request->auth_user_request) {
-        aLogEntry->cache.authuser =
-            xstrdup(authenticateUserRequestUsername(request->
-                                                    auth_user_request));
+        if (authenticateUserRequestUsername(request->auth_user_request))
+            aLogEntry->cache.authuser =
+                xstrdup(authenticateUserRequestUsername(request->auth_user_request));
+
         authenticateAuthUserRequestUnlock(request->auth_user_request);
+
         request->auth_user_request = NULL;
     }