Author: markt
Date: Tue Sep  4 19:48:27 2012
New Revision: 1380829

URL: http://svn.apache.org/viewvc?rev=1380829&view=rev
Log:
Various improvements to the DIGEST authenticator including <bug>52954</bug>, 
the disabling caching of an authenticated user in the session by default, 
tracking server rather than client nonces and better handling of stale nonce 
values.

Modified:
    
tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
    tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
    tomcat/tc6.0.x/trunk/webapps/docs/config/valve.xml

Modified: 
tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java?rev=1380829&r1=1380828&r2=1380829&view=diff
==============================================================================
--- 
tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
 (original)
+++ 
tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/DigestAuthenticator.java
 Tue Sep  4 19:48:27 2012
@@ -27,9 +27,9 @@ import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.StringTokenizer;
 
+import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-
 import org.apache.catalina.LifecycleException;
 import org.apache.catalina.Realm;
 import org.apache.catalina.connector.Request;
@@ -80,6 +80,7 @@ public class DigestAuthenticator extends
 
     public DigestAuthenticator() {
         super();
+        setCache(false);
         try {
             if (md5Helper == null)
                 md5Helper = MessageDigest.getInstance("MD5");
@@ -100,16 +101,16 @@ public class DigestAuthenticator extends
 
 
     /**
-     * List of client nonce values currently being tracked
+     * List of server nonce values currently being tracked
      */
-    protected Map<String,NonceInfo> cnonces;
+    protected Map<String,NonceInfo> nonces;
 
 
     /**
-     * Maximum number of client nonces to keep in the cache. If not specified,
+     * Maximum number of server nonces to keep in the cache. If not specified,
      * the default value of 1000 is used.
      */
-    protected int cnonceCacheSize = 1000;
+    protected int nonceCacheSize = 1000;
 
 
     /**
@@ -150,13 +151,13 @@ public class DigestAuthenticator extends
     }
 
 
-    public int getCnonceCacheSize() {
-        return cnonceCacheSize;
+    public int getNonceCacheSize() {
+        return nonceCacheSize;
     }
 
 
-    public void setCnonceCacheSize(int cnonceCacheSize) {
-        this.cnonceCacheSize = cnonceCacheSize;
+    public void setNonceCacheSize(int nonceCacheSize) {
+        this.nonceCacheSize = nonceCacheSize;
     }
 
 
@@ -263,18 +264,19 @@ public class DigestAuthenticator extends
         // Validate any credentials already included with this request
         String authorization = request.getHeader("authorization");
         DigestInfo digestInfo = new DigestInfo(getOpaque(), getNonceValidity(),
-                getKey(), cnonces, isValidateUri());
+                getKey(), nonces, isValidateUri());
         if (authorization != null) {
-            if (digestInfo.validate(request, authorization, config)) {
-                principal = digestInfo.authenticate(context.getRealm());
-            }
+            if (digestInfo.parse(request, authorization)) {
+                if (digestInfo.validate(request, config)) {
+                    principal = digestInfo.authenticate(context.getRealm());
+                }
             
-            if (principal != null) {
-                String username = parseUsername(authorization);
-                register(request, response, principal,
-                         Constants.DIGEST_METHOD,
-                         username, null);
-                return (true);
+                if (principal != null && !digestInfo.isNonceStale()) {
+                    register(request, response, principal,
+                            HttpServletRequest.DIGEST_AUTH,
+                            digestInfo.getUsername(), null);
+                    return true;
+                }
             }
         }
 
@@ -285,10 +287,9 @@ public class DigestAuthenticator extends
         String nonce = generateNonce(request);
 
         setAuthenticateHeader(request, response, config, nonce,
-                digestInfo.isNonceStale());
+                principal != null && digestInfo.isNonceStale());
         response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
-        //      hres.flushBuffer();
-        return (false);
+        return false;
 
     }
 
@@ -301,7 +302,10 @@ public class DigestAuthenticator extends
      * can be identified, return <code>null</code>
      *
      * @param authorization Authorization string to be parsed
+     *
+     * @deprecated  Unused. Will be removed in Tomcat 8.0.x
      */
+    @Deprecated
     protected String parseUsername(String authorization) {
 
         // Validate the authorization credentials format
@@ -345,7 +349,7 @@ public class DigestAuthenticator extends
         } else if (quotedString.length() > 2) {
             return quotedString.substring(1, quotedString.length() - 1);
         } else {
-            return new String();
+            return "";
         }
     }
 
@@ -376,7 +380,14 @@ public class DigestAuthenticator extends
             buffer = md5Helper.digest(ipTimeKey.getBytes());
         }
 
-        return currentTime + ":" + md5Encoder.encode(buffer);
+        String nonce = currentTime + ":" + md5Encoder.encode(buffer);
+
+        NonceInfo info = new NonceInfo(currentTime, 100);
+        synchronized (nonces) {
+            nonces.put(nonce, info);
+        }
+
+        return nonce;
     }
 
 
@@ -450,7 +461,7 @@ public class DigestAuthenticator extends
             setOpaque(generateSessionId());
         }
         
-        cnonces = new LinkedHashMap<String, DigestAuthenticator.NonceInfo>() {
+        nonces = new LinkedHashMap<String, DigestAuthenticator.NonceInfo>() {
 
             private static final long serialVersionUID = 1L;
             private static final long LOG_SUPPRESS_TIME = 5 * 60 * 1000;
@@ -462,7 +473,7 @@ public class DigestAuthenticator extends
                     Map.Entry<String,NonceInfo> eldest) {
                 // This is called from a sync so keep it simple
                 long currentTime = System.currentTimeMillis();
-                if (size() > getCnonceCacheSize()) {
+                if (size() > getNonceCacheSize()) {
                     if (lastLog < currentTime &&
                             currentTime - eldest.getValue().getTimestamp() <
                             getNonceValidity()) {
@@ -480,10 +491,10 @@ public class DigestAuthenticator extends
  
     private static class DigestInfo {
 
-        private String opaque;
-        private long nonceValidity;
-        private String key;
-        private Map<String,NonceInfo> cnonces;
+        private final String opaque;
+        private final long nonceValidity;
+        private final String key;
+        private final Map<String,NonceInfo> nonces;
         private boolean validateUri = true;
 
         private String userName = null;
@@ -495,21 +506,27 @@ public class DigestAuthenticator extends
         private String cnonce = null;
         private String realmName = null;
         private String qop = null;
+        private String opaqueReceived = null;
 
         private boolean nonceStale = false;
 
 
         public DigestInfo(String opaque, long nonceValidity, String key,
-                Map<String,NonceInfo> cnonces, boolean validateUri) {
+                Map<String,NonceInfo> nonces, boolean validateUri) {
             this.opaque = opaque;
             this.nonceValidity = nonceValidity;
             this.key = key;
-            this.cnonces = cnonces;
+            this.nonces = nonces;
             this.validateUri = validateUri;
         }
 
-        public boolean validate(Request request, String authorization,
-                LoginConfig config) {
+
+        public String getUsername() {
+            return userName;
+        }
+
+
+        public boolean parse(Request request, String authorization) {
             // Validate the authorization credentials format
             if (authorization == null) {
                 return false;
@@ -523,7 +540,6 @@ public class DigestAuthenticator extends
             String[] tokens = 
authorization.split(",(?=(?:[^\"]*\"[^\"]*\")+$)");
 
             method = request.getMethod();
-            String opaque = null;
 
             for (int i = 0; i < tokens.length; i++) {
                 String currentToken = tokens[i];
@@ -555,9 +571,13 @@ public class DigestAuthenticator extends
                 if ("response".equals(currentTokenName))
                     response = removeQuotes(currentTokenValue);
                 if ("opaque".equals(currentTokenName))
-                    opaque = removeQuotes(currentTokenValue);
+                    opaqueReceived = removeQuotes(currentTokenValue);
             }
 
+            return true;
+        }
+
+        public boolean validate(Request request, LoginConfig config) {
             if ( (userName == null) || (realmName == null) || (nonce == null)
                  || (uri == null) || (response == null) ) {
                 return false;
@@ -573,7 +593,23 @@ public class DigestAuthenticator extends
                     uriQuery = request.getRequestURI() + "?" + query;
                 }
                 if (!uri.equals(uriQuery)) {
-                    return false;
+                    // Some clients (older Android) use an absolute URI for
+                    // DIGEST but a relative URI in the request line.
+                    // request. 2.3.5 < fixed Android version <= 4.0.3
+                    String host = request.getHeader("host");
+                    String scheme = request.getScheme();
+                    if (host != null && !uriQuery.startsWith(scheme)) {
+                        StringBuilder absolute = new StringBuilder();
+                        absolute.append(scheme);
+                        absolute.append("://");
+                        absolute.append(host);
+                        absolute.append(uriQuery);
+                        if (!uri.equals(absolute.toString())) {
+                            return false;
+                        }
+                    } else {
+                        return false;
+                    }
                 }
             }
 
@@ -587,7 +623,7 @@ public class DigestAuthenticator extends
             }
             
             // Validate the opaque string
-            if (!this.opaque.equals(opaque)) {
+            if (!opaque.equals(opaqueReceived)) {
                 return false;
             }
 
@@ -606,7 +642,9 @@ public class DigestAuthenticator extends
             long currentTime = System.currentTimeMillis();
             if ((currentTime - nonceTime) > nonceValidity) {
                 nonceStale = true;
-                return false;
+                synchronized (nonces) {
+                    nonces.remove(nonce);
+                }
             }
             String serverIpTimeKey =
                 request.getRemoteAddr() + ":" + nonceTime + ":" + key;
@@ -625,7 +663,7 @@ public class DigestAuthenticator extends
             }
 
             // Validate cnonce and nc
-            // Check if presence of nc and nonce is consistent with presence 
of qop
+            // Check if presence of nc and Cnonce is consistent with presence 
of qop
             if (qop == null) {
                 if (cnonce != null || nc != null) {
                     return false;
@@ -634,7 +672,9 @@ public class DigestAuthenticator extends
                 if (cnonce == null || nc == null) {
                     return false;
                 }
-                if (nc.length() != 8) {
+                // RFC 2617 says nc must be 8 digits long. Older Android 
clients
+                // use 6. 2.3.5 < fixed Android version <= 4.0.3
+                if (nc.length() < 6 || nc.length() > 8) {
                     return false;
                 }
                 long count;
@@ -644,21 +684,18 @@ public class DigestAuthenticator extends
                     return false;
                 }
                 NonceInfo info;
-                synchronized (cnonces) {
-                    info = cnonces.get(cnonce);
+                synchronized (nonces) {
+                    info = nonces.get(nonce);
                 }
                 if (info == null) {
-                    info = new NonceInfo();
+                    // Nonce is valid but not in cache. It must have dropped 
out
+                    // of the cache - force a re-authentication
+                    nonceStale = true;
                 } else {
-                    if (count <= info.getCount()) {
+                    if (!info.nonceCountValid(count)) {
                         return false;
                     }
                 }
-                info.setCount(count);
-                info.setTimestamp(currentTime);
-                synchronized (cnonces) {
-                    cnonces.put(cnonce, info);
-                }
             }
             return true;
         }
@@ -685,19 +722,31 @@ public class DigestAuthenticator extends
     }
 
     private static class NonceInfo {
-        private volatile long count;
         private volatile long timestamp;
-        
-        public void setCount(long l) {
-            count = l;
+        private volatile boolean seen[];
+        private volatile int offset;
+        private volatile int count = 0;
+
+        public NonceInfo(long currentTime, int seenWindowSize) {
+            this.timestamp = currentTime;
+            seen = new boolean[seenWindowSize];
+            offset = seenWindowSize / 2;
         }
         
-        public long getCount() {
-            return count;
-        }
-        
-        public void setTimestamp(long l) {
-            timestamp = l;
+        public synchronized boolean nonceCountValid(long nonceCount) {
+            if ((count - offset) >= nonceCount ||
+                    (nonceCount > count - offset + seen.length)) {
+                return false;
+            }
+            int checkIndex = (int) ((nonceCount + offset) % seen.length);
+            if (seen[checkIndex]) {
+                return false;
+            } else {
+                seen[checkIndex] = true;
+                seen[count % seen.length] = false;
+                count++;
+                return true;
+            }
         }
         
         public long getTimestamp() {

Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1380829&r1=1380828&r2=1380829&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Tue Sep  4 19:48:27 2012
@@ -192,6 +192,12 @@
         when logging in when session IDs are being encoded as path parameters.
         (markt)
       </fix>
+      <fix>
+        Various improvements to the DIGEST authenticator including
+        <bug>52954</bug>, the disabling caching of an authenticated user in the
+        session by default, tracking server rather than client nonces and 
better
+        handling of stale nonce values. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">

Modified: tomcat/tc6.0.x/trunk/webapps/docs/config/valve.xml
URL: 
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/config/valve.xml?rev=1380829&r1=1380828&r2=1380829&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/config/valve.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/config/valve.xml Tue Sep  4 19:48:27 2012
@@ -574,6 +574,12 @@
 
     <attributes>
 
+      <attribute name="cache" required="false">
+        <p>Should we cache authenticated Principals if the request is part of 
an
+        HTTP session? If not specified, the default value of <code>false</code>
+        will be used.</p>
+      </attribute>
+      
       <attribute name="className" required="true">
         <p>Java class name of the implementation to use.  This MUST be set to
         
<strong>org.apache.catalina.authenticator.DigestAuthenticator</strong>.</p>
@@ -596,6 +602,32 @@
         <code>true</code> will be used.</p>
       </attribute>
 
+      <attribute name="key" required="false">
+        <p>The secret key used by digest authentication. If not set, a secure
+        random value is generated. This should normally only be set when it is
+        necessary to keep key values constant either across server restarts
+        and/or across a cluster.</p>
+      </attribute>
+
+      <attribute name="nonceCacheSize" required="false">
+        <p>To protect against replay attacks, the DIGEST authenticator tracks
+        server nonce and nonce count values. This attribute controls the size
+        of that cache. If not specified, the default value of 1000 is used.</p>
+      </attribute>
+
+      <attribute name="nonceValidity" required="false">
+        <p>The time, in milliseconds, that a server generated nonce will be
+        considered valid for use in authentication. If not specified, the
+        default value of 300000 (5 minutes) will be used.</p>
+      </attribute>
+
+      <attribute name="opaque" required="false">
+        <p>The opaque server string used by digest authentication. If not set, 
a
+        random value is generated. This should normally only be set when it is
+        necessary to keep opaque values constant either across server restarts
+        and/or across a cluster.</p>
+      </attribute>
+      
       <attribute name="securePagesWithPragma" required="false">
         <p>Controls the caching of pages that are protected by security
         constraints. Setting this to <code>false</code> may help work around
@@ -605,6 +637,14 @@
         If not set, the default value of <code>true</code> will be used.</p>
       </attribute>
 
+      <attribute name="validateUri" required="false">
+        <p>Should the URI be validated as required by RFC2617? If not 
specified,
+        the default value of <code>true</code> will be used. This should
+        normally only be set when Tomcat is located behind a reverse proxy and
+        the proxy is modifying the URI passed to Tomcat such that DIGEST
+        authentication always fails.</p>
+      </attribute>
+
     </attributes>
 
   </subsection>



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to