diff --git a/Http/Firewall/SwitchUserListener.php b/Http/Firewall/SwitchUserListener.php index 4aff3b458..dac5e02e2 100644 --- a/Http/Firewall/SwitchUserListener.php +++ b/Http/Firewall/SwitchUserListener.php @@ -93,7 +93,8 @@ public function handle(GetResponseEvent $event) try { $this->tokenStorage->setToken($this->attemptSwitchUser($request, $username)); } catch (AuthenticationException $e) { - throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage())); + // Generate 403 in any conditions to prevent user enumeration vulnerabilities + throw new AccessDeniedException('Switch User failed: '.$e->getMessage(), $e); } } @@ -130,7 +131,24 @@ private function attemptSwitchUser(Request $request, $username) throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername())); } - $user = $this->provider->loadUserByUsername($username); + $currentUsername = $token->getUsername(); + $nonExistentUsername = '_'.md5(random_bytes(8).$username); + + // To protect against user enumeration via timing measurements + // we always load both successfully and unsuccessfully + try { + $user = $this->provider->loadUserByUsername($username); + + try { + $this->provider->loadUserByUsername($nonExistentUsername); + throw new \LogicException('AuthenticationException expected'); + } catch (AuthenticationException $e) { + } + } catch (AuthenticationException $e) { + $this->provider->loadUserByUsername($currentUsername); + + throw $e; + } if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) { $exception = new AccessDeniedException(); diff --git a/Http/Tests/Firewall/SwitchUserListenerTest.php b/Http/Tests/Firewall/SwitchUserListenerTest.php index 6e3b0e30f..31cd45809 100644 --- a/Http/Tests/Firewall/SwitchUserListenerTest.php +++ b/Http/Tests/Firewall/SwitchUserListenerTest.php @@ -17,6 +17,7 @@ use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Core\Role\SwitchUserRole; use Symfony\Component\Security\Core\User\User; use Symfony\Component\Security\Http\Event\SwitchUserEvent; @@ -161,6 +162,7 @@ public function testExitUserDoesNotDispatchEventWithStringUser() public function testSwitchUserIsDisallowed() { $token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']); + $user = new User('username', 'password', []); $this->tokenStorage->setToken($token); $this->request->query->set('_switch_user', 'kuba'); @@ -169,6 +171,33 @@ public function testSwitchUserIsDisallowed() ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH']) ->willReturn(false); + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException()))); + + $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager); + $listener->handle($this->event); + } + + /** + * @expectedException \Symfony\Component\Security\Core\Exception\AccessDeniedException + */ + public function testSwitchUserTurnsAuthenticationExceptionTo403() + { + $token = new UsernamePasswordToken('username', '', 'key', ['ROLE_ALLOWED_TO_SWITCH']); + + $this->tokenStorage->setToken($token); + $this->request->query->set('_switch_user', 'kuba'); + + $this->accessDecisionManager->expects($this->never()) + ->method('decide'); + + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba'], ['username']) + ->will($this->onConsecutiveCalls($this->throwException(new UsernameNotFoundException()))); + $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager); $listener->handle($this->event); } @@ -185,9 +214,10 @@ public function testSwitchUser() ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user) ->willReturn(true); - $this->userProvider->expects($this->once()) - ->method('loadUserByUsername')->with('kuba') - ->willReturn($user); + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException()))); $this->userChecker->expects($this->once()) ->method('checkPostAuth')->with($user); @@ -215,9 +245,10 @@ public function testSwitchUserKeepsOtherQueryStringParameters() ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user) ->willReturn(true); - $this->userProvider->expects($this->once()) - ->method('loadUserByUsername')->with('kuba') - ->willReturn($user); + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException()))); $this->userChecker->expects($this->once()) ->method('checkPostAuth')->with($user); @@ -243,9 +274,10 @@ public function testSwitchUserWithReplacedToken() ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user) ->willReturn(true); - $this->userProvider->expects($this->any()) - ->method('loadUserByUsername')->with('kuba') - ->willReturn($user); + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException()))); $dispatcher = $this->getMockBuilder('Symfony\Component\EventDispatcher\EventDispatcherInterface')->getMock(); $dispatcher @@ -290,9 +322,10 @@ public function testSwitchUserStateless() ->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user) ->willReturn(true); - $this->userProvider->expects($this->once()) - ->method('loadUserByUsername')->with('kuba') - ->willReturn($user); + $this->userProvider->expects($this->exactly(2)) + ->method('loadUserByUsername') + ->withConsecutive(['kuba']) + ->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException()))); $this->userChecker->expects($this->once()) ->method('checkPostAuth')->with($user);