From f01b2aefeca467503458d2abc9d9f7e111f90919 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 2 Sep 2026 23:30:09 +0200 Subject: [PATCH] fix: refuse a legacy account's password at the same cost as any other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A row with `isMigrate` holds a sha1, md5 or crypt digest rather than a bcrypt hash, so `checkMigrateUser()`'s three comparisons take microseconds and its fourth hands that digest to password_verify(), which rejects it on sight. A wrong password for such an account refused in 0.3ms where a migrated account and an unknown login both paid 220ms for a real verify — measured by the test, which was written first and failed on it. That is the enumeration oracle the catch block below already deals with, reopened for a subset: a fast refusal said 'this login exists and is one of the old ones', naming both a real account and the ones whose stored hashes are weakest, to a caller who never guessed a password. The migrate branch now answers for itself and spends a verify against ABSENT_USER_HASH on the way to the same refusal. Falling through would not have spent it: that check is the same one checkMigrateUser() just made against the same non-bcrypt value, so the answer was false either way and short-circuiting is behaviour-preserving. --- .../Auth/Providers/Database/DatabaseAuth.php | 29 +++++++-- .../Providers/Database/DatabaseAuthTest.php | 59 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/src/Domain/Auth/Providers/Database/DatabaseAuth.php b/src/Domain/Auth/Providers/Database/DatabaseAuth.php index 2ff0c265f..485511f34 100644 --- a/src/Domain/Auth/Providers/Database/DatabaseAuth.php +++ b/src/Domain/Auth/Providers/Database/DatabaseAuth.php @@ -75,10 +75,31 @@ private function authUser(UserLoginDto $userLoginDto): UserDto|false $this->userService->getByLogin($userLoginDto->getLoginUser() ?? '') ); - if ($userDto->isMigrate && $this->checkMigrateUser($userDto, $userLoginDto)) { - $this->userPassService->migrateUserPassById($userDto->id, $userLoginDto->getLoginPass() ?? ''); - - return $userDto; + if ($userDto->isMigrate) { + if ($this->checkMigrateUser($userDto, $userLoginDto)) { + $this->userPassService->migrateUserPassById($userDto->id, $userLoginDto->getLoginPass() ?? ''); + + return $userDto; + } + + // A row still carrying a pre-migration hash refuses at the same cost as any other. + // + // Its `pass` holds a sha1, md5 or crypt digest, and the three comparisons above + // take microseconds; the fourth hands that digest to `password_verify()`, which + // rejects it on sight because it is not a bcrypt hash at all. So a wrong password + // here returned in about 0.3ms where a migrated account and an unknown login both + // paid ~220ms for a real verify — measured on this installation. + // + // That is the enumeration oracle the catch block below deals with, reopened for a + // subset: a fast refusal said "this login exists *and* is one of the old ones", + // naming both a real account and the ones whose stored hashes are weakest. + // + // Falling through to the check below would not have spent it either — that check + // is the same one `checkMigrateUser()` just made, against the same non-bcrypt + // value, so it fast-fails identically and the answer is false either way. + Hash::checkHashKey($userLoginDto->getLoginPass() ?? '', self::ABSENT_USER_HASH); + + return false; } if (Hash::checkHashKey($userLoginDto->getLoginPass() ?? '', $userDto->pass)) { diff --git a/tests/Unit/Domain/Auth/Providers/Database/DatabaseAuthTest.php b/tests/Unit/Domain/Auth/Providers/Database/DatabaseAuthTest.php index 757b8d414..b8b41fdea 100644 --- a/tests/Unit/Domain/Auth/Providers/Database/DatabaseAuthTest.php +++ b/tests/Unit/Domain/Auth/Providers/Database/DatabaseAuthTest.php @@ -163,6 +163,65 @@ static function (string $login) use ($existing) { ); } + /** + * And an account still carrying a pre-migration hash refuses at that same cost. + * + * A row with `isMigrate` holds a sha1, md5 or crypt digest rather than a bcrypt hash, and + * `checkMigrateUser()` compares against all three — three digests, microseconds — before + * falling through to `Hash::checkHashKey()`, which `password_verify()` rejects on sight + * because the stored value is not a bcrypt hash at all. So a wrong password for a legacy + * account returned in microseconds, while a wrong password for a migrated one and a login + * naming nobody both paid for a full bcrypt verify. + * + * That is the enumeration oracle above, reopened for a subset: a fast refusal said "this login + * exists *and* is one of the old ones", which is both a name confirmed and the accounts whose + * stored hashes are weakest, to an unauthenticated caller who never guessed a password. + * + * Compared against the migrated account rather than a fixed number of milliseconds, for the + * same reason as the test above. + */ + public function testALegacyAccountRefusesAPasswordAtTheSameCostAsAMigratedOne(): void + { + $password = self::$faker->password(); + $salt = 'a-legacy-salt'; + + $migrated = UserDataGenerator::factory()->buildUserData()->mutate( + ['login' => 'migrated', 'isMigrate' => false, 'pass' => Hash::hashKey('the-actual-password')] + ); + + // The three digests checkMigrateUser() tries, one of which this row actually holds. + $legacy = UserDataGenerator::factory()->buildUserData()->mutate( + [ + 'login' => 'legacy', + 'isMigrate' => true, + 'hashSalt' => $salt, + 'pass' => sha1($salt . 'the-actual-password'), + ] + ); + + $this->userService + ->method('getByLogin') + ->willReturnCallback( + static fn(string $login) => $login === 'legacy' ? $legacy : $migrated + ); + + $costOfMigrated = $this->timeAuthenticating('migrated', $password); + $costOfLegacy = $this->timeAuthenticating('legacy', $password); + + self::assertGreaterThan( + $costOfMigrated / 2, + $costOfLegacy, + sprintf( + 'a wrong password for an account that has not been migrated has to cost about what' + . ' one for an account that has costs, or the difference tells a caller which' + . ' logins are real and which of them still hold a legacy hash' + . ' (migrated: %.1fms, legacy: %.1fms)', + $costOfMigrated * 1000, + $costOfLegacy * 1000 + ) + ); + } + /** * Seconds spent on one authentication attempt, which must fail either way. */