Gabriel, On 9/22/14 7:56 PM, "Gabriel E. Sánchez Martínez" wrote: > > On 09/17/2014 04:36 AM, Mark Thomas wrote: >> On 16/09/2014 22:14, Christopher Schultz wrote: >>> Mark, >>> >>> On 9/16/14 3:39 PM, Mark Thomas wrote: >>>> Updated patch: >>>> http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v2.patch >>>> > It's looking good! >>> Looks good, but its missing a configuration for the digester to actually >>> read the configuration and set-up the CredentialHandler objects at >>> runtime. Existing MessageDigest-based configs will work, but explicit >>> class references won't. >> Ack. The docs need updating as well. >> >>> Speaking of which, I'd like to be able to nest CredentialHandler >>> instances. The use case is when switching from one type of >>> password-derivation method to another. We have done this at $work twice >>> and being able to handle more than one kind of valid credential in the >>> database is essential. >> OK. That seems like a reasonable requirement. >> >>> Given that we are giving better options to users than standard >>> single-pass MessageDigest password-mutators, we should help them >>> migrate. The only way to do that would be something like >>> CombinedCredentialHandler analogous to the CombinedRealm: you will >>> accept either MessageDigestCredentiaHandler{SHA1} /or/, say, PBKDF, >>> bcrypt, etc., by checking one CredentialHandler and then the second (or >>> third?) if the first one fails. > > This would be good. One could have an array of CredentialHandler to > check in order.
I think it makes sense to have a single CredentialHandler at the Realm level. We can nest them if necessary. > Is the idea that a password stored in an old format would be matched > using the old CredentialHandler and (upon first match) stored in the > upgraded format (the first CredentialHandler)? I assume the same idea > goes for when the same CredentialHandler is used but the number of > iterations has changed. An old credential would always be matched by the old algorithm. No auto-upgrade would occur or anything like that -- that's up to the application to handle. >>> Use of a CombinedCredentialHandler might result in a lot of spurious >>> warnings in the log about invalid credentials. Maybe the >>> CombinedCredentialHandler could tell the individual child >>> CredentialHandlers that they should not log invalid credentials? >> Yes, we'll need to make sure the logs don't fill up with false positives. > +1 >> >>> I'd like to get some other opinions on the public mutate() interface. I >>> think we might not be able to convince each other ;) >> You might be surprised. I was looking at using mutate() from match to >> reduce code duplication but if you limit mutate() to just generating new >> passwords then I agree there is no need for any other parameters. A >> protected method used by both mutate() and match() should work. I'll >> take another look. > > Without giving an opinion about what method should be called when, I > agree that salt size and iteration count should be taken from the stored > credential when matching. If the latest configuration asks for a > different algorithm, salt size, or iteration count, then the supplied > cleartext password should be re-hashed to the latest specs. > >> Regarding thread safety for the SecureRandom, we can do that now. It >> doesn't cost us very much and it prevents it tripping us up in the >> future. >> >> Mark >> >> > I saw that String.equals(String s) is being used. I'm not familiar with > the implementation but I imagine that if the string lengths differ or if > the first characters don't match, for example, the method returns false > without checking the rest of the characters. Perhaps that could lead to > a small vulnerability in which through many attempts and timing an an > attacker can infer whether the password length, etc. is right. I've > seen some implementations use a SecureEquals that tries to take > approximately the same time by comparing all characters of the strings > even if the lengths or first characters don't match. Is this a real > concern, or only theory? While timing attacks are certainly possible, String.equals() is only used at the very end of the operation and should be completely dominated by the iterated hashing algorithm which happens regardless of the correctness of the password. What might contribute to a timing attack is avoiding the iterated hashing operation under certain circumstances. For instance, in our own implementation of DataSourceRealm, if the user does not exist, we waste a bit of time hashing garbage and discard the result to avoid being able to determine whether a user exists simply by attempting a login and timing the response. We might want to have a "wasteTime" method added to the CredentialHandler interface to allow the Realm to ask the CH to just do a bunch of useless work to simulate a password match. Lazy implementations could simply do nothing. -chris
signature.asc
Description: OpenPGP digital signature
