All, In reference to bug 56403 (https://issues.apache.org/bugzilla/show_bug.cgi?id=56403) and specifically markt's proposed patch (http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v1.patch), I have the following comments.
I'm interested in what others have to say.
1. In terms of limiting converting to String values, we could base
everything on byte[] instead of String. That also makes it possible to
blank-out the credential arrays when we are done with them, slightly
improving security; as long as the j_password is never retrieved using
HttpServletRequest.getParameter, we can avoid String values entirely and
purge the characters from memory as soon as we are done. Nothing that I
know of can stop the memory manager from re-locating byte arrays in
memory (short of using native code to temporarily pin them, but that
gets very dangerous and messy).
2. I don't like CredentialHandler.mutate(String input, byte[] salt, int
iterations). I think it ties the method signature to the implementation
of the mutation algorithm. PBKDF2 for instance has both "salt" and
"iterations", but straight-up MD5 (e.g. our existing implementation)
does not. Also, non-hashing algorithms like any symmetric encryption
algorithm (e.g. AES, Blowfish, etc.) does not have such parameters, and
would require instead an encryption key. I think that mutate() should
only take a single argument but also have setters that can be called
from configuration, like this:
<Realm>
<CredentialHandler class="PBKDF2" saltSize="8" iterations="50000" />
</Realm>
In this case, the PBKDF2 class would have a setIterations(int) method
and a setSaltSize(int) method to set the number of salt bytes to use.
If we want the implementations for MessageDigest-based handlers as well
as things like PBKDF2 to extend the same base class that provides lots
of utility methods, that's find. I just don't like tying the interface
itself for the matches() method to a particular style of implementation.
3. MessageDigestCredentialHandler is good, but I think we ought to
push-down the iteration and salt members and methods from
BaseCredentialHandler into the subclass.
4. In MessageDigestCredentialHandler.matches, there are two uses of
StandardCharsets.ISO_8859_1 directly instead of using the client's
preferred character encoding. Is this a requirement of {MD5|SHA|SSHA}
formatting or an oversight?
5. matchesSaltIterationsEncoded method does not check for unexpected
formatting. Specifically, the values for sep1 and sep2 are never checked
to see if they are positive or that sep1 < sep2. I'm not sure which
would be better: StringArrayIndexOutOfBoundsException from the JVM or
something a little more nuanced from the CredentialHandler itself. The
JVM is going to perform the checks anyway... should we waste a few
cycles to check these values ourselves and throw a different kind of
exception, or is it unnecessary? (I suppose try/catch could be used to
convert SAIOOBE into something else with a better error message, but
try/catch has its own overhead as well).
6. Performance. There are places where we know the sizes of things
before they are allocated but don't use that to our advantage. An
example from CredentialHandlerBase.generate:
return HexUtils.toHexString(salt) + "$" + iterations + "$" +
serverCredential;
We know in advance the exact length of the string we need, but the
compiler will use a StringBuilder with a default capacity (16 characters
in Oracle Java 7). It would be better to pre-allocate the buffer we need:
StringBuilder credential = new StringBuilder(saltLength << 1 +
serverCredential.length() + 10 + 2);
credential.append(HexUtils.toString(salt))
.append('$')
.append(iterations)
.append('$')
.append(serverCredential);
return credential.toString();
I'm not sure if it matters too much, but a bit of space could be saved
by storing the iterations in hex encoding as well (max 8 characters
instead of 10).
7. Thread safety. Is it safe to check/set the "random" reference in
multiple threads? I suppose the worse thing that could happen would be
to create more SecureRandom objects than strictly necessary.
8. RealmBase.main checks all known CredentialHandlers for matching
algorithms (a bit inefficient, but its offline so who cares), but it
doesn't know about anyone's custom CH classes. We should allow the
caller to specify the CH class if they want, so they can use the
RealmBase driver with their own CredentiaHandler implementation.
I'm sure I'll come up with other comments later, but this ought to get
the ball rolling.
Thanks,
-chris
signature.asc
Description: OpenPGP digital signature
