E532 [Security] Support hashing the hashed password using crc32c when putting the user in the session by nicolas-grekas · Pull Request #59562 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Support hashing the hashed password using crc32c when putting the user in the session#59562

Merged
chalasr merged 1 commit intosymfony:7.3from
nicolas-grekas:sec-hash-pwd
Feb 10, 2025
Merged

[Security] Support hashing the hashed password using crc32c when putting the user in the session#59562
chalasr merged 1 commit intosymfony:7.3from
nicolas-grekas:sec-hash-pwd

Conversation

@nicolas-grekas
Copy link
Copy Markdown
Member
@nicolas-grekas nicolas-grekas commented Jan 20, 2025 •
edited
Loading
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:

   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:

'$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.

@carsonbot carsonbot added this to the 7.3 milestone Jan 20, 2025
nicolas-grekas
nicolas-grekas commented Jan 20, 2025
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the sec-hash-pwd branch 2 times, most recently from 46faf11 to d1ee5c4 Compare January 20, 2025 17:38
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 20, 2025
Closed
@nicolas-grekas nicolas-grekas changed the title [Security] Support hashing the hashed password using xxh3 when putting the user in the session [Security] Support hashing the hashed password using crc32c when putting the user in the session Jan 21, 2025
@nicolas-grekas
Copy link
Copy Markdown
Member Author
nicolas-grekas commented Jan 28, 2025

PR ready /cc @symfony/mergers

@nicolas-grekas nicolas-grekas force-pushed the sec-hash-pwd branch 2 times, most recently from 9cab9e1 to 7f7247b Compare January 29, 2025 10:23
@nicolas-grekas
Copy link
Copy Markdown
Member Author
nicolas-grekas commented Jan 29, 2025

(PR rebased)

@nicolas-grekas nicolas-grekas force-pushed the sec-hash-pwd branch 2 times, most recently from 8243e9a to f6742c6 Compare February 10, 2025 11:29
GromNaN
GromNaN reviewed Feb 10, 2025
View reviewed changes
Copy link
Copy Markdown
Member
@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a question in tests.

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

if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $refreshedUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) {
if ($originalUser instanceof LegacyPasswordAuthenticatedUserInterface && $originalUser->getSalt() !== $refreshedUser->getSalt()) {
Copy link
Copy Markdown
Member
@GromNaN GromNaN Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The psalm false-positive is reported vimeo/psalm#9956

@chalasr
Copy link
Copy Markdown
Member
chalasr commented Feb 10, 2025

Thank you @nicolas-grekas.
BAC0

@chalasr chalasr merged commit 802940e into symfony:7.3 Feb 10, 2025
@chalasr chalasr mentioned this pull request Feb 10, 2025
Closed
@nicolas-grekas nicolas-grekas deleted the sec-hash-pwd branch February 10, 2025 13:07
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GromNaN GromNaN GromNaN approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@chalasr chalasr chalasr approved these changes

Assignees

No one assigned

Labels

Feature Security Status: Reviewed

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas @chalasr @GromNaN @welcoMattic @mindaugasvcs @carsonbot
0