[Security][CSRF] Double Submit Cookies CSRF prevention strategy #18333
Conversation
| */ | ||
| public function __construct(ParameterBag $cookies, $secure) | ||
| { | ||
| $this->transientTokens = []; |
There was a problem hiding this comment.
Should be at variable setting not in constructor.
There was a problem hiding this comment.
i think its personal preference, i try to do all property initialization within constructors and cleanly separate it from property declaration.
There was a problem hiding this comment.
It seems to be set on properties in symfony core.
There was a problem hiding this comment.
@HeahDude is right. And you should use array() instead of [].
|
closes #13464 |
|
Any progress on this one? |
| { | ||
| /** | ||
| * {@inheritDoc} | ||
| * @see \Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface::getToken() |
There was a problem hiding this comment.
the @see tag should be removed everywhere as it does not add anything useful.
There was a problem hiding this comment.
i can remove it, but i disagree that it doesnt add anything useful: you see where the original declaration comes from, without doin any clicks in your IDE. you only see, if its an overridden method in IDEs, not who the original definer was (without doin more clicks for inspection). also on github you dont have any code assist regarding inheritance.
|
@backbone87 can you update PR according to suggested changes? |
- cookie token storage managing csrf tokens stored inside cookies (prerequisite for "double submit cookies" csrf prevention strategy) - kernel response event listener add the cookie headers of the cookie token storage to request responses - token storage factory interface to create token storages based on requests - session token storage factory using the requests session to create a session token storage - cookie token storage factory using the requests cookies to create a cookie token storage - request stack token storage managing and using token storages inside master request's attributes
590a59e to
e9573af
Compare
|
I have updated the code. There are still open questions, pls check TODOs and FIXMEs. Also #18115 is required to be resolved, before this can be used, or we start spamming the user with cookies. |
| /** | ||
| * {@inheritdoc} | ||
| * | ||
| * @see \Symfony\Component\Security\Csrf\TokenStorage\TokenStorageInterface::getToken() |
There was a problem hiding this comment.
all those @see should be removed, they do not add anything useful (the class implements TokenStorageInterface).
| if (!$session) { | ||
| throw new RuntimeException('Request has no session'); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
Ok, I have read a little bit more about DSC method, and I need help regarding determining the correct way to implement its usage within the form component. AFAIU you dont include the token value in a hidden field within the form, but are required that client side JS reads the cookie value and puts it into a hidden form field before posting the form. If you would include the token value when generating the form on the server then someone could just GET the form from a cross-site (unless you have CORS preventing JS GETs on the form HTML), This fact basically eliminates the use of DSC as the default method of Symfony, because you rely on client side JS execution. |
|
I have modified the CookieTokenStorage to use a second verification cookie to fix some shortcommings of the simple DSC method http://security.stackexchange.com/a/61039 also the tokens now have a configurable TTL to avoid permanent tokens when using session continuation in clients. the lifetime gets refreshed each time the token is retrieved while having less than half of its TTL left. i also created a SE fork with a manual configuration for end-to-end test of this PR: https://github.com/backbone87/symfony-standard/tree/test/ticket-18313 |
6ecc0c4 to
61b5483
Compare
e3b363a to
6e19ef9
Compare
|
I have reworked the CookieTokenStorage again. Its back to using one cookie (basically just the verification cookie left). How the token itself is passed to the client for resubmission in POST or Header data is subject to the developer (this is in line with the current CSRF workflow). The CookieTokenStorage now works on the raw HTTP Cookie request header. This is required to close certain attack vectors (overwriting existing cookies by applying a more specific path). Also the cookie name has a nonce suffix that is included in the signature, which makes it a lot harder to inject cookies obtained by the attacker in his own session beforehand. |
|
i cant finish this PR without review and feedback |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getToken($tokenId) |
There was a problem hiding this comment.
Does it makes sense to mark concrete public methods as final?
There was a problem hiding this comment.
I think final should only be used in very special cases when you absolutely rely on the implemented behavior and to keep the internal state of the class clean and valid, and this cannot be provided by subclasses (e.g. not being able to access private properties). Both is not the case here. Imagine someone wants to create a decorater that fixes the token ID to a certain value, using this class as a parent would not be possible with final methods.
| $this->cookies = self::parseCookieHeader($cookies); | ||
| $this->secure = (bool) $secure; | ||
| $this->secret = (string) $secret; | ||
| $this->ttl = $ttl === null ? 60 * 60 : (int) $ttl; |
| } | ||
| } | ||
|
|
||
| if ($token !== '') { |
| public static function getSubscribedEvents() | ||
| { | ||
| return array( | ||
| KernelEvents::RESPONSE => array(array('onKernelResponse', 0)), |
There was a problem hiding this comment.
I am not sure, if this will be the actual priority used in the end.
| public function __construct($secret, $ttl = null) | ||
| { | ||
| $this->secret = (string) $secret; | ||
| $this->ttl = $ttl === null ? 60 * 60 : (int) $ttl; |
| * @param TokenStorageFactoryInterface $factory | ||
| * @param string|null $tokenStorageKey | ||
| */ | ||
| public function __construct( |
There was a problem hiding this comment.
please inline the arguments for consistency with the code base
| public function getProxiedTokenStorage() | ||
| { | ||
| // TODO use master or current request? | ||
| $request = $this->requestStack->getMasterRequest(); |
| return $storage; | ||
| } | ||
|
|
||
| if ($storage !== null) { |
| } | ||
|
|
||
| if ($storage !== null) { | ||
| throw new UnexpectedValueException(sprintf( |
|
|
||
| if ($storage !== null) { | ||
| throw new UnexpectedValueException(sprintf( | ||
| 'Expected null or an instance of "Symfony\\Component\\Security\\Csrf\\TokenStorage\\TokenStorageInterface", got "%s"', |
There was a problem hiding this comment.
you could use "%s" with a replacement by TokenStorageInterface::class. Also the message should say implementation instead of instance since this is an interface
|
@backbone87 what's the status here? |
|
Someone with some knowledge of the Double Submit cookie CSRF protection needs to help review this. It looks like @backbone87 has put a lot of good work into it, but we haven't reviewed this yet on real, technical merit. But since feature freeze for 3.4 is only a few days away, we might not have time now. @backbone87 In your opinion, is that accurate? Basically, this PR is done from your point-of-view, and is just waiting on an opinion from core members? |
|
@weaverryan yes thats right for the main part, but there are other things that need to be done this PR contains the pure infrastructure and handling logic to supporting DSC CSRF what needs to be done:
|
|
Some more information on this, after i digged into this PR again. This should act like a "secure" storage inside browsers cookies. The cookies are marked HTTP-only and should not be able to be read by any client side scripts. The cookies have (partially) randomized names, contain the token_id, an expiry date (not to be confused with the cookie expiration date, which is always client session bound) and are signed with a server secret. The token id can be used inside hidden form fields. Security considerations:
edit: one way to inject cookies in the client even if your site doesnt have XSS vectors, is if you do not have full domain control. e.g. there are subdomains that can be controlled by an attacker |
|
Thanks for the summary @backbone87 :). I'd like this to move forward, so we can find out whether it's a "sound" idea and merge it or not. But, this will not happen for 3.4 - it would be too rushed. I've changed the milestone to 4.1. |
|
Is this PR still alive? |
|
Closing as there is no more activities here. |
(prerequisite for "double submit cookies" csrf prevention strategy)
token storage to request responses
requests
session token storage
cookie token storage
master request's attributes
assistance on the TODOs and FIXMEs as well as some general feedback is very welcome