8000 [Security][CSRF] Double Submit Cookies CSRF prevention strategy by backbone87 · Pull Request #18333 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security][CSRF] Double Submit Cookies CSRF prevention strategy #18333

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

[Security][CSRF] Double Submit Cookies CSRF prevention strategy #18333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d746f31
extend csrf token storage infrastructure
backbone87 Mar 28, 2016
e9573af
code style and feedback
backbone87 Sep 21, 2016
beed130
more codestyle fixes
backbone87 Sep 21, 2016
1cea0d2
remove @see
backbone87 Sep 21, 2016
a298be6
update suggestion notice in composer.json
backbone87 Sep 21, 2016
a533b0e
add a secureNamespace to SessionTokenStorageFactory for secure requests
backbone87 Sep 21, 2016
893fa0c
implement EventSubscriberInterface into CookieTokenStorageListener
backbone87 Sep 24, 2016
61b5483
add verification logic to CookieTokenStorage
backbone87 Sep 25, 2016
6e19ef9
rework CookieTokenStorage
backbone87 Sep 26, 2016
99dde3b
code style
backbone87 Sep 28, 2016
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
Prev Previous commit
Next Next commit
code style and feedback
  • Loading branch information
@backbone87
backbone87 committed Sep 21, 2016
commit e9573af5591e6da6e69c5bde586d512a089bae89
21 changes: 21 additions & 0 deletions src/Symfony/Component/Security/Core/Exception/UnexpectedValueException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Security\Core\Exception;

/**
* Base UnexpectedValueException for the Security component.
*
* @author Oliver Hoff <oliver@hofff.com>
*/
class UnexpectedValueException extends \UnexpectedValueException implements ExceptionInterface
{
}
20 changes: 10 additions & 10 deletions src/Symfony/Component/Security/Csrf/TokenStorage/CookieTokenStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
use Symfony\Component\Security\Csrf\Exception\TokenNotFoundException;

/**
Expand All @@ -27,7 +28,7 @@ class CookieTokenStorage implements TokenStorageInterface
/**
* @var array
8000 */
private $transientTokens;
private $transientTokens = array();

/**
* @var ParameterBag
Expand All @@ -45,7 +46,6 @@ class CookieTokenStorage implements TokenStorageInterface
*/
public function __construct(ParameterBag $cookies, $secure)
{
$this->transientTokens = [];
$this->cookies = $cookies;
$this->secure = (bool) $secure;
}
Expand All @@ -58,8 +58,8 @@ public function getToken($tokenId)
{
$token = $this->resolveToken($tokenId);

if (!strlen($token)) {
throw new TokenNotFoundException;
if ('' === $token) {
throw new TokenNotFoundException();
}

return $token;
Expand All @@ -71,7 +71,7 @@ public function getToken($tokenId)
*/
public function hasToken($tokenId)
{
return strlen($this->resolveToken($tokenId)) > 0;
return '' !== $this->resolveToken($tokenId);
}

/**
Expand All @@ -80,8 +80,10 @@ public function hasToken($tokenId)
*/
public function setToken($tokenId, $token)
{
if (!strlen($token)) {
throw new \InvalidArgumentException('Empty tokens are not allowed');
$token = (string) $token;

if ('' === $token) {
8000 throw new InvalidArgumentException('Empty tokens are not allowed');
}

$this->updateToken($tokenId, $token);
Expand All @@ -97,7 +99,7 @@ public function removeToken($tokenId)

$this->updateToken($tokenId, '');

return strlen($token) ? $token : null;
return '' === $token ? null : $token;
}

/**
Expand All @@ -113,7 +115,6 @@ public function createCookies()
// the problem is the that the value of deleted cookies get set to
// the string "deleted" and not the empty string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's preventing you to normalize it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the question is: is the behavior of Cookie correct to overwrite a zero length cookie value with the value "deleted" (and a past expiration date). opinions on stackoverflow suggest using the empty string value and a past expiration date to "delete" a cookie. (in general a past expiration date should be enough, but browsers are not required to dismiss expired cookies, so in the case the browser decides to submit the cookie anyway, we must detect it as "deleted". But there is no way to distinguish between a "deleted" cookie and a cookie actually containing the value "deleted".)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the string "deleted" used anywhere else in the code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would clash, if someone tries to setToken($tokenId, 'deleted') (basically in every use case, where the cookie value "deleted" is in the allowed value set of the cookie)


// TODO http only?
$name = $this->generateCookieName($tokenId);
$cookies[] = new Cookie($name, $token, 0, null, null, $this->secure, true);
}
Expand Down Expand Up @@ -143,7 +144,6 @@ protected function resolveToken($tokenId)
*/
protected function updateToken($tokenId, $token)
{
$token = (string) $token;
$name = $this->generateCookieName($tokenId);

if ($token === $this->cookies->get($name, '')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be $this->cookies->get($name, false) to be sure that's the name which is empty ? Or it doesn't matter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty string token is the non-existant token. Thats because we dont have an explicit "delete" cookie header. If you want to delete a cookie, you need to overwrite a previously existing one. Thats also why it is disallowed to pass an empty string token to setToken (and the empty token makes no sense from a security point of view anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So update can be an alias to remove ? of courses not it is protected

Expand Down
21 changes: 15 additions & 6 deletions src/Symfony/Component/Security/Csrf/TokenStorage/RequestStackTokenStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
namespace Symfony\Component\Security\Csrf\TokenStorage;

use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Security\Core\Exception\RuntimeException;
use Symfony\Component\Security\Core\Exception\UnexpectedValueException;

/**
* Forwards token storage calls to a token storage stored in the master
Expand Down Expand Up @@ -67,18 +69,25 @@ public function getProxiedTokenStorage()
$request = $this->requestStack->getMasterRequest();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master?


if (!$request) {
throw new \RuntimeException('Not in a request context');
throw new RuntimeException('Not in a request context');
}

$storage = $request->attributes->get($this->tokenStorageKey);

// TODO what if storage is set and not an instance of TokenStorageInterface?
// error out? overwrite?
if (!$storage instanceof TokenStorageInterface) {
$storage = $this->factory->createTokenStorage($request);
$request->attributes->set($this->tokenStorageKey, $storage);
if ($storage instanceof TokenStorageInterface) {
return $storage;
}

if ($storage !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yoda

throw new UnexpectedValueException(sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be inlined

'Expected null or an instance of "Symfony\\Component\\Security\\Csrf\\TokenStorage\\TokenStorageInterface", got "%s"',
Copy link
Contributor
@HeahDude HeahDude Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could use "%s" with a replacement by TokenStorageInterface::class. Also the message should say implementation instead of instance since this is an interface

is_object($storage) ? get_class($storage) : gettype($storage)
));
}

$storage = $this->factory->createTokenStorage($request);
$request->attributes->set($this->tokenStorageKey, $storage);

return $storage;
}

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/Security/Csrf/TokenStorage/SessionTokenStorageFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Security\Csrf\TokenStorage;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Security\Core\Exception\RuntimeException;

/**
* Creates CSRF token storages based on the requests session.
Expand All @@ -30,7 +31,7 @@ class SessionTokenStorageFactory implements TokenStorageFactoryInterface
*/
public function __construct($namespace = null)
{
$this->namespace = $namespace === null ? SessionTokenStorage::SESSION_NAMESPACE : $namespace;
$this->namespace = $namespace === null ? SessionTokenStorage::SESSION_NAMESPACE : (string) $namespace;
}

/**
Expand All @@ -40,7 +41,7 @@ public function __construct($namespace = null)
public function createTokenStorage(Request $request) {
$session = $request->getSession();
if (!$session) {
throw new \RuntimeException('Request has no session');
throw new RuntimeException('Request has no session');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could modify the namespace here, to use a different one for SSL connections, because tokens transmitted via non SSL connections are subject to MITM attacks.

return new SessionTokenStorage($session, $this->namespace);
Expand Down
0