Bug 54060

Summary: DigestAuthenticator doesn't parse Authorization header correctly
Product: Tomcat 7 Reporter: Mark Thornton <mthornton>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal CC: mthornton
Priority: P2    
Version: 7.0.30   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: Simple test of the parsing
Proposed patch fixes problem
Patch in alternative format

Description Mark Thornton 2012-10-28 21:36:52 UTC
From DigestAuthenticator at line 546

            // Bugzilla 37132: http://issues.apache.org/bugzilla/show_bug.cgi?id=37132
            String[] tokens = authorization.split(",(?=(?:[^\"]*\"[^\"]*\")+$)");

if the last term in the line is not enclosed in quotes, only a single 'term' results. For example:

Header: username="mthornton", qop=auth
token[0] is username="mthornton", qop=auth

Header: username="mthornton", qop=auth, cnonce="9926cb3c334ede11"
token[0] is username="mthornton"
token[1] is  qop=auth
token[2] is  cnonce="9926cb3c334ede11"

(Headers abbreviated for clarity).
Comment 1 Mark Thornton 2012-10-28 21:45:15 UTC
Created attachment 29515 [details]
Simple test of the parsing

Test code for the erroneous line in DigestAuthenticator.

Includes two examples taken from actual logs of submitted headers.
Plus two pruned examples.
Comment 2 Mark Thornton 2012-10-29 10:10:50 UTC
The current parsing would also be broken by including an embedded quote in fields (e.g. in the cnonce field). E.g. cnonce="Mgytrr\"gfh"

Looking for a regular expression that correctly handles this syntax may not be sensible.
Comment 3 Mark Thornton 2012-10-29 15:21:54 UTC
Created attachment 29518 [details]
Proposed patch fixes problem

A more rigorous parsing of header lines might be appropriate
Comment 4 Mark Thornton 2012-10-29 15:28:00 UTC
The patch is against 7.0.30 because that is the most recent version packaged for Ubuntu 12.10.
Comment 5 Mark Thomas 2012-10-29 17:10:02 UTC
diff -u format for the patch please. I'm tempted to use the HTTP header parser for this. It'll need a fair bit of work for this though.
Comment 6 Mark Thornton 2012-10-29 18:26:00 UTC
Created attachment 29519 [details]
Patch in alternative format

I hope this the preferred format
result of git diff -p
Comment 7 Mark Thomas 2012-10-29 20:54:34 UTC
Thanks. Much more readable.

I'm currently mulling over how to handle this. The HttpParser is very heavy-weight but the simple approach is demonstrably prone to failure. I'm wondering if writing a generic HTTP header value parser is the way to go. I may experiment a little and see what I can come up with.
Comment 8 Mark Thomas 2012-11-01 21:30:45 UTC
I have added a first cut of a new HTTP header parser to trunk. The tests attached to this issue pass but there is more work to do before the new parser can be used to solve this issue.
Comment 9 Mark Thomas 2012-11-03 15:51:01 UTC
This has been fixed in trunk and 7.0.x and will be included in 7.0.33 onwards.
Comment 10 Sean Owen 2012-12-12 22:37:39 UTC
Hello all, first I would like to say that I think this patch is entirely right. I even checked against RFC 2617. But after this change I'm noticing that DIGEST authentication stops working in Safari, curl, and Java's SDK. Chrome is fine.

The reason, it seems, is that their Digest response includes either algorithm="MD5" when it should be algorithm=MD5, or qop="auth" when it should be qop=auth.

For example, from curl:

* Connection #0 to host localhost left intact
* Issue another request to this URL: 'https://localhost:8453/ready'
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 8453 (#0)
* Server auth using Digest with user 'foo'
> HEAD /ready HTTP/1.1
> Authorization: Digest username="foo", realm="Myrrix", nonce="1355351469307:f5864c38c03153e941d0e0ec6e6b625f", uri="/ready", cnonce="MTM1NTM1", nc=00000001, qop="auth", response="cccab2adb7a9c59f9eeac8b6981e79c0", opaque="B1094CC78FA4B4D9288C50B02C975C0F"
> User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5
> Host: localhost:8453
> Accept: */*

In the new implementation this means the parser rejects it since it is not expecting a quoted field.

Going back to the prior version of Tomcat works in the sense that the old parser was (too) lenient on incorrect quoting. Also changing these fields to be treated like type 'FIELD_TYPE_QUOTED_STRING' works.

It's not a bug in Tomcat though AFAICT. But if it really means a lot of popular implementations don't work with it, I dunno, maybe worth revisiting.
Comment 11 Konstantin Kolinko 2013-01-10 02:59:34 UTC
(In reply to comment #10)
> The reason, it seems, is that their Digest response includes either
> algorithm="MD5" when it should be algorithm=MD5, or qop="auth" when it
> should be qop=auth.

Quoting of qop was allowed by r1429124 (following bug 54372 ).

What clients quote the algorithm field?
(I do not mind to fix it in the same way, but I would like to know the reason).
Comment 12 Sean Owen 2013-01-25 07:29:34 UTC
Hello Konstantin, it's the JVM that seems to send a bad algorithm value. I've reproduced it in the following, at least:

- Java 1.6.0_37 for Mac OS X
- Java 1.7.0_10 for Mac OS X
- OpenJDK 1.7.0_09 for Linux

They send something like...

Digest username="foo", realm="Myrrix", nonce="1359097999996:13ed87b1b78c157232d609a099bcdb6e", nc=00000001, uri="/ready", response="b6f80b049b4b39000da79c96442e0740", algorithm="MD5", opaque="3E8794E4CE80B19E5DF888D615FFBBA5", cnonce="DGKKOPAFPJKCKKBDLFECINONACKFJIFNDOGKGLIO", qop="auth"

algorithm="MD5" is the culprit.

(Indeed looks like this is just wrong in the latest OpenJDK 7 source, at least: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/sun/net/www/protocol/http/DigestAuthentication.java?av=f#367 I will see about filing a bug.)

If I modify the code such that the "algorithm" field is treated as "FIELD_TYPE_QUOTED_STRING" then it works, and other clients seem to still work (Safari, Chrome, curl at least).

It would indeed be great to apply the same workaround, thanks!
Comment 13 Konstantin Kolinko 2013-01-28 12:42:10 UTC
REOPENing the issue to address Comment 12
Comment 14 Mark Thomas 2013-01-28 15:13:09 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.36 onwards.

algorithm is now permitted to be a quoted token. Note no such definition exists in the specs. It is <"><TOKEN><"> as some clients insist on adding quotes to tokens.
Comment 15 Mark Thornton 2013-05-21 13:00:40 UTC
HttpClient is a culprit in quoting algorithm. Unfortunately Ubuntu 13.04 uses TomCat 7.0.35 and is thus affected. I have filed an issue with HttpClient: