Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PoC] Proactive hash algorithm upgrade path #356

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/EventListener/SignedCookieListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,12 +30,15 @@ final class SignedCookieListener
*/
private $signedCookieNames;

private LegacySignatureCookieTrackerInterface $legacySignatureCookieTracker;

/**
* @param list<string> $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 {
Expand All @@ -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);
}
}
}
Expand Down
69 changes: 69 additions & 0 deletions src/EventListener/SignedCookieUpgradeListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?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\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;
}
}
35 changes: 33 additions & 2 deletions src/Resources/config/signed_cookie.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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,
])
Expand All @@ -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')
;
};
6 changes: 6 additions & 0 deletions src/Resources/doc/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----------------------

Expand Down
40 changes: 40 additions & 0 deletions src/SignedCookie/LegacySignatureCookieTracker.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?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\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 = [];
}
}
26 changes: 26 additions & 0 deletions src/SignedCookie/LegacySignatureCookieTrackerInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?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\SignedCookie;

interface LegacySignatureCookieTrackerInterface
{
public function flagForUpgrade(string $cookieName): void;

/**
* @return string[]
*/
public function getCookiesForUpgrade(): array;

public function clear(): void;
}
19 changes: 19 additions & 0 deletions src/SignedCookie/SignatureUpgradeCheckerInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?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\SignedCookie;

interface SignatureUpgradeCheckerInterface
{
public function needsUpgrade(string $signedValue): bool;
}
21 changes: 21 additions & 0 deletions src/SignedCookie/UpgradedCookieBuilderInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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\SignedCookie;

use Symfony\Component\HttpFoundation\Cookie;

interface UpgradedCookieBuilderInterface
{
public function build(string $name, ?string $value): ?Cookie;
}
44 changes: 44 additions & 0 deletions src/SignedCookie/UpgradedCookieBuilderRegistry.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?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\SignedCookie;

use Symfony\Component\HttpFoundation\Cookie;

class UpgradedCookieBuilderRegistry implements UpgradedCookieBuilderInterface
{
/**
* @var iterable<UpgradedCookieBuilderInterface>
*/
private iterable $builders;

/**
* @param iterable<UpgradedCookieBuilderInterface> $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;
}
}
18 changes: 17 additions & 1 deletion src/Signer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -86,6 +87,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);
Expand Down
Loading
Loading