Prevent remember-me cookie from being set when two-factor authenticat…

…ion is required, fixes "Bypass 2fa" #253

DependencyInjection/Compiler/RememberMeServicesDecoratorCompilerPass.php

@@ -0,0 +1,44 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Scheb\TwoFactorBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ChildDefinition;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * Decorates all remember-me services instances so that the remember-me cookie doesn't leak when two-factor
+ * authentication is required.
+ */
+class RememberMeServicesDecoratorCompilerPass implements CompilerPassInterface
+{
+    private const REMEMBER_ME_LISTENER_ID_PREFIX = 'security.authentication.listener.rememberme.';
+
+    public function process(ContainerBuilder $container)
+    {
+        // Find all remember-me listener definitions
+        $prefixLength = strlen(self::REMEMBER_ME_LISTENER_ID_PREFIX);
+        foreach ($container->getDefinitions() as $definitionId => $definition) {
+            if (substr($definitionId, 0, $prefixLength) === self::REMEMBER_ME_LISTENER_ID_PREFIX) {
+                $this->decorateRememberMeServices($container, $definition);
+            }
+        }
+    }
+
+    private function decorateRememberMeServices(ContainerBuilder $container, Definition $authListenerDefinition): void
+    {
+        // Get the remember-me services from the listener and decorate it
+        $rememberMeServicesId = (string) $authListenerDefinition->getArgument(1);
+        if ($rememberMeServicesId) {
+            $decoratedServiceId = $rememberMeServicesId.'.two_factor_decorator';
+            $container
+                ->setDefinition($decoratedServiceId, new ChildDefinition('scheb_two_factor.security.rememberme_services_decorator'))
+                ->setDecoratedService($rememberMeServicesId)
+                ->replaceArgument(0, new Reference($decoratedServiceId.'.inner'));
+        }
+    }
+}

Resources/config/security.xml

@@ -55,6 +55,10 @@
 			<argument /> <!-- Decorated listener -->
 		</service>
 
+		<service id="scheb_two_factor.security.rememberme_services_decorator" class="\Scheb\TwoFactorBundle\Security\Authentication\RememberMe\RememberMeServicesDecorator" abstract="true">
+			<argument /> <!-- Decorated remember-me services instance -->
+		</service>
+
 		<service id="scheb_two_factor.security.provider_preparation_listener" class="Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderPreparationListener" abstract="true">
 			<argument type="service" id="scheb_two_factor.provider_registry" />
 			<argument type="service" id="scheb_two_factor.provider_preparation_recorder" />

SchebTwoFactorBundle.php

@@ -5,6 +5,7 @@
 namespace Scheb\TwoFactorBundle;
 
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\AuthenticationProviderDecoratorCompilerPass;
+use Scheb\TwoFactorBundle\DependencyInjection\Compiler\RememberMeServicesDecoratorCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\TwoFactorFirewallConfigCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\TwoFactorProviderCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Factory\Security\TwoFactorFactory;
@@ -19,6 +20,7 @@ public function build(ContainerBuilder $container)
         parent::build($container);
 
         $container->addCompilerPass(new AuthenticationProviderDecoratorCompilerPass());
+        $container->addCompilerPass(new RememberMeServicesDecoratorCompilerPass());
         $container->addCompilerPass(new TwoFactorProviderCompilerPass());
         $container->addCompilerPass(new TwoFactorFirewallConfigCompilerPass());
 

Security/Authentication/RememberMe/RememberMeServicesDecorator.php

@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Scheb\TwoFactorBundle\Security\Authentication\RememberMe;
+
+use Scheb\TwoFactorBundle\Security\Authentication\Token\TwoFactorTokenInterface;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Http\Logout\LogoutHandlerInterface;
+use Symfony\Component\Security\Http\RememberMe\RememberMeServicesInterface;
+
+class RememberMeServicesDecorator implements RememberMeServicesInterface, LogoutHandlerInterface
+{
+    /**
+     * @var RememberMeServicesInterface|LogoutHandlerInterface
+     */
+    private $decoratedRememberMeServices;
+
+    public function __construct($decoratedRememberMeServices)
+    {
+        $this->decoratedRememberMeServices = $decoratedRememberMeServices;
+    }
+
+    public function loginSuccess(Request $request, Response $response, TokenInterface $token)
+    {
+        if ($token instanceof TwoFactorTokenInterface) {
+            // Create a fake response to capture the remember-me cookie but not let it leak to the real response.
+            $cookieCaptureResponse = new Response();
+            $this->decoratedRememberMeServices->loginSuccess($request, $cookieCaptureResponse, $token);
+            $rememberMeCookies = $cookieCaptureResponse->headers->getCookies();
+            $token->setAttribute(TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE, $rememberMeCookies);
+        } else {
+            // Not a TwoFactorToken => default behaviour
+            $this->decoratedRememberMeServices->loginSuccess($request, $response, $token);
+        }
+    }
+
+    public function autoLogin(Request $request)
+    {
+        return $this->decoratedRememberMeServices->autoLogin($request);
+    }
+
+    public function loginFail(Request $request, \Exception $exception = null)
+    {
+        $this->decoratedRememberMeServices->loginFail($request, $exception);
+    }
+
+    public function logout(Request $request, Response $response, TokenInterface $token)
+    {
+        $this->decoratedRememberMeServices->logout($request, $response, $token);
+    }
+}

Security/Authentication/Token/TwoFactorTokenInterface.php

@@ -6,6 +6,8 @@
 
 interface TwoFactorTokenInterface extends TokenInterface
 {
+    public const ATTRIBUTE_NAME_REMEMBER_ME_COOKIE = 'remember_me_cookie';
+
     /**
      * Return the authenticated token.
      *

Security/Http/Firewall/TwoFactorListener.php

@@ -207,7 +207,7 @@ private function attemptAuthentication(Request $request, TwoFactorTokenInterface
             $this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::ATTEMPT, $request, $token);
             $resultToken = $this->authenticationManager->authenticate($token);
 
-            return $this->onSuccess($request, $resultToken);
+            return $this->onSuccess($request, $resultToken, $currentToken);
         } catch (AuthenticationException $failed) {
             return $this->onFailure($request, $failed);
         }
@@ -223,7 +223,7 @@ private function onFailure(Request $request, AuthenticationException $failed): R
         return $this->failureHandler->onAuthenticationFailure($request, $failed);
     }
 
-    private function onSuccess(Request $request, TokenInterface $token): Response
+    private function onSuccess(Request $request, TokenInterface $token, TwoFactorTokenInterface $previousTwoFactorToken): Response
     {
         if ($this->logger) {
             $this->logger->info('User has been two-factor authenticated successfully.', ['username' => $token->getUsername()]);
@@ -244,7 +244,10 @@ private function onSuccess(Request $request, TokenInterface $token): Response
             $this->trustedDeviceManager->addTrustedDevice($token->getUser(), $this->firewallName);
         }
 
-        return $this->successHandler->onAuthenticationSuccess($request, $token);
+        $response = $this->successHandler->onAuthenticationSuccess($request, $token);
+        $this->addRememberMeCookies($previousTwoFactorToken, $response);
+
+        return $response;
     }
 
     private function hasTrustedDeviceParameter(Request $request): bool
@@ -263,4 +266,15 @@ private function dispatchTwoFactorAuthenticationEvent(string $eventType, Request
             $this->eventDispatcher->dispatch($eventType, $event);
         }
     }
+
+    private function addRememberMeCookies(TwoFactorTokenInterface $twoFactorToken, Response $response): void
+    {
+        // Add the remember-me cookie that was previously suppressed by two-factor authentication
+        if ($twoFactorToken->hasAttribute(TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE)) {
+            $rememberMeCookies = $twoFactorToken->getAttribute(TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE);
+            foreach ($rememberMeCookies as $cookie) {
+                $response->headers->setCookie($cookie);
+            }
+        }
+    }
 }

Tests/DependencyInjection/Compiler/RememberMeServicesDecoratorCompilerPassTest.php

@@ -0,0 +1,76 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Scheb\TwoFactorBundle\Tests\DependencyInjection\Compiler;
+
+use Scheb\TwoFactorBundle\DependencyInjection\Compiler\AuthenticationProviderDecoratorCompilerPass;
+use Scheb\TwoFactorBundle\DependencyInjection\Compiler\RememberMeServicesDecoratorCompilerPass;
+use Scheb\TwoFactorBundle\Tests\TestCase;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\Security\Http\Firewall\RememberMeListener;
+use Symfony\Component\Security\Http\RememberMe\AbstractRememberMeServices;
+
+class RememberMeServicesDecoratorCompilerPassTest extends TestCase
+{
+    /**
+     * @var ContainerBuilder
+     */
+    private $container;
+
+    /**
+     * @var AuthenticationProviderDecoratorCompilerPass
+     */
+    private $compilerPass;
+
+    protected function setUp(): void
+    {
+        $this->container = new ContainerBuilder();
+        $this->compilerPass = new RememberMeServicesDecoratorCompilerPass();
+    }
+
+    private function stubRememberMeListenersWithServices(array $firewalls): void
+    {
+        foreach ($firewalls as $firewallName) {
+            $rememberMeServicesId = 'rememberme_services.'.$firewallName;
+            $rememberMeServicesDefinition = new Definition(AbstractRememberMeServices::class);
+            $this->container->setDefinition($rememberMeServicesId, $rememberMeServicesDefinition);
+
+            $listenerId = 'security.authentication.listener.rememberme.'.$firewallName;
+            $listenerDefinition = new Definition(RememberMeListener::class);
+            $listenerDefinition->setArgument(1, new Reference($rememberMeServicesId));
+            $this->container->setDefinition($listenerId, $listenerDefinition);
+        }
+    }
+
+    private function assertContainerHasDecoratedProvider(string $rememberMeServicesId): void
+    {
+        $expectedDecoratorId = $rememberMeServicesId.'.two_factor_decorator';
+        $expectedDecoratedId = $expectedDecoratorId.'.inner';
+
+        $this->assertTrue($this->container->hasDefinition($expectedDecoratorId), 'Must have service "'.$expectedDecoratorId.'" defined.');
+
+        $decoratorDefinition = $this->container->getDefinition($expectedDecoratorId);
+        $decoratedServiceReference = $decoratorDefinition->getArgument(0);
+        $this->assertEquals($expectedDecoratedId, (string) $decoratedServiceReference);
+        $this->assertEquals($rememberMeServicesId, $decoratorDefinition->getDecoratedService()[0]);
+    }
+
+    /**
+     * @test
+     */
+    public function process_hasMultipleRemembermeServices_decorateAll(): void
+    {
+        $this->stubRememberMeListenersWithServices([
+            'firewall1',
+            'firewall2',
+        ]);
+
+        $this->compilerPass->process($this->container);
+
+        $this->assertContainerHasDecoratedProvider('rememberme_services.firewall1');
+        $this->assertContainerHasDecoratedProvider('rememberme_services.firewall2');
+    }
+}

Tests/SchebTwoFactorBundleTest.php

@@ -5,6 +5,7 @@
 namespace Scheb\TwoFactorBundle\Tests;
 
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\AuthenticationProviderDecoratorCompilerPass;
+use Scheb\TwoFactorBundle\DependencyInjection\Compiler\RememberMeServicesDecoratorCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\TwoFactorFirewallConfigCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Compiler\TwoFactorProviderCompilerPass;
 use Scheb\TwoFactorBundle\DependencyInjection\Factory\Security\TwoFactorFactory;
@@ -23,10 +24,11 @@ public function build_initializeBundle_addCompilerPass(): void
 
         //Expect compiler pass to be added
         $containerBuilder
-            ->expects($this->exactly(3))
+            ->expects($this->exactly(4))
             ->method('addCompilerPass')
             ->with($this->logicalOr(
                 $this->isInstanceOf(AuthenticationProviderDecoratorCompilerPass::class),
+                $this->isInstanceOf(RememberMeServicesDecoratorCompilerPass::class),
                 $this->isInstanceOf(TwoFactorProviderCompilerPass::class),
                 $this->isInstanceOf(TwoFactorFirewallConfigCompilerPass::class)
             ));

Tests/Security/Authentication/RememberMe/RememberMeServicesDecoratorTest.php

@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Scheb\TwoFactorBundle\Tests\Security\Authentication\RememberMe;
+
+use PHPUnit\Framework\MockObject\MockObject;
+use Scheb\TwoFactorBundle\Security\Authentication\RememberMe\RememberMeServicesDecorator;
+use Scheb\TwoFactorBundle\Security\Authentication\Token\TwoFactorTokenInterface;
+use Scheb\TwoFactorBundle\Tests\TestCase;
+use Symfony\Component\HttpFoundation\Cookie;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Http\RememberMe\RememberMeServicesInterface;
+
+class RememberMeServicesDecoratorTest extends TestCase
+{
+    /**
+     * @var MockObject|Request
+     */
+    private $request;
+
+    /**
+     * @var MockObject|Response
+     */
+    private $response;
+
+    /**
+     * @var MockObject|RememberMeServicesInterface
+     */
+    private $innerRememberMeServices;
+
+    /**
+     * @var RememberMeServicesDecorator
+     */
+    private $decorator;
+
+    protected function setUp(): void
+    {
+        $this->request = $this->createMock(Request::class);
+        $this->response = $this->createMock(Response::class);
+        $this->innerRememberMeServices = $this->createMock(RememberMeServicesInterface::class);
+        $this->decorator = new RememberMeServicesDecorator($this->innerRememberMeServices);
+    }
+
+    /**
+     * @test
+     */
+    public function loginSuccess_noATwoFactorToken_forwardCall()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $this->innerRememberMeServices
+            ->expects($this->once())
+            ->method('loginSuccess')
+            ->with(
+                $this->identicalTo($this->request),
+                $this->identicalTo($this->response),
+                $this->identicalTo($token)
+            );
+
+        $this->decorator->loginSuccess($this->request, $this->response, $token);
+    }
+
+    /**
+     * @test
+     */
+    public function loginSuccess_isTwoFactorToken_setRememberMeAttribute()
+    {
+        $token = $this->createMock(TwoFactorTokenInterface::class);
+
+        $responseCallback = function ($argument) {
+            /** @var Response $argument */
+            $this->assertInstanceOf(Response::class, $argument);
+            $this->assertFalse($argument === $this->response, 'Response objects must NOT be identical');
+            $argument->headers->setCookie(new Cookie('name', 'value'));
+
+            return true;
+        };
+
+        $this->innerRememberMeServices
+            ->expects($this->once())
+            ->method('loginSuccess')
+            ->with(
+                $this->identicalTo($this->request),
+                $this->callback($responseCallback), // 2nd argument is a different Response instance
+                $this->identicalTo($token)
+            );
+
+        $token
+            ->expects($this->once())
+            ->method('setAttribute')
+            ->with(TwoFactorTokenInterface::ATTRIBUTE_NAME_REMEMBER_ME_COOKIE, $this->callback(function ($argument) {
+                $this->assertContainsOnlyInstancesOf(Cookie::class, $argument);
+
+                return true;
+            }));
+
+        $this->decorator->loginSuccess($this->request, $this->response, $token);
+    }
+}