Secure password-hashing in Java

Question

Here is an article on password hashing, along with an implementation.

1. Is this code secure with number of iterations 10000, key length 256 and salt bytes 32?

2. Is there a rule-of-thumb for key length vs. salt bytes?

3. Regarding key.destroy() and pbeKeySpec.clearPassword(), is it necessary to "manually" destroy the key and clear the password? If so, does the order matter (e.g. should the key be destroyed before the password is cleared or vice-versa)?

4. Is it considered good practice to "zero out" password char array in the below example?

5. Are there vulnerabilities or incorrectly implemented sections in this code?

import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;
import javax.xml.bind.DatatypeConverter;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;

/**
 * Request for code review on Stack Exchange:
 * https://codereview.stackexchange.com/questions/56939/secure-password-hashing-in-java
 *
 * Some reference pages from around the web:
 * https://crackstation.net/hashing-security.htm
 * https://owasp.org/index.php/Hashing_Java
 * http://stackoverflow.com/questions/2267036/work-sun-misc-base64encoder-decoder-for-getting-byte
 * http://stackoverflow.com/questions/2860943/suggestions-for-library-to-hash-passwords-in-java
 * https://stackoverflow.com/questions/6464662/web-application-storing-a-password
 * https://stackoverflow.com/questions/6126061/pbekeyspec-what-do-the-iterationcount-and-keylength-parameters-influence
 * https://stackoverflow.com/questions/992019/java-256-bit-aes-password-based-encryption
 */
public abstract class PasswordDigester
{
    private static final int NUM_ITERATIONS = 10000;
    private static final int KEY_LENGTH = 256;
    private static final int SALT_BYTES = 32;

    /**
     * Hash a plain text password into the format iterations:salt:hashed-password.
     * @param newPassword A plain text password that needs to be hashed.
     * @return a hashed password in the format iterations:salt:hashed-password.
     */
    public static String getSaltedHash(String newPassword)
    {
        final byte[] nextRandomSalt = new byte[SALT_BYTES];
        try { SecureRandom.getInstance("SHA1PRNG").nextBytes(nextRandomSalt); }
        catch(NoSuchAlgorithmException e) { throw new IllegalStateException(e); }
        return getSaltedHash(NUM_ITERATIONS, nextRandomSalt, newPassword.toCharArray(), KEY_LENGTH);
    }

    /**
     * Obtain the number of iterations and the salt from a previously hashed password and use them to salt- and
     * hash a password candidate.  Useful to validate a password.
     * @param unhashedPasswordCandidate A password submitted by a client presumably to login to an application.
     * @param previouslyHashedPassword The password on record whose iterations and salt will be used to hash
     *                                 the unhashed password candidate.
     * @return a hashed password that can be validated for correctness against the previously hashed password.
     */
    public static String getSaltedHash(String unhashedPasswordCandidate, String previouslyHashedPassword)
    {
        String[] params = previouslyHashedPassword.split(":");
        int iterations = Integer.parseInt(params[0]);
        byte[] randomizedSalt = DatatypeConverter.parseBase64Binary(params[1]);
        return getSaltedHash(iterations, randomizedSalt, unhashedPasswordCandidate.toCharArray(), KEY_LENGTH);
    }

    private static String getSaltedHash(int iterations, byte[] randomizedSalt, char[] password, int keyLength)
    {
        try
        {
            PBEKeySpec pbeKeySpec = new PBEKeySpec(password, randomizedSalt, iterations, keyLength);
            SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
            SecretKey key = secretKeyFactory.generateSecret(pbeKeySpec);
            String result =
                iterations +
                ":" +
                DatatypeConverter.printBase64Binary(randomizedSalt) +
                ":" +
                DatatypeConverter.printBase64Binary(key.getEncoded());

            //  Note: key.destroy() always throws DestroyFailedException, so I've commented it out.
            //key.destroy();
            pbeKeySpec.clearPassword();

            //  For security, "zero out" password char array.
            for(int i = 0; i < password.length; i++) { password[i] = '0'; }

            return result;
        }
        catch(InvalidKeySpecException | NoSuchAlgorithmException e)
        {
            throw new IllegalStateException(e);
        }
    }
}

With the above settings this is generated (new lines after each colon):

10000:

pwYOF5KPSB+hbLVmuBxuVG0H12GReS+8A+UgSiWrpbM=:

63M+swkV4binnRKXQYgZJbJJtzT9asR0w0bJgytJZpg=

Any advice or words of caution or ways to secure or correct this code would be welcome.

Answer 1

Okay, be warned, I know a little Java syntax, but I am really unfamiliar with the objects of Java. So if I get something incorrect, I'd appreciate a friendly reminder!

I'll see what I can point out...

So at first I notice you're supplying a static number of iterations. The number of iterations should vary depending on the computer that's encrypting. There's a good Q&A on Sec.SE that explains how you should determine the number of iterations you should use. Basically though, it boils down to as many as your machine can do without affecting performance dramatically. After you determine how many iterations suits you best, that number should be the static number.

This next one I had to do a little research for, but let's quickly note this line:

SecureRandom.getInstance("SHA1PRNG")

Here's an article (a bit old, but I believe relevant) which gives some background on this method. The author proposes that it's best to include the provider to prevent different PRNGs on different installations.

As said at the beginning, I'm not familiar with this area of Java, but it looks like you're doing what's desired, which would be encrypt then MAC. If I read your code wrong, let me know!

In regards to your third question, I think it would make more sense to destroy and then clear.

In regards to your fourth, I can't see a benefit or a downside. However, I'm not very familiar with Java vulnerabilities! So there may be some sort of leakage I'm not aware of, which could compromise the security of PBEKeySpec's password. But you clear the password with clearPassword(). My best guess would be that it's not completely necessary!

---

Other than that, the largest flaw I can see is the lack of formatting.

What I mean by this are things such as the two-lined try/catch:

try { SecureRandom.getInstance("SHA1PRNG").nextBytes(nextRandomSalt); }
catch(NoSuchAlgorithmException e) { throw new IllegalStateException(e); }

Compressing this into these two lines only made it harder to read. I suggest you give the code some space to breathe!

It's Java convention to have the first brace on the same line as the statement. So your two lines of

try
{

really should just be one. But that's just nit-picking!

I don't see a reason not to have the last for loop like

for (int i = 0; i < password.length; i++) {
    password[i] = '0'; 
}

Also note that I added a space between the for and (.

The opening brace after the last catch should be on the same line as the catch!