From 06dbfe48da80e428a434e215d11c992c832e4b88 Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Sat, 2 Jan 2021 17:06:12 +0100 Subject: [PATCH] [Security][Guard] Prevent user enumeration via response content --- .../Provider/UserAuthenticationProvider.php | 3 +- .../UserAuthenticationProviderTest.php | 8 +-- .../Firewall/GuardAuthenticationListener.php | 13 ++++- .../GuardAuthenticationListenerTest.php | 51 +++++++++++++++++++ 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/Core/Authentication/Provider/UserAuthenticationProvider.php b/Core/Authentication/Provider/UserAuthenticationProvider.php index 172556ac2..9557fa000 100644 --- a/Core/Authentication/Provider/UserAuthenticationProvider.php +++ b/Core/Authentication/Provider/UserAuthenticationProvider.php @@ -13,6 +13,7 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; +use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\Exception\AuthenticationServiceException; use Symfony\Component\Security\Core\Exception\BadCredentialsException; @@ -83,7 +84,7 @@ public function authenticate(TokenInterface $token) $this->userChecker->checkPreAuth($user); $this->checkAuthentication($user, $token); $this->userChecker->checkPostAuth($user); - } catch (BadCredentialsException $e) { + } catch (AccountStatusException $e) { if ($this->hideUserNotFoundExceptions) { throw new BadCredentialsException('Bad credentials.', 0, $e); } diff --git a/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php b/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php index 7b984e304..c20b6ca2e 100644 --- a/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php +++ b/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php @@ -79,7 +79,7 @@ public function testAuthenticateWhenProviderDoesNotReturnAnUserInterface() public function testAuthenticateWhenPreChecksFails() { - $this->expectException('Symfony\Component\Security\Core\Exception\CredentialsExpiredException'); + $this->expectException(BadCredentialsException::class); $userChecker = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserCheckerInterface')->getMock(); $userChecker->expects($this->once()) ->method('checkPreAuth') @@ -97,7 +97,7 @@ public function testAuthenticateWhenPreChecksFails() public function testAuthenticateWhenPostChecksFails() { - $this->expectException('Symfony\Component\Security\Core\Exception\AccountExpiredException'); + $this->expectException(BadCredentialsException::class); $userChecker = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserCheckerInterface')->getMock(); $userChecker->expects($this->once()) ->method('checkPostAuth') @@ -116,7 +116,7 @@ public function testAuthenticateWhenPostChecksFails() public function testAuthenticateWhenPostCheckAuthenticationFails() { $this->expectException('Symfony\Component\Security\Core\Exception\BadCredentialsException'); - $this->expectExceptionMessage('Bad credentials'); + $this->expectExceptionMessage('Bad credentials.'); $provider = $this->getProvider(); $provider->expects($this->once()) ->method('retrieveUser') @@ -124,7 +124,7 @@ public function testAuthenticateWhenPostCheckAuthenticationFails() ; $provider->expects($this->once()) ->method('checkAuthentication') - ->willThrowException(new BadCredentialsException()) + ->willThrowException(new CredentialsExpiredException()) ; $provider->authenticate($this->getSupportedToken()); diff --git a/Guard/Firewall/GuardAuthenticationListener.php b/Guard/Firewall/GuardAuthenticationListener.php index 11bda3cd1..190290c46 100644 --- a/Guard/Firewall/GuardAuthenticationListener.php +++ b/Guard/Firewall/GuardAuthenticationListener.php @@ -17,7 +17,10 @@ use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Guard\AbstractGuardAuthenticator; use Symfony\Component\Security\Guard\AuthenticatorInterface; use Symfony\Component\Security\Guard\GuardAuthenticatorHandler; @@ -40,6 +43,7 @@ class GuardAuthenticationListener implements ListenerInterface private $guardAuthenticators; private $logger; private $rememberMeServices; + private $hideUserNotFoundExceptions; /** * @param GuardAuthenticatorHandler $guardHandler The Guard handler @@ -48,7 +52,7 @@ class GuardAuthenticationListener implements ListenerInterface * @param iterable|AuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider * @param LoggerInterface $logger A LoggerInterface instance */ - public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, $providerKey, $guardAuthenticators, LoggerInterface $logger = null) + public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, $providerKey, $guardAuthenticators, LoggerInterface $logger = null, $hideUserNotFoundExceptions = true) { if (empty($providerKey)) { throw new \InvalidArgumentException('$providerKey must not be empty.'); @@ -59,6 +63,7 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat $this->providerKey = $providerKey; $this->guardAuthenticators = $guardAuthenticators; $this->logger = $logger; + $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions; } /** @@ -163,6 +168,12 @@ private function executeGuardAuthenticator($uniqueGuardKey, GuardAuthenticatorIn $this->logger->info('Guard authentication failed.', ['exception' => $e, 'authenticator' => \get_class($guardAuthenticator)]); } + // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status) + // to prevent user enumeration via response content + if ($this->hideUserNotFoundExceptions && ($e instanceof UsernameNotFoundException || $e instanceof AccountStatusException)) { + $e = new BadCredentialsException('Bad credentials.', 0, $e); + } + $response = $this->guardHandler->handleAuthenticationFailure($e, $request, $guardAuthenticator, $this->providerKey); if ($response instanceof Response) { diff --git a/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php b/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php index 6572696d4..a2e4671d5 100644 --- a/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php +++ b/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php @@ -16,6 +16,9 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\LockedException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Guard\AbstractGuardAuthenticator; use Symfony\Component\Security\Guard\AuthenticatorInterface; use Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener; @@ -208,6 +211,54 @@ public function testHandleCatchesAuthenticationException() $listener->handle($this->event); } + /** + * @dataProvider exceptionsToHide + */ + public function testHandleHidesInvalidUserExceptions(AuthenticationException $exceptionToHide) + { + $authenticator = $this->createMock(AuthenticatorInterface::class); + $providerKey = 'my_firewall2'; + + $authenticator + ->expects($this->once()) + ->method('supports') + ->willReturn(true); + $authenticator + ->expects($this->once()) + ->method('getCredentials') + ->willReturn(['username' => 'robin', 'password' => 'hood']); + + $this->authenticationManager + ->expects($this->once()) + ->method('authenticate') + ->willThrowException($exceptionToHide); + + $this->guardAuthenticatorHandler + ->expects($this->once()) + ->method('handleAuthenticationFailure') + ->with($this->callback(function ($e) use ($exceptionToHide) { + return $e instanceof BadCredentialsException && $exceptionToHide === $e->getPrevious(); + }), $this->request, $authenticator, $providerKey); + + $listener = new GuardAuthenticationListener( + $this->guardAuthenticatorHandler, + $this->authenticationManager, + $providerKey, + [$authenticator], + $this->logger + ); + + $listener->handle($this->event); + } + + public function exceptionsToHide() + { + return [ + [new UsernameNotFoundException()], + [new LockedException()], + ]; + } + /** * @group legacy */