diff --git a/packages/docs-markdown/docs/packages/hashing.md b/packages/docs-markdown/docs/packages/hashing.md index 13c3b59a1..b4f032bb5 100644 --- a/packages/docs-markdown/docs/packages/hashing.md +++ b/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: diff --git a/packages/hashing/src/Hash/BcryptHasher.php b/packages/hashing/src/Hash/BcryptHasher.php index 9cff1dad5..905d3c260 100644 --- a/packages/hashing/src/Hash/BcryptHasher.php +++ b/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 */ diff --git a/packages/hashing/tests/Unit/Hash/BcryptHasherTest.php b/packages/hashing/tests/Unit/Hash/BcryptHasherTest.php index c3b1f65ee..aac70287e 100644 --- a/packages/hashing/tests/Unit/Hash/BcryptHasherTest.php +++ b/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); +}); diff --git a/packages/hashing/tests/Unit/HashManagerTest.php b/packages/hashing/tests/Unit/HashManagerTest.php index 3c73a68ed..318f7656b 100644 --- a/packages/hashing/tests/Unit/HashManagerTest.php +++ b/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"]); +});