CVE-2019-10642

@@ -2,6 +2,7 @@
 
 ## DEV
 
+ * Fix the request token check in the front end (see CVE-2019-10642).
  * Invalidate old opt-in tokens when a token is confirmed (see CVE-2019-10643).
  * Invalidate the user sessions if a password changes (see CVE-2019-10641).
  * Correctly check if a file or folder is excluded from synchronization (see 410).

@@ -0,0 +1,111 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CoreBundle\EventListener;
+
+use Contao\Config;
+use Contao\CoreBundle\Exception\InvalidRequestTokenException;
+use Contao\CoreBundle\Framework\ContaoFramework;
+use Contao\CoreBundle\Routing\ScopeMatcher;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\Security\Csrf\CsrfToken;
+use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
+
+/**
+ * Validates the request token if the request is a Contao request.
+ */
+class RequestTokenListener
+{
+    /**
+     * @var ContaoFramework
+     */
+    private $framework;
+
+    /**
+     * @var ScopeMatcher
+     */
+    private $scopeMatcher;
+
+    /**
+     * @var CsrfTokenManagerInterface
+     */
+    private $csrfTokenManager;
+
+    /**
+     * @var string
+     */
+    private $csrfTokenName;
+
+    public function __construct(ContaoFramework $framework, ScopeMatcher $scopeMatcher, CsrfTokenManagerInterface $csrfTokenManager, string $csrfTokenName)
+    {
+        $this->framework = $framework;
+        $this->scopeMatcher = $scopeMatcher;
+        $this->csrfTokenManager = $csrfTokenManager;
+        $this->csrfTokenName = $csrfTokenName;
+    }
+
+    /**
+     * @throws InvalidRequestTokenException
+     */
+    public function onKernelRequest(GetResponseEvent $event): void
+    {
+        $request = $event->getRequest();
+
+        // Only check the request token if a) the request is a POST request, b)
+        // the request is not an Ajax request, c) the _token_check attribute is
+        // not false and d) the _token_check attribute is set or the request is
+        // a Contao request
+        if (
+            'POST' !== $request->getRealMethod()
+            || $request->isXmlHttpRequest()
+            || false === $request->attributes->get('_token_check')
+            || (!$request->attributes->has('_token_check') && !$this->scopeMatcher->isContaoRequest($request))
+        ) {
+            return;
+        }
+
+        /** @var Config $config */
+        $config = $this->framework->getAdapter(Config::class);
+
+        if (\defined('BYPASS_TOKEN_CHECK')) {
+            @trigger_error('Defining the BYPASS_TOKEN_CHECK constant has been deprecated and will no longer work in Contao 5.0.', E_USER_DEPRECATED);
+
+            return;
+        }
+
+        if ($config->get('disableRefererCheck')) {
+            @trigger_error('Using the "disableRefererCheck" setting has been deprecated and will no longer work in Contao 5.0.', E_USER_DEPRECATED);
+
+            return;
+        }
+
+        if ($config->get('requestTokenWhitelist')) {
+            @trigger_error('Using the "requestTokenWhitelist" setting has been deprecated and will no longer work in Contao 5.0.', E_USER_DEPRECATED);
+
+            $hostname = gethostbyaddr($request->getClientIp());
+
+            foreach ($config->get('requestTokenWhitelist') as $domain) {
+                if ($domain === $hostname || preg_match('/\.' . preg_quote($domain, '/') . '$/', $hostname)) {
+                    return;
+                }
+            }
+        }
+
+        $token = new CsrfToken($this->csrfTokenName, $request->request->get('REQUEST_TOKEN'));
+
+        if ($this->csrfTokenManager->isTokenValid($token)) {
+            return;
+        }
+
+        throw new InvalidRequestTokenException('Invalid CSRF token. Please reload the page and try again.');
+    }
+}

@@ -15,7 +15,6 @@
 use Contao\ClassLoader;
 use Contao\Config;
 use Contao\CoreBundle\Exception\IncompleteInstallationException;
-use Contao\CoreBundle\Exception\InvalidRequestTokenException;
 use Contao\CoreBundle\Routing\ScopeMatcher;
 use Contao\CoreBundle\Security\Authentication\Token\TokenChecker;
 use Contao\CoreBundle\Session\LazySessionAccess;
@@ -383,9 +382,6 @@ private function triggerInitializeSystemHook(): void
         }
     }
 
-    /**
-     * @throws InvalidRequestTokenException
-     */
     private function handleRequestToken(): void
     {
         /** @var RequestToken $requestToken */
@@ -395,12 +391,6 @@ private function handleRequestToken(): void
         if (!\defined('REQUEST_TOKEN')) {
             \define('REQUEST_TOKEN', 'cli' === \PHP_SAPI ? null : $requestToken->get());
         }
-
-        if ($this->canSkipTokenCheck() || $requestToken->validate($this->request->request->get('REQUEST_TOKEN'))) {
-            return;
-        }
-
-        throw new InvalidRequestTokenException('Invalid request token. Please reload the page and try again.');
     }
 
     private function iniSet(string $key, string $value): void
@@ -419,16 +409,6 @@ private function getSession(): ?SessionInterface
         return $this->request->getSession();
     }
 
-    private function canSkipTokenCheck(): bool
-    {
-        return null === $this->request
-            || 'POST' !== $this->request->getRealMethod()
-            || $this->request->isXmlHttpRequest()
-            || !$this->request->attributes->has('_token_check')
-            || false === $this->request->attributes->get('_token_check')
-        ;
-    }
-
     private function registerHookListeners(): void
     {
         foreach ($this->hookListeners as $hookName => $priorities) {

@@ -150,6 +150,17 @@ services:
             # The priority must be lower than the one of the Symfony route listener (defaults to 32)
             - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest, priority: 20 }
 
+    contao.listener.request_token:
+        class: Contao\CoreBundle\EventListener\RequestTokenListener
+        arguments:
+            - "@contao.framework"
+            - "@contao.routing.scope_matcher"
+            - "@contao.csrf.token_manager"
+            - "%contao.csrf_token_name%"
+        tags:
+            # The priority must be lower than the one of the Symfony route listener (defaults to 32)
+            - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest, priority: 30 }
+
     contao.listener.response_exception:
         class: Contao\CoreBundle\EventListener\ResponseExceptionListener
         tags:

@@ -49,6 +49,7 @@
 use Contao\CoreBundle\EventListener\MergeHttpHeadersListener;
 use Contao\CoreBundle\EventListener\PrettyErrorScreenListener;
 use Contao\CoreBundle\EventListener\RefererIdListener;
+use Contao\CoreBundle\EventListener\RequestTokenListener;
 use Contao\CoreBundle\EventListener\ResponseExceptionListener;
 use Contao\CoreBundle\EventListener\StoreRefererListener;
 use Contao\CoreBundle\EventListener\SwitchUserListener;
@@ -119,7 +120,12 @@
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
 use Symfony\Component\DependencyInjection\ServiceLocator;
 use Symfony\Component\HttpFoundation\RequestMatcher;
+use Symfony\Component\HttpKernel\EventListener\AbstractSessionListener;
+use Symfony\Component\HttpKernel\EventListener\ExceptionListener;
+use Symfony\Component\HttpKernel\EventListener\LocaleListener as BaseLocaleListener;
+use Symfony\Component\HttpKernel\EventListener\RouterListener;
 use Symfony\Component\Security\Csrf\CsrfTokenManager;
+use Symfony\Component\Security\Http\Firewall;
 
 class ContaoCoreExtensionTest extends TestCase
 {
@@ -161,6 +167,34 @@ public function testReturnsTheCorrectAlias(): void
         $this->assertSame('contao', $extension->getAlias());
     }
 
+    public function testValidatesTheSymfonyListenerPriorities(): void
+    {
+        $events = AbstractSessionListener::getSubscribedEvents();
+
+        $this->assertSame('onKernelResponse', $events['kernel.response'][0]);
+        $this->assertSame(-1000, $events['kernel.response'][1]);
+
+        $events = BaseLocaleListener::getSubscribedEvents();
+
+        $this->assertSame('onKernelRequest', $events['kernel.request'][0][0]);
+        $this->assertSame(16, $events['kernel.request'][0][1]);
+
+        $events = ExceptionListener::getSubscribedEvents();
+
+        $this->assertSame('onKernelException', $events['kernel.exception'][1][0]);
+        $this->assertSame(-128, $events['kernel.exception'][1][1]);
+
+        $events = Firewall::getSubscribedEvents();
+
+        $this->assertSame('onKernelRequest', $events['kernel.request'][0]);
+        $this->assertSame(8, $events['kernel.request'][1]);
+
+        $events = RouterListener::getSubscribedEvents();
+
+        $this->assertSame('onKernelRequest', $events['kernel.request'][0][0]);
+        $this->assertSame(32, $events['kernel.request'][0][1]);
+    }
+
     /**
      * @dataProvider getCommandTestData
      */
@@ -508,6 +542,27 @@ public function testRegistersTheRefererIdListener(): void
         $this->assertSame(20, $tags['kernel.event_listener'][0]['priority']);
     }
 
+    public function testRegistersTheRequestTokenListener(): void
+    {
+        $this->assertTrue($this->container->has('contao.listener.request_token'));
+
+        $definition = $this->container->getDefinition('contao.listener.request_token');
+
+        $this->assertSame(RequestTokenListener::class, $definition->getClass());
+        $this->assertTrue($definition->isPrivate());
+        $this->assertSame('contao.framework', (string) $definition->getArgument(0));
+        $this->assertSame('contao.routing.scope_matcher', (string) $definition->getArgument(1));
+        $this->assertSame('contao.csrf.token_manager', (string) $definition->getArgument(2));
+        $this->assertSame('%contao.csrf_token_name%', (string) $definition->getArgument(3));
+
+        $tags = $definition->getTags();
+
+        $this->assertArrayHasKey('kernel.event_listener', $tags);
+        $this->assertSame('kernel.request', $tags['kernel.event_listener'][0]['event']);
+        $this->assertSame('onKernelRequest', $tags['kernel.event_listener'][0]['method']);
+        $this->assertSame(30, $tags['kernel.event_listener'][0]['priority']);
+    }
+
     public function testRegistersTheResponseExceptionListener(): void
     {
         $this->assertTrue($this->container->has('contao.listener.response_exception'));

@@ -0,0 +1,227 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CoreBundle\Tests\EventListener;
+
+use Contao\Config;
+use Contao\CoreBundle\EventListener\RequestTokenListener;
+use Contao\CoreBundle\Exception\InvalidRequestTokenException;
+use Contao\CoreBundle\Routing\ScopeMatcher;
+use Contao\CoreBundle\Tests\TestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
+
+class RequestTokenListenerTest extends TestCase
+{
+    public function testValidatesTheRequestToken(): void
+    {
+        $config = $this->mockConfiguredAdapter(['get' => false]);
+        $framework = $this->mockContaoFramework([Config::class => $config]);
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+        $csrfTokenManager
+            ->expects($this->once())
+            ->method('isTokenValid')
+            ->willReturn(true)
+        ;
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+        $request->attributes->set('_token_check', true);
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+
+    public function testValidatesTheRequestTokenUponContaoRequests(): void
+    {
+        $config = $this->mockConfiguredAdapter(['get' => false]);
+        $framework = $this->mockContaoFramework([Config::class => $config]);
+
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+        $scopeMatcher
+            ->expects($this->once())
+            ->method('isContaoRequest')
+            ->willReturn(true)
+        ;
+
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+        $csrfTokenManager
+            ->expects($this->once())
+            ->method('isTokenValid')
+            ->willReturn(true)
+        ;
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+
+    public function testFailsIfTheRequestTokenIsInvalid(): void
+    {
+        $config = $this->mockConfiguredAdapter(['get' => false]);
+        $framework = $this->mockContaoFramework([Config::class => $config]);
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+        $csrfTokenManager
+            ->expects($this->once())
+            ->method('isTokenValid')
+            ->willReturn(false)
+        ;
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+        $request->attributes->set('_token_check', true);
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+
+        $this->expectException(InvalidRequestTokenException::class);
+
+        $listener->onKernelRequest($event);
+    }
+
+    public function testDoesNotValidateTheRequestTokenUponNonPostRequests(): void
+    {
+        $framework = $this->mockContaoFramework();
+        $framework
+            ->expects($this->never())
+            ->method('getAdapter')
+        ;
+
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+
+        $request = Request::create('/account.html');
+        $request->setMethod('GET');
+        $request->attributes->set('_token_check', true);
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+
+    public function testDoesNotValidateTheRequestTokenUponAjaxRequests(): void
+    {
+        $framework = $this->mockContaoFramework();
+        $framework
+            ->expects($this->never())
+            ->method('getAdapter')
+        ;
+
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+        $request->attributes->set('_token_check', true);
+        $request->headers->set('X-Requested-With', 'XMLHttpRequest');
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+
+    public function testDoesNotValidateTheRequestTokenIfTheRequestAttributeIsFalse(): void
+    {
+        $framework = $this->mockContaoFramework();
+        $framework
+            ->expects($this->never())
+            ->method('getAdapter')
+        ;
+
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+        $request->attributes->set('_token_check', false);
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+
+    public function testDoesNotValidateTheRequestTokenIfNoRequestAttributeAndNotAContaoRequest(): void
+    {
+        $framework = $this->mockContaoFramework();
+        $framework
+            ->expects($this->never())
+            ->method('getAdapter')
+        ;
+
+        $scopeMatcher = $this->createMock(ScopeMatcher::class);
+        $scopeMatcher
+            ->expects($this->once())
+            ->method('isContaoRequest')
+            ->willReturn(false)
+        ;
+
+        $csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
+
+        $request = Request::create('/account.html');
+        $request->setMethod('POST');
+
+        $event = $this->createMock(GetResponseEvent::class);
+        $event
+            ->expects($this->once())
+            ->method('getRequest')
+            ->willReturn($request)
+        ;
+
+        $listener = new RequestTokenListener($framework, $scopeMatcher, $csrfTokenManager, 'contao_csrf_token');
+        $listener->onKernelRequest($event);
+    }
+}

@@ -328,142 +328,6 @@ public function testOverridesTheErrorLevel(): void
         error_reporting($errorReporting);
     }
 
-    public function testValidatesTheRequestToken(): void
-    {
-        $request = Request::create('/contao/login');
-        $request->attributes->set('_route', 'dummy');
-        $request->attributes->set('_token_check', true);
-        $request->setMethod('POST');
-        $request->request->set('REQUEST_TOKEN', 'foobar');
-
-        $framework = $this->mockFramework($request);
-        $framework->setContainer($this->mockContainer());
-        $framework->initialize();
-
-        $this->addToAssertionCount(1);  // does not throw an exception
-    }
-
-    /**
-     * @runInSeparateProcess
-     * @preserveGlobalState disabled
-     */
-    public function testFailsIfTheRequestTokenIsInvalid(): void
-    {
-        $request = Request::create('/contao/login');
-        $request->attributes->set('_route', 'dummy');
-        $request->attributes->set('_token_check', true);
-        $request->setMethod('POST');
-        $request->request->set('REQUEST_TOKEN', 'invalid');
-
-        $requestStack = new RequestStack();
-        $requestStack->push($request);
-
-        $framework = new ContaoFramework(
-            $requestStack,
-            $this->mockScopeMatcher(),
-            $this->createMock(TokenChecker::class),
-            $this->getTempDir(),
-            error_reporting()
-        );
-
-        $framework->setContainer($this->mockContainer());
-
-        $adapters = [
-            Config::class => $this->mockConfigAdapter(),
-            RequestToken::class => $this->mockRequestTokenAdapter(false),
-        ];
-
-        $ref = new \ReflectionObject($framework);
-        $adapterCache = $ref->getProperty('adapterCache');
-        $adapterCache->setAccessible(true);
-        $adapterCache->setValue($framework, $adapters);
-
-        $this->expectException(InvalidRequestTokenException::class);
-
-        $framework->initialize();
-    }
-
-    public function testDoesNotValidateTheRequestTokenUponAjaxRequests(): void
-    {
-        $request = Request::create('/contao/login');
-        $request->attributes->set('_route', 'dummy');
-        $request->attributes->set('_token_check', true);
-        $request->setMethod('POST');
-        $request->headers->set('X-Requested-With', 'XMLHttpRequest');
-
-        $requestStack = new RequestStack();
-        $requestStack->push($request);
-
-        $framework = new ContaoFramework(
-            $requestStack,
-            $this->mockScopeMatcher(),
-            $this->createMock(TokenChecker::class),
-            $this->getTempDir(),
-            error_reporting()
-        );
-
-        $framework->setContainer($this->mockContainer());
-
-        $adapters = [
-            Config::class => $this->mockConfigAdapter(),
-            RequestToken::class => $this->mockRequestTokenAdapter(false),
-        ];
-
-        $ref = new \ReflectionObject($framework);
-        $adapterCache = $ref->getProperty('adapterCache');
-        $adapterCache->setAccessible(true);
-        $adapterCache->setValue($framework, $adapters);
-
-        $framework->initialize();
-
-        $this->addToAssertionCount(1);  // does not throw an exception
-    }
-
-    public function testDoesNotValidateTheRequestTokenIfTheRequestAttributeIsFalse(): void
-    {
-        $request = Request::create('/contao/login');
-        $request->attributes->set('_route', 'dummy');
-        $request->attributes->set('_token_check', false);
-        $request->setMethod('POST');
-        $request->request->set('REQUEST_TOKEN', 'foobar');
-
-        $requestStack = new RequestStack();
-        $requestStack->push($request);
-
-        $framework = new ContaoFramework(
-            $requestStack,
-            $this->mockScopeMatcher(),
-            $this->createMock(TokenChecker::class),
-            $this->getTempDir(),
-            error_reporting()
-        );
-
-        $framework->setContainer($this->mockContainer());
-
-        $adapter = $this->mockAdapter(['get', 'validate']);
-        $adapter
-            ->method('get')
-            ->willReturn('foobar')
-        ;
-
-        $adapter
-            ->expects($this->never())
-            ->method('validate')
-        ;
-
-        $adapters = [
-            Config::class => $this->mockConfigAdapter(),
-            RequestToken::class => $adapter,
-        ];
-
-        $ref = new \ReflectionObject($framework);
-        $adapterCache = $ref->getProperty('adapterCache');
-        $adapterCache->setAccessible(true);
-        $adapterCache->setValue($framework, $adapters);
-
-        $framework->initialize();
-    }
-
     /**
      * @runInSeparateProcess
      * @preserveGlobalState disabled