Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2020
Merged

[Security/Http] Hash Persistent RememberMe token #35960

merged 1 commit into from
Mar 5, 2020

Conversation

guillbdx
Copy link

@guillbdx guillbdx commented Mar 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.

@chalasr chalasr added Security and removed Bug labels Mar 4, 2020
@chalasr chalasr added this to the next milestone Mar 4, 2020
@lyrixx
Copy link
Member

lyrixx commented Mar 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.

@chalasr
Copy link
Member

chalasr commented Mar 4, 2020

SHA256 seems good enough as per #27910 (comment)

nicolas-grekas
nicolas-grekas reviewed Mar 4, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas 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 :)

@guillbdx
Copy link
Author

guillbdx commented Mar 4, 2020

Don't miss adding some tests :)

I've updated PersistentTokenBasedRememberMeServicesTest, so it now uses tokens with hashed values.
I also annotated the testAutoLogin 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.

@nicolas-grekas nicolas-grekas changed the title Hash Persistent RememberMe token [Security/Http] Hash Persistent RememberMe token Mar 4, 2020
nicolas-grekas
nicolas-grekas approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas 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.

LGTM thanks

@nicolas-grekas nicolas-grekas mentioned this pull request Mar 4, 2020
Closed
@nicolas-grekas nicolas-grekas requested a review from chalasr March 4, 2020 17:00
chalasr
chalasr approved these changes Mar 4, 2020
View reviewed changes
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.

The Security CHANGELOG needs to be updated.

@chalasr
Copy link
Member

chalasr commented Mar 5, 2020

Thank you @guillbdx.

@chalasr chalasr closed this in 45c4ffa Mar 5, 2020
@chalasr chalasr merged commit 45c4ffa into symfony:master Mar 5, 2020
@aschempp aschempp mentioned this pull request Mar 10, 2020
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@wouterj wouterj wouterj approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

RememberMe token should be hashed in the database
6 participants
@guillbdx @lyrixx @chalasr @nicolas-grekas @wouterj @carsonbot