Password encrypt & check

Question

When a user registers (creates a new account), I want to encrypt password before storing in a database, and when a user logs on (with username & password) I want to check password. I did it in the following way:

function password_encrypt($pass) {
    $hash_format = "$2y$10$"; // Tells PHP to use Blowfish with a "cost" of a 10
    $salt_length = 22; // Blowfish salts should be 22-characters or more
    $salt = generate_salt($salt_length);
    $format_and_salt = $hash_format . $salt;
    $hash = crypt($pass, $format_and_salt);
    return $hash;
}

function generate_salt($salt_length) {
    // Not 100% unique, not 100% random, but good enough for a salt
    // MD5 returns 32 characters
    $unique_random_string = md5(uniqid(mt_rand(), true));

    // Valid characters for a salt are [a-zA-Z0-9./]
    $base64_string = base64_encode($unique_random_string);

    // But not '+' which is valid in base64 encoding
    $modified_base64_string = str_replace('+', '.', $base64_string);

    // Truncate string to the correct length
    $salt = substr($modified_base64_string, 0, $salt_length);

    return $salt;
}

function password_check($password, $existing_hash) {
    // existing hash contains format and salt at start
    $hash = crypt($password, $existing_hash);
    if ($hash === $existing_hash) {
        return true;
    } else {
        return false;
    }
}

Can you do a review of this code, give some suggestions?

Also, I have one more question: Is it a good idea to use these functions as private methods of

• User Model Entity Class OR
• User Repository Class

or simply to have a file called functions.php and to put them there and to use them in controller?

Answer 1

Why reinvent the wheel when you can just use password_hash? Your code could be replaced by this:

$hash = password_hash($password);

password_verify($password, $hash);

It uses bcrypt (which uses blowfish internally), and it even manages salts for you.

This should also answer your additional questions: You don't really need any of the functions.

Misc

• Blowfish is an encryption algorithm, but when used with crypt, what you do is still hashing, not encryption. So your function should be called password_hash.
• === isn't timing safe. This is more of a theoretical problem, but it's still better to use a timing safe function here.
• if (cond) {return true;} else {return false;} can be written as return cond;
• Contrary to your comments, generated salts may contain =.
• You should document that your salt generation function can only generate salts with a max length of 44, in case it's reused in a different context.
• One-time variables can be nice to give something a name, thus resulting in easier to read code (eg in your first two functions). But sometimes they are not really needed. In your second function, I would simply write return crypt($password, $existing_hash) === $existing_hash, it's shorter and I don't think you loose any readability.