Skip to content

PersistentRememberMeHandler - race condition in returned cookies #42343

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
Closed
zerkms opened this issue Aug 2, 2021 · 9 comments · Fixed by #47488
Closed

PersistentRememberMeHandler - race condition in returned cookies #42343

zerkms opened this issue Aug 2, 2021 · 9 comments · Fixed by #47488
Labels
Bug Security Status: Needs Review

Comments

@zerkms
Copy link
Contributor

zerkms commented Aug 2, 2021 •
edited
Loading

Symfony version(s) affected: 5.3.6

Description

This bug was originally raised at #28314 and (almost) fixed in #41175

The current implementation creates the cookie in this line

symfony/src/Symfony/Component/Security/Http/RememberMe/PersistentRememberMeHandler.php

Line 100 in 4876967

$this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue));

The problem with that line is that it creates a race condition between 2 concurrent requests (which #28314 was originally about):

So the chronology is:

  1. Browser emits 2 concurrent requests with identical remember-me cookie
  2. One request wins this race, authenticates a user with remember-me token, regenerates token, stores old in the cache, stores new in database, returns new token in the Set-Cookie
  3. Now the second request is to be handled, because it's within 60 seconds and because the old value is still in the cache - it passes validation. But now we create a cookie in that line 100 and put the old value into it.

Now the browser looks fine, but it contains the remember-me token with not actual value, because the second request, which has lost the race - re-set the cookie value to the old one.

How to reproduce

  1. Authenticate with remember me
  2. Write down the remember me cookie somewhere
  3. Wait for at least a minute
  4. Delete session cookie
  5. Refresh the page - now you should reauthenticate with remember me
  6. Restore remember me cookie with the value from the step 2
  7. Wait for another minute and if you're using a cache with appendOnly - ensure you clear it.
  8. Delete session cookie again
  9. Refresh the page

Now the session is lost and in the logs you can read This token was already used. The account is possibly compromised.

Possible Solution

The cookie should only be returned if the new token value has been generated.

eg:

        // if a token was regenerated less than a minute ago, there is no need to regenerate it
        // if multiple concurrent requests reauthenticate a user we do not want to update the token several times
        if ($persistentToken->getLastUsed()->getTimestamp() + 60 < time()) {
            $tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
            $tokenLastUsed = new \DateTime();
            if ($this->tokenVerifier) {
                $this->tokenVerifier->updateExistingToken($persistentToken, $tokenValue, $tokenLastUsed);
            }
            $this->tokenProvider->updateToken($series, $tokenValue, $tokenLastUsed);

            $this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue)); // moved inside condition body
        }

Additional context

/cc @Seldaek
@zerkms zerkms added the Bug label Aug 2, 2021
@zerkms
Copy link
Contributor Author

zerkms commented Aug 2, 2021 •
edited
Loading

Ok, it looks like it's not exactly what happens, and I need a bit more time to understand before I reopen it again.

But something certainly does not work as expected.

@zerkms zerkms closed this as completed Aug 2, 2021
@zerkms
Copy link
Contributor Author

zerkms commented Aug 2, 2021

Okay, I found what exactly happens and updated it.

@zerkms zerkms reopened this Aug 2, 2021
@zerkms zerkms changed the title PersistentRememberMeHandler caches the wrong value PersistentRememberMeHandler - race condition in returned cookies Aug 2, 2021
@zerkms
Copy link
Contributor Author

zerkms commented Aug 2, 2021

/cc @wouterj

@xabbuh xabbuh added the Security label Aug 2, 2021
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@zerkms
Copy link
Contributor Author

zerkms commented Feb 7, 2022

It is still relevant.

@carsonbot carsonbot removed the Stalled label Feb 7, 2022
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@zerkms
Copy link
Contributor Author

zerkms commented Aug 28, 2022

I have found a workaround, it's explained in the first post.

@carsonbot carsonbot removed the Stalled label Aug 28, 2022
@Seldaek
Copy link
Member

Seldaek commented Aug 30, 2022

We have fixed this issue somehow - not sure if it was done in #46760 or only as a workaround on our end, maybe @heiglandreas knows.

@zerkms
Copy link
Contributor Author

zerkms commented Aug 30, 2022

@Seldaek okay, I personally would prefer solution I provided above (less code and more sense to me), but it's a personal choice, neither is "better" from the technical perspective. Nonetheless I take it as "it's fixed now", thank you for letting me know, I finally can remove the monkey-patch from my projects.

@zerkms zerkms closed this as completed Aug 30, 2022
@zerkms zerkms mentioned this issue Aug 30, 2022
Merged
@zerkms zerkms reopened this Aug 30, 2022
zerkms pushed a commit to zerkms/symfony that referenced this issue 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 issue Sep 4, 2022
Merged
zerkms pushed a commit to zerkms/symfony that referenced this issue 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 issue 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
nicolas-grekas added a commit that referenced this issue Sep 8, 2022
@nicolas-grekas
Merge branch '5.4' into 6.0
563ef98 
* 5.4:
  [HttpClient] Fix computing retry delay when using RetryableHttpClient
  [Uid] Fix validating UUID variant bits
  [Validator][UID] Stop to first ULID format violation
  [Bridge] Fix mkdir() race condition in ProxyCacheWarmer
  [Cache] update readme
  Bug #42343 [Security] Fix valid remember-me token exposure to the second consequent request
  Prevent exception if request stack is empty
  Psr18Client ignore invalid HTTP headers
  skip a transient test on AppVeyor
symfony-splitter pushed a commit to symfony/security-http that referenced this issue 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
nicolas-grekas added a commit that referenced this issue Sep 8, 2022
@nicolas-grekas
Merge branch '6.0' into 6.1
b0daf10 
* 6.0:
  [HttpClient] Fix computing retry delay when using RetryableHttpClient
  [Uid] Fix validating UUID variant bits
  [Validator][UID] Stop to first ULID format violation
  [Bridge] Fix mkdir() race condition in ProxyCacheWarmer
  [Cache] update readme
  Bug #42343 [Security] Fix valid remember-me token exposure to the second consequent request
  Prevent exception if request stack is empty
  Psr18Client ignore invalid HTTP headers
  skip a transient test on AppVeyor
nicolas-grekas added a commit that referenced this issue Sep 8, 2022
@nicolas-grekas
Merge branch '6.1' into 6.2
3b33bde 
* 6.1:
  [HttpClient] Fix computing retry delay when using RetryableHttpClient
  [Uid] Fix validating UUID variant bits
  [Validator][UID] Stop to first ULID format violation
  [Bridge] Fix mkdir() race condition in ProxyCacheWarmer
  [Cache] update readme
  Bug #42343 [Security] Fix valid remember-me token exposure to the second consequent request
  Prevent exception if request stack is empty
  Psr18Client ignore invalid HTTP headers
  skip a transient test on AppVeyor
@zerkms zerkms mentioned this issue Mar 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Security] Fix valid remember-me token exposure to the second consequent request zerkms/symfony
5 participants
@Seldaek @nicolas-grekas @zerkms @xabbuh @carsonbot