Optimizing duplicate password checking function

Question

we're a payment gateway, similar to PayPal, each user requires an account and recently we've been having resourcing issues from the create account process.

We have narrowed down that it's our duplicate password security function calling the database that's grinding the system to a halt. We have provided the code for the function below. There are >500,000 rows in the user_accounts table. Is there anything we can do to optimise this function?

P.S. We understand that SHA1 is a deprecated way of storing passwords, we are considering upgrades.

function duplicate_passwd_check($users_password) {
  $pass_array = $sql->fetch("SELECT * FROM user_accounts");

  foreach ($pass_array as $db_key => $db_value) {
    // perform hashing
    $salt = md5($users_password . SITE_PEPPER);
    $hashed_users_password = sha1($users_password . $salt);
    // perform check
    if ($db_value['hashed_password'] == $hashed_users_password) {
      $error->add("Password already in use, please use a different one.");
      locate("register"); // redirect to register page
    }
  }
  return 0;
}

Answer 1

First, detecting duplicate passwords is almost certainly a mistake. I advise against it.

To elaborate: Duplicate password detection creates an additional side-channel. Here's how to exploit it.

1. Attempt to register with a username and password.
2. If the username is already taken, build a list of valid usernames. (Many sites make this public, though, so you may not need it.)
3. If the password is already taken, build a list of valid passwords.
4. You can now map usernames to passwords much faster than a traditional brute force.

Consequently, the only winning move is not to play.

Second, use password_hash() and password_verify() and make sure you follow a non-opportunistic strategy for migrating your legacy hashes.

If you still want to detect duplicate passwords, don't use a fast hash function. If an attacker gets your database, they'll attack that instead of the proper password hash.

Instead, consider something like this:

class FooBarAuth
{
    const DUPE_SALT = '$2a$13$s1OlQvuC1wmgFMEYsGNzA.';

    public function doSignup(string $username, string $password)
    {
        $passwordHash = password_hash($password, PASSWORD_DEFAULT, ['cost' => 12]);
        $dupeHash = crypt($password, static::DUPE_SALT);

        // (Using paragonie/easydb in this example)
        if ($this->db->exists('SELECT count(*) FROM users WHERE dupe_hash = ?', $dupeHash)) {
            throw new Exception('Duplicate password detected');
        }
        // Proceed with the rest of your onboarding logic here
    }
}

Some key points:

1. Your passwords are hashed using bcrypt, as per the password_hash() default. Each password hash has a unique random salt and an appropriate cost factor (e.g. 12).
2. Your "duplicate password check" uses bcrypt with a static salt and a higher cost factor (e.g. 12+1 = 13).

Since the duplicate-detecting checksum is provided by bcrypt, attackers don't gain an advantage here like they would if you used e.g. MD5. However, since the salt isn't unique-per-user (otherwise it wouldn't function as intended for detecting duplicates), we increase the cost factor.

Answer 2

Problem :

Why you are checking duplicate record against all records ? Try to use where query.

Optimized code :

function duplicate_passwd_check($users_password) {
  $salt = md5($users_password . SITE_PEPPER);
  $hashed_users_password = sha1($users_password . $salt);
  $pass_array = $sql->fetch("SELECT COUNT(*) as TotalNumberUsed FROM user_accounts WHERE hashed_password = '".$hashed_users_password."'");
  if((int)$pass_array["TotalNumberUsed"] > 0){
    $error->add("Password already in use, please use a different one.");
    locate("register"); // redirect to register page
    return 1;
  }
  return 0;
}

Answer 3

It is hard to imagine that all users must have different password instead of not using their old password. But if you wish to do that anyway, this should be the best way to archieve:

function duplicate_passwd_check($users_password) {
    $salt = md5($users_password . SITE_PEPPER);
    $hashed_users_password = sha1($users_password . $salt);
    $duplicated_row = $sql->result("SELECT COUNT(user_id) FROM user_accounts WHERE hashed_password = '" . mysql_real_escape_string($hashed_users_password . "'");

    // perform check
    if ($duplicated_row >= 1) {
          $error->add("Password already in use, please use a different one.");
          locate("register"); // redirect to register page
    }
}

Notes

1. I don't know which SQL class you are using, I assume you are using a MySQL class. To prevent SQL injection I put mysql_real_escape_string on it.
2. I don't know if you have $sql->result but this function should output the result instead of sorting it as an array.
3. user_id should be your primary key to store the user information. You should change it if my naming-guess is wrong.
4. This check will also count the current client that is going to change the password. Please do not use it to check if the old password is corrent or not.

How this code works

You do not need to compare it one by one. It waste both MySQL and PHP process time. You should leave it to MySQL. My code will get how many rows. Normally the if should either return 0 or 1. To make it more strict I used equal or greater.