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?

Why don't you use password_hash function? php.net/password_hash There is a link to Userland implementation, if you want to make it work with older version of PHP. — Naktibalda
– Naktibalda, Commented Nov 24, 2015 at 16:36

tim · Accepted Answer · 2015-11-24 16:53:32Z

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.

Thank you. I currently use PHP 5 <= 5.5.0, and I watch some outdated courses / sources that have been created before PHP 5.5.0... so I didn't know about password_hash() and password_verify() ...Anyway, I'll have to update PHP version as soon as possible. — PeraMika
– PeraMika, Commented Nov 26, 2015 at 8:00

Stack Exchange Network

Password encrypt & check

1 Answer 1

You must log in to answer this question.

Linked

Hot Network Questions

Password encrypt & check

1 Answer 1

You must log in to answer this question.

Linked

Related

Hot Network Questions