Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

When hashing a password, I understand it is best not to use functions such as SHA1 or MD5.

This function is working fine and I want to know if it can be improved to increase site security.

class safePassword
{
    public static function makeSafe($password)
    {
        $salt = "mySalt";
        $hash = hash("sha256", $salt.".".$password);
        return $hash;
    }
}
share|improve this question
5  
There are like a zillion resources on this on Security.SE; have you spent 15 minutes reading through them? e.g., security.stackexchange.com/q/211/971. –  D.W. yesterday
9  
A fixed salt is no salt. –  CodesInChaos yesterday
1  
learn and use PHP's crypt() function –  Octopus yesterday
14  
Why re-invent the wheel? Just use: php.net/password_hash and php.net/password_verify. (In case you don't have PHP 5.5+: github.com/ircmaxell/password_compat) –  Rocket Hazmat yesterday
1  
@RocketHazmat Those two functions may be simple wrappers around crypt, but they certainly do protect against the most obvious mistakes one could make in calling crypt directly. As for the compatibility library, I am shocked by the way the password_verify function compares two strings. Is it really that complicated to compare two strings in php? –  kasperd 18 hours ago

2 Answers 2

I understand it is best not to use functions such as SHA1 or MD5.

That is true. But sha256 is not that much better.

Problem: simple sha256

There are basically two problems with your approach:

  • you don't use a user-based salt, only a site-based salt (also called a pepper).
  • you only use simple sha256, which is way too fast.

These are serious issues.

The main problem with missing user-based salts is that an attacker only needs to hash each password once and can then compare it to the hashes of all users, instead of needing to hash the password with each salt. This speeds up cracking considerably.

The problem with the second point should be obvious. Hardware has become quite powerful over the years, and simple hashing just doesn't hold up anymore (and hasn't for quite a while). Here for example is a benchmark of 3090.3 Million guesses per second on a pretty standard PC.

If your database is ever leaked - for example because of a Code Execution or SQL Injection vulnerability - it will be quite easy for an attacker to crack a good percentage of your users passwords.

Solution: Bcrypt

What you want to use is bcrpyt. With PHP, this can be done via password_hash (which is also the first result when googling "php hash password").

Example:

// hash
$hash = password_hash("password", PASSWORD_DEFAULT);

echo $hash;

// check
echo "<br>" . password_verify("password", $hash);

User-based salts are managed automatically for you, and it uses multiple rounds of hashing, slowing the process down, and thus increasing the resources an attacker needs to crack your hashes.

You can still use a pepper with bcrypt if you want to.

For more information about password hashing in general see here.

Misc

  • makeSafe is not a good function name. Make safe how? hash would be more accurate.
share|improve this answer
    
In regards to "pepper", it is better to encrypt the hash using AES and your pepper would be your secret key. –  Petah yesterday
    
@Petah, that allows an attacker who knows the salt to precompute hashes for password guesses. Using the pepper as input in the first step avoids that. –  otus 14 hours ago
    
@otus I have a hard time coming up with a realistic scenario where that'd matter. Encryption at the end has at least two advantages, you can easily change the key by re-encrypting the hashes and you could delegate encryption (but not the expensive hashing) to a HSM that only allows encryption but not decryption or key extraction. –  CodesInChaos 13 hours ago
    
@CodesInChaos, a scenario where it would apply would be quite convoluted (specific target individual, salt known before hand, short window of opportunity after compromise of the hash). It's of course possible to combine both if you want. –  otus 9 hours ago
    
@otus not sure what you mean here. I was talking about pepper, not salt. –  Petah 4 hours ago

There's two sides to this review:

Security & Style.


Style:

  • Directly return $hash instead of assigning it.
  • The string concatenation can be improved

into:

public static function makeSafe($password)
{
    $salt = "mySalt";
    return hash("sha256", "{$salt}.{$password}");
}

Security:

When using a fixed salt, if they managed to break the compilation of the code and retrieve the plaintext salt, it would render the encrypted password down to a matching of common hashes.

Use a different salt for each user (make a random integer, more than 10 digits even), and store the salt beside the password inside the database.

share|improve this answer
    
This isn't necessarily improving the concatenation. If anything, "{$salt}.{$password}" is slightly slower than $salt.".".$password because it has to parse the string, extract the curly braced entries, substitute and rebuild. –  Octopus yesterday
4  
How about "Don't use SHA, at all, for password hashing".... –  Petah yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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