NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Security/Http] Hash Persistent RememberMe token#35960

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

chalasr merged 1 commit intosymfony:masterfromguillbdx:feature/27910-hash-persistent-rememberme-token

Mar 5, 2020

Merged

[Security/Http] Hash Persistent RememberMe token#35960

chalasr merged 1 commit intosymfony:masterfromguillbdx:feature/27910-hash-persistent-rememberme-token

Mar 5, 2020

Conversation

Copy link

guillbdx commentedMar 4, 2020•
edited by nicolas-grekas
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#27910
License	MIT
Doc PR	Not sure this enhancement needs documentation

The purpose of this PR is to enhance the Remember Me persistent token feature: instead of storing cleared token value in DB, the values will be hashed.
To make sure that existing remember me cookies will keep being valid after this change, we prefix the new token values with 'hash_'. In case the token value doesn't match this prefix, we keep validating it the old way.

carsonbot added Status: Needs Review Bug Feature labels

Mar 4, 2020

chalasr added Security and removed Bug labels

Mar 4, 2020

chalasr added this to thenext milestone

Mar 4, 2020

Copy link

Member

lyrixx commentedMar 4, 2020

You need random data in the hash (nonce) to make the secret unknowable.
I would use the password API or the libsodium API for this use case.

Copy link

Member

chalasr commentedMar 4, 2020

SHA256 seems good enough as per#27910 (comment)

nicolas-grekas reviewed

Mar 4, 2020

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Don't miss adding some tests :)

src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php OutdatedShow resolvedHide resolved

nicolas-grekas reviewed

Mar 4, 2020

View reviewed changes

...fony/Component/Security/Http/Tests/RememberMe/PersistentTokenBasedRememberMeServicesTest.php OutdatedShow resolvedHide resolved

Copy link

Author

guillbdx commentedMar 4, 2020

Don't miss adding some tests :)

I've updatedPersistentTokenBasedRememberMeServicesTest, so it now uses tokens with hashed values.
I also annotated thetestAutoLogin test to be run twice: once with a hashed token value, and once with a clear token value, to make sure the old way keeps working.