4
\$\begingroup\$

This method is part of my login system. Could I improve it further?

protected function _hashPassword($password = NULL, $rounds = 1000, $i = 1)
{
    if (! isset($password)) throw new Exception('No password set!');

    $salt   = 'K^^%/m>(|{z= $1^>%>W[=4U5*p|,E';
    $pepper = '08[)^,&%^^771^=>&,E[XP::4})h*I';

    $dinner = $salt.$password.$pepper;

    if ($i >= $rounds) return sha1($dinner);
    return sha1($this->_hashPassword($dinner, $rounds, $i+1).$password);
}
\$\endgroup\$
5
\$\begingroup\$

I would use a for loop instead of recursive calls:

protected function _hashPassword($password = NULL, $rounds = 1000) {
    if (!isset($password)) throw new Exception('No password set!');

    $salt   = 'K^^%/m>(|{z= $1^>%>W[=4U5*p|,E';
    $pepper = '08[)^,&%^^771^=>&,E[XP::4})h*I';

    $dinner = $salt . $password . $pepper;

    for ($i = 0; $i < $rounds; $i++) {
        $dinner = sha1($dinner . $password);
    }
    return $dinner;
}

From http://php.net/manual/en/functions.user-defined.php:

It is possible to call recursive functions in PHP. However avoid recursive function/method calls with over 100-200 recursion levels as it can smash the stack and cause a termination of the current script.

Furthermore, I would use the username as 'pepper'. It would make your hash more unpredictable.

\$\endgroup\$
  • \$\begingroup\$ Great, now we know that there is a good reason why you think that a loop is of better use :) \$\endgroup\$ – rzetterberg Nov 30 '11 at 21:47
  • \$\begingroup\$ Was actually thinking to remake it to that yesterday, but thanks for confirming it! Also, now I can use a slightly higher number for rounds. \$\endgroup\$ – Seralize Nov 30 '11 at 22:26
5
\$\begingroup\$

A few comments on the approach you are taking here:

  1. If possible I would use bcrypt to hash your password, rather than writing your own multi-round hashing algorithm. (Here's a php library for password hashing).

  2. Salting serves three benefits. Firstly and least importantly it means that if someone gets your database but not your application code it is a little harder for them to crack your passwords. Secondly by choosing a salt with special characters in it, you make your hashes less susceptible to rainbow table attacks. Thirdly by using a different hash for each user, an attacker has to attempt to crack each password separately, rather than being able to check your entire database for the most common passwords in one go. Your salting approach here provides the first and second of these but not the third. As mentioned, username is a good candidate for a unique identifier to include as part of the salt.

On the code itself:

  1. Why are you providing a default parameter for the $password field, but then checking to see if it is set? If it must be set remove the default. You probably also want to check using empty not isset.
  2. As a matter of style I like to pull constants like $salt and $pepper out to the top of a class, rather than having them hidden inline in individual methods. I find getting in the habit of always doing this reduces the chance of accidentally duplicating them in multiple methods.
\$\endgroup\$
  • \$\begingroup\$ Code comment #1: I've been annoyed by this lately myself. If I leave out a required parameter I will get an error, so why make an exception? As long I always validate the input first, then it shouldn't be necessary to check if value is set? Hash comment #3: I'm not intending to use salt-per-user in this project, it's pretty much a sandbox at the moment, otherwise I would. \$\endgroup\$ – Seralize Dec 5 '11 at 19:19

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.