fix: run a dummy bcrypt check before rejecting unhashable values in BcryptHasher::verify

packages/docs-markdown/docs/packages/hashing.md

@@ -69,7 +69,7 @@ class UserService
 
 ### Bcrypt Length Limit
 
-Bcrypt only uses the first 72 bytes of a value and cannot hash a NUL (`\0`) byte. Rather than silently truncate, `BcryptHasher::hash()` throws `InvalidValueException` for a value longer than 72 bytes (`BcryptHasher::MAX_VALUE_BYTES`) or containing a NUL byte. `verify()` returns `false` for such a value and never throws, since no stored bcrypt hash can match it.
+Bcrypt only uses the first 72 bytes of a value and cannot hash a NUL (`\0`) byte. Rather than silently truncate, `BcryptHasher::hash()` throws `InvalidValueException` for a value longer than 72 bytes (`BcryptHasher::MAX_VALUE_BYTES`) or containing a NUL byte. `verify()` returns `false` for such a value and never throws, since no stored bcrypt hash can match it. Before returning `false` it still verifies the value against a dummy hash of the configured cost, so rejecting an over-long or NUL-containing value takes as long as checking a wrong password and `HashManager::verify()` leaks no timing difference between the two.
 
 The limit is in bytes, not characters: a password of multibyte UTF-8 characters reaches it sooner. Validate password length with `strlen()` on registration and password change so users get a form error instead of an exception:
 

packages/hashing/src/Hash/BcryptHasher.php

@@ -17,8 +17,16 @@
      */
     public const int MAX_VALUE_BYTES = 72;
 
+    /**
+     * Salt and digest of a bcrypt hash no real value is expected to match; the cost prefix is
+     * added per instance so a dummy verification costs exactly as much as a real one.
+     */
+    private const string DUMMY_HASH_BODY = 'ZxSPF.27pFWWE6Ew0jDzE.HiJNsq8davb61hvjFFesMqHyLDmjNbe';
+
     private int $cost;
 
+    private string $dummyHash;
+
     /**
      * @throws InvalidHasherConfigException
      */
@@ -27,6 +35,7 @@ public function __construct(
     ) {
         $this->cost = $cost ?? self::DEFAULT_COST;
         $this->validateCost();
+        $this->dummyHash = sprintf('$2y$%02d$%s', $this->cost, self::DUMMY_HASH_BODY);
     }
 
     /**
@@ -48,13 +57,16 @@ public function hash(
 
     /**
      * Returns false for a value bcrypt cannot hash in full (over 72 bytes or containing a NUL byte),
-     * since no hash() result can belong to it.
+     * since no hash() result can belong to it. A dummy hash of the configured cost is still checked
+     * so rejecting such a value costs the same time as checking a real one.
      */
     public function verify(
         string $value,
         string $hash,
     ): bool {
         if (strlen($value) > self::MAX_VALUE_BYTES || str_contains($value, "\0")) {
+            $this->verifyDummy($value);
+
             return false;
         }
 
@@ -72,6 +84,16 @@ public function algorithm(): string
         return 'bcrypt';
     }
 
+    /**
+     * Runs a full-cost bcrypt verification and discards the result, so a rejected value takes as
+     * long as a real check.
+     */
+    protected function verifyDummy(
+        string $value,
+    ): void {
+        password_verify($value, $this->dummyHash);
+    }
+
     /**
      * @throws InvalidHasherConfigException
      */

packages/hashing/tests/Unit/Hash/BcryptHasherTest.php

@@ -158,3 +158,75 @@
 
     expect($hasher->verify("secret\0", $hash))->toBeFalse();
 });
+
+/**
+ * A BcryptHasher that records every dummy verification it runs before running it for real.
+ */
+function createDummyRecordingBcryptHasher(
+    ArrayObject $dummyChecks,
+): BcryptHasher {
+    return new readonly class ($dummyChecks) extends BcryptHasher
+    {
+        public function __construct(
+            private ArrayObject $dummyChecks,
+        ) {
+            parent::__construct(cost: 4);
+        }
+
+        protected function verifyDummy(
+            string $value,
+        ): void {
+            $this->dummyChecks->append($value);
+
+            parent::verifyDummy($value);
+        }
+    };
+}
+
+it('runs a dummy verification before rejecting a value longer than 72 bytes', function () {
+    $dummyChecks = new ArrayObject();
+    $hasher = createDummyRecordingBcryptHasher($dummyChecks);
+    $value = str_repeat('a', 73);
+
+    expect($hasher->verify($value, $hasher->hash('secret')))->toBeFalse()
+        ->and($dummyChecks->getArrayCopy())->toBe([$value]);
+});
+
+it('runs a dummy verification before rejecting a value containing a NUL byte', function () {
+    $dummyChecks = new ArrayObject();
+    $hasher = createDummyRecordingBcryptHasher($dummyChecks);
+
+    expect($hasher->verify("secret\0", $hasher->hash('secret')))->toBeFalse()
+        ->and($dummyChecks->getArrayCopy())->toBe(["secret\0"]);
+});
+
+it('does not run a dummy verification for a value bcrypt can check', function () {
+    $dummyChecks = new ArrayObject();
+    $hasher = createDummyRecordingBcryptHasher($dummyChecks);
+    $hash = $hasher->hash('secret');
+
+    expect($hasher->verify('secret', $hash))->toBeTrue()
+        ->and($hasher->verify('wrong', $hash))->toBeFalse()
+        ->and($dummyChecks->count())->toBe(0);
+});
+
+it('verifies dummy values against a well-formed bcrypt hash of the configured cost', function () {
+    $hasher = new BcryptHasher(cost: 5);
+    $dummyHash = new ReflectionProperty(BcryptHasher::class, 'dummyHash')->getValue($hasher);
+
+    $info = password_get_info($dummyHash);
+
+    // A malformed hash would make password_verify() fail instantly instead of doing bcrypt work
+    expect($info['algo'])->toBe(PASSWORD_BCRYPT)
+        ->and($info['options']['cost'])->toBe(5)
+        ->and(strlen($dummyHash))->toBe(60)
+        ->and(strlen(crypt('attacker-guess', $dummyHash)))->toBe(60)
+        ->and($hasher->needsRehash($dummyHash))->toBeFalse();
+});
+
+it('uses the default cost for the dummy hash when none is configured', function () {
+    $hasher = new BcryptHasher();
+    $dummyHash = new ReflectionProperty(BcryptHasher::class, 'dummyHash')->getValue($hasher);
+
+    expect(password_get_info($dummyHash)['options']['cost'])->toBe(BcryptHasher::DEFAULT_COST);
+});

packages/hashing/tests/Unit/HashManagerTest.php

@@ -215,3 +215,45 @@ public function withScope(
     expect($manager->hasher('bcrypt')->verify('password', $bcryptHash))->toBeTrue()
         ->and($manager->hasher('argon2id')->verify('password', $argon2Hash))->toBeTrue();
 });
+
+it('runs the bcrypt dummy verification when verifying a value bcrypt refuses', function () {
+    $dummyChecks = new ArrayObject();
+    $hashConfig = $this->createStub(HashConfig::class);
+    $hashConfig->method('defaultHasher')->willReturn('bcrypt');
+    $factory = new readonly class ($hashConfig, $dummyChecks) extends HasherFactory
+    {
+        public function __construct(
+            HashConfig $config,
+            private ArrayObject $dummyChecks,
+        ) {
+            parent::__construct($config);
+        }
+
+        public function make(
+            string $name,
+        ): HasherInterface {
+            return new readonly class ($this->dummyChecks) extends BcryptHasher
+            {
+                public function __construct(
+                    private ArrayObject $dummyChecks,
+                ) {
+                    parent::__construct(cost: 4);
+                }
+
+                protected function verifyDummy(
+                    string $value,
+                ): void {
+                    $this->dummyChecks->append($value);
+
+                    parent::verifyDummy($value);
+                }
+            };
+        }
+    };
+    $manager = new HashManager($hashConfig, $factory);
+    $hash = $manager->hash('secret');
+
+    expect($manager->verify(str_repeat('a', 73), $hash))->toBeFalse()
+        ->and($manager->verify("secret\0", $hash))->toBeFalse()
+        ->and($dummyChecks->getArrayCopy())->toBe([str_repeat('a', 73), "secret\0"]);
+});