5
\$\begingroup\$

I've written a Java class that will provide the utility of, in essence, hashing user passwords for an Android app that integrates Couchbase mobile into it, and then, checking whether or not the hashing of the password string entered by the user matches the hash stored in the DB to grant access.

Does there appear to be any major issues with this code to anyone? I know crypto is a very tricky thing and mistakes can be easily made.

package fitfast.security;

import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.KeySpec;
import java.util.Arrays;

import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;


public class Authenticator {

    public static byte[] generateHash(String password, byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException {
        String algorithm = "PBKDF2WithHmacSHA512";
        int length = 512;
        int iterations = 60000;
        KeySpec sp = new PBEKeySpec(password.toCharArray(), salt, iterations, length);
        SecretKeyFactory kf = SecretKeyFactory.getInstance(algorithm);
        return kf.generateSecret(sp).getEncoded();

    }

    public static byte[] generateSalt() throws NoSuchAlgorithmException {
        SecureRandom sr = SecureRandom.getInstance("SHA1PRNG");
        byte[] salt = new byte[8];
        sr.nextBytes(salt);
        return salt;
    }

    public static boolean check(byte[] hash_Input, String hash_User) {
        return Arrays.equals(hash_Input, hash_User.getBytes());
    }



}
\$\endgroup\$
2
\$\begingroup\$

By everything I know the code looks OK from security POV.

Parameter naming convention

Your check function takes hash_User and hash_Input as parameters. This is a pretty strange naming convention. Standard Java convention would be hashUser and hashInput (which you already use in method names). hash_user and hash_input (note the lack of capital letters) would also be pretty OK.

Constants

As chillworld stated in his answer I'd extract algorithm, length and iterations as nicely named constants with some nice JavaDoc. It'll be easier to understand the code later (not an issue at this point), and easier to reuse it it next methods. While you're at it you could also declare the length of generated salt.

Encoding

Your generateHash function takes String and uses getBytes on it. Note that this is influenced by the platform's default encoding. If you later move to another platform, your hashes suddenly won't match anymore. You could force encoding by using one of the other variants of getBytes.

Verifying the password

Your check method takes byte[] hash_Input, String hash_User. Now, I'm having trouble guessing which one should be the hash stored in the DB and which one the new one user is trying to log in with. Also, how do you plan to get the one that's represented as String? Considering that the generateHash function returns byte[] you'd have to convert it to String and then back to byte array, but that's not reversible - if you use e.g. UTF-8 more than one byte sequence maps to the same String. And do you ever plan to use it really in that way? Why not having a function boolean check(byte[] hash, String password, byte[] salt) instead?

Plan for the future

This one is less clear. Suppose at some point later you want to change your algorithm/parameters (say 60000 iterations is no longer enough). Suddenly all your old hashes are no longer valid. If it's a new installation it's no problem, but what if you update installation which already has 10000 users? You can try to decode using both ways and log in if one succeeds. This increases the CPU cost of login, allows for possible collisions (unlikely). And what if you then change the algorithm second time? You could somehow encode the algorithm and parameters in the hash. This removes the possibility of collisions and later allows you to cheaply select which one of the (costly) password hashing functions to choose. And it scales well with subsequent changes. You could then update the stored hash if a user with old hash logs in. And since distinguishing users with old hash methods is easy, after some time you could ping the ones that didn't log on. The bad part: After some time one could forcefully hack your DB and replace with the weaker hash. You'll have to think carefully whether you want this one.

Plan for tests

Another not so clear one. In the current way whoever uses this class has to use this exact specification. Normally this is exactly what you want, but in case you'd like to unit-test anything that uses it the test will become unnecessary long (PBE algorithms are designed exactly to be slow, and will slow down your test in this case). You can work around this by declaring an interface with two implementations - this one, and the mock one used in tests (say generateHash returns simply password and salt concatenated and generateSalt returns a constant array). Good part: Your tests no longer need 1 second each to run. Bad part: You need to somehow get that instance in each and every class that needs it. Could be as easy as using Sprint and @Autowire on the correct field. Could be as hard as having every object own a reference to this object only to pass on to newly generated objects, while a handful of those deep in the creation chain really uses that reference. Second bad part: Somehow could inject your objects with another, weaker implementation. And this could really be a problem (compare with previous point) when you use e.g. applet or a client-side application to generate the hashes. You'd also need to consider the good and bad consequences here.

\$\endgroup\$
1
\$\begingroup\$

Helper classes

You have a nice helper class, all the public methods are static, what is good. There are 2 things I miss.
First of all, your class should be declared as final.
The reason why is so nobody could extends your class and implement other behaviour.

Second one is a private constructor, because no constructor means 1 public constructor with no arguments.
You don't want people to make instances of your class and hold them somewhere.
You are smart enough not to do so, but is your (future) colleague that also?

Private static variable

While it's not wrong you declare the variable like algorithm,length,... in the methods.
Why not make a global private static final int LENGTH = 512;
Like this it's only instanciated once and you could reuse this value in multiple methods (if your class grows in the future).

Summary

For the rest I can't see directly any issues, but of course I'm also a human being with faults.
I really should ask to that programmer who created me why he added so many faults ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.