Skip to content

Validate redirect targets using the session cookie domain #24995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 16, 2017
Merged

Validate redirect targets using the session cookie domain #24995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions ...ony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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));
}
}
3 changes: 3 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
131 changes: 131 additions & 0 deletions .../SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
}
9 changes: 8 additions & 1 deletion src/Symfony/Component/Security/Http/HttpUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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);
}

Expand Down
32 changes: 32 additions & 0 deletions src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down