Skip to content

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.

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
Jun 26, 2022
Merged

Fix double authentication via RememberMe resulting in wrong RememberMe cookie being set in client #46760

merged 1 commit into from
Jun 26, 2022

Conversation

heiglandreas
Copy link
Contributor

@heiglandreas heiglandreas commented Jun 24, 2022 •
edited
Loading

Q A
Branch? 6.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets There has not yet an issue opened for this.
License MIT
Doc PR n.a.

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:

  • Customer sends a request "A" with rem-cookie with token "tokenA"
  • The user is authenticated via the token "tokenA", the token is refreshed with "tokenB".
  • Customer sends a request "B" with rem-cookie with token "tokenA"
  • Set-Cookie is sent back to the customer in response to request "A" with Remember-Me token "tokenB"
  • The user is authenticated via the token "tokenA" via a custom verifier. The last token was created less than 60 seconds ago, so let's not refresh the token but resend the current token.
  • Set-Cookie is sent back to the customer in response to request "B" with Remember-Me token "tokenA"
  • Remember-Me token "tokenA" overwrites the previously received token "tokenB" in the client.
  • Customer sends a request "C" with rem-cookie with "tokenA"
  • The user is not authenticated any more as the custom.verifier no longer accepts old token "tokenA" and "tokenA" doesn't match the persisted token "tokenB". The rem-cookie is overwritten with deleted.

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

@carsonbot carsonbot added this to the 6.2 milestone Jun 24, 2022
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@heiglandreas heiglandreas changed the title Add test to handle external validations Fix double authentication via RememberMe resulting in wrong RememberMe cookie being set in client Jun 24, 2022
@heiglandreas heiglandreas force-pushed the allowOldTokensToBeUsed branch from ab816cc to 18c5efb Compare June 24, 2022 08:07
@heiglandreas heiglandreas changed the base branch from 6.2 to 6.0 June 24, 2022 08:08
@heiglandreas heiglandreas force-pushed the allowOldTokensToBeUsed branch from 18c5efb to 2044ee3 Compare June 24, 2022 08:16
@carsonbot
Copy link

Hey!

I think @Seldaek has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@Seldaek
Copy link
Member

Seldaek commented Jun 25, 2022

Probably should target 5.4 but otherwise lgtm!

@fabpot fabpot changed the base branch from 6.0 to 5.4 June 26, 2022 10:07
@fabpot
Copy link
Member

fabpot commented Jun 26, 2022

Thank you @heiglandreas.

@fabpot fabpot force-pushed the allowOldTokensToBeUsed branch from 2044ee3 to 4dff49f Compare June 26, 2022 10:08
@fabpot fabpot merged commit a390266 into symfony:5.4 Jun 26, 2022
This was referenced Jun 26, 2022
Merged
Merged
Merged
@xabbuh xabbuh modified the milestones: 6.2, 5.4 Aug 5, 2022
@Seldaek Seldaek mentioned this pull request Aug 30, 2022
Closed
@zerkms
Copy link
Contributor

zerkms commented Aug 30, 2022

Does this not break the series-based security? Now if request-A is sent from the valid user, and request-B is sent from a malicious user that stole a token, then you return them the new valid token again.

See the solution from my #42343 which is free of this problem.

@zerkms
Copy link
Contributor

zerkms commented Aug 30, 2022

@Seldaek it looks like I was too quick to close my report right?

@wouterj
Copy link
Member

wouterj commented Sep 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
Copy link
Contributor

zerkms commented Sep 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
Copy link
Contributor

zerkms commented Sep 4, 2022

Btw, PR is coming in next 10-15 minutes.

@wouterj
Copy link
Member

wouterj commented Sep 4, 2022

If you verified it is broken, please follow the procedure in http://symfony.com/security to report security vulnerabilities.

@zerkms
Copy link
Contributor

zerkms commented Sep 4, 2022

PR is coming, it does not need a dedicated report, all details have already been exposed here.

zerkms pushed a commit to zerkms/symfony that referenced this pull request Sep 4, 2022
Bug symfony#42343 [Security] Fix valid remember-me token exposure to …
7e9f27c 
…the second consequent request

Close symfony#42343
Fix symfony#46760
@zerkms zerkms mentioned this pull request Sep 4, 2022
Merged
@zerkms
Copy link
Contributor

zerkms commented Sep 4, 2022

#47488 PR is published.

zerkms pushed a commit to zerkms/symfony that referenced this pull request Sep 5, 2022
Bug symfony#42343 [Security] Fix valid remember-me token exposure to …
62ceded 
…the second consequent request

Close symfony#42343
Fix symfony#46760
nicolas-grekas added a commit that referenced this pull request Sep 8, 2022
@nicolas-grekas
bug #47488 [Security] Fix valid remember-me token exposure to the sec…
64be67e 
…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 request

Close #42343
Fix #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 (see https://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 follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://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
symfony-splitter pushed a commit to symfony/security-http that referenced this pull request Sep 8, 2022
Bug #42343 [Security] Fix valid remember-me token exposure to the sec…
b2fb7ff 
…ond consequent request

Close symfony/symfony#42343
Fix symfony/symfony#46760
@heiglandreas heiglandreas mentioned this pull request Mar 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@Seldaek Seldaek Seldaek approved these changes

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
7 participants
@heiglandreas @carsonbot @Seldaek @fabpot @zerkms @wouterj @xabbuh