Merge branch '3.4' into 4.4

* 3.4: [Security][Guard] Prevent user enumeration via response content
symfony · May 12, 2021 · 9c1ce3e · 9c1ce3e
2 parents 9527320 + 06dbfe4
commit 9c1ce3e
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 5 deletions.
diff --git a/Core/Authentication/Provider/UserAuthenticationProvider.php b/Core/Authentication/Provider/UserAuthenticationProvider.php
@@ -14,6 +14,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
 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;
@@ -80,7 +81,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
@@ -83,7 +83,7 @@ public function testAuthenticateWhenProviderDoesNotReturnAnUserInterface()
 
     public function testAuthenticateWhenPreChecksFails()
     {
-        $this->expectException(CredentialsExpiredException::class);
+        $this->expectException(BadCredentialsException::class);
         $userChecker = $this->createMock(UserCheckerInterface::class);
         $userChecker->expects($this->once())
                     ->method('checkPreAuth')
@@ -101,7 +101,7 @@ public function testAuthenticateWhenPreChecksFails()
 
     public function testAuthenticateWhenPostChecksFails()
     {
-        $this->expectException(AccountExpiredException::class);
+        $this->expectException(BadCredentialsException::class);
         $userChecker = $this->createMock(UserCheckerInterface::class);
         $userChecker->expects($this->once())
                     ->method('checkPostAuth')
@@ -128,7 +128,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
@@ -17,7 +17,10 @@
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 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\AuthenticatorInterface;
 use Symfony\Component\Security\Guard\GuardAuthenticatorHandler;
 use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken;
@@ -44,12 +47,13 @@ class GuardAuthenticationListener extends AbstractListener implements ListenerIn
     private $guardAuthenticators;
     private $logger;
     private $rememberMeServices;
+    private $hideUserNotFoundExceptions;
 
     /**
      * @param string                            $providerKey         The provider (i.e. firewall) key
      * @param iterable|AuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider
      */
-    public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null)
+    public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true)
     {
         if (empty($providerKey)) {
             throw new \InvalidArgumentException('$providerKey must not be empty.');
@@ -60,6 +64,7 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat
         $this->providerKey = $providerKey;
         $this->guardAuthenticators = $guardAuthenticators;
         $this->logger = $logger;
+        $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions;
     }
 
     /**
@@ -164,6 +169,12 @@ private function executeGuardAuthenticator(string $uniqueGuardKey, Authenticator
                 $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
@@ -19,6 +19,9 @@
 use Symfony\Component\Security\Core\Authentication\AuthenticationProviderManager;
 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\AuthenticatorInterface;
 use Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener;
 use Symfony\Component\Security\Guard\GuardAuthenticatorHandler;
@@ -211,6 +214,54 @@ public function testHandleCatchesAuthenticationException()
         $listener($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($this->event);
+    }
+
+    public function exceptionsToHide()
+    {
+        return [
+            [new UsernameNotFoundException()],
+            [new LockedException()],
+        ];
+    }
+
     public function testSupportsReturnFalseSkipAuth()
     {
         $authenticator = $this->createMock(AuthenticatorInterface::class);