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

[Security] Added Security\Csrf sub-component with better token generation #6554

Merged
merged 6 commits into from Sep 30, 2013

Conversation

@webmozart
Copy link
Contributor

webmozart commented Jan 4, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR TODO

Update September 27, 2013

This PR simplifies the CSRF mechanism to generate completely random tokens. A random token is generated once per intention token ID and then stored in the session. Tokens are valid until the session expires.

Since the CSRF token generator depends on StringUtils and SecureRandom from Security\Core, and since Security\Http currently depends on the Form component for token generation, I decided to add a new Security\Csrf sub-component that contains the improved CSRF token generator. Consequences:

  • Security\Http now depends on Security\Csrf instead of Form
  • Form now optionally depends on Security\Csrf
  • The configuration for the "security.secure_random" service and the "security.csrf.*" services was moved to FrameworkBundle to guarantee BC

In the new Security\Csrf sub-component, I tried to improve the naming where I could do so without breaking BC:

  • CSRF "providers" are now called "token generators"
  • CSRF "intentions" are now called "token IDs", because that's really what they are
TODO
  • Make sure SecureRandom never blocks for CsrfTokenGenerator
@stof
stof reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/AbstractCsrfProvider.php Outdated
*
* @see http://blog.ircmaxell.com/2012/12/seven-ways-to-screw-up-bcrypt.html
*/
private function timingSafeEquals($a, $b) {

This comment has been minimized.

Copy link
@stof

stof Jan 4, 2013

Member

This method should be removed as it is unused

This comment has been minimized.

Copy link
@mvrhov

mvrhov Jan 5, 2013

Also if you do need such a method it's already present in Symfony\Component\Security\Core\Util\StringUtils

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 5, 2013

Author Contributor

which is currently used, if you check the code ;)

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/form_csrf.xml Outdated
@@ -11,7 +11,7 @@
<services>
<service id="form.csrf_provider" class="%form.csrf_provider.class%">
<argument type="service" id="session" />
<argument>%kernel.secret%</argument>
<!-- TODO inject SecureRandom -->

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

Is this to be done later or a WIP?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 4, 2013

Author Contributor

WIP, see the description of the PR under "Issues to be solved"

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/CHANGELOG.md Outdated
@@ -7,6 +7,7 @@ CHANGELOG
* TrimListener now removes unicode whitespaces
* deprecated getParent(), setParent() and hasParent() in FormBuilderInterface
* FormInterface::add() now accepts a FormInterface instance OR a field's name, type and options
* changed CSRF tokens to completely random tokens. No secret is used for CSRF token generation anymore.

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

Is it also necessary have an upgrade note in UPGRADING-2.2 about removing the %kernel.secret% from configs?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 4, 2013

Author Contributor

I don't think so. The %kernel.secret% setting is meant to be a global secret, that was used for CSRF among other things. See also e72f1a9

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/SessionCsrfProvider.php Outdated
*/
protected function getSessionValue($name)
{
return $this->session->get(self::SESSION_NAMESPACE . '/' . $name);

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

Shouldn't this be static::SESSION_NAMESPACE to allow overriding of the constant?

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 5, 2013

Author Contributor

Fixed

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/SessionCsrfProvider.php Outdated
*/
protected function setSessionValue($name, $value)
{
$this->session->set(self::SESSION_NAMESPACE . '/' . $name, $value);

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

Same here static::

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 5, 2013

Author Contributor

Fixed

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/DefaultCsrfProvider.php Outdated
return sha1($this->secret.$intention.$this->getSessionId());
}
if (isset($_SESSION[self::SESSION_NAMESPACE][$name])) {
return $_SESSION[self::SESSION_NAMESPACE][$name];

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

Use static::

@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/DefaultCsrfProvider.php Outdated
@@ -12,63 +12,48 @@
namespace Symfony\Component\Form\Extension\Csrf\CsrfProvider;

/**
* Default implementation of CsrfProviderInterface.
* CSRF provider that uses PHP's native session handling.

This comment has been minimized.

Copy link
@ghost
@ghost
ghost reviewed Jan 4, 2013
View changes
src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/DefaultCsrfProvider.php Outdated
}

return session_id();
$_SESSION[self::SESSION_NAMESPACE][$name] = $value;

This comment has been minimized.

Copy link
@ghost

ghost Jan 4, 2013

use static::

@mvrhov

This comment has been minimized.

Copy link

mvrhov commented Jan 4, 2013

IMO we do not need cryptographic safe tokens here.
What I'm afraid of is emptying a entropy pretty quickly. Now when there is no more entropy the application WILL block.
This could pose a big problem on a busy server. AFAIK only a couple of bytes of entropy s generated each second.

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Jan 5, 2013

@mvrhov good catch! This could indeed slow the server pretty badly (I was not aware of the problem until I read a forum post explaining why Android sometimes lags - same root cause)

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Jan 5, 2013

@mvrhov I made the tokens URI safe now.

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Jan 7, 2013

Backwards compatibility break: no

Really ?

@padraic

This comment has been minimized.

Copy link

padraic commented Jan 7, 2013

@mvrhov You're correct, the randomness need not be very cryptographically secure. The CSRF token is already a sub-unit of a randomly generated session ID though even that has some predictive weaknesses (needs better entropy) due to be improved in PHP 5.4.

The options currently in favour are to use openssl_pseudo_random_bytes() under PHP 5.3.4+ or fall back to a (concatenated) token generated using mt_rand(). Neither is, as far as I'm aware, blocking and mt_rand() is better than you'd expect if Suhosin is installed. I think openssl seeds itself using /dev/urandom intelligently (i.e. the non-blocking but lower entropy partner to /dev/random).

Only thing you have to watch for is that openssl's random functions were blocking under Windows due to an openssl bug. This was fixed but you should avoid using openssl_pseudo_random_bytes() under PHP 5.3.3 or less and go straight to mt_rand().

CSRF tokens need to be random - just not as crazily random as if we needed a high value long-term key :P.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Jan 8, 2013

Postponed to 2.3

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Jan 8, 2013

@bschussek I have created a 2.3 milestone, you can set it in the PR header (which I've done for this one).

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Jan 8, 2013

Thanks! :)

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Apr 18, 2013

I'm currently trying to get this into 2.3. I'm not sure which way to go though:

(a) As is, plus a new parameter $blocking in SecureRandomInterface:
public function nextBytes($nbBytes, $blocking = true)

This parameter would guarantee a non-blocking behavior when set to false (at the cost of cryptographic security).

Advantage: SecureRandom can be reused
Disadvantage: dependency on the Security component in Form and FrameworkBundle

(b) Copy code from SecureRandom and StringUtil to AbstractCsrfProvider

The code in question is:

// initialize seed
if (null === $this->seed) {
    if (null === $this->seedFile) {
        throw new \RuntimeException('You need to specify a file path to store the seed.');
    }

    if (is_file($this->seedFile)) {
        list($this->seed, $this->seedLastUpdatedAt) = $this->readSeed();
    } else {
        $this->seed = uniqid(mt_rand(), true);
        $this->updateSeed();
    }
}

$bytes = '';
while (strlen($bytes) < $nbBytes) {
    static $incr = 1;
    $bytes .= hash('sha512', $incr++.$this->seed.uniqid(mt_rand(), true).$nbBytes, true);
    $this->seed = base64_encode(hash('sha512', $this->seed.$bytes.$nbBytes, true));
    $this->updateSeed();
}

return substr($bytes, 0, $nbBytes);

and

public static function equals($knownString, $userInput)
{
    // Prevent issues if string length is 0
    $knownString .= chr(0);
    $userInput .= chr(0);

    $knownLen = strlen($knownString);
    $userLen = strlen($userInput);

    // Set the result to the difference between the lengths
    $result = $knownLen - $userLen;

    // Note that we ALWAYS iterate over the user-supplied length
    // This is to prevent leaking length information
    for ($i = 0; $i < $userLen; $i++) {
        // Using % here is a trick to prevent notices
        // It's safe, since if the lengths are different
        // $result is already non-0
        $result |= (ord($knownString[$i % $knownLen]) ^ ord($userInput[$i]));
    }

    // They are only identical strings if $result is exactly 0...
    return 0 === $result;
}

Advantage: no dependency on Security in Form and FrameworkBundle
Disadvantage: code duplication, increased code complexity and maintenance costs, higher security risks

(c) As in (b), but use === instead of constant-time comparison; don't use seeds

Advantage: no dependency on Security in Form and FrameworkBundle
Disadvantage: less cryptographic security

Your opinions please?

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Apr 21, 2013

@fabpot @padraic Do you have an opinion on this?

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Apr 30, 2013

@fabpot @padraic Since today is feature freeze for the 2.3 LTS and this was on the top-priority list of tickets, can you please help me to resolve it? Which direction do we go?

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented May 6, 2013

I'm for option (c)

fabpot added a commit that referenced this pull request Sep 18, 2013
This PR was merged into the master branch.

Discussion
----------

[Security] Split the component into 3 sub-components Core, ACL, HTTP

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9047, #8848
| License       | MIT
| Doc PR        | -

The rationale behind this PR is to be able to use any of the sub components without requiring all the dependencies of the other sub components. Specifically, I'd like to use the core utils for an improved CSRF protection mechanism (#6554).

Commits
-------

14e9f46 [Security] removed unneeded hard dependencies in Core
5dbec8a [Security] fixed README files
62bda79 [Security] copied the Resources/ directory to Core/Resources/
7826781 [Security] Split the component into 3 sub-components Core, ACL, HTTP
@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 27, 2013

I updated this PR now. I deprecated the old CSRF implementation in the Form component and added a new Security CSRF sub-component with a better implementation that depends on Security Core. See the CHANGELOGs in the diff for more information.

I need some feedback regarding the service wiring.

@stof
stof reviewed Sep 27, 2013
View changes
UPGRADE-3.0.md Outdated
@@ -168,6 +168,13 @@ UPGRADE FROM 2.x to 3.0
`ChoiceListInterface::getChoicesForValues()` and
`ChoiceListInterface::getValuesForChoices()` should be sufficient.

* The interface `CsrfProviderInterface` and all of its implementations were

This comment has been minimized.

Copy link
@stof

stof Sep 27, 2013

Member

you should use the FQCN

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 27, 2013

Author Contributor

wow you're fast ;)

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 27, 2013

Author Contributor

fixed

@stof
stof reviewed Sep 27, 2013
View changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/form_csrf.xml Outdated
<argument type="service" id="session" />
<argument>%kernel.secret%</argument>
</service>
<service id="form.csrf_provider" alias="security.csrf.token_generator" />

This comment has been minimized.

Copy link
@stof

stof Sep 27, 2013

Member

This alias looks weird to me. To preserve BC, the id should still provide a service implementing the deprecated interface (wrapping the new implementation)

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 27, 2013

Author Contributor

fixed

@stof
stof reviewed Sep 27, 2013
View changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/security_csrf.xml Outdated
</parameters>

<services>
<service id="security.csrf.token_storage" class="%security.csrf.token_storage.class%">

This comment has been minimized.

Copy link
@stof

stof Sep 27, 2013

Member

shouldn't it be private ?

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 27, 2013

Author Contributor

fixed

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Sep 27, 2013

You can also remove symfony/form from the SecurityBundle composer.json file.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 27, 2013

I just updated the PR description above for more information.

@stof
stof reviewed Sep 27, 2013
View changes
src/Symfony/Component/Security/Csrf/TokenStorage/SessionTokenStorage.php Outdated
* @param string $namespace The namespace under which the token is stored
* in the session
*/
public function __construct(Session $session, $namespace = self::SESSION_NAMESPACE)

This comment has been minimized.

Copy link
@stof

stof Sep 27, 2013

Member

should be SessionInterface

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 27, 2013

Author Contributor

fixed

@stof

This comment has been minimized.

Copy link
Member

stof commented Sep 27, 2013

the root composer.json needs to replace the component too

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 27, 2013

Fixed all mentioned issues.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 27, 2013

The SecurityBundle currently features the configuration values "csrf_provider" and "intention". Is it possible to add aliases "csrf_token_generator" and "csrf_token_id" for these settings?

"symfony/validator": "",
"symfony/http-foundation": ""
"symfony/validator": "For form validation.",
"symfony/security-csrf": "For protecting forms against CSRF attacks."

This comment has been minimized.

Copy link
@stloyd

stloyd Sep 27, 2013

Contributor

You probably should add this also in the require-dev section.

This comment has been minimized.

Copy link
@Tobion

Tobion Sep 27, 2013

Member

true because its used in tests. also its missing suggestions for dependency injection, httpfoundation, templating and kernel for all the extensions.

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 30, 2013

Author Contributor

I've added this to require-dev and added suggestions for symfony/twig-bridge and symfony/framework-bundle. I did not add suggestions for dependency injection, HTTP templating or kernel, because the corresponding form extensions don't add functionality, but only provide code to integrate with those components.

"php": ">=5.3.3"
},
"require-dev": {
"symfony/security-core": "~2.4"

This comment has been minimized.

Copy link
@Tobion

Tobion Sep 27, 2013

Member

You need to put this to require as well because its required for the tokengenerator. Without it the csrf component is not really useful.
Also you probably do not need ~2.4. I guess something like ~2.2 is enough (needs examination).
Furthermore, its missing HttpFoundation which should probably be in suggest as its needed for SessionTokenStorage.

This comment has been minimized.

Copy link
@stof

stof Sep 27, 2013

Member

and HttpFoundation should be in require-dev too, so that running the testsuite with the individual component does not break

This comment has been minimized.

Copy link
@webmozart

webmozart Sep 30, 2013

Author Contributor

Done.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Sep 27, 2013

You need to update the require(-dev) sections for all libraries/bundles that use the new CsrfTokenGeneratorInterface to ~2.4, e.g. twigbridge, frameworkbundle, securitybundle.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 30, 2013

Updated.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Sep 30, 2013

FrameworkBundle also needs the security for security_csrf.xml.

@webmozart

This comment has been minimized.

Copy link
Contributor Author

webmozart commented Sep 30, 2013

Updated.

*
* @param string $tokenId An ID that identifies the token
*/
public function generateCsrfToken($tokenId);

This comment has been minimized.

Copy link
@Tobion

Tobion Sep 30, 2013

Member

missing @return

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Sep 30, 2013

👍

@fabpot fabpot merged commit 7f02304 into symfony:master Sep 30, 2013
1 check failed
1 check failed
default The Travis CI build failed
Details
fabpot added a commit that referenced this pull request Oct 7, 2013
…ger and TokenGenerator (bschussek)

This PR was merged into the master branch.

Discussion
----------

[Security\Csrf] Split CsrfTokenGenerator into CsrfTokenManager and TokenGenerator

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9210
| License       | MIT
| Doc PR        | -

This is a follow-up PR of #6554 that splits the CsrfTokenGenerator into two separate classes for generating and managing CSRF tokens. As a consequence, it is now possible to explicitly remove or refresh CSRF tokens if they should be used only once. See #9210 for more information.

Commits
-------

d4bb5f4 [Security\Csrf] Split CsrfTokenGenerator into CsrfTokenManager and TokenGenerator
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jan 16, 2016
…lfiren, Aaron Valandra, xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

csrf_token_generator and csrf_token_id documentation

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes (symfony/symfony#6554, symfony/symfony#9587)
| Applies to    | 2.4+
| Fixed tickets | #3059, #5942

Commits
-------

304d7a5 finish csrf_token_generator and csrf_token_id docs
3ceb61c Improper markdown for versionadded.
91b5e2e Updated documentation as requested by @stof and @xabbuh
0044aa2 Updated csrf_in_login_form.rst to include csrf_token_id and csrf_token_generator
@chalasr chalasr mentioned this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.