On 16/09/2014 16:20, Christopher Schultz wrote:
> 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).
No objections to this if it is doable without too much impact to the
existing code but:
a) I'm not convinced it is worth it
b) I want to look at when the String -> bytes conversions would need to
take place.
> 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.
I take the point but having fixed values for saltSize and iterations
then limits you to using the same values for every entry in the Realm
which creates a different set of issues. I'm not sure there is an API
that can meet all those requirements but it is worth spending some time
thinking about it.
> 3. MessageDigestCredentialHandler is good, but I think we ought to
> push-down the iteration and salt members and methods from
> BaseCredentialHandler into the subclass.
Fair point if we start having CredentialHandlers based on symmetric
encryption. I'm not convinced that is a good idea but I take the point.
> 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?
Don't know. It is what the current code does so I am reluctant to change
it without a report that something is broken.
> 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).
We should probably log a warning message for the sys admin.
> 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();
Fair enough.
> 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).
No strong opinion.
> 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.
It is only used in off-line mode so I think this is safe.
> 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.
That is doable but it raises some of the issues you've highlighted in 2)
and 3) above.
> I'm sure I'll come up with other comments later, but this ought to get
> the ball rolling.
>
> Thanks,
> -chris
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
