diff --git a/src/EventListener/SignedCookieListener.php b/src/EventListener/SignedCookieListener.php index aa2057e..48b9239 100644 --- a/src/EventListener/SignedCookieListener.php +++ b/src/EventListener/SignedCookieListener.php @@ -13,6 +13,9 @@ namespace Nelmio\SecurityBundle\EventListener; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTracker; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTrackerInterface; +use Nelmio\SecurityBundle\SignedCookie\SignatureUpgradeCheckerInterface; use Nelmio\SecurityBundle\Signer\SignerInterface; use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpKernel\Event\RequestEvent; @@ -27,12 +30,15 @@ final class SignedCookieListener */ private $signedCookieNames; + private LegacySignatureCookieTrackerInterface $legacySignatureCookieTracker; + /** * @param list $signedCookieNames */ - public function __construct(SignerInterface $signer, array $signedCookieNames) + public function __construct(SignerInterface $signer, array $signedCookieNames, ?LegacySignatureCookieTrackerInterface $legacySignatureCookieTracker = null) { $this->signer = $signer; + $this->legacySignatureCookieTracker = $legacySignatureCookieTracker ?? new LegacySignatureCookieTracker(); if (\in_array('*', $signedCookieNames, true)) { $this->signedCookieNames = true; } else { @@ -46,17 +52,25 @@ public function onKernelRequest(RequestEvent $e): void return; } + $this->legacySignatureCookieTracker->clear(); + $request = $e->getRequest(); $names = true === $this->signedCookieNames ? $request->cookies->keys() : $this->signedCookieNames; foreach ($names as $name) { - if ($request->cookies->has($name)) { - $cookie = $request->cookies->get($name); - if ($this->signer->verifySignedValue($cookie)) { - $request->cookies->set($name, $this->signer->getVerifiedRawValue($cookie)); - } else { - $request->cookies->remove($name); + if (!$request->cookies->has($name)) { + continue; + } + + $cookie = $request->cookies->get($name); + if ($this->signer->verifySignedValue($cookie)) { + $request->cookies->set($name, $this->signer->getVerifiedRawValue($cookie)); + + if ($this->signer instanceof SignatureUpgradeCheckerInterface && $this->signer->needsUpgrade($cookie)) { + $this->legacySignatureCookieTracker->flagForUpgrade($name); } + } else { + $request->cookies->remove($name); } } } diff --git a/src/EventListener/SignedCookieUpgradeListener.php b/src/EventListener/SignedCookieUpgradeListener.php new file mode 100644 index 0000000..12ab06b --- /dev/null +++ b/src/EventListener/SignedCookieUpgradeListener.php @@ -0,0 +1,69 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\EventListener; + +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTrackerInterface; +use Nelmio\SecurityBundle\SignedCookie\UpgradedCookieBuilderInterface; +use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\Event\ResponseEvent; + +class SignedCookieUpgradeListener +{ + private LegacySignatureCookieTrackerInterface $legacySignatureCookieTracker; + + private UpgradedCookieBuilderInterface $upgradedCookieBuilder; + + public function __construct(LegacySignatureCookieTrackerInterface $legacySignatureCookieTracker, UpgradedCookieBuilderInterface $upgradedCookieBuilder) + { + $this->legacySignatureCookieTracker = $legacySignatureCookieTracker; + $this->upgradedCookieBuilder = $upgradedCookieBuilder; + } + + public function onKernelResponse(ResponseEvent $event): void + { + if (!$event->isMainRequest()) { + return; + } + + $response = $event->getResponse(); + $request = $event->getRequest(); + + $currentResponseCookies = $this->extractCookieNames($response); + foreach ($this->legacySignatureCookieTracker->getCookiesForUpgrade() as $name) { + if (\in_array($name, $currentResponseCookies, true)) { + continue; + } + $cookie = $this->upgradedCookieBuilder->build($name, $request->cookies->get($name)); + if (null === $cookie) { + continue; + } + + $response->headers->setCookie($cookie); + } + } + + /** + * @return string[] + */ + private function extractCookieNames(Response $response): array + { + $names = []; + $cookies = $response->headers->getCookies(); + foreach ($cookies as $cookie) { + $names[] = $cookie->getName(); + } + + return $names; + } +} diff --git a/src/Resources/config/signed_cookie.php b/src/Resources/config/signed_cookie.php index 1b4fa4d..becb1a8 100644 --- a/src/Resources/config/signed_cookie.php +++ b/src/Resources/config/signed_cookie.php @@ -12,10 +12,18 @@ */ use Nelmio\SecurityBundle\EventListener\SignedCookieListener; +use Nelmio\SecurityBundle\EventListener\SignedCookieUpgradeListener; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTracker; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTrackerInterface; +use Nelmio\SecurityBundle\SignedCookie\UpgradedCookieBuilderInterface; +use Nelmio\SecurityBundle\SignedCookie\UpgradedCookieBuilderRegistry; use Nelmio\SecurityBundle\Signer; use Nelmio\SecurityBundle\Signer\SignerInterface; use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use Symfony\Component\DependencyInjection\Loader\Configurator\ReferenceConfigurator; +use Symfony\Component\HttpKernel\KernelEvents; + +use function Symfony\Component\DependencyInjection\Loader\Configurator\tagged_iterator; return static function (ContainerConfigurator $containerConfigurator): void { $containerConfigurator->services() @@ -24,14 +32,15 @@ ->args([ new ReferenceConfigurator('nelmio_security.signer'), '%nelmio_security.signed_cookie.names%', + new ReferenceConfigurator('nelmio_security.legacy_signature_cookie_tracker'), ]) ->tag('kernel.event_listener', [ - 'event' => 'kernel.request', + 'event' => KernelEvents::REQUEST, 'method' => 'onKernelRequest', 'priority' => 10000, ]) ->tag('kernel.event_listener', [ - 'event' => 'kernel.response', + 'event' => KernelEvents::RESPONSE, 'method' => 'onKernelResponse', 'priority' => -10000, ]) @@ -45,5 +54,27 @@ ]) ->alias(SignerInterface::class, 'nelmio_security.signer') + + ->set('nelmio_security.signed_cookie_upgrade_listener', SignedCookieUpgradeListener::class) + ->args([ + new ReferenceConfigurator('nelmio_security.legacy_signature_cookie_tracker'), + new ReferenceConfigurator('nelmio_security.upgraded_cookie_builder_registry'), + ]) + ->tag('kernel.event_listener', [ + 'event' => KernelEvents::RESPONSE, + 'method' => 'onKernelResponse', + 'priority' => -9990, + ]) + + ->set('nelmio_security.legacy_signature_cookie_tracker', LegacySignatureCookieTracker::class) + + ->alias(LegacySignatureCookieTrackerInterface::class, 'nelmio_security.legacy_signature_cookie_tracker') + + ->set('nelmio_security.upgraded_cookie_builder_registry', UpgradedCookieBuilderRegistry::class) + ->args([ + tagged_iterator('nelmio_security.upgraded_cookie_builder'), + ]) + + ->alias(UpgradedCookieBuilderInterface::class, 'nelmio_security.upgraded_cookie_builder_registry') ; }; diff --git a/src/Resources/doc/index.rst b/src/Resources/doc/index.rst index 1061f8d..0b5a6ab 100644 --- a/src/Resources/doc/index.rst +++ b/src/Resources/doc/index.rst @@ -525,6 +525,12 @@ to `sha256` and `hash_algo` to `sha3-256`. The `legacy_hash_algo` option can expose your application to downgrade attacks and should only be used temporarily for backward compatibility. +To shorten the timeframe with `legacy_hash_algo` enabled, the bundle can actively upgrade existing cookies with a legacy +signature. To do this, your app needs to register a service that implements `UpgradedCookieBuilderInterface` and tag it +with `nelmio_security.upgraded_cookie_builder` in the container. The service is responsible for creating the `Cookie` +from the provided name and value, so your application can set the appropriate cookie properties, such as expiration and +path. + Clickjacking Protection ----------------------- diff --git a/src/SignedCookie/LegacySignatureCookieTracker.php b/src/SignedCookie/LegacySignatureCookieTracker.php new file mode 100644 index 0000000..885b78e --- /dev/null +++ b/src/SignedCookie/LegacySignatureCookieTracker.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 Nelmio\SecurityBundle\SignedCookie; + +class LegacySignatureCookieTracker implements LegacySignatureCookieTrackerInterface +{ + /** + * @var string[] + */ + private array $names = []; + + public function flagForUpgrade(string $cookieName): void + { + if (\in_array($cookieName, $this->names, true)) { + return; + } + $this->names[] = $cookieName; + } + + public function getCookiesForUpgrade(): array + { + return $this->names; + } + + public function clear(): void + { + $this->names = []; + } +} diff --git a/src/SignedCookie/LegacySignatureCookieTrackerInterface.php b/src/SignedCookie/LegacySignatureCookieTrackerInterface.php new file mode 100644 index 0000000..7465550 --- /dev/null +++ b/src/SignedCookie/LegacySignatureCookieTrackerInterface.php @@ -0,0 +1,26 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\SignedCookie; + +interface LegacySignatureCookieTrackerInterface +{ + public function flagForUpgrade(string $cookieName): void; + + /** + * @return string[] + */ + public function getCookiesForUpgrade(): array; + + public function clear(): void; +} diff --git a/src/SignedCookie/SignatureUpgradeCheckerInterface.php b/src/SignedCookie/SignatureUpgradeCheckerInterface.php new file mode 100644 index 0000000..54ddd64 --- /dev/null +++ b/src/SignedCookie/SignatureUpgradeCheckerInterface.php @@ -0,0 +1,19 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\SignedCookie; + +interface SignatureUpgradeCheckerInterface +{ + public function needsUpgrade(string $signedValue): bool; +} diff --git a/src/SignedCookie/UpgradedCookieBuilderInterface.php b/src/SignedCookie/UpgradedCookieBuilderInterface.php new file mode 100644 index 0000000..b478f3e --- /dev/null +++ b/src/SignedCookie/UpgradedCookieBuilderInterface.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\SignedCookie; + +use Symfony\Component\HttpFoundation\Cookie; + +interface UpgradedCookieBuilderInterface +{ + public function build(string $name, ?string $value): ?Cookie; +} diff --git a/src/SignedCookie/UpgradedCookieBuilderRegistry.php b/src/SignedCookie/UpgradedCookieBuilderRegistry.php new file mode 100644 index 0000000..7aefb4b --- /dev/null +++ b/src/SignedCookie/UpgradedCookieBuilderRegistry.php @@ -0,0 +1,44 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\SignedCookie; + +use Symfony\Component\HttpFoundation\Cookie; + +class UpgradedCookieBuilderRegistry implements UpgradedCookieBuilderInterface +{ + /** + * @var iterable + */ + private iterable $builders; + + /** + * @param iterable $builders + */ + public function __construct(iterable $builders) + { + $this->builders = $builders; + } + + public function build(string $name, ?string $value): ?Cookie + { + foreach ($this->builders as $builder) { + $cookie = $builder->build($name, $value); + if (null !== $cookie) { + return $cookie; + } + } + + return null; + } +} diff --git a/src/Signer.php b/src/Signer.php index 5dcf2c2..112d2dc 100644 --- a/src/Signer.php +++ b/src/Signer.php @@ -13,9 +13,10 @@ namespace Nelmio\SecurityBundle; +use Nelmio\SecurityBundle\SignedCookie\SignatureUpgradeCheckerInterface; use Nelmio\SecurityBundle\Signer\SignerInterface; -final class Signer implements SignerInterface +final class Signer implements SignerInterface, SignatureUpgradeCheckerInterface { private string $secret; private string $algo; @@ -89,6 +90,21 @@ public function getVerifiedRawValue(string $signedValue): string return $valueSignatureTuple[0]; } + public function needsUpgrade(string $signedValue): bool + { + if (null === $this->legacyAlgo) { + return false; + } + [$value, $signature] = $this->splitSignatureFromSignedValue($signedValue); + if (null === $signature) { + return false; + } + + $expectedLegacySignature = $this->generateSignature($value, $this->legacyAlgo); + + return hash_equals($expectedLegacySignature, $signature); + } + private function generateSignature(string $value, string $algo): string { return hash_hmac($algo, $value, $this->secret); diff --git a/tests/Listener/SignedCookieListenerTest.php b/tests/Listener/SignedCookieListenerTest.php index cef7ad0..10d6425 100644 --- a/tests/Listener/SignedCookieListenerTest.php +++ b/tests/Listener/SignedCookieListenerTest.php @@ -14,6 +14,7 @@ namespace Nelmio\SecurityBundle\Tests\Listener; use Nelmio\SecurityBundle\EventListener\SignedCookieListener; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTrackerInterface; use Nelmio\SecurityBundle\Signer; use PHPUnit\Framework\MockObject\Stub; use Symfony\Component\HttpFoundation\Cookie; @@ -134,4 +135,34 @@ public function testCookieWritingSkipsSubReqs(): void $cookies = $response->headers->getCookies(ResponseHeaderBag::COOKIES_ARRAY); $this->assertSame('bar', $cookies['']['/']['foo']->getValue()); } + + public function testTrackerIsClearedOnRequest(): void + { + $tracker = $this->createMock(LegacySignatureCookieTrackerInterface::class); + $tracker->expects(self::once()) + ->method('clear'); + + $listener = new SignedCookieListener($this->signer, ['*'], $tracker); + $request = Request::create('/'); + + $event = $this->createRequestEventWithKernel($this->kernel, $request, true); + $listener->onKernelRequest($event); + } + + public function testFlagsLegacyCookieForUpgrade(): void + { + $tracker = $this->createMock(LegacySignatureCookieTrackerInterface::class); + $tracker->expects(self::once()) + ->method('flagForUpgrade') + ->with('legacy'); + + $listener = new SignedCookieListener($this->signer, ['*'], $tracker); + $request = Request::create('/', Request::METHOD_GET, [], [ + 'legacy' => 'bar.d42bb85e6f20b90034d986ad68501a2d', + 'foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e', + ]); + + $event = $this->createRequestEventWithKernel($this->kernel, $request, true); + $listener->onKernelRequest($event); + } } diff --git a/tests/Listener/SignedCookieUpgradeListenerTest.php b/tests/Listener/SignedCookieUpgradeListenerTest.php new file mode 100644 index 0000000..6bebe46 --- /dev/null +++ b/tests/Listener/SignedCookieUpgradeListenerTest.php @@ -0,0 +1,136 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\Tests\Listener; + +use Nelmio\SecurityBundle\EventListener\SignedCookieUpgradeListener; +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTracker; +use Nelmio\SecurityBundle\SignedCookie\UpgradedCookieBuilderInterface; +use PHPUnit\Framework\MockObject\Stub; +use Symfony\Component\HttpFoundation\Cookie; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpFoundation\ResponseHeaderBag; +use Symfony\Component\HttpKernel\HttpKernelInterface; + +class SignedCookieUpgradeListenerTest extends ListenerTestCase +{ + /** + * @var Stub&HttpKernelInterface + */ + private $kernel; + + private LegacySignatureCookieTracker $tracker; + + protected function setUp(): void + { + $this->kernel = $this->createStub(HttpKernelInterface::class); + + $this->tracker = new LegacySignatureCookieTracker(); + } + + public function testSkipsSubRequests(): void + { + $this->tracker->flagForUpgrade('legacy'); + $listener = new SignedCookieUpgradeListener( + $this->tracker, + $this->createBuilder(Cookie::create('legacy', 'foobar_upgraded')) + ); + + $request = Request::create('/', Request::METHOD_GET, [], ['legacy' => 'foobar']); + $response = new Response(); + + $event = $this->createResponseEventWithKernel($this->kernel, $request, false, $response); + $listener->onKernelResponse($event); + + $this->assertEmpty($response->headers->getCookies()); + } + + public function testEligibleLegacyCookieIsCopiedToResponse(): void + { + $this->tracker->flagForUpgrade('legacy'); + $listener = new SignedCookieUpgradeListener( + $this->tracker, + $this->createBuilder(Cookie::create('legacy', 'foobar_upgraded')) + ); + + $request = Request::create('/', Request::METHOD_GET, [], ['legacy' => 'foobar']); + $response = new Response(); + + $event = $this->createResponseEventWithKernel($this->kernel, $request, true, $response); + $listener->onKernelResponse($event); + + $cookies = $response->headers->getCookies(ResponseHeaderBag::COOKIES_ARRAY); + $this->assertSame('foobar_upgraded', $cookies['']['/']['legacy']->getValue()); + } + + public function testIneligibleLegacyCookieIsNotCopiedToResponse(): void + { + $listener = new SignedCookieUpgradeListener( + $this->tracker, + $this->createBuilder(Cookie::create('legacy', 'foobar_upgraded')) + ); + + $request = Request::create('/', Request::METHOD_GET, [], ['legacy' => 'foobar']); + $response = new Response(); + + $event = $this->createResponseEventWithKernel($this->kernel, $request, true, $response); + $listener->onKernelResponse($event); + + $this->assertEmpty($response->headers->getCookies()); + } + + public function testEligibleLegacyCookieIsIgnoredWhenNoBuilder(): void + { + $this->tracker->flagForUpgrade('legacy'); + $listener = new SignedCookieUpgradeListener($this->tracker, $this->createBuilder(null)); + + $request = Request::create('/', Request::METHOD_GET, [], ['legacy' => 'foobar']); + $response = new Response(); + + $event = $this->createResponseEventWithKernel($this->kernel, $request, true, $response); + $listener->onKernelResponse($event); + + $this->assertEmpty($response->headers->getCookies()); + } + + public function testExistingEligibleLegacyCookieIsIgnored(): void + { + $this->tracker->flagForUpgrade('legacy'); + $listener = new SignedCookieUpgradeListener( + $this->tracker, + $this->createBuilder(Cookie::create('legacy', 'foobar_upgraded')) + ); + + $request = Request::create('/', Request::METHOD_GET, [], ['legacy' => 'foobar']); + $response = new Response(); + $response->headers->setCookie(Cookie::create('legacy', 'foobar_already_on_response')); + + $event = $this->createResponseEventWithKernel($this->kernel, $request, true, $response); + $listener->onKernelResponse($event); + + $cookies = $response->headers->getCookies(ResponseHeaderBag::COOKIES_ARRAY); + $this->assertSame('foobar_already_on_response', $cookies['']['/']['legacy']->getValue()); + } + + private function createBuilder(?Cookie $cookie): UpgradedCookieBuilderInterface + { + $mock = $this->createMock(UpgradedCookieBuilderInterface::class); + + if (null !== $cookie) { + $mock->method('build')->willReturn($cookie); + } + + return $mock; + } +} diff --git a/tests/SignedCookie/LegacySignatureCookieTrackerTest.php b/tests/SignedCookie/LegacySignatureCookieTrackerTest.php new file mode 100644 index 0000000..a3d5bd3 --- /dev/null +++ b/tests/SignedCookie/LegacySignatureCookieTrackerTest.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\Tests\SignedCookie; + +use Nelmio\SecurityBundle\SignedCookie\LegacySignatureCookieTracker; +use PHPUnit\Framework\TestCase; + +class LegacySignatureCookieTrackerTest extends TestCase +{ + public function testCollectsCookieNames(): void + { + $tracker = new LegacySignatureCookieTracker(); + $tracker->flagForUpgrade('legacy_cookie'); + $tracker->flagForUpgrade('another_legacy_cookie'); + $tracker->flagForUpgrade('legacy_cookie'); + + $this->assertSame(['legacy_cookie', 'another_legacy_cookie'], $tracker->getCookiesForUpgrade()); + } + + public function testClearsCookieNames(): void + { + $tracker = new LegacySignatureCookieTracker(); + $tracker->flagForUpgrade('legacy_cookie'); + + $this->assertSame(['legacy_cookie'], $tracker->getCookiesForUpgrade()); + + $tracker->clear(); + + $this->assertEmpty($tracker->getCookiesForUpgrade()); + } +} diff --git a/tests/SignerTest.php b/tests/SignerTest.php index 642fd83..ffb1f86 100644 --- a/tests/SignerTest.php +++ b/tests/SignerTest.php @@ -118,4 +118,16 @@ public function testShouldRejectInvalidLegacySignature(): void $this->assertFalse($signer->verifySignedValue('foobar.not_a_signature')); $this->assertFalse($signer->verifySignedValue('foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5')); } + + public function shouldNotRequireUpgradeForDefaultAlgorithm(): void + { + $signer = new Signer('secret', 'sha3-256', 'sha256'); + $this->assertFalse($signer->needsUpgrade('foobar.039c77168ddbd0686efe9ac99194fcea8f66be5cd2097ab0c03965b24b168ddb')); + } + + public function shouldRequireUpgradeForLegacyAlgorithm(): void + { + $signer = new Signer('secret', 'sha3-256', 'sha256'); + $this->assertFalse($signer->needsUpgrade('foobar.4fcc06915b43d8a49aff193441e9e18654e6a27c2c428b02e8fcc41ccc2299f9')); + } }