8000 feature #59562 [Security] Support hashing the hashed password using c… · symfony/symfony@802940e · GitHub
[go: up one dir, main page]
Skip to content

Commit 802940e

Browse files
chalasrchalasr
committed
feature #59562 [Security] Support hashing the hashed password using crc32c when putting the user in the session (nicolas-grekas)
This PR was merged into the 7.3 branch. Discussion ---------- [Security] Support hashing the hashed password using crc32c when putting the user in the session | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT This PR builds on #59539 following the discussion that happened there. > Instead of putting the hashed password in the DB to invalidate sessions on password changes, we could store a hash of the hashed password, eg a truncated xxh128 would be fine. This can be implemented in userland with the EquatableInterface. Should we consider making this more "core" somehow? > > DB storage and session storage have different security features, so definitely worth it to me yes. > I had a look at other frameworks, and django, spring, Express.js, RoR, ASP.net all have something for that in the mean of a password_changed timestamp or similar token that triggers the invalidation. Notably neither Laravel nor Symfony have anything out of the box on the topic (Symfony stores the hashed password, Laravel doesn't, which means it doesn't invalidate on password changes either IIUC.) Here is what I added to PasswordAuthenticatedUserInterface to explain what this PR enables: > The __serialize/__unserialize() magic methods can be used on the user class to prevent the password hash from being > stored in the session. If the password is not stored at all in the session, getPassword() should return null after > unserialization, and then, changing the user's password won't invalidate its sessions. > In order to invalidate the user sessions while not storing the password hash in the session, it's also possible to > hash the password hash before serializing it; crc32c is the only algorithm supported. For example: > > ```php > public function __serialize(): array > { > $data = (array) $this; > $data["\0".self::class."\0password"] = hash('crc32c', $this->password); > > return $data; > } > ``` > > Implement EquatableInteface if you need another logic. > crc32c is selected because its probability to change when the password hash changes is high, so that the invalidation of sessions is effective. But it's also selected because there are many possible valid password hashes that generate the same crc32c. This protects against brute-forcing the password hash: let's say one is able to find a password hash that has the same crc32c as the real password hash: one would still be unable to confirm that this password hash is the correct one. To do so, they would have to brute-force the password hash itself, and this would require brute-forcing bcrypt et al. The cost of doing so on one bcrypted-password is already prohibitive. Doing so with a very high number of possible candidates (as collisions are generated) would be even more prohibitive. Note that to generate a collision, one just needs to generate a random string that's formatted as a real hash, like this line for a bcrypted-password: ```php '$2y$12$'.substr(strtr(base64_encode(random_bytes(40)), '+', '.'), 0, 53) ``` (one could likely create a crc32c-aware collision generator for this purpose, but that wouldn't reduce the difficulty of validating the generated hashes). On the contrary, using a more collision resistant hashing algorithm would make it too easy to validate that a generated hash is the real hash. Commits ------- a4f8a76 [Security] Support hashing the hashed password using crc32c when putting the user in the session
2 parents 3918524 + a4f8a76 commit 802940e

File tree

5 files changed

+51
-11
lines changed

5 files changed

+51
-11
lines changed

‎src/Symfony/Component/Security/Core/User/PasswordAuthenticatedUserInterface.php

+20-3
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,26 @@
1414
/**
1515
* For users that can be authenticated using a password.
1616
*
17+
* The __serialize/__unserialize() magic methods can be implemented on the user
18+
* class to prevent hashed passwords from being put in the session storage.
19+
* If the password is not stored at all in the session, getPassword() should
20+
* return null after unserialization, and then, changing the user's password
21+
* won't invalidate its sessions.
22+
* In order to invalidate the user sessions while not storing the password hash
23+
* in the session, it's also possible to hash the password hash before
24+
* serializing it; crc32c is the only algorithm supported.
25+
* For example:
26+
*
27+
* public function __serialize(): array
28+
* {
29+
* $data = (array) $this;
30+
* $data["\0".self::class."\0password"] = hash('crc32c', $this->password);
31+
*
32+
* return $data;
33+
* }
34+
*
35+
* Implement EquatableInteface if you need another logic.
36+
*
1737
* @author Robin Chalas <robin.chalas@gmail.com>
1838
* @author Wouter de Jong <wouter@wouterj.nl>
1939
*/
@@ -23,9 +43,6 @@ interface PasswordAuthenticatedUserInterface
2343
* Returns the hashed password used to authenticate the user.
2444
*
2545
* Usually on authentication, a plain-text password will be compared to this value.
26-
*
27-
* The __serialize/__unserialize() magic methods can be implemented on the user
28-
* class to prevent hashed passwords from being put in the session storage.
2946
*/
3047
public function getPassword(): ?string;
3148
}

‎src/Symfony/Component/Security/Http/CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ CHANGELOG
77
* Add encryption support to `OidcTokenHandler` (JWE)
88
* Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor
99
* Add argument `$identifierNormalizer` to `UserBadge::__construct()` to allow normalizing the identifier
10+
* Support hashing the hashed password using crc32c when putting the user in the session
1011

1112
7.2
1213
---

‎src/Symfony/Component/Security/Http/Firewall/ContextListener.php

+11-4
Original file line numberDiff line numberDiff line change
@@ -292,9 +292,16 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa
292292
}
293293

294294
if ($originalUser instanceof PasswordAuthenticatedUserInterface || $refreshedUser instanceof PasswordAuthenticatedUserInterface) {
295-
if (!$originalUser instanceof PasswordAuthenticatedUserInterface
296-
|| !$refreshedUser instanceof PasswordAuthenticatedUserInterface
297-
|| $refreshedUser->getPassword() !== ($originalUser->getPassword() ?? $refreshedUser->getPassword())
295+
if (!$originalUser instanceof PasswordAuthenticatedUserInterface || !$refreshedUser instanceof PasswordAuthenticatedUserInterface) {
296+
return true;
297+
}
298+
299+
$originalPassword = $originalUser->getPassword();
300+
$refreshedPassword = $refreshedUser->getPassword();
301+
302+
if (null !== $originalPassword
303+
&& $refreshedPassword !== $originalPassword
304+
&& (8 !== \strlen($originalPassword) || hash('crc32c', $refreshedPassword ?? $originalPassword) !== $originalPassword)
298305
) {
299306
return true;
300307
}
@@ -303,7 +310,7 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa
303310
return true;
304311
}
305312

306-
if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $refreshedUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) {
313+
if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) {
307314
return true;
308315
}
309316
}

‎src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php

+7-2
Original file line numberDiff line numberDiff line change
@@ -377,9 +377,14 @@ public function testOnKernelResponseRemoveListener()
377377
$this->assertEmpty($dispatcher->getListeners());
378378
}
379379

380-
public function testRemovingPasswordFromSessionDoesntInvalidateTheToken()
380+
/**
381+
* @testWith [true]
382+
* [false]
383+
* [null]
384+
*/
385+
public function testNullOrHashedPasswordInSessionDoesntInvalidateTheToken(?bool $hashPassword)
381386
{
382-
$user = new CustomUser('user', ['ROLE_USER'], 'pass');
387+
$user = new CustomUser('user', ['ROLE_USER'], 'pass', $hashPassword);
383388

384389
$userProvider = $this->createMock(UserProviderInterface::class);
385390
$userProvider->expects($this->once())

‎src/Symfony/Component/Security/Http/Tests/Fixtures/CustomUser.php

+12-2
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ final class CustomUser implements UserInterface, PasswordAuthenticatedUserInterf
1919
public function __construct(
2020
private string $username,
2121
private array $roles,
22-
private ?string $password = null,
22+
private ?string $password,
23+
private ?bool $hashPassword,
2324
) {
2425
}
2526

@@ -44,6 +45,15 @@ public function eraseCredentials(): void
4445

4546
public function __serialize(): array
4647
{
47-
return [\sprintf("\0%s\0username", self::class) => $this->username];
48+
$data = (array) $this;
49+
$passwordKey = \sprintf("\0%s\0password", self::class);
50+
51+
if ($this->hashPassword) {
52+
$data[$passwordKey] = hash('crc32c', $this->password);
53+
} elseif (null !== $this->hashPassword) {
54+
unset($data[$passwordKey]);
55+
}
56+
57+
return $data;
4858
}
4959
}

0 commit comments

Comments
 (0)
0