4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 5
    \$\begingroup\$ 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. \$\endgroup\$ – D.W. Sep 16 '15 at 16:11
  • 9
    \$\begingroup\$ A fixed salt is no salt. \$\endgroup\$ – CodesInChaos Sep 16 '15 at 17:40
  • 1
    \$\begingroup\$ learn and use PHP's crypt() function \$\endgroup\$ – Octopus Sep 16 '15 at 21:12
  • 15
    \$\begingroup\$ 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) \$\endgroup\$ – Rocket Hazmat Sep 16 '15 at 21:17
  • 1
    \$\begingroup\$ @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? \$\endgroup\$ – kasperd Sep 17 '15 at 7:20
17
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ In regards to "pepper", it is better to encrypt the hash using AES and your pepper would be your secret key. \$\endgroup\$ – Petah Sep 16 '15 at 22:42
  • \$\begingroup\$ @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. \$\endgroup\$ – otus Sep 17 '15 at 11:12
  • \$\begingroup\$ @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. \$\endgroup\$ – CodesInChaos Sep 17 '15 at 12:01
  • \$\begingroup\$ @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. \$\endgroup\$ – otus Sep 17 '15 at 15:33
  • 1
    \$\begingroup\$ @Petah, If you assume access to the file system and database is game over you might as well use plaintext or simply encrypted passwords. That is precisely the situation that strong password storage is meant to protect against. (Encryption key rotation does nothing to prevent cracking the already stolen hashes.) \$\endgroup\$ – otus Sep 18 '15 at 6:43
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Octopus Sep 16 '15 at 21:14
  • 5
    \$\begingroup\$ How about "Don't use SHA, at all, for password hashing".... \$\endgroup\$ – Petah Sep 16 '15 at 22:38

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.