Secure password-hashing in Java

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

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

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

-
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)?

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

-
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 newPasswor

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!

StackExchange Code Review Q#56939, answer score: 7