security #cve-2023-46733 [Security] Fix possible session fixation when only the *token* changes (RobertMe)

nicolas-grekas · nicolas-grekas · commit 7467bd7e3f88 · 2023-11-09T19:15:13.000+01:00
This PR was merged into the 5.4 branch.
diff --git a/src/Symfony/Component/Security/Http/EventListener/SessionStrategyListener.php b/src/Symfony/Component/Security/Http/EventListener/SessionStrategyListener.php
@@ -48,7 +48,7 @@ public function onSuccessfulLogin(LoginSuccessEvent $event): void
             $user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
             $previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
 
-            if ('' !== ($user ?? '') && $user === $previousUser) {
+            if ('' !== ($user ?? '') && $user === $previousUser && \get_class($token) === \get_class($previousToken)) {
                 return;
             }
         }
diff --git a/src/Symfony/Component/Security/Http/Tests/EventListener/SessionStrategyListenerTest.php b/src/Symfony/Component/Security/Http/Tests/EventListener/SessionStrategyListenerTest.php
@@ -15,6 +15,7 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\Security\Core\Authentication\Token\NullToken;
+use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\User\InMemoryUser;
 use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
@@ -81,6 +82,26 @@ public function testRequestWithSamePreviousUser()
         $this->listener->onSuccessfulLogin($event);
     }
 
+    public function testRequestWithSamePreviousUserButDifferentTokenType()
+    {
+        $this->configurePreviousSession();
+
+        $token = $this->createMock(NullToken::class);
+        $token->expects($this->once())
+            ->method('getUserIdentifier')
+            ->willReturn('test');
+        $previousToken = $this->createMock(UsernamePasswordToken::class);
+        $previousToken->expects($this->once())
+            ->method('getUserIdentifier')
+            ->willReturn('test');
+
+        $this->sessionAuthenticationStrategy->expects($this->once())->method('onAuthentication')->with($this->request, $token);
+
+        $event = new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('test', function () {})), $token, $this->request, null, 'main_firewall', $previousToken);
+
+        $this->listener->onSuccessfulLogin($event);
+    }
+
     private function createEvent($firewallName)
     {
         return new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('test', function ($username) { return new InMemoryUser($username, null); })), $this->token, $this->request, null, $firewallName);

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ public function onSuccessfulLogin(LoginSuccessEvent $event): void`
`48`	`48`	`$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();`
`49`	`49`	`$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();`
`50`	`50`
`51`		`- if ('' !== ($user ?? '') && $user === $previousUser) {`
	`51`	`+ if ('' !== ($user ?? '') && $user === $previousUser && \get_class($token) === \get_class($previousToken)) {`
`52`	`52`	`return;`
`53`	`53`	`}`
`54`	`54`	`}`