Custom password hashing

Question

I have been creating a class for users on my account. OOP and classes are not my strong point.

Below is an extract from my class which I use to create a hash (that will be stored in a database) and the second function is used to check a value against a hash.

I know that I should just use bcrypt for my hashing but I was experimenting with adding a cost factor to other hashing algorithms.

// cryptography settings
private $cost = 10; // the amount of times to run the hashing algo
private $algo = "bcrypt"; // the hash to use on the passwords
private $salt = "mySalt"; // put whatever function you want here to create a random salt
private $pepper = "myPepper"; // a pepper is a secret key not stored anywhere (except here)

/**
 * Use the specified hash in order hash the password
 * @param  string $password The password in need of being hashed
 * @return string           The hashed version of the password
 */
public function hashPassword($password)
{
    $this->algo = strtolower($this->algo); // lowerise the algo
    $hash = ""; // the hashed password
    $password .= $this->salt . $this->pepper; // add salt and pepper to the password

    // BCrypt
    if ($this->algo == "bcrypt")
        $hash = password_hash($password, PASSWORD_BCRYPT, ["cost" => $this->cost]);

    // hash algorithms
    else if (in_array($this->algo, hash_algos()))
    {
        $hash = hash($this->algo, $password, false); // hash the password
        // loop to hash the password $cost times
        for ($i = 0; $i < $this->cost; $i++)
            $hash = hash($this->algo, $hash, false);
    }

    // the hashed password
    return $hash;
}

/**
 * Check if the hashed password matches hash
 * @param  string $pass The password that is in need of hashing
 * @param  string $hash The already hashed password
 * @return bool         $pass == $hash
 */
public function checkHash($pass, $hash)
{
    // cannot have anything other than integer to loop
    if (!is_int($this->cost))
        throw new Exception("Cost must be an integer.", 1);

    // use lowercase for if statement
    $this->algo = strtolower($this->algo); // lowerise the algo
    $success = false; // if hash matches password hash
    $pass .= $this->salt . $this->pepper; // add salt and pepper to the password

    // BCrypt
    if ($this->algo == "bcrypt")
        $success = password_verify($pass, $hash);

    // hash algorithms
    else if (in_array($this->algo, hash_algos()))
    {
        $temp = hash($this->algo, $pass, false); // hash the password
        // do same amount of looping
        for ($i = 0; $i < $this->cost; $i++)
            $temp = hash($this->algo, $temp, false);

        // check if the new hash equals the original hash
        $success = $temp == $hash;
    }

    return $success;
}

The two functions can be used as such. Note that $user is an instance of my class.

$hash = $user->hashPassword("test"); // might produce: $2y$10$lVSgcHrtuWmRIbRryAlXE.gJQZZhFKjI49RI5bnm.4k9DyV0uqHUi

The above will be used so that I can insert that into the database.
Then I would check when logging in using the below.

// get the hash from the database
$query = $this->conn->prepare("SELECT password FROM users WHERE username = ?");
$query->bind_param("s", $_POST["username"]);
$query->execute();
$query->store_result();
$query->bind_result($hash);
$query->fetch();

// check the hash against the posted password
if ($user->checkHash($_POST["password"], $hash))
    echo "You are logged in";
else
    echo "Login failed";

Firstly, is this a good idea or should I just use password_hash? The reason I decided to create these methods was in case my passwords got compromised and then I could easily change the hashing algorithm and ask users to change their passwords.

Secondly, am I checking the password correctly? That is a lot of lines to extract the hash. It seems wrong but I don't know any other way.

Answer 1

Security

General Approach

Your reason for this approach are not reasonable. The only reason to provide a different hash function is to deal with systems which do not support password_hash. And really, those systems should be updated.

You definitely do not want to switch to a weaker hash function after a breach. You can ask users to change their passwords via other mechanisms.

tl;dr: Yes, just use bcrypt.

Salt vs Pepper

Based on your comments, you seem to understand the difference between a salt and a pepper. But your code doesn't seem to differentiate, both your salt and your pepper are a pepper (this is also not as easy to fix as "replace this string with a random function", as you not only need to generate the salt, but also store and retrieve it).

A salt should be unique for each hash, so that it actually slows down attacks.

Bcrypt usage

The bcrypt part of your approach is fine; you are using password_hash and password_verify correctly.

Note that bcrypt actually handles salts for you though, so adding your own salt is unnecessary. As bcrypt can only handle passwords up to 72 characters, your approach may actually be harmful, as a long salt + password + pepper may cut of (parts of) the pepper. I would at least comment on this, but ideally, just remove the salt for bcrypt.

Custom Implementation: Empty Hash

If you set your $algo to something invalid, you will - silently - store an empty string instead.

Currently, you actually return false on the check, so it's not catastrophic, but it can lead to usability issues. I would throw an exception here.