Use a random seed instead of the session_id for CSRF token generation

issue that is postponed on this is critical, so I'm pretty sure that means we want this one critical also. (or that that other issue is not actually postponed on this, or the other is not actually critical)

The proposal makes sense to me, especially given the referenced other issues.

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -289,4 +289,27 @@ protected function isCli() {
   +  protected function isEmptySession(array $session = NULL) {
   

   Any chance to rename this to just isEmpty()?

   Or if "session" is really necessary, can we at least switch it around into isSessionEmpty()?

   Second, the $session parameter doesn't make sense to me (and is undocumented).

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -289,4 +289,27 @@ protected function isCli() {
   +      $session = $_SESSION;
   ...
   +    unset($session['csrf_token_seed']);
   +
   +    if (empty($session)) {
   

   Instead of copying things around, an isset() && array_diff_key() would be much faster.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -289,4 +289,27 @@ protected function isCli() {
+  protected function isEmptySession(array $session = NULL) {

any reason for this argument? looks unused and not documented

Awesome and clean!

just fixed nits like @todo coding style and third person verb on one line summaries of methods.
changed Tests ... to Determines ... on the isSessionEmpty() cause at first I thought it was a test. :)

Why exactly do we treat the session as empty when it only has the crsf seed in it?

drupal_valid_token() does allow using tokens for anonymous users. Seems like the seed could get set differently in $_SESSION each request then discarded (since we don't save empty sessions).

Very quick look and I couldn't find test coverage for drupal_valid_token() with $skip_anonymous = TRUE.

Those unit tests don't cover the bug I'm thinking of, it would only happen when the token is generated for one request, then validated on the next.

@catch: we have no uses of fully anonymous CSRF tokens in core, and if #2245117: Remove the optional $skip_anonymous parameter from CsrfTokenGenerator::validate and remove the dependency on current_user service gets in we will completely stop supporting this.

Tokens will work as long as the user has an active session, before or after this patch.

On the other hand, trying to generate a CSRF token without an active session will not work (before or after this patch). I'm wonder if we should not try to trigger an exception in that case.

Well we should either throw an exception, or start a session.

More generally we need to figure out what to do with forms that get rendered on cached pages.

session_id() returns an empty string when there is no session, so all sessionless users get the same token. That's the only reason that anonymous users can submit forms on cached pages at all. #1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching is related.

That's already horribly broken, but the change here makes it more severe.

Arggh I'm clearly not awake. I'd completely forgotten 'no form token for anonymous users' then it got worse from there.

I think this is OK then, except we should decide what happens when generating a csrf token with no session since that could still happen with the link case, even if forms prevent it now, opened #2247259: Either create a session or throw an exception when a csrf token is generated with no session for that.

Like @catch, the only part I don't get is why we're considering a session as empty if it is actually not empty? (containing a CSRF token)

That could really use a more elaborate comment (in code) to explain the idea behind that. (if it is valid/legit in the first place)

the interdiff in #22 is nice. :)

I am all for this idea, as it will make dual mode stuff much better amongst other things. Should we consider baking this into the session manager instead though? I am not sure about how the current patch divides the logic for ignoring/setting this value.

Something like this (quick patch, not tested). If you feel this is wrong, or don't like it. Please shoot down! :) You have looked into this stuff more than I.

31: 2245003-31.patch queued for re-testing.

@znerol's last patch looks good to me. (Thanks for adding the comment/@todo!)

@damiankloip: When @znerol and I discussed this patch, I was actually arguing the other way around: SessionManager shouldn't have any knowledge about any particular $_SESSION key. The csrf_token is a custom concept of CsrfTokenManager.

Ideally, isSessionObsolete() would trigger an event or something that allows all consumers to remove their data. If no data is left after dispatching, then the session is obsolete. @znerol said that something like that might be possible when we introduce the MetadataBag. This built-in/hard-coded check within SessionManager is meant to be temporary for now.

Ok, that seems reasonable. It's just a bit crappy having that knowledge spread around like that. As well as just having the globals in the token manager.

Cleaned up docs/comments.

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -204,6 +204,13 @@ public function regenerate() {
   +    // @todo Move the token seed into Drupal\Core\Session\MetadataBag and notify
   +    //   the metadata bag when the token should be regenerated.
   +    // @see https://drupal.org/node/2238087
   

   So. I want to clarify. The @see to the issue is what this todo is dependent on, but not the issue that will do it, right?

   So I guess #2238087: Rebase SessionManager onto Symfony NativeSessionStorage would have a child follow-up to do this @todo? Unless it went in first, and then we could make the child here.

   Let's tag this needs follow-up incase we dont have issues for these @todo's when this get's rtbc.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php
   @@ -215,6 +216,75 @@ protected function testHttpsSession() {
   +   * Ensure that a CSRF form token is shared in SSL mixed mode.
   

   fixed nit to have one line summaries start with a third person verb.

   "summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""
   https://drupal.org/node/1354#functions

3. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php
   @@ -0,0 +1,51 @@
   + * Contains \Drupal\session_test\Form\SessionTestForm
   

   another nit to have this be a "sentence" , in other words, should have a period at the end.

4. Also the name of the class, when I changed it to be a third person verb, I noticed that the name includes "session" but to comment did not.

   /**
    * Provides the test config edit forms.
    */
   class SessionTestForm extends FormBase {
   

   So, I looked at the form, it just saves one text field called input.

   So it looks like this just needed to be any old form that could be saved.

   It's used here:

   session_test.form:
   +  path: '/session-test/form'
   +  defaults:
   +    _form: '\Drupal\session_test\Form\SessionTestForm'
   +    _title: 'Test form'
   +  requirements:
   +    _access: 'TRUE'
   

   Maybe we could explain why we need a form in order to test session things?
   That it is used in testCsrfTokenWithMixedModeSsl

   And I dont see where it is actually editing config...

   so I ended up with:

   /**
    * Provides a form that can be submitted, to test session tokens.
    *
    * @see \Drupal\system\Tests\Session\SessionHttpsTest::testCsrfTokenWithMixedModeSsl()
    */
   

   is this sessiontestform meant to be reused for other more generic things? Or is this doc mentioning the specific test fine?

Thanks! :-)

I don't think we need (yet) another follow-up for that @todo, since we already have a bunch of issues that are trying to refactor this code entirely.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -70,7 +73,10 @@ public function get($value = '') {
+    return $token === Crypt::hmacBase64($value, $_SESSION['csrf_token_seed'] . $this->privateKey->get() . drupal_get_hash_salt());

Why can't we just call get() again here? You are already checking the session 'csrf_token_seed' value above and returning before this if it's not there.

But if it was not present in the session it would not ever read h the get() call?

I think the computeToken() method makes it clearer as to what is going on. Looks good

Committed/pushed to 8.x, thanks!

Actually if we're ever going to fix this in 7.x, it'll probably be a backport from here.

Marking #961508: Dual http/https session cookies interfere with drupal_valid_token() as duplicate.

that interdiff in #46 was not accurate.

#46 had un-intentional changes.

diffing the two patches (which I know is messy) is:

548 $ diff session.csrf_.40.patch 2245003-use-random-seed-instead-of-session-id-for-csrf-token-46.diff
2c2
< index 9918610..b847db4 100644
---
> index 9918610..7d68750 100644
5c5,24
< @@ -55,7 +55,10 @@ public function __construct(PrivateKey $private_key) {
---
> @@ -38,7 +38,7 @@ public function __construct(PrivateKey $private_key) {
>    /**
>     * Generates a token based on $value, the user session, and the private key.
>     *
> -   * The generated token is based on the session ID of the current user. Normally,
> +   * The generated token is based on the session of the current user. Normally,
>     * anonymous users do not have a session, so the generated token will be
>     * different on every page request. To generate a token for users without a
>     * session, manually start a session prior to calling this function.
> @@ -47,15 +47,19 @@ public function __construct(PrivateKey $private_key) {
>     *   (optional) An additional value to base the token on.
>     *
>     * @return string
> -   *   A 43-character URL-safe token for validation, based on the user session
> -   *   ID, the hash salt provided by drupal_get_hash_salt(), and the
> +   *   A 43-character URL-safe token for validation, based on the token seed,
> +   *   the hash salt provided by drupal_get_hash_salt(), and the
>     *   'drupal_private_key' configuration variable.
>     *
>     * @see drupal_get_hash_salt()
13c32,33
< +    return Crypt::hmacBase64($value, $_SESSION['csrf_token_seed'] . $this->privateKey->get() . drupal_get_hash_salt());
---
> +
> +    return $this->computeToken($_SESSION['csrf_token_seed'], $value);
17c37
< @@ -70,7 +73,10 @@ public function get($value = '') {
---
> @@ -70,7 +74,28 @@ public function get($value = '') {
25c45,63
< +    return $token === Crypt::hmacBase64($value, $_SESSION['csrf_token_seed'] . $this->privateKey->get() . drupal_get_hash_salt());
---
> +
> +    return $token === $this->computeToken($_SESSION['csrf_token_seed'], $value);
> +  }
> +
> +  /**
> +   * Generates a token based on $value, the token seed, and the private key.
> +   *
> +   * @param string $seed
> +   *   The per-session token seed.
> +   * @param string $value
> +   *   (optional) An additional value to base the token on.
> +   *
> +   * @return string
> +   *   A 43-character URL-safe token for validation, based on the token seed,
> +   *   the hash salt provided by drupal_get_hash_salt(), and the
> +   *   'drupal_private_key' configuration variable.
> +   */
> +  protected function computeToken($seed, $value = '') {
> +    return Crypt::hmacBase64($value, $seed . $this->privateKey->get() . drupal_get_hash_salt());
30c68
< index 1c37dc3..f7c22a3 100644
---
> index 1c37dc3..6a08c6d 100644
51c89
< @@ -204,6 +204,13 @@ public function regenerate() {
---
> @@ -204,6 +204,14 @@ public function regenerate() {
55,57c93,96
< +    // @todo Move the token seed into Drupal\Core\Session\MetadataBag and notify
< +    //   the metadata bag when the token should be regenerated.
< +    // @see https://drupal.org/node/2238087
---
> +    // @todo As soon as https://drupal.org/node/2238087 lands, the token seed
> +    //   can be moved onto Drupal\Core\Session\MetadataBag. The session manager
> +    //   then needs to notify the metadata bag when the token should be
> +    //   regenerated.
65c104
< @@ -289,4 +296,28 @@ protected function isCli() {
---
> @@ -289,4 +297,30 @@ protected function isCli() {
70c109
< +   * Determines whether the session does not contain user data.
---
> +   * Determines whether the session contains user data.
73,74c112,113
< +   *   TRUE when the session does not contain any values and can be destroyed,
< +   *   FALSE otherwise.
---
> +   *   TRUE when the session does not contain any values and therefore can be
> +   *   destroyed.
83,89c122,130
< +    // @todo Anonymous users normally do not get a CSRF token, but if they do,
< +    //   then the originating code is responsible for cleaning up the session
< +    //   once obsolete. Since that is not guaranteed to be the case, this check
< +    //   force-ignores the CSRF token, so as to avoid performance regressions.
< +    // @todo Move the CSRF token seed into \Drupal\Core\Session\MetadataBag, so
< +    //   it will be ignored automatically.
< +    // @see https://drupal.org/node/2238087
---
> +    //
> +    // @todo Anonymous users should not get a CSRF token at any time, or if they
> +    //   do, then the originating code is responsible for cleaning up the
> +    //   session once obsolete. Since that is not guaranteed to be the case,
> +    //   this check force-ignores the CSRF token, so as to avoid performance
> +    //   regressions.
> +    //   As soon as https://drupal.org/node/2238087 lands, the token seed can be
> +    //   moved onto \Drupal\Core\Session\MetadataBag. This will result in the
> +    //   CSRF token to be ignored automatically.
95c136
< index 02523d0..18bd3bb 100644
---
> index 02523d0..f1779f4 100644
98c139
< @@ -10,9 +10,10 @@
---
> @@ -10,6 +10,7 @@
105,109c146
< - * Ensure that when running under HTTPS two session cookies are generated.
< + * Tests that when running under HTTPS two session cookies are generated.
<   */
<  class SessionHttpsTest extends WebTestBase {
<
---
>   * Ensure that when running under HTTPS two session cookies are generated.
174c211
< +   * Returns the token of the current form.
---
> +   * Return the token of the current form.
188c225
< index 0000000..e611b1e
---
> index 0000000..706c79c
191c228
< @@ -0,0 +1,53 @@
---
> @@ -0,0 +1,51 @@
196c233
< + * Contains \Drupal\session_test\Form\SessionTestForm.
---
> + * Contains Drupal\session_test\Form\SessionTestForm
205,207c242
< + * Provides a form that can be submitted, to test session tokens.
< + *
< + * @see \Drupal\system\Tests\Session\SessionHttpsTest::testCsrfTokenWithMixedModeSsl()
---
> + * Form controller for the test config edit forms.
262c297
< index 8607304..a205731 100644
---
> index 8607304..81ac83d 100644
270c305
< +    $this->generator->get();
---
> +    $ignored_token = $this->generator->get();
280c315
< +    $this->generator->get();
---
> +    $ignored_token = $this->generator->get();

so, this looks like a reasonable change, but I don't really see how it adds to security other than by decoupling session ID itself from CSRF tokens. However, if the session cookie is sniffed, you can get the token values anyhow by becoming authenticated?

one of the changes not shown in the interdiff
< + $this->generator->get();
---
> + $ignored_token = $this->generator->get();

was adding an unused var.

adding #2081153: Remove unused local variables from core/tests as related now...

Here is a backport to Drupal 7, as best as I can figure out. The only thing I couldn't figure out was where Drupal\Tests\Core\Access\CsrfTokenGeneratorTest lives. Do we not have tests for drupal_random_key() or drupal_hmac_base64() in D7?

I deliberately didn't backport #2245117: Remove the optional $skip_anonymous parameter from CsrfTokenGenerator::validate and remove the dependency on current_user service since that's an API change, so if $skip_anonymous is passed, then the token will be considered valid.

Missed changing isSecure() back to the global $is_https.

Huh. Don't know how that test worked on my local site.

Changed drupalPostForm() to drupalPost().

Forgot to close function documentation. Awesome.

And to reset the status. Ugh.

I think storing something in $_SESSION will actually write the session?

Did you verify that a SESSION cookie is not send to the user if drupal_session_is_obsolete() is true.

For most reverse proxies, it is not enough to send public headers, they also check for existance of SESSION cookie.

Wouldn't it be better to _only_ use the SESSION seed if we have an authenticated user in the first place?

Is drupal_get_token() defined if a user is anonymous? What does it 'mean' for the user - if the token is either shared anyway (cached) or not working (cached)?

Or should any drupal_get_token() call actually always create a SESSION to distinguish for anonymous users, but then the obsolete part is pretty senseless.

I am really sorry for all that worked on this issue, but after reviewing my last comment even the original patch in 8.0.x is broken.

I spoke with catch and I am putting this back to 8.0.x and I think the sanest would be to revert the patch for now and do a new one for 8.0.x first.

Points that should be fixed are:

- There is two approaches to do drupal_get_token() for anonymous users:

a) Create one token that is shared - this allows it to work with caching, but does only prevent CSRF attacks if the attacker does not visit the site to get the current token, so not really useful. In this case performance would be kept, but security would suffer.

b) Create a token that is not shared, but treat the anonymous user as 'logged in anonymous' from then on with no caching, etc. in place anymore. This is no worse than opening a session before in any module like a shopping cart. If someone calls drupal_get_token for anon users they need to fix their code according to their business logic to make sense.

- The session_is_obsolete() stuff should be completely removed - if we have a SESSION we have a session, no way around that.

- Either do not set SESSION as seed for anon users (and use a fixed seed) OR open a SESSION, but keep it open.

Overall I agree with the random seed in SESSION and overall we need to speak about tokens in core with render_caching more - see e.g. cacheable_csrf module and if the hardening could be optional for system that need to cache more, but that is follow-up.

Thanks and sorry for being too late for this party!

I was able to test it and yes it works correctly - I misread the patch.

I still think anonymous token generation is totally broken, but that is indeed another issue.

Question on the D7 patch:

- Why not a simple isset($_SESSION['csrf_token']) instead of the complicated count(array_intersect_key(...)) == 0 thingy?

Oh, yes you are right.

In that case RTBC - it is no worse than before.

Back to RTBC

@Fabianx is the issue you mentioned in #69 still needed?

Nope, this is okay. I misread the patch, we still need to talk about anonymous tokens in general somewhere, but that is not critical.

 function drupal_valid_token($token, $value = '', $skip_anonymous = FALSE) {
   global $user;
-  return (($skip_anonymous && $user->uid == 0) || ($token === drupal_get_token($value)));
+  if (!$skip_anonymous && empty($_SESSION['csrf_token_seed'])) {
+    return FALSE;
+  }
+
+  return (($skip_anonymous && $user->uid == 0) || ($token === drupal_compute_token($_SESSION['csrf_token_seed'], $value)));

I am not sure the logic here is correct. If the caller set $skip_anonymous to TRUE but the current user is not anonymous, don't we need to bail out early when $_SESSION['csrf_token_seed'] is empty then also? Otherwise, seems like there would be PHP notices, at least.

-  if (empty($user->uid) && empty($_SESSION)) {
+  if (empty($user->uid) && drupal_session_obsolete()) {

I have to say these kinds of changes look scary, though I understand why they're there. Essentially it's deliberately preserving an existing bug (where drupal_get_token() doesn't work at all for anonymous users who don't have a session) for the sake of not hurting performance :)

But something just seems fishy about putting data in $_SESSION and then throwing it out later in the page request... any code that is examining $_SESSION in the interim could get confused by that.

I am wondering if another way to do this would be to have drupal_get_token() not set the session value directly (at least for anonymous users), but rather do something to indicate that it wants a session to be set. Call it drupal_set_conditional_session_data($key, $value) or something like that. Later on, drupal_session_commit() checks the data from that function and and adds it to $_SESSION only if something else is already in there (and also removes it from $_SESSION if it's already there from a previous request but $_SESSION is otherwise empty). In that case drupal_session_initialize() wouldn't need to change at all, I don't think.

Does that make sense? Pros/cons? I think that might make the code easier to understand plus a bit safer as well (this patch is already scary enough for a stable release as it is :)

+ * Generates a token_based on $value, the token seed, and the private key.

(minor) "token_based" => "token based"

I 100% agree with #80, that have also been my concerns. Just opening a SESSION to then not use it was like ... ugh ... for me.

It still all works correctly, but yes scary changes.

I agree that your way is much better with the conditional session data.

Thanks!

For 8, did this change actually weaken the CSRF prevention system compared to what it could be since the seed is stored plain in the session data?

Now this makes it a little closer to being able to CSRF you with values from a DB dump, with your protect falling back to maybe only the hope that the hash salt is in the code or on the filesystem and not exposed at the same time.

If we generated the token based on something from the cookie (the session ID) sent by the browser, Drupal would never be storing the raw value in 8.

Checking status on this critical issue. Has anyone had a chance to consider #80-82 in the last year in terms of a new patch we could try?

just re-uploading for the bots. Can't test the patch in 64.

It looks like this is still broken, then. Did I see right from the test that it's failing in a mixed-mode scenario? Seems like that would be predicted, based on the above conversations.

I'm sure this is a stupid question, but is the test system configured to support the https conf setting that allows mixed-mode?
$conf['https'] = TRUE;

FYI the patch in #87 resolves a problem I had on one site where SecurePages was causing an infinite loop of redirects. I'll try to help fix the remaining tests.

Rerolled.

The login attempt on line 555 of session.test results in a failed login attempt..

What's the code in testCsrfTokenWithMixedModeSsl() supposed to actually do?

I have applied #95:
Hunk #1 FAILED at 5187
And after that I could not use the forms in my site as I got every time the following message:
The form has become outdated. Copy any unsaved work in the form below

Rerolled.

#100 solve the #99 issues.
Many thanks for this updated patch.
There are still fews warning when applying it:

patching file includes/common.inc
Hunk #1 succeeded at 5209 (offset 7 lines).
Hunk #2 succeeded at 5245 (offset 7 lines).
patching file includes/session.inc
patching file modules/simpletest/tests/session.test
patching file modules/simpletest/tests/session_test.module

Rerolled.

Quick reroll to make it apply cleanly.

Whoops, sorry about that.

Reverting to proper status.