security #24995 Validate redirect targets using the session cookie do…

…main (nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

Validate redirect targets using the session cookie domain

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
- Please fill in this template according to the PR you're about to submit.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

52b06f1 [Security] Validate redirect targets using the session cookie domain

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php

@@ -0,0 +1,40 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+
+/**
+ * Uses the session domain to restrict allowed redirection targets.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class AddSessionDomainConstraintPass implements CompilerPassInterface
+{
+    /**
+     * {@inheritdoc}
+     */
+    public function process(ContainerBuilder $container)
+    {
+        if (!$container->hasParameter('session.storage.options') || !$container->has('security.http_utils')) {
+            return;
+        }
+
+        $sessionOptions = $container->getParameter('session.storage.options');
+        $domainRegexp = empty($sessionOptions['cookie_domain']) ? '%s' : sprintf('(?:%%s|(?:.+\.)?%s)', preg_quote(trim($sessionOptions['cookie_domain'], '.')));
+        $domainRegexp = (empty($sessionOptions['cookie_secure']) ? 'https?://' : 'https://').$domainRegexp;
+
+        $container->findDefinition('security.http_utils')->addArgument(sprintf('{^%s$}i', $domainRegexp));
+    }
+}

src/Symfony/Bundle/SecurityBundle/SecurityBundle.php

@@ -12,8 +12,10 @@
 namespace Symfony\Bundle\SecurityBundle;
 
 use Symfony\Component\HttpKernel\Bundle\Bundle;
+use Symfony\Component\DependencyInjection\Compiler\PassConfig;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSessionDomainConstraintPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpBasicFactory;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpDigestFactory;
@@ -47,5 +49,6 @@ public function build(ContainerBuilder $container)
 
         $extension->addUserProviderFactory(new InMemoryFactory());
         $container->addCompilerPass(new AddSecurityVotersPass());
+        $container->addCompilerPass(new AddSessionDomainConstraintPass(), PassConfig::TYPE_AFTER_REMOVING);
     }
 }

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php

@@ -0,0 +1,131 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\Compiler;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSessionDomainConstraintPass;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\HttpFoundation\Request;
+
+class AddSessionDomainConstraintPassTest extends TestCase
+{
+    public function testSessionCookie()
+    {
+        $container = $this->createContainer(array('cookie_domain' => '.symfony.com.', 'cookie_secure' => true));
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('https://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('https://www.symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://localhost/foo')->isRedirect('https://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.localhost/foo')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+    }
+
+    public function testSessionNoDomain()
+    {
+        $container = $this->createContainer(array('cookie_secure' => true));
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://localhost/foo')->isRedirect('https://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.localhost/foo')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+    }
+
+    public function testSessionNoSecure()
+    {
+        $container = $this->createContainer(array('cookie_domain' => '.symfony.com.'));
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('https://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('https://www.symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://localhost/foo')->isRedirect('https://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.localhost/foo')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+    }
+
+    public function testSessionNoSecureAndNoDomain()
+    {
+        $container = $this->createContainer(array());
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://localhost/foo')->isRedirect('https://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://localhost/foo')->isRedirect('http://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.localhost/foo')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://localhost/'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://localhost/'));
+    }
+
+    public function testNoSession()
+    {
+        $container = $this->createContainer(null);
+
+        $utils = $container->get('security.http_utils');
+        $request = Request::create('/', 'get');
+
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://symfony.com/blog')->isRedirect('https://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.symfony.com/blog')->isRedirect('https://www.symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://localhost/foo')->isRedirect('https://localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'https://www.localhost/foo')->isRedirect('https://www.localhost/foo'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://symfony.com/blog')->isRedirect('http://symfony.com/blog'));
+        $this->assertTrue($utils->createRedirectResponse($request, 'http://pirate.com/foo')->isRedirect('http://pirate.com/foo'));
+    }
+
+    private function createContainer($sessionStorageOptions)
+    {
+        $container = new ContainerBuilder();
+        $container->setParameter('kernel.cache_dir', __DIR__);
+        $container->setParameter('kernel.charset', 'UTF-8');
+        $container->setParameter('kernel.container_class', 'cc');
+        $container->setParameter('kernel.debug', true);
+        $container->setParameter('kernel.root_dir', __DIR__);
+        $container->setParameter('kernel.secret', __DIR__);
+        if (null !== $sessionStorageOptions) {
+            $container->setParameter('session.storage.options', $sessionStorageOptions);
+        }
+        $container->setParameter('request_listener.http_port', 80);
+        $container->setParameter('request_listener.https_port', 443);
+
+        $config = array(
+            'security' => array(
+                'providers' => array('some_provider' => array('id' => 'foo')),
+                'firewalls' => array('some_firewall' => array('security' => false)),
+            ),
+        );
+
+        $ext = new FrameworkExtension();
+        $ext->load(array(), $container);
+
+        $ext = new SecurityExtension();
+        $ext->load($config, $container);
+
+        (new AddSessionDomainConstraintPass())->process($container);
+
+        return $container;
+    }
+}

src/Symfony/Component/Security/Http/HttpUtils.php

@@ -29,20 +29,23 @@ class HttpUtils
 {
     private $urlGenerator;
     private $urlMatcher;
+    private $domainRegexp;
 
     /**
      * @param UrlGeneratorInterface                       $urlGenerator A UrlGeneratorInterface instance
      * @param UrlMatcherInterface|RequestMatcherInterface $urlMatcher   The URL or Request matcher
+     * @param string|null                                 $domainRegexp A regexp that the target of HTTP redirections must match, scheme included
      *
      * @throws \InvalidArgumentException
      */
-    public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatcher = null)
+    public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatcher = null, $domainRegexp = null)
     {
         $this->urlGenerator = $urlGenerator;
         if (null !== $urlMatcher && !$urlMatcher instanceof UrlMatcherInterface && !$urlMatcher instanceof RequestMatcherInterface) {
             throw new \InvalidArgumentException('Matcher must either implement UrlMatcherInterface or RequestMatcherInterface.');
         }
         $this->urlMatcher = $urlMatcher;
+        $this->domainRegexp = $domainRegexp;
     }
 
     /**
@@ -56,6 +59,10 @@ public function __construct(UrlGeneratorInterface $urlGenerator = null, $urlMatc
      */
     public function createRedirectResponse(Request $request, $path, $status = 302)
     {
+        if (null !== $this->domainRegexp && preg_match('#^https?://[^/]++#i', $path, $host) && !preg_match(sprintf($this->domainRegexp, preg_quote($request->getHttpHost())), $host[0])) {
+            $path = '/';
+        }
+
         return new RedirectResponse($this->generateUri($request, $path), $status);
     }
 

src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php

@@ -38,6 +38,38 @@ public function testCreateRedirectResponseWithAbsoluteUrl()
         $this->assertTrue($response->isRedirect('http://symfony.com/'));
     }
 
+    public function testCreateRedirectResponseWithDomainRegexp()
+    {
+        $utils = new HttpUtils($this->getUrlGenerator(), null, '#^https?://symfony\.com$#i');
+        $response = $utils->createRedirectResponse($this->getRequest(), 'http://symfony.com/blog');
+
+        $this->assertTrue($response->isRedirect('http://symfony.com/blog'));
+    }
+
+    public function testCreateRedirectResponseWithRequestsDomain()
+    {
+        $utils = new HttpUtils($this->getUrlGenerator(), null, '#^https?://%s$#i');
+        $response = $utils->createRedirectResponse($this->getRequest(), 'http://localhost/blog');
+
+        $this->assertTrue($response->isRedirect('http://localhost/blog'));
+    }
+
+    public function testCreateRedirectResponseWithBadRequestsDomain()
+    {
+        $utils = new HttpUtils($this->getUrlGenerator(), null, '#^https?://%s$#i');
+        $response = $utils->createRedirectResponse($this->getRequest(), 'http://pirate.net/foo');
+
+        $this->assertTrue($response->isRedirect('http://localhost/'));
+    }
+
+    public function testCreateRedirectResponseWithProtocolRelativeTarget()
+    {
+        $utils = new HttpUtils($this->getUrlGenerator(), null, '#^https?://%s$#i');
+        $response = $utils->createRedirectResponse($this->getRequest(), '//evil.com/do-bad-things');
+
+        $this->assertTrue($response->isRedirect('http://localhost//evil.com/do-bad-things'), 'Protocol-relative redirection should not be supported for security reasons');
+    }
+
     public function testCreateRedirectResponseWithRouteName()
     {
         $utils = new HttpUtils($urlGenerator = $this->getMockBuilder('Symfony\Component\Routing\Generator\UrlGeneratorInterface')->getMock());