Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix double authentication via RememberMe resulting in wrong RememberMe cookie being set in client#46760
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJun 24, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
ab816cc to18c5efbCompare18c5efb to2044ee3Comparecarsonbot commentedJun 25, 2022
Hey! I think@Seldaek has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Seldaek commentedJun 25, 2022
Probably should target 5.4 but otherwise lgtm! |
…e cookie being set in client
fabpot commentedJun 26, 2022
Thank you@heiglandreas. |
zerkms commentedAug 30, 2022
Does this not break the series-based security? Now if See the solution from my#42343 which is free of this problem. |
zerkms commentedAug 30, 2022
@Seldaek it looks like I was too quick to close my report right? |
wouterj commentedSep 4, 2022
@zerkms can you please open a pull request that adds a failing test showing your use-case, and adds your solution - to show that it fixes both use-cases? |
zerkms commentedSep 4, 2022
@wouterj it's a security vulnerability report, do we really need a PR to explain how the series-based remember me works? |
zerkms commentedSep 4, 2022
Btw, PR is coming in next 10-15 minutes. |
wouterj commentedSep 4, 2022
If you verified it is broken, please follow the procedure inhttp://symfony.com/security to report security vulnerabilities. |
zerkms commentedSep 4, 2022
PR is coming, it does not need a dedicated report, all details have already been exposed here. |
…the second consequent requestClosesymfony#42343Fixsymfony#46760
zerkms commentedSep 4, 2022
#47488 PR is published. |
…the second consequent requestClosesymfony#42343Fixsymfony#46760
…ond consequent request (Ivan Kurnosov)This PR was merged into the 5.4 branch.Discussion----------[Security] Fix valid remember-me token exposure to the second consequent requestClose#42343Fix#46760| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fix#42343,Fix#46760 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix.This will help reviewers and should be a good start for the documentation.Additionally (seehttps://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should followhttps://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (seehttps://symfony.com/bc).-->#46760 PR together with a fix produces a security vulnerability when a malicious actor may get a **new and valid** remember me token if makes a request right after the legit user.Commits-------62ceded Bug#42343 [Security] Fix valid remember-me token exposure to the second consequent request
…ond consequent requestClosesymfony/symfony#42343Fixsymfony/symfony#46760
Uh oh!
There was an error while loading.Please reload this page.
In the RememberMe feature we have noticed that with a custom tokenVerifier it is possible to verify an old token. That works as expected but leads to the problematic issue that it will still result in the old token being integrated into the cookie that is sent back to the client.
So the flow is as follows:
rem-cookie with token "tokenA"rem-cookie with token "tokenA"rem-cookie with "tokenA"rem-cookie is overwritten withdeleted.This PR will pass the persisted token (in the example "tokenB") in request "B" instead of the provided token (in the example "tokenA") and therefore not causing an overwrite of the Remember-Me cookie with a wrong value and a continuing functionality also when authentication-requests are sent within one minute of one another.
How that can happen? By using double clicks for example in certain setups where then two authenticating requests are sent right after one another...