Skip to content

PersistentRememberMeHandler - race condition in returned cookies #42343

Closed
@zerkms

Description

@zerkms

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

$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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions