Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Security] Don't invalidate the user when the password was not stored in the session #59539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 17, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

Related to #59106: this PR does is considering that if $originalUser (the object coming from the session) has a null password, then we don't consider it changed from $refreshedUser. Aka we don't log out the user in such case.

The benefit is allowing to not put the hashed password in the session. I think that's desirable.

@stof
Copy link
Member

stof commented Jan 17, 2025

what is the use case for having a PasswordAuthenticatedUserInterface if you don't store the password hash in the DB ? This prevents you from authenticating as you cannot verify the password hash

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 17, 2025

This PR is about not having the hashed password in the session storage ;)
(PR description updated @stof, I made a mistake that was leading to this confusing)

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

A test case would be nice e.g in src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php (if possible ofc)

@nicolas-grekas nicolas-grekas force-pushed the sec-user-pwd-less branch 4 times, most recently from 874cd11 to 05cd2bc Compare January 20, 2025 11:33
public function __construct(
private string $username,
private array $roles,
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

CPP FTW

public function getSalt(): ?string
{
return null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

legacy stuff

@@ -187,7 +187,7 @@ public function onKernelResponse(ResponseEvent $event): void
*
* @throws \RuntimeException
*/
protected function refreshUser(TokenInterface $token): ?TokenInterface
private function refreshUser(TokenInterface $token): ?TokenInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

the class is @final

if (!$originalUser instanceof PasswordAuthenticatedUserInterface || !$refreshedUser instanceof PasswordAuthenticatedUserInterface || $originalUser->getPassword() !== $refreshedUser->getPassword()) {
if (!$originalUser instanceof PasswordAuthenticatedUserInterface
|| !$refreshedUser instanceof PasswordAuthenticatedUserInterface
|| $refreshedUser->getPassword() !== ($originalUser->getPassword() ?? $refreshedUser->getPassword())
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the meat of this PR: ignore null passwords from the session storage

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this also mean that you lose the ability to detect that the user changed the password?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. See the main thread of this PR for some thoughts on the topic.

@@ -42,11 +42,7 @@ public function testHandleWhenTheAccessDecisionManagerDecidesToRefuseAccess()
->willReturn([['foo' => 'bar'], null])
;

$token = new class extends AbstractToken {
public function getCredentials(): mixed
Copy link
Member Author

Choose a reason for hiding this comment

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

legacy stuff also

@nicolas-grekas
Copy link
Member Author

Sidenote: 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?

@chalasr
Copy link
Member

chalasr commented Jan 20, 2025

Is it worth it given the hash has to be stored in the DB anyway in order to be able to authenticate?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 20, 2025

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 mean it doesn't invalidate on password changes either IIUC.)

@nicolas-grekas nicolas-grekas merged commit 0134078 into symfony:7.3 Jan 29, 2025
11 checks passed
@nicolas-grekas nicolas-grekas deleted the sec-user-pwd-less branch January 29, 2025 10:16
chalasr added a commit that referenced this pull request Feb 10, 2025
…rc32c 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.