Change Pbkdf2Hash output hash to be consistent with native password…

… hash output
NexusPHP · Dec 19, 2024 · 7bfec53 · 7bfec53
1 parent c56731d
commit 7bfec53
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 57 deletions.
diff --git a/src/Nexus/Password/Hash/Pbkdf2Hash.php b/src/Nexus/Password/Hash/Pbkdf2Hash.php
@@ -43,12 +43,6 @@
      */
     private int $length;
 
-    /**
-     * Used on `::verify()` to return early if provided hash's length
-     * does not match this hasher's instance's supposed hash length.
-     */
-    private int $hashLength;
-
     /**
      * @param array{
      *  iterations?: int,
@@ -79,8 +73,6 @@ public function __construct(
             $this->defaultIterations(),
             self::DEFAULT_LENGTH,
         );
-
-        $this->hashLength = \strlen($this->hash('password', salt: random_bytes(16)));
     }
 
     /**
@@ -101,7 +93,19 @@ public function hash(#[\SensitiveParameter] string $password, array $options = [
             $this->length,
         );
 
-        return hash_pbkdf2($this->algorithm->value, $password, $salt, $iterations, $length);
+        return \sprintf(
+            '$%s$i=%d,l=%d$%s$%s',
+            $this->algorithm->value,
+            $iterations,
+            $length,
+            base64_encode($salt),
+            base64_encode($this->pbkdf2(
+                $password,
+                $salt,
+                $iterations,
+                $length,
+            )),
+        );
     }
 
     public function needsRehash(string $hash, array $options = []): bool
@@ -115,22 +119,72 @@ public function verify(string $password, string $hash, string $salt = ''): bool
             return false;
         }
 
-        if (\strlen($hash) !== $this->hashLength) {
+        if (preg_match('/^\$([^\$]+)\$([^\$]+)\$([^\$]+)\$([^\$]+)$/', $hash, $parts) !== 1) {
+            return false;
+        }
+
+        array_shift($parts);
+
+        if (Algorithm::tryFrom($parts[0]) === null) {
+            return false;
+        }
+
+        if (preg_match('/i=(\-?\d+),l=(\-?\d+)/', $parts[1], $matches) !== 1) {
+            return false;
+        }
+
+        try {
+            ['iterations' => $iterations,'length' => $length] = $this->validatedOptions(
+                [],
+                (int) $matches[1],
+                (int) $matches[2],
+            );
+        } catch (HashException) {
             return false;
         }
 
-        if (str_contains($hash, '$')) {
+        if (base64_decode($parts[2], true) === false) {
             return false;
         }
 
-        return hash_equals($hash, $this->hash($password, salt: $salt));
+        $rawHash = base64_decode($parts[3], true);
+
+        if (false === $rawHash) {
+            return false;
+        }
+
+        return hash_equals(
+            $rawHash,
+            $this->pbkdf2(
+                $password,
+                $salt,
+                $iterations,
+                $length,
+            ),
+        );
     }
 
     public function valid(): bool
     {
         return \function_exists('hash_pbkdf2');
     }
 
+    /**
+     * @param int<1, max> $iterations
+     * @param int<0, max> $length
+     */
+    private function pbkdf2(string $password, string $salt, int $iterations, int $length): string
+    {
+        return hash_pbkdf2(
+            $this->algorithm->value,
+            $password,
+            $salt,
+            $iterations,
+            $length,
+            true,
+        );
+    }
+
     /**
      * @see https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2
      */

diff --git a/tests/Password/Hash/Pbkdf2HashTest.php b/tests/Password/Hash/Pbkdf2HashTest.php
@@ -17,7 +17,6 @@
 use Nexus\Password\Hash\AbstractHash;
 use Nexus\Password\Hash\Pbkdf2Hash;
 use Nexus\Password\HashException;
-use Nexus\Password\Password;
 use Nexus\PHPUnit\Tachycardia\Attribute\TimeLimit;
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\Attributes\DataProvider;
@@ -77,32 +76,6 @@ public static function provideDefaultIterationsCases(): iterable
         yield 'sha512' => [Algorithm::Pbkdf2HmacSha512, 210_000];
     }
 
-    #[DataProvider('provideLengthProvidedCorrespondsToHashLengthCases')]
-    public function testLengthProvidedCorrespondsToHashLength(int $result, int $length, string $algo): void
-    {
-        $algorithm = Algorithm::from($algo);
-        $hasher = new Pbkdf2Hash($algorithm, compact('length'));
-        $hash = $hasher->hash('password');
-
-        self::assertSame($result, \strlen($hash));
-    }
-
-    /**
-     * @return iterable<string, array{int, int, string}>
-     */
-    public static function provideLengthProvidedCorrespondsToHashLengthCases(): iterable
-    {
-        yield 'sha1 0' => [40, 0, 'sha1'];
-
-        yield 'sha256 0' => [64, 0, 'sha256'];
-
-        yield 'sha512 0' => [128, 0, 'sha512'];
-
-        foreach (['sha1', 'sha256', 'sha512'] as $algo) {
-            yield $algo.' 30' => [30, 30, $algo];
-        }
-    }
-
     #[TimeLimit(3.0)]
     public function testBasicPasswordHashing(): void
     {
@@ -114,7 +87,6 @@ public function testBasicPasswordHashing(): void
 
         $hash = $hasher->hash($password, salt: $salt);
         self::assertFalse($hasher->needsRehash($hash));
-        self::assertTrue($hasher->verify($password, $hash, $salt));
     }
 
     public function testInvalidPasswordForHash(): void
@@ -125,26 +97,57 @@ public function testInvalidPasswordForHash(): void
         (new Pbkdf2Hash(Algorithm::Pbkdf2HmacSha256))->hash('aa');
     }
 
+    #[DataProvider('providePasswordVerifyCases')]
     #[TimeLimit(3.0)]
-    public function testPasswordVerify(): void
+    public function testPasswordVerify(bool $result, ?string $password, string $hash): void
     {
-        $pass1 = "abcd\0e";
-        $pass2 = 'password';
-        $pass3 = 'pass';
-        $salt = random_bytes(16);
+        $password ??= 'password';
         $hasher = new Pbkdf2Hash(Algorithm::Pbkdf2HmacSha256);
 
-        $hash = $hasher->hash($pass2, salt: $salt);
-        self::assertFalse($hasher->verify($pass1, $hash, $salt));
-        self::assertTrue($hasher->verify($pass2, $hash, $salt));
-        self::assertFalse($hasher->verify($pass3, $hash, $salt));
-        self::assertFalse($hasher->verify(
-            $pass2,
-            substr(Password::fromAlgorithm(Algorithm::Argon2i)->hash($pass2), 0, 40),
-        ));
-        self::assertFalse($hasher->verify(
-            $pass2,
-            (new Pbkdf2Hash(Algorithm::Pbkdf2HmacSha256, ['length' => 0]))->hash($pass2, salt: $salt),
-        ));
+        self::assertSame($result, $hasher->verify($password, $hash, 'salt'));
+    }
+
+    /**
+     * @return iterable<string, array{bool, null|string, string}>
+     */
+    public static function providePasswordVerifyCases(): iterable
+    {
+        $hash = (new Pbkdf2Hash(Algorithm::Pbkdf2HmacSha256))->hash('password', [], 'salt');
+
+        yield 'empty' => [false, '', $hash];
+
+        yield 'nul' => [false, "pass\0word", $hash];
+
+        yield 'short' => [false, 'pass', $hash];
+
+        yield 'not hash' => [false, null, 'hash'];
+
+        yield 'truncated' => [false, null, substr($hash, 7)];
+
+        yield 'not hash hmac' => [false, null, str_replace('sha256', 'ghost', $hash)];
+
+        yield 'alternated options' => [false, null, preg_replace(
+            '/(i=\d+),(l=\d+)/',
+            '$2,$1',
+            $hash,
+        ) ?? $hash];
+
+        yield 'invalid iterations' => [false, null, str_replace('i=600000', 'i=900', $hash)];
+
+        yield 'invalid length' => [false, null, str_replace('l=40', 'l=-1', $hash)];
+
+        yield 'invalid base 64 salt' => [false, null, preg_replace(
+            '/^\$([^\$]+)\$([^\$]+)\$([^\$]+)\$([^\$]+)$/',
+            '\$$1\$$2\$$3=\$$4',
+            $hash,
+        ) ?? $hash];
+
+        yield 'invalid base 64 hash' => [false, null, preg_replace(
+            '/^\$([^\$]+)\$([^\$]+)\$([^\$]+)\$([^\$]+)$/',
+            '\$$1\$$2\$$3\$$4=',
+            $hash,
+        ) ?? $hash];
+
+        yield 'valid hash' => [true, null, $hash];
     }
 }