Securely hash passwords

Question

Since hashing algorithms isn't something to mess around with as a beginner. Several howto's explain that one should use already made solutions. Examples of such pages:

• https://crackstation.net/hashing-security.htm
• https://owasp.org/www-project-cheat-sheets/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2

So I opted for ready-made solutions from java.security and java.crypto library packages. But I feel there is still room for messing up, especially with regard to the parameters in PBEKeySpec(password, salt, iterations, keyLength).

According to some HOWTOs, salt should be as long in bytes as the output from the hash function. So I chose 64, since SHA-512 outputs (64*8=512 bits) 64 bytes. (Although, it seems I can get larger output simply by increasing keyLength argument above 512, which makes me wonder).

So basically. Do you guys/girls find this code reasonable? Especially in regards to the arguments passed? If not, please tell me what to change, how and why.

package my.application;

import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.util.Arrays;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;
import org.apache.commons.codec.binary.Hex;

public class PasswordHasher {
    private int ammountOfBytes = 64;
    private int keyLength = 512;
    private int iterations = 100000;
    private byte[] salt;
    private byte[] hash;
    private char[] password;
    private String saltHex;
    private String hashHex;

    public void hash(String rawPassword) {
        makeSalt();
        password = rawPassword.toCharArray();
        PBEKeySpec spec = new PBEKeySpec(password, salt, iterations, keyLength);
        SecretKey key;
        try {
            SecretKeyFactory generator = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512");
            key = generator.generateSecret(spec);
            hash = key.getEncoded();
            hashHex = Hex.encodeHexString(hash);
            spec.clearPassword();
            key = null;
        } 
        catch (NoSuchAlgorithmException e) {
            e.printStackTrace();
        } 
        catch (InvalidKeySpecException e) {
            e.printStackTrace();
        }

    }

    public void destroy() {
        saltHex = null;
        hashHex = null;
        Arrays.fill(password, (char) 0);    
        Arrays.fill(salt, (byte) 0);
        Arrays.fill(hash, (byte) 0);
    }

    private String makeSalt() {
        try {
            SecureRandom random = SecureRandom.getInstance("SHA1PRNG");
            salt = new byte[ammountOfBytes];
            random.nextBytes(salt);
            saltHex = Hex.encodeHexString(salt);
        } 
        catch (NoSuchAlgorithmException e) {
            e.printStackTrace();
        }
        return saltHex;

    }

    public String getSaltHex() {
        return saltHex;
    }

    public String getHashHex() {
        return hashHex;
    }

}

Answer 1

There are a few things wrong security wise before we even get to the parameters.

private char[] password;

Storing the plain text password in an instance field makes it discoverable from memory dumps for a considerably long time. There is no need for the field to be an instance field since it is only used in the hash method.

public void hash(String rawPassword) {

Passing plain text password as a String parameter makes it impossible to be securely disposed of. When being passed around, passwords need to be char arrays and they need to be cleared immediately when they are no longer needed.

public void destroy() {

You have the right idea that the class needs to be cleared from sensitive data, but here you have made the clearing the responsibility of the caller while the data needing to be cleared is completely irrelevant to the caller. You should avoid having to rely on other people when dealing with sensitive data. Someone will forget to call destroy() because it is not something that needs to be done in a garbage collected environment. At least you could make it Closeable so there is some common contractual indication about the class needing cleanup. Because the method also clears the actual payload it is guaranteed that the sensitive data will be in memory as long as the payload is needed, which is much longer than the sensitive data needs to exist. But it's better to write the class so that it doesn't need external cleanup at all.

private int ammountOfBytes = 64;
private int keyLength = 512;
private int iterations = 100000;

These should be static and final constants, since you don't have any means to change them.

private String saltHex;
private String hashHex;

These are the only fields that have a information that needs to be kept for a longer period. Instead of wrapping all the fields into a same PasswordHasher class, you should put these two fields into a dedicated data class called HashedPassword, have all the other code be static utilities and have the hash method return the data class.

If we look at the naming, PasswordHasher implies that the class is an actor that implements an algorithm. Your implementation uses it as both a data container and the algorithm. It is better to separate the responsibilities to two different classes (single responsibility principle)