Bug 32137 - Random "401" responses for Digest - DigestAuthenticator thread un-safe use of MessageDigest
Summary: Random "401" responses for Digest - DigestAuthenticator thread un-safe use of...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 5.0.29
Hardware: Macintosh All
: P3 major (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-11-10 01:14 UTC by Chris Sharp
Modified: 2004-12-09 05:56 UTC (History)
0 users



Attachments
RealmBase patch (914 bytes, patch)
2004-12-08 22:29 UTC, Chris Sharp
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Sharp 2004-11-10 01:14:49 UTC
Static use of java.security.MessageDigest is not thread-safe. A symptom of this bug would be "random" 
401 responses to Digest Authentication challenges. The static declaration:
protected static MessageDigest md5Helper;

The  use of this idiom in the DigestAuthenticator (and RealmBase) class means that all threads are 
effectively using the same instance of the MessageDigest. Highly concurrent testing showed this defect. 
In reviewing the java docs for MessageDigest, there is nothing that indicates that it is thread-safe. 

There are two solutions, either create a wrapper class which synchronizes access, or create new 
instances when needed. For performance reasons it would probably be a good idea to create a wrapper 
class as creating new instances can be expensive.
Comment 1 Remy Maucherat 2004-11-12 10:30:51 UTC
I looked in MessageDigest, and indeed, it is not thread safe. Problems are very
unlikely to occur, though.
Comment 2 sam 2004-11-20 18:21:18 UTC
As far as I can see DigestAuthenticator only ever wants
a digested AND encoded String given a String.

It might simplify the code to factor out the synchronize+digest+encode
into another utility method in MD5Encoder that accepts single 
String parameter and returns an encoded MD5 digest String result. 
This method would synchronize on its MessageDigest.

Addition to MD5Encoder could be something like:

	private static MessageDigest md5Helper;

	static {
		try {
			if (md5Helper == null)
				md5Helper = MessageDigest.getInstance("MD5");
		} catch (NoSuchAlgorithmException e) {
			throw new IllegalStateException();
		}
	}
	
	public String digestAndEncode(String p) {
	    synchronized (md5Helper) {
	        return encode(md5Helper.digest(p.getBytes()));
	    }
	}

Sorry for lack of a proper patch but Ive not got proper setup at moment.
Comment 3 Chris Sharp 2004-12-08 22:28:37 UTC
Checked nightly build and there are other places where this bug could rear it's ugly head:
1. RealmBase
2. org.apache.catalina.servlets.WebdavServlet

Comment 4 Chris Sharp 2004-12-08 22:29:14 UTC
Created attachment 13686 [details]
RealmBase patch
Comment 5 Yoav Shapira 2004-12-09 14:54:02 UTC
The issue is rare, but possible, and I like this patch because it's small, and 
safe.  The performance impact is negligible.  So I'll go ahead and commit it 
for RealmBase -- thanks for submitting it.

WebDAV is a bit of a different story: it's not nearly as much of a concern, 
just a little lightweight implementation not subject to highly concurrent use.

Done for 5.0.31, doing 5.5.6 next.