1

Is there an obvious flaw with this hashing function (written in PHP)?

function hash($password) {
  sleep(1); // Make hashing slow
  $hash = "";
  $iterations = strlen($password);
  for ( $i = 0; $i < $iterations; $i++ ) {
      $hash = hash('sha512', USER_PASSWORD_SALT . $password);
  }
  return $hash;
}
3
  • 5
    Slowing it down with the sleep(1) in your implementation doesn't directly affect attackers. May as well remove it. Commented Apr 2, 2015 at 16:39
  • 5
    Consider using the function password_hash instead, which does everything you're trying to do securely: php.net/manual/en/function.password-hash.php
    – Simon
    Commented Apr 2, 2015 at 16:47
  • 1
    @Simon is correct. Don't recreate the wheel. PHP does this already. Commented Apr 2, 2015 at 19:54

2 Answers 2

16

Yes, there are obvious flaws. Here are some:

  • You can’t make hashing slow by introducing simple delays. An attacker doesn’t have to evaluate the hash the same way you did; they just need to get the same answer in the end. That means whatever is making the hash slow needs to be a necessary component of the hash — it must be impossible to compute the hash without doing that. In fact, the sleep(1) is the kind of thing you have to avoid, in that it makes legitimate hashing slower than an attacker’s hashing (the whole point of hash design is to minimize the advantage of an attacker).
  • The number of iterations is the length of the password. This isn’t enough. You want at least tens of thousands of iterations.
  • The salt appears to be global. That’s not a salt; a salt has to be different for each user.
  • This isn’t a standard algorithm. That means that it’s not a good idea to trust it for actual use. There are three algorithms with a fair amount of analysis (scrypt, bcrypt, PBKDF2). These have been looked at by actual cryptographers, and seem secure. Use one of them.
  • The correct way to do a password hash in PHP is with the password_hash function, which handles everything for you (implementing an algorithm yourself is a poor idea for security just as much as creating your own algorithm is).
1
1

no. it looks like you're using a global constant for the salt. you want it to change per user record.

see https://stackoverflow.com/questions/2999197/do-i-need-a-random-salt-once-per-password-or-only-once-per-database

also, @coverosgene is right - there's no point in slowing your own hash method down.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .