Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/Domain/Auth/Providers/Database/DatabaseAuth.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
59 changes: 59 additions & 0 deletions tests/Unit/Domain/Auth/Providers/Database/DatabaseAuthTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down