From: Nicolas Grekas Date: Wed, 19 Jul 2017 11:06:40 +0200 Subject: [Security] Validate redirect targets using the session cookie domain [CVE-2017-16652] https://symfony.com/blog/cve-2017-16652-open-redirect-vulnerability-on-security-handlers Origin: upstream, https://github.com/symfony/symfony/commit/52b06f1c21c69a4557ab3716afe56dea4815dfdf --- .../Compiler/AddSessionDomainConstraintPass.php | 40 +++++++ .../Bundle/SecurityBundle/SecurityBundle.php | 3 + .../AddSessionDomainConstraintPassTest.php | 131 +++++++++++++++++++++ src/Symfony/Component/Security/Http/HttpUtils.php | 9 +- .../Security/Http/Tests/HttpUtilsTest.php | 32 +++++ 5 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php create mode 100644 src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php new file mode 100644 index 0000000..0adc143 --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSessionDomainConstraintPass.php @@ -0,0 +1,40 @@ + + * + * 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 + */ +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)); + } +} diff --git a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php index f2dfc99..61649f3 100644 --- a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php +++ b/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\FormLoginLdapFactory; use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpBasicFactory; @@ -55,5 +57,6 @@ class SecurityBundle extends Bundle $extension->addUserProviderFactory(new InMemoryFactory()); $extension->addUserProviderFactory(new LdapFactory()); $container->addCompilerPass(new AddSecurityVotersPass()); + $container->addCompilerPass(new AddSessionDomainConstraintPass(), PassConfig::TYPE_AFTER_REMOVING); } } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php new file mode 100644 index 0000000..f476b5e --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSessionDomainConstraintPassTest.php @@ -0,0 +1,131 @@ + + * + * 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; + } +} diff --git a/src/Symfony/Component/Security/Http/HttpUtils.php b/src/Symfony/Component/Security/Http/HttpUtils.php index ed737a2..adfac12 100644 --- a/src/Symfony/Component/Security/Http/HttpUtils.php +++ b/src/Symfony/Component/Security/Http/HttpUtils.php @@ -29,22 +29,25 @@ class HttpUtils { private $urlGenerator; private $urlMatcher; + private $domainRegexp; /** * Constructor. * * @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 ($urlMatcher !== null && !$urlMatcher instanceof UrlMatcherInterface && !$urlMatcher instanceof RequestMatcherInterface) { throw new \InvalidArgumentException('Matcher must either implement UrlMatcherInterface or RequestMatcherInterface.'); } $this->urlMatcher = $urlMatcher; + $this->domainRegexp = $domainRegexp; } /** @@ -58,6 +61,10 @@ class HttpUtils */ 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); } diff --git a/src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php b/src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php index 45a0281..93be3cf 100644 --- a/src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php +++ b/src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php @@ -37,6 +37,38 @@ class HttpUtilsTest extends \PHPUnit_Framework_TestCase $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->getMock('Symfony\Component\Routing\Generator\UrlGeneratorInterface'));