-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Description
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:
- Browser emits 2 concurrent requests with identical remember-me cookie
- 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 - 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
- Authenticate with remember me
- Write down the remember me cookie somewhere
- Wait for at least a minute
- Delete session cookie
- Refresh the page - now you should reauthenticate with remember me
- Restore remember me cookie with the value from the step 2
- Wait for another minute and if you're using a cache with
appendOnly- ensure you clear it. - Delete session cookie again
- 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