diff --git a/UPGRADING.md b/UPGRADING.md index 68f8960de..eb72a3e59 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -10,6 +10,22 @@ - `CodeIgniter\Shield\Config\BaseAuthToken` is now `abstract`. Code that instantiated it directly with `new BaseAuthToken()` will now throw an `Error`. +### Login Timing Hardening + +Failed password logins now return `Auth.badAttempt` for both unknown users and +incorrect passwords. `Auth.invalidPassword` remains in the language files for +application code that uses it, so no new translations are required. Update any +custom views or tests that expect the old incorrect-password message. + +Unknown-user logins now verify against `Auth::$dummyPasswordHash`. The default +hash uses bcrypt cost 12. If your application changes the password algorithm or +cost, run `php spark shield:generate-dummy-hash` with its current configuration. +Copy the printed property declaration into **app/Config/Auth.php**, or set +`auth.dummyPasswordHash = ''` in **.env** using only the hash +value. The command does not modify either file. Accounts with older password +hash settings can still have different verification times until their hashes +are upgraded. + ## Version 1.2 to 1.3 ### JWT: Minimum Key Length Now Enforced diff --git a/docs/guides/strengthen_password.md b/docs/guides/strengthen_password.md index fbf6dc9a0..6aa7f5f09 100644 --- a/docs/guides/strengthen_password.md +++ b/docs/guides/strengthen_password.md @@ -99,6 +99,31 @@ public int $hashTimeCost = PASSWORD_ARGON2_DEFAULT_TIME_COST; public int $hashThreads = PASSWORD_ARGON2_DEFAULT_THREADS; ``` +### Update the Dummy Password Hash + +When you change the password hashing algorithm or its cost settings, update +`Auth::$dummyPasswordHash` as well. Shield uses this hash to +verify a password when the login identifier does not match an account, reducing +timing differences between failed login attempts. The default dummy hash uses +bcrypt with cost 12. + +After applying the new hash settings in the target environment, run: + +```sh +php spark shield:generate-dummy-hash +``` + +Copy the printed property declaration into `app/Config/Auth.php`, or put only the +generated hash in your `.env` file: + +```ini +auth.dummyPasswordHash = '' +``` + +The command reads the current hash settings and does not edit either file. +Existing user hashes may still use older settings until they are rehashed after +a successful login. + ## Maximum Password Length By default, Shield has the validation rules for maximum password length. diff --git a/src/Authentication/Authenticators/HmacSha256.php b/src/Authentication/Authenticators/HmacSha256.php index b9441e212..53233a686 100644 --- a/src/Authentication/Authenticators/HmacSha256.php +++ b/src/Authentication/Authenticators/HmacSha256.php @@ -142,7 +142,16 @@ public function check(array $credentials): Result } // Extract UserToken and HMACSHA256 Signature from Authorization token - [$userToken, $signature] = $this->getHmacAuthTokens($credentials['token']); + $authTokens = $this->getHmacAuthTokens($credentials['token']); + + if ($authTokens === null || count($authTokens) !== 2) { + return new Result([ + 'success' => false, + 'reason' => lang('Auth.badToken'), + ]); + } + + [$userToken, $signature] = $authTokens; $identityModel = model(UserIdentityModel::class); @@ -160,7 +169,7 @@ public function check(array $credentials): Result // Check signature... $hash = hash_hmac('sha256', (string) $credentials['body'], $secretKey); - if ($hash !== $signature) { + if (! hash_equals($hash, $signature)) { return new Result([ 'success' => false, 'reason' => lang('Auth.badToken'), diff --git a/src/Authentication/Authenticators/Session.php b/src/Authentication/Authenticators/Session.php index d98458157..147db31a3 100644 --- a/src/Authentication/Authenticators/Session.php +++ b/src/Authentication/Authenticators/Session.php @@ -245,7 +245,7 @@ public function checkAction(UserIdentity $identity, string $token): bool throw new LogicException('Cannot get the User.'); } - if ($token === '' || $token !== $identity->secret) { + if ($token === '' || ! hash_equals((string) $identity->secret, $token)) { return false; } @@ -365,21 +365,25 @@ public function check(array $credentials): Result // Find the existing user $user = $this->provider->findByCredentials($credentials); + /** @var Passwords $passwords */ + $passwords = service('passwords'); + if ($user === null) { + // Verify against a fixed hash so an unknown identifier still does + // the expensive password check. The result is never accepted. + $passwords->verify($givenPassword, config('Auth')->dummyPasswordHash); + return new Result([ 'success' => false, 'reason' => lang('Auth.badAttempt'), ]); } - /** @var Passwords $passwords */ - $passwords = service('passwords'); - // Now, try matching the passwords. if (! $passwords->verify($givenPassword, $user->password_hash)) { return new Result([ 'success' => false, - 'reason' => lang('Auth.invalidPassword'), + 'reason' => lang('Auth.badAttempt'), ]); } diff --git a/src/Commands/GenerateDummyPasswordHash.php b/src/Commands/GenerateDummyPasswordHash.php new file mode 100644 index 000000000..3d6e6f590 --- /dev/null +++ b/src/Commands/GenerateDummyPasswordHash.php @@ -0,0 +1,36 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Shield\Commands; + +class GenerateDummyPasswordHash extends BaseCommand +{ + protected $name = 'shield:generate-dummy-hash'; + protected $description = 'Generate a dummy password hash using the current Auth password settings.'; + protected $usage = 'shield:generate-dummy-hash'; + + public function run(array $params): int + { + $hash = service('passwords')->hash(bin2hex(random_bytes(32))); + + if (! is_string($hash)) { + $this->error('Could not generate a dummy password hash.'); + + return EXIT_ERROR; + } + + $this->write('public string $dummyPasswordHash = \'' . $hash . '\';'); + + return EXIT_SUCCESS; + } +} diff --git a/src/Config/Auth.php b/src/Config/Auth.php index db2a0a4a7..e8d779905 100644 --- a/src/Config/Auth.php +++ b/src/Config/Auth.php @@ -379,6 +379,15 @@ class Auth extends BaseConfig */ public int $hashCost = 12; + /** + * Hash used when a login identifier does not match an account. It must use + * the same password algorithm and cost as real accounts so that failed + * logins take comparable time. Override this when changing hash settings. + * Run `php spark shield:generate-dummy-hash` to generate a matching hash. + * The corresponding plaintext password is never accepted. + */ + public string $dummyPasswordHash = '$2y$12$lGwEWbbJHNlNehaFdwrcH.e7c0S3L2sMFVZr6LX/IHjU0ONwWzogq'; + /* * //////////////////////////////////////////////////////////////////// * OTHER SETTINGS diff --git a/tests/Authentication/Authenticators/AccessTokenAuthenticatorTest.php b/tests/Authentication/Authenticators/AccessTokenAuthenticatorTest.php index 57d16f9b7..bb365a1d6 100644 --- a/tests/Authentication/Authenticators/AccessTokenAuthenticatorTest.php +++ b/tests/Authentication/Authenticators/AccessTokenAuthenticatorTest.php @@ -168,7 +168,7 @@ public function testCheckSuccess(): void $this->assertSame($user->id, $result->extraInfo()->id); $updatedToken = $result->extraInfo()->currentAccessToken(); - $this->assertNotEmpty($updatedToken->last_used_at); + $this->assertInstanceOf(Time::class, $updatedToken->last_used_at); // Checking token in the same second does not throw "DataException : There is no data to update." $this->auth->check(['token' => $token->raw_token]); diff --git a/tests/Authentication/Authenticators/HmacAuthenticatorTest.php b/tests/Authentication/Authenticators/HmacAuthenticatorTest.php index 53679bf60..f0dc10a67 100644 --- a/tests/Authentication/Authenticators/HmacAuthenticatorTest.php +++ b/tests/Authentication/Authenticators/HmacAuthenticatorTest.php @@ -155,6 +155,17 @@ public function testCheckBadSignature(): void $this->assertSame(lang('Auth.badToken'), $result->reason()); } + public function testCheckMalformedToken(): void + { + $result = $this->auth->check([ + 'token' => 'abc123', + 'body' => 'bar', + ]); + + $this->assertFalse($result->isOK()); + $this->assertSame(lang('Auth.badToken'), $result->reason()); + } + public function testCheckOldToken(): void { $user = fake(UserModel::class); @@ -198,7 +209,7 @@ public function testCheckSuccess(): void $this->assertSame($user->id, $result->extraInfo()->id); $updatedToken = $result->extraInfo()->currentHmacToken(); - $this->assertNotEmpty($updatedToken->last_used_at); + $this->assertInstanceOf(Time::class, $updatedToken->last_used_at); // Checking token in the same second does not throw "DataException : There is no data to update." $this->auth->check(['token' => $rawToken, 'body' => 'bar']); diff --git a/tests/Authentication/Authenticators/SessionAuthenticatorTest.php b/tests/Authentication/Authenticators/SessionAuthenticatorTest.php index 24dd6d69e..88ab88bcb 100644 --- a/tests/Authentication/Authenticators/SessionAuthenticatorTest.php +++ b/tests/Authentication/Authenticators/SessionAuthenticatorTest.php @@ -17,6 +17,7 @@ use CodeIgniter\Shield\Authentication\Authentication; use CodeIgniter\Shield\Authentication\AuthenticationException; use CodeIgniter\Shield\Authentication\Authenticators\Session; +use CodeIgniter\Shield\Authentication\Passwords; use CodeIgniter\Shield\Config\Auth; use CodeIgniter\Shield\Entities\User; use CodeIgniter\Shield\Exceptions\LogicException; @@ -272,6 +273,29 @@ public function testCheckCannotFindUser(): void $this->assertSame(lang('Auth.badAttempt'), $result->reason()); } + public function testCheckUnknownUserStillVerifiesPassword(): void + { + $passwords = new class (new Auth()) extends Passwords { + public array $checks = []; + + public function verify(string $password, string $hash): bool + { + $this->checks[] = [$password, $hash]; + + return false; + } + }; + Services::injectMock('passwords', $passwords); + + $result = $this->auth->check([ + 'email' => 'unknown@example.com', + 'password' => 'secret', + ]); + + $this->assertFalse($result->isOK()); + $this->assertSame([['secret', config('Auth')->dummyPasswordHash]], $passwords->checks); + } + public function testCheckBadPassword(): void { $this->user->createEmailIdentity([ @@ -286,7 +310,7 @@ public function testCheckBadPassword(): void $this->assertInstanceOf(Result::class, $result); $this->assertFalse($result->isOK()); - $this->assertSame(lang('Auth.invalidPassword'), $result->reason()); + $this->assertSame(lang('Auth.badAttempt'), $result->reason()); } public function testCheckSuccess(): void diff --git a/tests/Authentication/Filters/GroupFilterTest.php b/tests/Authentication/Filters/GroupFilterTest.php index bc6e1d233..14bf07d35 100644 --- a/tests/Authentication/Filters/GroupFilterTest.php +++ b/tests/Authentication/Filters/GroupFilterTest.php @@ -45,7 +45,6 @@ public function testFilterNotAuthorizedStoresRedirectToEntranceUrlIntoSession(): $result->assertRedirectTo('/login'); - $this->assertNotEmpty(session()->getTempdata('beforeLoginUrl')); $this->assertSame(site_url('protected-route'), session()->getTempdata('beforeLoginUrl')); } diff --git a/tests/Authentication/Filters/PermissionFilterTest.php b/tests/Authentication/Filters/PermissionFilterTest.php index 5ca3039c1..a118bca24 100644 --- a/tests/Authentication/Filters/PermissionFilterTest.php +++ b/tests/Authentication/Filters/PermissionFilterTest.php @@ -45,7 +45,6 @@ public function testFilterNotAuthorizedStoresRedirectToEntranceUrlIntoSession(): $result->assertRedirectTo('/login'); - $this->assertNotEmpty(session()->getTempdata('beforeLoginUrl')); $this->assertSame(site_url('protected-route'), session()->getTempdata('beforeLoginUrl')); } diff --git a/tests/Authentication/Filters/SessionFilterTest.php b/tests/Authentication/Filters/SessionFilterTest.php index 34493aae7..eece7ac15 100644 --- a/tests/Authentication/Filters/SessionFilterTest.php +++ b/tests/Authentication/Filters/SessionFilterTest.php @@ -145,7 +145,6 @@ public function testStoreRedirectsToEntraceUrlIntoSession(): void $result->assertRedirectTo('/login'); $session = session(); - $this->assertNotEmpty($session->get('beforeLoginUrl')); $this->assertSame(site_url('protected-route'), $session->get('beforeLoginUrl')); } } diff --git a/tests/Authorization/AuthorizableTest.php b/tests/Authorization/AuthorizableTest.php index 692c6acd6..22a31d15b 100644 --- a/tests/Authorization/AuthorizableTest.php +++ b/tests/Authorization/AuthorizableTest.php @@ -121,7 +121,7 @@ public function testRemoveGroupExistingGroup(): void ]); $this->user->removeGroup('admin'); - $this->assertEmpty($this->user->getGroups()); + $this->assertSame([], $this->user->getGroups()); $this->dontSeeInDatabase($this->tables['groups_users'], [ 'user_id' => $this->user->id, 'group' => 'admin', @@ -242,7 +242,7 @@ public function testRemovePermissionExistingPermissions(): void ]); $this->user->removePermission('admin.access'); - $this->assertEmpty($this->user->getPermissions()); + $this->assertSame([], $this->user->getPermissions()); $this->dontSeeInDatabase($this->tables['permissions_users'], [ 'user_id' => $this->user->id, 'permission' => 'admin.access', diff --git a/tests/Commands/GenerateDummyPasswordHashTest.php b/tests/Commands/GenerateDummyPasswordHashTest.php new file mode 100644 index 000000000..ef22c0d73 --- /dev/null +++ b/tests/Commands/GenerateDummyPasswordHashTest.php @@ -0,0 +1,79 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace Tests\Commands; + +use CodeIgniter\Shield\Commands\GenerateDummyPasswordHash; +use CodeIgniter\Shield\Test\MockInputOutput; +use Tests\Support\TestCase; + +/** + * @internal + */ +final class GenerateDummyPasswordHashTest extends TestCase +{ + protected function tearDown(): void + { + GenerateDummyPasswordHash::resetInputOutput(); + + parent::tearDown(); + } + + public function testGeneratesHashUsingCurrentPasswordSettings(): void + { + $config = config('Auth'); + $config->hashCost = 5; + + $hash = $this->generateHash(); + + $this->assertSame(PASSWORD_BCRYPT, password_get_info($hash)['algo']); + $this->assertSame(5, password_get_info($hash)['options']['cost']); + $this->assertFalse(service('passwords')->needsRehash($hash)); + } + + public function testGeneratesHashUsingCurrentArgon2idSettings(): void + { + if (! defined('PASSWORD_ARGON2ID')) { + $this->markTestSkipped('Argon2id is not available.'); + } + + $config = config('Auth'); + $config->hashAlgorithm = PASSWORD_ARGON2ID; + $config->hashMemoryCost = 8192; + $config->hashTimeCost = 2; + $config->hashThreads = 1; + + $hash = $this->generateHash(); + $info = password_get_info($hash); + + $this->assertSame(PASSWORD_ARGON2ID, $info['algo']); + $this->assertSame(8192, $info['options']['memory_cost']); + $this->assertSame(2, $info['options']['time_cost']); + $this->assertFalse(service('passwords')->needsRehash($hash)); + } + + private function generateHash(): string + { + $io = new MockInputOutput(); + GenerateDummyPasswordHash::setInputOutput($io); + + $this->assertNotFalse(command('shield:generate-dummy-hash')); + + $output = trim($io->getOutputs()); + $prefix = 'public string $dummyPasswordHash = \''; + $this->assertStringStartsWith($prefix, $output); + $this->assertStringEndsWith("';", $output); + + return substr($output, strlen($prefix), -2); + } +} diff --git a/tests/Controllers/LoginTest.php b/tests/Controllers/LoginTest.php index bc0851406..cf9c96b49 100644 --- a/tests/Controllers/LoginTest.php +++ b/tests/Controllers/LoginTest.php @@ -68,7 +68,6 @@ public function testLoginBadEmail(): void 'success' => 0, ]); - $this->assertNotEmpty(session('error')); $this->assertSame(lang('Auth.badAttempt'), session('error')); } diff --git a/tests/Language/AbstractTranslationTestCase.php b/tests/Language/AbstractTranslationTestCase.php index 5233c2f42..733611bca 100644 --- a/tests/Language/AbstractTranslationTestCase.php +++ b/tests/Language/AbstractTranslationTestCase.php @@ -113,7 +113,7 @@ final public function testAllConfiguredLanguageFilesAreTranslated(string $locale sort($filesNotTranslated); $count = count($filesNotTranslated); - $this->assertEmpty($filesNotTranslated, sprintf( + $this->assertSame([], $filesNotTranslated, sprintf( 'Failed asserting that language %s "%s" in the main repository %s translated in "%s" locale.', $count > 1 ? 'files' : 'file', implode('", "', $filesNotTranslated), @@ -137,7 +137,7 @@ final public function testAllTranslatedLanguageFilesAreConfigured(string $locale sort($filesNotConfigured); $count = count($filesNotConfigured); - $this->assertEmpty($filesNotConfigured, sprintf( + $this->assertSame([], $filesNotConfigured, sprintf( 'Failed asserting that translated language %s "%s" in "%s" locale %s configured in the main repository.', $count > 1 ? 'files' : 'file', implode('", "', $filesNotConfigured), @@ -169,7 +169,7 @@ final public function testAllConfiguredLanguageKeysAreIncluded(string $locale): sort($keysNotIncluded); $count = count($keysNotIncluded); - $this->assertEmpty($keysNotIncluded, sprintf( + $this->assertSame([], $keysNotIncluded, sprintf( 'Failed asserting that the language %s "%s" in the main repository %s included for translation in "%s" locale.', $count > 1 ? 'keys' : 'key', implode('", "', $keysNotIncluded), @@ -201,7 +201,7 @@ final public function testAllIncludedLanguageKeysAreConfigured(string $locale): sort($keysNotConfigured); $count = count($keysNotConfigured); - $this->assertEmpty($keysNotConfigured, sprintf( + $this->assertSame([], $keysNotConfigured, sprintf( 'Failed asserting that the translated language %s "%s" in "%s" locale %s configured in the main repository.', $count > 1 ? 'keys' : 'key', implode('", "', $keysNotConfigured), @@ -246,7 +246,7 @@ final public function testAllIncludedLanguageKeysAreTranslated(string $locale): sort($keysNotTranslated); $count = count($keysNotTranslated); - $this->assertEmpty($keysNotTranslated, sprintf( + $this->assertSame([], $keysNotTranslated, sprintf( 'Failed asserting that the translated language %s "%s" in "%s" locale %s from the original keys in the main repository.', $count > 1 ? 'keys' : 'key', implode('", "', $keysNotTranslated), @@ -289,7 +289,7 @@ final public function testAllConfiguredLanguageKeysAreInOrder(string $locale): v } } - $this->assertEmpty($diffs, sprintf( + $this->assertSame([], $diffs, sprintf( "Failed asserting that the translated language keys in \"%s\" locale are ordered correctly.\n%s\n%s", $locale, CLI::color('--- Original', 'red') . "\n" . CLI::color('+++ Translated', 'green'), @@ -341,7 +341,7 @@ final public function testAllLocalizationParametersAreNotTranslated(string $loca ksort($diffs); - $this->assertEmpty($diffs, sprintf( + $this->assertSame([], $diffs, sprintf( "Failed asserting that parameters of translation keys are not translated:\n%s", implode("\n", array_map( static fn (string $key, array $values): string => sprintf(' * %s => %s', $key, implode(', ', $values)), diff --git a/tests/Unit/UserTest.php b/tests/Unit/UserTest.php index 8397846af..310a07230 100644 --- a/tests/Unit/UserTest.php +++ b/tests/Unit/UserTest.php @@ -39,7 +39,7 @@ final class UserTest extends DatabaseTestCase public function testGetIdentitiesNone(): void { // when none, returns empty array - $this->assertEmpty($this->user->identities); + $this->assertSame([], $this->user->identities); } public function testGetIdentitiesSome(): void @@ -62,7 +62,7 @@ public function testGetIdentitiesByType(): void $this->assertCount(1, $identities); $this->assertInstanceOf(UserIdentity::class, $identities[0]); $this->assertSame('access_token', $identities[0]->type); - $this->assertEmpty($this->user->getIdentities('foo')); + $this->assertSame([], $this->user->getIdentities('foo')); } public function testModelFindAllWithIdentities(): void