ASP.NET Password Hashing reimplemented in PHP

Question

Live link to Code: https://github.com/sqlcollaborative/AlwaysEncryptedSample/blob/features/phpSampleApp/AlwaysEncryptedSample.PHP/src/Security/AspNetIdentity.php

Unit tests to prove it works: https://github.com/sqlcollaborative/AlwaysEncryptedSample/blob/features/phpSampleApp/AlwaysEncryptedSample.PHP/tests/Security/AspNetIdentityTest.php

I am making a php CLI apps to add users to an AlwaysEncrypted application. Here is how I am encrypting the password. Its almost a direct port of the ASP.NET C# code and has 100% code coverage so I'm pretty sure it works. Just wondering if there is a better way to do things.

<?php
declare(strict_types = 1);

namespace SqlCollaborative\AlwaysEncryptedSample\Security;

use RuntimeException;

/**
 * Class AspNetIdentity
 * @package SqlCollaborative\AlwaysEncryptedSample\Security
 * A direct port of the algorithm to generate and verify ASP.NET password hashes found here:
 * @link https://aspnetidentity.codeplex.com/SourceControl/latest#src/Microsoft.AspNet.Identity.Core/Crypto.cs
 * For mor information:
 * @link http://stackoverflow.com/questions/20621950/asp-net-identity-default-password-hasher-how-does-it-work-and-is-it-secure
 */
class AspNetIdentity
{
    const PBKDF2_ITER_COUNT = 1000; // default for Rfc2898DeriveBytes
    const PBKDF2_SUBKEY_LENGTH = 256/8; // 256 bits
    const SALT_SIZE = 128/8; // 128 bits

    private function generateRandomSalt() : string
    {
        return random_bytes(self::SALT_SIZE);
    }

    public function getPasswordSalt(string $hashedPassword) : string
    {
        $hashedPasswordByteString = base64_decode($hashedPassword);
        $expectedHashLength = 1 + self::SALT_SIZE + self::PBKDF2_SUBKEY_LENGTH;
        $actualHashLength = strlen($hashedPasswordByteString);

        $header = substr($hashedPasswordByteString, 0, 1);
        if ($header != "\0"){
            throw new RuntimeException("Incorrect header [ $header ]");
        }

        if ($actualHashLength != $expectedHashLength)
        {
            throw new RuntimeException (
                "Salted hash is wrong length [ expected = {$expectedHashLength}, actual = {$actualHashLength} ]"
            );
        }

        return substr($hashedPasswordByteString, 1, self::SALT_SIZE);
    }

    /**
     * Creates a PBKDF2 (AKA Rfc2898) hash from a plaintext password.
     * @param string $password A plaintext password
     * @param string|null $salt Optionally specify the hash. This should only be used to verify existin passwords.
     * @return string The base64 encoded password hash
     */
    public function hashPassword(string $password, string $salt = null) : string
    {
        $salt = $salt ?? $this->generateRandomSalt();
        $subkey = hash_pbkdf2(
            'sha1',  // The SHA1 exploit google discovered is irrevelevant to HMAC_SHA1
            $password,
            $salt,
            self::PBKDF2_ITER_COUNT,
            self::PBKDF2_SUBKEY_LENGTH,
            true
        );

        return base64_encode("\0" . $salt . $subkey);
    }

    /**
     * verifies a plaintext password matches its hash.
     * @param string $hashedPassword must be of the format of HashWithPassword (salt + Hash(salt+input)
     * @param string $password the plain text version of the password
     * @return bool true if the password matches false if it does not.
     */
    public function verifyHashedPassword(string $hashedPassword, string $password) : bool
    {
        try {
            $salt = $this->getPasswordSalt($hashedPassword);
        }
        catch (RuntimeException $ex) {
            trigger_error($ex->getMessage());
            return false;
        }
        $actualHashedPassword = $this->hashPassword($password, $salt);
        return ($actualHashedPassword === $hashedPassword);
    }
}

Answer 1

I don't have any great concern over the implementation here when thinking of it in the context of a code port.

I do have some concern that you would be continuing with a potentially bad design.

For example:

• An iteration count of 1000 for PBKDF2 seems low. Most standard implementation now use 10K+, though ultimately you would need to tune this based on the hardware on which you are running the algorithm and the desired hash computation time you are looking to achieve. You code right now gives you no way to tune this.
• Which, leads my to the concern that you are not storing any information about the key length or iteration count on the hash, meaning you will never be able to "adapt" your encryption operation to keep pace with hardware changes. You are going to be stuck with 1000 iteration computational work factor for passwords in this application until you make code changes to increase the computation time.

If possible for your implementation, I would consider adding algorithmic information (including work factor info) to your result hash such that you can adaptively change your application configuration at some point in the future without having to change code. Say for example you decided to increase the iterations from 1000 to 10,000. At the point of password verification, you could catch cases with outdated encryption modes and proactively regenerate the password hash to meet your new standard (of course only for cases where the password verification is successful).

---

Does your getPasswordSalt() method really need to be public? Is there context in which you are expecting callers to invoke this method outside of password verification?

---

I don't understand the need for exception being thrown by getPasswordSalt() if you are basically just swallowing it anyway in verifyHashedPassword(). I really like the idea of logging errors on expected behavior, but I would question making whether throwing/catching here is proper pattern.

I also question having an early return upon hash string validation failure. Though early exits such as this are typically a good idea, in this specific case you are potentially leaking information about the password hash (that it is invalid) to an attack who is trying to exploit the system internally.

Consider logging the unexpected input like you are now doing, but then just creating a new hash for the input password anyway even though you are going to send a false result. This way, you ensure that you always spend hash computation time during this verification step.

---

It would seem that the ASP equivalent does not allow caller to pass the salt. Do you really want to allow your caller to send in an arbitrary salt to hashPassword()? Should you move the main hash generation logic into a protected/private function that can be used from inside the class, with the public interface only allowing passing of password?

For example:

public function hashPassword(string $password) : string
{
    return $this->generateHash($password);
}

private function generateHash(string $password, string $salt = null) : string
{
    // your current hashPassword() logic here
}

---

Should your hash string validations around first byte and overall length live on verifyHashedPassword() method instead of getPasswordSalt()? I don't know why a potentially private method like getPasswordSalt() should have validation within it. Perhaps these could even be encapsulated into their own private validateHash() method.

This would also remove the odd exception throwing/swallowing pattern mentioned before. So you could have something like this in the verification method:

public function verifyHashedPassword(string $hashedPassword, string $password) : bool
{
    $result = $this->validateHash($hashedPassword);
    if($result === false) {
          // let's kill some time
          $this->generateHash($password);
          return false;
    }
    $salt = $this->getPasswordSalt($hashedPassword);
    $actualHashedPassword = $this->generateHash($password, $salt);
    return ($actualHashedPassword === $hashedPassword);
}

private function validateHash(string $hash) : boolean
{
    // perform validation
    // log errors if necessary
    // return boolean result
}

Even if you choose to keep getPasswordSalt() as public method, you could still reference common validation function like:

public function getPasswordSalt(string $hashedPassword) : string
{
    $result = $this->validateHash($hashedPassword);
    // early exit for validation failure
    // rest of method logic for valid hash
}

---

Should this be a concrete class at all? These seem like they should be static methods. There is nothing that you are doing here that would seem to necessitate a concrete instantiation.

---

If you are going to use doc blocks (a good thing) you should use them uniformly across your code, not just certain methods.

Answer 2

Just commenting on the security aspect:

Instead of returning

($actualHashedPassword === $hashedPassword)

you should use hash_equals and return

hash_equals($actualHashedPassword, $hashedPassword)

to mitigate timing attacks.

Reason: The public function verifyHashedPassword should only return one information - whether the password and the hash match or not. But right now, it leaks more information to an attacker who observes the time this function takes to complete. Whether or not that information can and will be used to the advantage of the attacker is dependent on many factors (e.g. preimage attacks) and unless you have taken all those possible factors and scenarios into account, you better use the safe and simple hash_equals.