Merge pull request #351 from martijnc/feature/cookie-algo-upgrade-path

Add `legacy_hash_algo` to support backward-compatible `hash_algo` changes

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;

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']) {

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<string>|true
@@ -30,7 +30,7 @@ final class SignedCookieListener
     /**
      * @param list<string> $signedCookieNames
      */
-    public function __construct(Signer $signer, array $signedCookieNames)
+    public function __construct(SignerInterface $signer, array $signedCookieNames)
     {
         $this->signer = $signer;
         if (\in_array('*', $signedCookieNames, true)) {

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')
+    ;
 };

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
 -----------------------
 

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,17 +86,17 @@ 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);
     }
 
     /**
      * @return array{string, string|null}
      */
     private function splitSignatureFromSignedValue(string $signedValue): array
     {
-        $pos = strrpos($signedValue, '.');
+        $pos = strrpos($signedValue, $this->separator);
         if (false === $pos) {
             return [$signedValue, null];
         }

src/Signer/SignerInterface.php

@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the Nelmio SecurityBundle.
+ *
+ * (c) Nelmio <[email protected]>
+ *
+ * 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;
+}

tests/DependencyInjection/NelmioSecurityExtensionTest.php

@@ -42,13 +42,17 @@ public function testLoadSignedCookie(): void
                     'names' => ['name1', 'name2'],
                     'secret' => 's3cr3t',
                     'hash_algo' => 'hash',
+                    'legacy_hash_algo' => 'legacy_hash',
+                    'separator' => 'value_separator',
                 ],
             ],
         ], $container);
 
         $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);

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'], []],
         ];
     }
 

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'));
+    }
 }