diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 7fbff8f..f086987 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -254,6 +254,11 @@ private function getSignedCookiesNode(): ArrayNodeDefinition ->end() ->scalarNode('secret')->defaultValue('%kernel.secret%')->end() ->scalarNode('hash_algo')->defaultValue('sha256')->end() + ->scalarNode('legacy_hash_algo') + ->defaultNull() + ->info('Fallback algorithm to allow for frictionless hash algorithm upgrades. Use with caution and as a temporary measure as it allows for downgrade attacks.') + ->end() + ->scalarNode('separator')->defaultValue('.')->end() ->end(); return $node; diff --git a/src/DependencyInjection/NelmioSecurityExtension.php b/src/DependencyInjection/NelmioSecurityExtension.php index 176526b..d49c05b 100644 --- a/src/DependencyInjection/NelmioSecurityExtension.php +++ b/src/DependencyInjection/NelmioSecurityExtension.php @@ -36,6 +36,8 @@ public function load(array $configs, ContainerBuilder $container): void $container->setParameter('nelmio_security.signed_cookie.names', $config['signed_cookie']['names']); $container->setParameter('nelmio_security.signer.secret', $config['signed_cookie']['secret']); $container->setParameter('nelmio_security.signer.hash_algo', $config['signed_cookie']['hash_algo']); + $container->setParameter('nelmio_security.signer.legacy_hash_algo', $config['signed_cookie']['legacy_hash_algo']); + $container->setParameter('nelmio_security.signer.separator', $config['signed_cookie']['separator']); } if (isset($config['clickjacking']) && [] !== $config['clickjacking']) { diff --git a/src/EventListener/SignedCookieListener.php b/src/EventListener/SignedCookieListener.php index 6b162a2..aa2057e 100644 --- a/src/EventListener/SignedCookieListener.php +++ b/src/EventListener/SignedCookieListener.php @@ -13,14 +13,14 @@ namespace Nelmio\SecurityBundle\EventListener; -use Nelmio\SecurityBundle\Signer; +use Nelmio\SecurityBundle\Signer\SignerInterface; use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\Event\ResponseEvent; final class SignedCookieListener { - private Signer $signer; + private SignerInterface $signer; /** * @var list|true @@ -30,7 +30,7 @@ final class SignedCookieListener /** * @param list $signedCookieNames */ - public function __construct(Signer $signer, array $signedCookieNames) + public function __construct(SignerInterface $signer, array $signedCookieNames) { $this->signer = $signer; if (\in_array('*', $signedCookieNames, true)) { diff --git a/src/Resources/config/signed_cookie.php b/src/Resources/config/signed_cookie.php index 9184cbb..1b4fa4d 100644 --- a/src/Resources/config/signed_cookie.php +++ b/src/Resources/config/signed_cookie.php @@ -13,6 +13,7 @@ use Nelmio\SecurityBundle\EventListener\SignedCookieListener; use Nelmio\SecurityBundle\Signer; +use Nelmio\SecurityBundle\Signer\SignerInterface; use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use Symfony\Component\DependencyInjection\Loader\Configurator\ReferenceConfigurator; @@ -39,5 +40,10 @@ ->args([ '%nelmio_security.signer.secret%', '%nelmio_security.signer.hash_algo%', - ]); + '%nelmio_security.signer.legacy_hash_algo%', + '%nelmio_security.signer.separator%', + ]) + + ->alias(SignerInterface::class, 'nelmio_security.signer') + ; }; diff --git a/src/Resources/doc/index.rst b/src/Resources/doc/index.rst index 6900de5..1061f8d 100644 --- a/src/Resources/doc/index.rst +++ b/src/Resources/doc/index.rst @@ -503,6 +503,28 @@ Additional, optional configuration settings: secret: this_is_very_secret # defaults to global %secret% parameter hash_algo: sha512 # defaults to sha256, see ``hash_algos()`` for available algorithms +Upgrading the Hash Algorithm +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +With advancements in computational power and security research, upgrading to more secure hashing algorithms is +essential for maintaining application security. However, simply changing the `hash_algo` value could break existing +cookies. To facilitate a smooth transition, this bundle offers a `legacy_hash_algo` option. If your application +currently uses `sha-256` and you wish to upgrade to the more secure `sha3-256` algorithm, set `legacy_hash_algo` +to `sha256` and `hash_algo` to `sha3-256`. + +.. code-block:: yaml + + # config/packages/nelmio_security.yaml + nelmio_security: + signed_cookie: + hash_algo: sha3-256 + legacy_hash_algo: sha256 + +.. caution:: + + The `legacy_hash_algo` option can expose your application to downgrade attacks and should only be used temporarily + for backward compatibility. + Clickjacking Protection ----------------------- diff --git a/src/Signer.php b/src/Signer.php index c0c6284..da605af 100644 --- a/src/Signer.php +++ b/src/Signer.php @@ -13,45 +13,66 @@ namespace Nelmio\SecurityBundle; -final class Signer +use Nelmio\SecurityBundle\Signer\SignerInterface; + +final class Signer implements SignerInterface { private string $secret; private string $algo; + private ?string $legacyAlgo; + + /** + * @var non-empty-string + */ + private string $separator; - public function __construct(string $secret, string $algo) + /** + * @param non-empty-string $separator + */ + public function __construct(string $secret, string $algo, ?string $legacyAlgo = null, string $separator = '.') { $this->secret = $secret; $this->algo = $algo; + $this->legacyAlgo = $legacyAlgo; + $this->separator = $separator; if (!\in_array($this->algo, hash_algos(), true)) { throw new \InvalidArgumentException(sprintf("The supplied hashing algorithm '%s' is not supported by this system.", $this->algo)); } + + if (null !== $this->legacyAlgo && !\in_array($this->legacyAlgo, hash_algos(), true)) { + throw new \InvalidArgumentException(sprintf("The supplied legacy hashing algorithm '%s' is not supported by this system.", $this->legacyAlgo)); + } } public function getSignedValue(string $value, ?string $signature = null): string { if (null === $signature) { - $signature = $this->generateSignature($value); + $signature = $this->generateSignature($value, $this->algo); } - return $value.'.'.$signature; + return $value.$this->separator.$signature; } public function verifySignedValue(string $signedValue): bool { [$value, $signature] = $this->splitSignatureFromSignedValue($signedValue); - $signature2 = $this->generateSignature($value); - - if (null === $signature || \strlen($signature) !== \strlen($signature2)) { + if (null === $signature) { return false; } - $result = 0; - for ($i = 0, $j = \strlen($signature); $i < $j; ++$i) { - $result |= \ord($signature[$i]) ^ \ord($signature2[$i]); + $expectedSignature = $this->generateSignature($value, $this->algo); + if (hash_equals($expectedSignature, $signature)) { + return true; } - return 0 === $result; + if (null === $this->legacyAlgo) { + return false; + } + + $expectedLegacySignature = $this->generateSignature($value, $this->legacyAlgo); + + return hash_equals($expectedLegacySignature, $signature); } public function getVerifiedRawValue(string $signedValue): string @@ -65,9 +86,9 @@ public function getVerifiedRawValue(string $signedValue): string return $valueSignatureTuple[0]; } - private function generateSignature(string $value): string + private function generateSignature(string $value, string $algo): string { - return hash_hmac($this->algo, $value, $this->secret); + return hash_hmac($algo, $value, $this->secret); } /** @@ -75,7 +96,7 @@ private function generateSignature(string $value): string */ private function splitSignatureFromSignedValue(string $signedValue): array { - $pos = strrpos($signedValue, '.'); + $pos = strrpos($signedValue, $this->separator); if (false === $pos) { return [$signedValue, null]; } diff --git a/src/Signer/SignerInterface.php b/src/Signer/SignerInterface.php new file mode 100644 index 0000000..19e1431 --- /dev/null +++ b/src/Signer/SignerInterface.php @@ -0,0 +1,23 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Nelmio\SecurityBundle\Signer; + +interface SignerInterface +{ + public function getSignedValue(string $value, ?string $signature = null): string; + + public function verifySignedValue(string $signedValue): bool; + + public function getVerifiedRawValue(string $signedValue): string; +} diff --git a/tests/DependencyInjection/NelmioSecurityExtensionTest.php b/tests/DependencyInjection/NelmioSecurityExtensionTest.php index a97313d..050cbdb 100644 --- a/tests/DependencyInjection/NelmioSecurityExtensionTest.php +++ b/tests/DependencyInjection/NelmioSecurityExtensionTest.php @@ -42,6 +42,8 @@ public function testLoadSignedCookie(): void 'names' => ['name1', 'name2'], 'secret' => 's3cr3t', 'hash_algo' => 'hash', + 'legacy_hash_algo' => 'legacy_hash', + 'separator' => 'value_separator', ], ], ], $container); @@ -49,6 +51,8 @@ public function testLoadSignedCookie(): void $this->assertContainerWithParameterValue($container, 'nelmio_security.signed_cookie.names', ['name1', 'name2']); $this->assertContainerWithParameterValue($container, 'nelmio_security.signer.secret', 's3cr3t'); $this->assertContainerWithParameterValue($container, 'nelmio_security.signer.hash_algo', 'hash'); + $this->assertContainerWithParameterValue($container, 'nelmio_security.signer.legacy_hash_algo', 'legacy_hash'); + $this->assertContainerWithParameterValue($container, 'nelmio_security.signer.separator', 'value_separator'); $this->assertServiceIdClass($container, 'nelmio_security.signed_cookie_listener', SignedCookieListener::class); $this->assertServiceIdClass($container, 'nelmio_security.signer', Signer::class); diff --git a/tests/Listener/SignedCookieListenerTest.php b/tests/Listener/SignedCookieListenerTest.php index 11e4b27..cef7ad0 100644 --- a/tests/Listener/SignedCookieListenerTest.php +++ b/tests/Listener/SignedCookieListenerTest.php @@ -33,7 +33,7 @@ class SignedCookieListenerTest extends ListenerTestCase protected function setUp(): void { - $this->signer = new Signer('secret', 'sha1'); + $this->signer = new Signer('secret', 'sha1', 'md5'); $this->kernel = $this->createStub(HttpKernelInterface::class); } @@ -63,7 +63,10 @@ public function provideCookieReading(): array [['foo'], ['foo' => 'bar'], []], [['foo'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e'], ['foo' => 'bar']], [['*'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e'], ['foo' => 'bar']], + [['*'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795d'], []], [['*'], ['foo' => '.25af6174a0fcecc4d346680a72b7ce644b9a88e8'], ['foo' => '']], + [['*'], ['legacy' => 'bar.d42bb85e6f20b90034d986ad68501a2d'], ['legacy' => 'bar']], + [['*'], ['legacy' => 'bar.d42bb85e6f20b90034d986ad68501a2a'], []], ]; } diff --git a/tests/SignerTest.php b/tests/SignerTest.php index d662e57..642fd83 100644 --- a/tests/SignerTest.php +++ b/tests/SignerTest.php @@ -25,6 +25,13 @@ public function testConstructorShouldVerifyHashAlgo(): void new Signer('secret', 'invalid_hash_algo'); } + public function testConstructorShouldVerifyHashLegacyAlgo(): void + { + $this->expectException('InvalidArgumentException'); + + new Signer('secret', hash_algos()[0], 'invalid_hash_algo'); + } + public function testShouldVerifyValidSignature(): void { $signer = new Signer('secret', 'sha1'); @@ -47,6 +54,14 @@ public function testShouldRejectInvalidSignature(): void $this->assertFalse($signer->verifySignedValue($signedValue)); } + public function testShouldRejectMissingSignature(): void + { + $signer = new Signer('secret', 'sha1'); + + $this->assertFalse($signer->verifySignedValue('foobar')); + $this->assertFalse($signer->verifySignedValue('foobar.')); + } + public function testThrowsExceptionWithInvalidSignature(): void { $signer = new Signer('secret', 'sha1'); @@ -63,4 +78,44 @@ public function testSignatureShouldDependOnSecret(): void $this->assertNotSame($signer1->getSignedValue('foobar'), $signer2->getSignedValue('foobar')); } + + public function testHandlesValueWithSeparator(): void + { + $value = 'foo.bar'; + + $signer = new Signer('secret1', 'sha3-256', null, '.'); + $signature = $signer->getSignedValue($value); + + $this->assertTrue($signer->verifySignedValue($signature)); + $this->assertSame($value, $signer->getVerifiedRawValue($signature)); + } + + public function testHandlesCustomSeparator(): void + { + $value = 'foobar'; + + $signer = new Signer('secret1', 'sha3-256', null, ';'); + $signature = $signer->getSignedValue($value); + + $this->assertTrue($signer->verifySignedValue($signature)); + $this->assertSame($value, $signer->getVerifiedRawValue($signature)); + $this->assertStringContainsString(';', $signature); + } + + public function testShouldVerifyValidLegacySignature(): void + { + $value = 'foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5'; + + $signer = new Signer('secret', 'sha3-256', 'sha1'); + + $this->assertTrue($signer->verifySignedValue($value)); + } + + public function testShouldRejectInvalidLegacySignature(): void + { + $signer = new Signer('secret', 'sha3-256', 'sha256'); + + $this->assertFalse($signer->verifySignedValue('foobar.not_a_signature')); + $this->assertFalse($signer->verifySignedValue('foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5')); + } }