Add legacy_hash_algo to support backward-compatible hash_algo cha…

…nges

src/DependencyInjection/Configuration.php

@@ -253,6 +253,10 @@ 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 for cookies created before 3.x (without embedded algorithm) and to allow frictionless hash algorithm upgrades. Use with caution and as a temporary measure as it allows for downgrade attacks.')
+                ->end()
             ->end();
 
         return $node;

src/DependencyInjection/NelmioSecurityExtension.php

@@ -36,6 +36,7 @@ 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']);
         }
 
         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\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

@@ -39,5 +39,7 @@
             ->args([
                 '%nelmio_security.signer.secret%',
                 '%nelmio_security.signer.hash_algo%',
+                '%nelmio_security.signer.legacy_hash_algo%',
+                '.',
             ]);
 };

src/Resources/doc/index.rst

@@ -505,6 +505,27 @@ 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
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Increasing computational power and security research makes upgrading to more secure hashing algorithms an important
+aspect of keeping applications secure. However, simply changing the `hash_algo` value would break existing cookies. To
+make upgrading frictionless, this bundle provides a `legacy_hash_algo` option. If your application has been using
+`sha-256` and wishes to upgrade to the more secure `sha3-256` algorithm, you can 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::
+
+    `legacy_hash_algo` allows for downgrade attacks and should only be used as a temporary measure for backward-compatibility.
+
 Clickjacking Protection
 -----------------------
 

src/Signer.php

@@ -13,35 +13,53 @@
 
 namespace Nelmio\SecurityBundle;
 
-final class Signer
+final class Signer implements SignerInterface
 {
     private string $secret;
     private string $algo;
+    private ?string $legacyAlgo;
 
-    public function __construct(string $secret, string $algo)
+    /**
+     * @var non-empty-string
+     */
+    private string $separator;
+
+    /**
+     * @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 implode($this->separator, [$value, $signature, $this->algo]);
     }
 
     public function verifySignedValue(string $signedValue): bool
     {
-        [$value, $signature] = $this->splitSignatureFromSignedValue($signedValue);
-        $signature2 = $this->generateSignature($value);
+        [$value, $signature, $algorithm] = $this->splitSignedValue($signedValue);
+        if (null === $algorithm || !\in_array($algorithm, $this->allowedAlgorithms(), true)) {
+            return false;
+        }
 
+        $signature2 = $this->generateSignature($value, $algorithm);
         if (null === $signature || \strlen($signature) !== \strlen($signature2)) {
             return false;
         }
@@ -60,26 +78,47 @@ public function getVerifiedRawValue(string $signedValue): string
             throw new \InvalidArgumentException(sprintf("The signature for '%s' was invalid.", $signedValue));
         }
 
-        $valueSignatureTuple = $this->splitSignatureFromSignedValue($signedValue);
+        $valueSignatureTuple = $this->splitSignedValue($signedValue);
 
         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, string|null}
+     */
+    private function splitSignedValue(string $signedValue): array
+    {
+        $parts = explode($this->separator, $signedValue);
+        $length = \count($parts);
+        if ($length >= 3) {
+            return [
+                implode($this->separator, \array_slice($parts, 0, $length - 2)),
+                $parts[$length - 2],
+                $parts[$length - 1],
+            ];
+        }
+
+        if (2 === \count($parts)) {
+            return [$parts[0], $parts[1], $this->legacyAlgo ?? $this->algo];
+        }
+
+        return [implode($this->separator, $parts), null, null];
     }
 
     /**
-     * @return array{string, string|null}
+     * @return string[]
      */
-    private function splitSignatureFromSignedValue(string $signedValue): array
+    private function allowedAlgorithms(): array
     {
-        $pos = strrpos($signedValue, '.');
-        if (false === $pos) {
-            return [$signedValue, null];
+        if (null === $this->legacyAlgo) {
+            return [$this->algo];
         }
 
-        return [substr($signedValue, 0, $pos), substr($signedValue, $pos + 1)];
+        return [$this->algo, $this->legacyAlgo];
     }
 }

src/SignerInterface.php

@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the Nelmio SecurityBundle.
+ *
+ * (c) Nelmio <hello@nelm.io>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Nelmio\SecurityBundle;
+
+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,15 @@ public function testLoadSignedCookie(): void
                     'names' => ['name1', 'name2'],
                     'secret' => 's3cr3t',
                     'hash_algo' => 'hash',
+                    'legacy_hash_algo' => 'legacy_hash',
                 ],
             ],
         ], $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->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);
     }
 
@@ -61,9 +61,12 @@ public function provideCookieReading(): array
             [[], [], []],
             [[], ['foo' => 'bar'], ['foo' => 'bar']],
             [['foo'], ['foo' => 'bar'], []],
-            [['foo'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e'], ['foo' => 'bar']],
-            [['*'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e'], ['foo' => 'bar']],
-            [['*'], ['foo' => '.25af6174a0fcecc4d346680a72b7ce644b9a88e8'], ['foo' => '']],
+            [['foo'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e.sha1'], ['foo' => 'bar']],
+            [['*'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e.sha1'], ['foo' => 'bar']],
+            [['*'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795d.sha1'], []],
+            [['*'], ['foo' => '.25af6174a0fcecc4d346680a72b7ce644b9a88e8.sha1'], ['foo' => '']],
+            [['*'], ['legacy' => 'bar.d42bb85e6f20b90034d986ad68501a2d'], ['legacy' => 'bar']],
+            [['*'], ['legacy' => 'bar.d42bb85e6f20b90034d986ad68501a2a'], []],
         ];
     }
 
@@ -100,9 +103,9 @@ public function provideCookieWriting(): array
         return [
             [[], [], []],
             [[], ['foo' => 'bar'], ['foo' => 'bar']],
-            [['foo'], ['foo' => 'bar'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e']],
-            [['*'], ['foo' => 'bar'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e']],
-            [['*'], ['foo' => null], ['foo' => '.25af6174a0fcecc4d346680a72b7ce644b9a88e8']],
+            [['foo'], ['foo' => 'bar'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e.sha1']],
+            [['*'], ['foo' => 'bar'], ['foo' => 'bar.ca3756f81d3728a023bdc8a622c0906f373b795e.sha1']],
+            [['*'], ['foo' => null], ['foo' => '.25af6174a0fcecc4d346680a72b7ce644b9a88e8.sha1']],
         ];
     }
 

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,77 @@ 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
+    {
+        // SHA-1
+        $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.not_a_signature.'));
+        $this->assertFalse($signer->verifySignedValue('foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5.'));
+    }
+
+    public function testShouldRejectInvalidLegacyAlgorithmSignature(): void
+    {
+        // SHA-1
+        $value = 'foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5';
+
+        $signer = new Signer('secret', 'sha3-256', 'sha3-256');
+
+        $this->assertFalse($signer->verifySignedValue($value));
+    }
+
+    public function testShouldRejectUnsupportedAlgorithm(): void
+    {
+        $signer = new Signer('secret', 'sha1', 'sha256');
+
+        $this->assertFalse($signer->verifySignedValue('foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5.sha3-256'));
+    }
+
+    public function testShouldRejectInvalidAlgorithm(): void
+    {
+        $signer = new Signer('secret', 'sha1', 'sha256');
+
+        $this->assertFalse($signer->verifySignedValue('foobar.7f5c0e9cb2f07137b1c0249108d5c400a3c39be5.not_an_algorithm'));
+    }
+
+    public function testShouldRejectValidSignatureWithUnsupportedAlgorithm(): void
+    {
+        $signer = new Signer('secret', 'sha1', 'sha256');
+
+        $this->assertFalse($signer->verifySignedValue('foobar.c52fa63f4d2ea54d30b0cdeff523305e.md5'));
+    }
 }