Recursive hashing function

Question

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);
}

Answer 1

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.

Answer 2

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.