From: =?utf-8?q?K=C3=A9vin_Dunglas?= Date: Mon, 6 Nov 2017 23:19:48 +0100 Subject: [Security] Namespace generated CSRF tokens depending of the current scheme [CVE-2017-16653] https://symfony.com/blog/cve-2017-16653-csrf-protection-does-not-use-different-tokens-for-http-and-https Origin: backport, https://github.com/symfony/symfony/commit/cdb4271975667ed4a31365ab086dec63f25c1e1c --- .../Resources/config/security_csrf.xml | 1 + .../Component/Security/Csrf/CsrfTokenManager.php | 58 +++++-- .../Security/Csrf/Tests/CsrfTokenManagerTest.php | 193 ++++++++++++++------- 3 files changed, 172 insertions(+), 80 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/security_csrf.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/security_csrf.xml index dc57fd6..4bab610 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/security_csrf.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/security_csrf.xml @@ -20,6 +20,7 @@ + diff --git a/src/Symfony/Component/Security/Csrf/CsrfTokenManager.php b/src/Symfony/Component/Security/Csrf/CsrfTokenManager.php index cdda543..35da1ca 100644 --- a/src/Symfony/Component/Security/Csrf/CsrfTokenManager.php +++ b/src/Symfony/Component/Security/Csrf/CsrfTokenManager.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Security\Csrf; +use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\Security\Core\Exception\InvalidArgumentException; use Symfony\Component\Security\Csrf\TokenGenerator\UriSafeTokenGenerator; use Symfony\Component\Security\Csrf\TokenGenerator\TokenGeneratorInterface; use Symfony\Component\Security\Csrf\TokenStorage\NativeSessionTokenStorage; @@ -20,6 +22,7 @@ use Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface; * Default implementation of {@link CsrfTokenManagerInterface}. * * @author Bernhard Schussek + * @author Kévin Dunglas */ class CsrfTokenManager implements CsrfTokenManagerInterface { @@ -32,18 +35,39 @@ class CsrfTokenManager implements CsrfTokenManagerInterface * @var TokenStorageInterface */ private $storage; + private $namespace; /** - * Creates a new CSRF provider using PHP's native session storage. - * - * @param TokenGeneratorInterface|null $generator The token generator - * @param TokenStorageInterface|null $storage The storage for storing - * generated CSRF tokens + * @param null|string|RequestStack|callable $namespace + * * null: generates a namespace using $_SERVER['HTTPS'] + * * string: uses the given string + * * RequestStack: generates a namespace using the current master request + * * callable: uses the result of this callable (must return a string) */ - public function __construct(TokenGeneratorInterface $generator = null, TokenStorageInterface $storage = null) + public function __construct(TokenGeneratorInterface $generator = null, TokenStorageInterface $storage = null, $namespace = null) { $this->generator = $generator ?: new UriSafeTokenGenerator(); $this->storage = $storage ?: new NativeSessionTokenStorage(); + + $superGlobalNamespaceGenerator = function () { + return !empty($_SERVER['HTTPS']) && 'off' !== strtolower($_SERVER['HTTPS']) ? 'https-' : ''; + }; + + if (null === $namespace) { + $this->namespace = $superGlobalNamespaceGenerator; + } elseif ($namespace instanceof RequestStack) { + $this->namespace = function () use ($namespace, $superGlobalNamespaceGenerator) { + if ($request = $namespace->getMasterRequest()) { + return $request->isSecure() ? 'https-' : ''; + } + + return $superGlobalNamespaceGenerator(); + }; + } elseif (is_callable($namespace) || is_string($namespace)) { + $this->namespace = $namespace; + } else { + throw new InvalidArgumentException(sprintf('$namespace must be a string, a callable returning a string, null or an instance of "RequestStack". "%s" given.', gettype($namespace))); + } } /** @@ -51,12 +75,13 @@ class CsrfTokenManager implements CsrfTokenManagerInterface */ public function getToken($tokenId) { - if ($this->storage->hasToken($tokenId)) { - $value = $this->storage->getToken($tokenId); + $namespacedId = $this->getNamespace().$tokenId; + if ($this->storage->hasToken($namespacedId)) { + $value = $this->storage->getToken($namespacedId); } else { $value = $this->generator->generateToken(); - $this->storage->setToken($tokenId, $value); + $this->storage->setToken($namespacedId, $value); } return new CsrfToken($tokenId, $value); @@ -67,9 +92,10 @@ class CsrfTokenManager implements CsrfTokenManagerInterface */ public function refreshToken($tokenId) { + $namespacedId = $this->getNamespace().$tokenId; $value = $this->generator->generateToken(); - $this->storage->setToken($tokenId, $value); + $this->storage->setToken($namespacedId, $value); return new CsrfToken($tokenId, $value); } @@ -79,7 +105,7 @@ class CsrfTokenManager implements CsrfTokenManagerInterface */ public function removeToken($tokenId) { - return $this->storage->removeToken($tokenId); + return $this->storage->removeToken($this->getNamespace().$tokenId); } /** @@ -87,10 +113,16 @@ class CsrfTokenManager implements CsrfTokenManagerInterface */ public function isTokenValid(CsrfToken $token) { - if (!$this->storage->hasToken($token->getId())) { + $namespacedId = $this->getNamespace().$token->getId(); + if (!$this->storage->hasToken($namespacedId)) { return false; } - return hash_equals($this->storage->getToken($token->getId()), $token->getValue()); + return hash_equals($this->storage->getToken($namespacedId), $token->getValue()); + } + + private function getNamespace() + { + return is_callable($ns = $this->namespace) ? $ns() : $ns; } } diff --git a/src/Symfony/Component/Security/Csrf/Tests/CsrfTokenManagerTest.php b/src/Symfony/Component/Security/Csrf/Tests/CsrfTokenManagerTest.php index 3112038..59ab621 100644 --- a/src/Symfony/Component/Security/Csrf/Tests/CsrfTokenManagerTest.php +++ b/src/Symfony/Component/Security/Csrf/Tests/CsrfTokenManagerTest.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Security\Csrf\Tests; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Security\Csrf\CsrfToken; use Symfony\Component\Security\Csrf\CsrfTokenManager; @@ -20,145 +22,202 @@ use Symfony\Component\Security\Csrf\CsrfTokenManager; class CsrfTokenManagerTest extends \PHPUnit_Framework_TestCase { /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @dataProvider getManagerGeneratorAndStorage */ - private $generator; - - /** - * @var \PHPUnit_Framework_MockObject_MockObject - */ - private $storage; - - /** - * @var CsrfTokenManager - */ - private $manager; - - protected function setUp() - { - $this->generator = $this->getMock('Symfony\Component\Security\Csrf\TokenGenerator\TokenGeneratorInterface'); - $this->storage = $this->getMock('Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface'); - $this->manager = new CsrfTokenManager($this->generator, $this->storage); - } - - protected function tearDown() - { - $this->generator = null; - $this->storage = null; - $this->manager = null; - } - - public function testGetNonExistingToken() + public function testGetNonExistingToken($namespace, $manager, $storage, $generator) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('hasToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue(false)); - $this->generator->expects($this->once()) + $generator->expects($this->once()) ->method('generateToken') ->will($this->returnValue('TOKEN')); - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('setToken') - ->with('token_id', 'TOKEN'); + ->with($namespace.'token_id', 'TOKEN'); - $token = $this->manager->getToken('token_id'); + $token = $manager->getToken('token_id'); $this->assertInstanceOf('Symfony\Component\Security\Csrf\CsrfToken', $token); $this->assertSame('token_id', $token->getId()); $this->assertSame('TOKEN', $token->getValue()); } - public function testUseExistingTokenIfAvailable() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testUseExistingTokenIfAvailable($namespace, $manager, $storage) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('hasToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue(true)); - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('getToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue('TOKEN')); - $token = $this->manager->getToken('token_id'); + $token = $manager->getToken('token_id'); $this->assertInstanceOf('Symfony\Component\Security\Csrf\CsrfToken', $token); $this->assertSame('token_id', $token->getId()); $this->assertSame('TOKEN', $token->getValue()); } - public function testRefreshTokenAlwaysReturnsNewToken() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testRefreshTokenAlwaysReturnsNewToken($namespace, $manager, $storage, $generator) { - $this->storage->expects($this->never()) + $storage->expects($this->never()) ->method('hasToken'); - $this->generator->expects($this->once()) + $generator->expects($this->once()) ->method('generateToken') ->will($this->returnValue('TOKEN')); - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('setToken') - ->with('token_id', 'TOKEN'); + ->with($namespace.'token_id', 'TOKEN'); - $token = $this->manager->refreshToken('token_id'); + $token = $manager->refreshToken('token_id'); $this->assertInstanceOf('Symfony\Component\Security\Csrf\CsrfToken', $token); $this->assertSame('token_id', $token->getId()); $this->assertSame('TOKEN', $token->getValue()); } - public function testMatchingTokenIsValid() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testMatchingTokenIsValid($namespace, $manager, $storage) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('hasToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue(true)); - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('getToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue('TOKEN')); - $this->assertTrue($this->manager->isTokenValid(new CsrfToken('token_id', 'TOKEN'))); + $this->assertTrue($manager->isTokenValid(new CsrfToken('token_id', 'TOKEN'))); } - public function testNonMatchingTokenIsNotValid() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testNonMatchingTokenIsNotValid($namespace, $manager, $storage) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('hasToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue(true)); - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('getToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue('TOKEN')); - $this->assertFalse($this->manager->isTokenValid(new CsrfToken('token_id', 'FOOBAR'))); + $this->assertFalse($manager->isTokenValid(new CsrfToken('token_id', 'FOOBAR'))); } - public function testNonExistingTokenIsNotValid() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testNonExistingTokenIsNotValid($namespace, $manager, $storage) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('hasToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue(false)); - $this->storage->expects($this->never()) + $storage->expects($this->never()) ->method('getToken'); - $this->assertFalse($this->manager->isTokenValid(new CsrfToken('token_id', 'FOOBAR'))); + $this->assertFalse($manager->isTokenValid(new CsrfToken('token_id', 'FOOBAR'))); } - public function testRemoveToken() + /** + * @dataProvider getManagerGeneratorAndStorage + */ + public function testRemoveToken($namespace, $manager, $storage) { - $this->storage->expects($this->once()) + $storage->expects($this->once()) ->method('removeToken') - ->with('token_id') + ->with($namespace.'token_id') ->will($this->returnValue('REMOVED_TOKEN')); - $this->assertSame('REMOVED_TOKEN', $this->manager->removeToken('token_id')); + $this->assertSame('REMOVED_TOKEN', $manager->removeToken('token_id')); + } + + public function testNamespaced() + { + $generator = $this->getMockBuilder('Symfony\Component\Security\Csrf\TokenGenerator\TokenGeneratorInterface')->getMock(); + $storage = $this->getMockBuilder('Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface')->getMock(); + + $requestStack = new RequestStack(); + $requestStack->push(new Request(array(), array(), array(), array(), array(), array('HTTPS' => 'on'))); + + $manager = new CsrfTokenManager($generator, $storage, null, $requestStack); + + $token = $manager->getToken('foo'); + $this->assertSame('foo', $token->getId()); + } + + public function getManagerGeneratorAndStorage() + { + $data = array(); + + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('', new CsrfTokenManager($generator, $storage, ''), $storage, $generator); + + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('https-', new CsrfTokenManager($generator, $storage), $storage, $generator); + + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('aNamespace-', new CsrfTokenManager($generator, $storage, 'aNamespace-'), $storage, $generator); + + $requestStack = new RequestStack(); + $requestStack->push(new Request(array(), array(), array(), array(), array(), array('HTTPS' => 'on'))); + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('https-', new CsrfTokenManager($generator, $storage, $requestStack), $storage, $generator); + + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('generated-', new CsrfTokenManager($generator, $storage, function () { + return 'generated-'; + }), $storage, $generator); + + $requestStack = new RequestStack(); + $requestStack->push(new Request()); + list($generator, $storage) = $this->getGeneratorAndStorage(); + $data[] = array('', new CsrfTokenManager($generator, $storage, $requestStack), $storage, $generator); + + return $data; + } + + private function getGeneratorAndStorage() + { + return array( + $this->getMockBuilder('Symfony\Component\Security\Csrf\TokenGenerator\TokenGeneratorInterface')->getMock(), + $this->getMockBuilder('Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface')->getMock(), + ); + } + + public function setUp() + { + $_SERVER['HTTPS'] = 'on'; + } + + public function tearDown() + { + parent::tearDown(); + + unset($_SERVER['HTTPS']); } }