tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Burch <>
Subject Re: Tomcat 7.0.41 JNDIRealm revision 1491394
Date Thu, 27 Jun 2013 18:23:18 GMT
On 27/06/13 18:51, Konstantin Kolinko wrote:
> 2013/6/27 Brian Burch <>:
>> I eventually got round to integration testing of 7.0.41 yesterday and was
>> baffled to find I couldn't logon!
>> To cut a long debugging story short, revision 1491394 has a bug that was
>> introduced as part of the standardisation of our Base64 handling. I wasn't
>> sure whether I ought to open a new bug...
> Your numbering is wrong, that revision is not ours. It was this one:

Yes, I see what you mean and agree. I'm puzzled that my svn diff 
reported the wrong revision, which is a pity because I put it in the new 
thread subject and we will have to live with it unless you can 
retrospectively edit the entries?

Thanks for finding the correct revision.

>> Here is the diff that works for me:
>> Index: java/org/apache/catalina/realm/
>> ===================================================================
>> --- java/org/apache/catalina/realm/       (revision 1491394)
>> +++ java/org/apache/catalina/realm/       (working copy)
>> @@ -1573,9 +1573,10 @@
>>                       password = password.substring(5);
>>                       md.reset();
>> md.update(credentials.getBytes(Charset.defaultCharset()));
>> -                    byte[] decoded = Base64.decodeBase64(md.digest());
>> +                    byte[] digest = md.digest();
>> +                    byte[] base64 = Base64.encodeBase64(digest);
>>                       String digestedPassword =
>> -                            new String(decoded, B2CConverter.ISO_8859_1);
>> +                            new String(base64, B2CConverter.ISO_8859_1);
>>                       validated = password.equals(digestedPassword);
>>                   }
>>               } else if (password.startsWith("{SSHA}")) {
> In short,  s/ decodeBase64 / encodeBase64 /.

Yes. I wanted to break the logic up because I needed to encode/decode my 
test cases by hand!

I didn't trust my own external cribs and suspected I was digging into a 
difference between the sun and openjdk digest engines. 
Pencil/paper/hexadecimal calculator...

  I didn't want to go round the loop of another source 
change/build/deploy/test/debug/diff. You got the idea, just as I expected.

Imagine my surprise when I got 80% through the exercise and spotted the 
"backwards" choice of Base64 method!

> It is fun that {MD5}&{SHA} branch and {SSHA} branch there use
> different approaches there
> (encoding the user-supplied password vs. decoding the stored one).

Actually, I can sort-of understand the person adding SSHA doing his/her 
own thing and not wanting to change logic that is stable and works 
(well, it did in 7.0.28!).

If we are looking for a vote, I prefer the way MD5/SHA does it. It takes 
the user-supplied cleartext and digest/encodes it in a forward 
direction, such that the directory hash is never exposed in situations 
where the supplied value is incorrect.

>> BTW. The code is identical in trunk, so this patch works there too.
>> Thinks... pity some of this stuff doesn't have some lightweight unit tests.
>> Sorry to be a informal with this notification, but I thought timeliness was
>> more important than style!

So.... will you deal with it from now on, or would you like me to open 
bugs on tc7 and tc8?


> Best regards,
> Konstantin Kolinko
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message