Issue #2245003 by znerol, YesCT, sun, damiankloip: Use a random seed instead of the session_id for CSRF token generation.

core/lib/Drupal/Core/Access/CsrfTokenGenerator.php

@@ -38,7 +38,7 @@ class CsrfTokenGenerator {
   /**
    * 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 @@ class CsrfTokenGenerator {
    *   (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()
    * @see \Drupal\Core\Session\SessionManager::start()
    */
   public function get($value = '') {
-    return Crypt::hmacBase64($value, session_id() . $this->privateKey->get() . drupal_get_hash_salt());
+    if (empty($_SESSION['csrf_token_seed'])) {
+      $_SESSION['csrf_token_seed'] = Crypt::randomBytesBase64();
+    }
+
+    return $this->computeToken($_SESSION['csrf_token_seed'], $value);
   }
 
   /**
@@ -70,7 +74,28 @@ class CsrfTokenGenerator {
    *   TRUE for a valid token, FALSE for an invalid token.
    */
   public function validate($token, $value = '') {
-    return $token === $this->get($value);
+    if (empty($_SESSION['csrf_token_seed'])) {
+      return FALSE;
+    }
+
+    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());
   }
 
 }

core/lib/Drupal/Core/Session/SessionManager.php

@@ -83,7 +83,7 @@ class SessionManager implements SessionManagerInterface {
       // anonymous users not use a session cookie unless something is stored in
       // $_SESSION. This allows HTTP proxies to cache anonymous pageviews.
       $this->start();
-      if ($user->isAuthenticated() || !empty($_SESSION)) {
+      if ($user->isAuthenticated() || !$this->isSessionObsolete()) {
         drupal_page_is_cacheable(FALSE);
       }
     }
@@ -138,7 +138,7 @@ class SessionManager implements SessionManagerInterface {
       return;
     }
 
-    if ($user->isAnonymous() && empty($_SESSION)) {
+    if ($user->isAnonymous() && $this->isSessionObsolete()) {
       // There is no session data to store, destroy the session if it was
       // previously started.
       if ($this->isStarted()) {
@@ -204,6 +204,14 @@ class SessionManager implements SessionManagerInterface {
     }
     session_id(Crypt::randomBytesBase64());
 
+    // @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.
+    if (!empty($_SESSION)) {
+      unset($_SESSION['csrf_token_seed']);
+    }
+
     if (isset($old_session_id)) {
       $params = session_get_cookie_params();
       $expire = $params['lifetime'] ? REQUEST_TIME + $params['lifetime'] : 0;
@@ -289,4 +297,30 @@ class SessionManager implements SessionManagerInterface {
     return PHP_SAPI === 'cli';
   }
 
+  /**
+   * Determines whether the session contains user data.
+   *
+   * @return bool
+   *   TRUE when the session does not contain any values and therefore can be
+   *   destroyed.
+   */
+  protected function isSessionObsolete() {
+    // Return early when $_SESSION is empty or not initialized.
+    if (empty($_SESSION)) {
+      return TRUE;
+    }
+
+    // Ignore the CSRF token seed.
+    //
+    // @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.
+    return count(array_diff_key($_SESSION, array('csrf_token_seed' => TRUE))) == 0;
+  }
+
 }

core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php

@@ -10,6 +10,7 @@ namespace Drupal\system\Tests\Session;
 use Drupal\simpletest\WebTestBase;
 use Symfony\Component\HttpFoundation\Request;
 use Drupal\Component\Utility\Crypt;
+use Drupal\Component\Utility\String;
 
 /**
  * Ensure that when running under HTTPS two session cookies are generated.
@@ -214,6 +215,75 @@ class SessionHttpsTest extends WebTestBase {
     $this->assertResponse(200);
   }
 
+  /**
+   * Ensure that a CSRF form token is shared in SSL mixed mode.
+   */
+  protected function testCsrfTokenWithMixedModeSsl() {
+    if ($this->request->isSecure()) {
+      $secure_session_name = session_name();
+      $insecure_session_name = substr(session_name(), 1);
+    }
+    else {
+      $secure_session_name = 'S' . session_name();
+      $insecure_session_name = session_name();
+    }
+
+    // Enable mixed mode SSL.
+    $this->settingsSet('mixed_mode_sessions', TRUE);
+    // Write that value also into the test settings.php file.
+    $settings['settings']['mixed_mode_sessions'] = (object) array(
+      'value' => TRUE,
+      'required' => TRUE,
+    );
+    $this->writeSettings($settings);
+
+    $user = $this->drupalCreateUser(array('access administration pages'));
+
+    // Login using the HTTPS user-login form.
+    $this->drupalGet('user');
+    $form = $this->xpath('//form[@id="user-login-form"]');
+    $form[0]['action'] = $this->httpsUrl('user');
+    $edit = array('name' => $user->getUsername(), 'pass' => $user->pass_raw);
+    $this->drupalPostForm(NULL, $edit, t('Log in'));
+
+    // Collect session id cookies.
+    $sid = $this->cookies[$insecure_session_name]['value'];
+    $ssid = $this->cookies[$secure_session_name]['value'];
+    $this->assertSessionIds($sid, $ssid, 'Session has both secure and insecure SIDs');
+
+    // Retrieve the form via HTTP.
+    $this->curlClose();
+    $this->drupalGet($this->httpUrl('session-test/form'), array(), array('Cookie: ' . $insecure_session_name . '=' . $sid));
+    $http_token = $this->getFormToken();
+
+    // Verify that submitting form values via HTTPS to a form originally
+    // retrieved over HTTP works.
+    $form = $this->xpath('//form[@id="session-test-form"]');
+    $form[0]['action'] = $this->httpsUrl('session-test/form');
+    $edit = array('input' => $this->randomName(32));
+    $this->curlClose();
+    $this->drupalPostForm(NULL, $edit, 'Save', array('Cookie: ' . $secure_session_name . '=' . $ssid));
+    $this->assertText(String::format('Ok: @input', array('@input' => $edit['input'])));
+
+    // Retrieve the same form via HTTPS.
+    $this->curlClose();
+    $this->drupalGet($this->httpsUrl('session-test/form'), array(), array('Cookie: ' . $secure_session_name . '=' . $ssid));
+    $https_token = $this->getFormToken();
+
+    // Verify that CSRF token values are the same for a form regardless of
+    // whether it was accessed via HTTP or HTTPS when SSL mixed mode is enabled.
+    $this->assertEqual($http_token, $https_token, 'Form token is the same on HTTP as well as HTTPS form');
+  }
+
+  /**
+   * Return the token of the current form.
+   */
+  protected function getFormToken() {
+    $token_fields = $this->xpath('//input[@name="form_token"]');
+    $this->assertEqual(count($token_fields), 1, 'One form token field on the page');
+    return (string) $token_fields[0]['value'];
+  }
+
   /**
    * Test that there exists a session with two specific session IDs.
    *

core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php

@@ -0,0 +1,51 @@
+<?php
+
+/**
+ * @file
+ * Contains Drupal\session_test\Form\SessionTestForm
+ */
+
+namespace Drupal\session_test\Form;
+
+use Drupal\Component\Utility\String;
+use Drupal\Core\Form\FormBase;
+
+/**
+ * Form controller for the test config edit forms.
+ */
+class SessionTestForm extends FormBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getFormID() {
+    return 'session_test_form';
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state) {
+    $form['input'] = array(
+      '#type' => 'textfield',
+      '#title' => 'Input',
+      '#required' => TRUE,
+    );
+
+    $form['actions'] = array('#type' => 'actions');
+    $form['actions']['submit'] = array(
+      '#type' => 'submit',
+      '#value' => 'Save',
+    );
+
+    return $form;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, array &$form_state) {
+    drupal_set_message(String::format('Ok: @input', array('@input' => $form_state['values']['input'])));
+  }
+
+}

core/modules/system/tests/modules/session_test/session_test.routing.yml

@@ -75,3 +75,11 @@ session_test.is_logged_in:
     _content: '\Drupal\session_test\Controller\SessionTestController::isLoggedIn'
   requirements:
     _user_is_logged_in: 'TRUE'
+
+session_test.form:
+  path: '/session-test/form'
+  defaults:
+    _form: '\Drupal\session_test\Form\SessionTestForm'
+    _title: 'Test form'
+  requirements:
+    _access: 'TRUE'

core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php

@@ -84,6 +84,9 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
    * @dataProvider providerTestValidateParameterTypes
    */
   public function testValidateParameterTypes($token, $value) {
+    // Ensure that there is a valid token seed on the session.
+    $ignored_token = $this->generator->get();
+
     // The following check might throw PHP fatals and notices, so we disable
     // error assertions.
     set_error_handler(function () {return TRUE;});
@@ -117,6 +120,9 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
    * @expectedException InvalidArgumentException
    */
   public function testInvalidParameterTypes($token, $value = '') {
+    // Ensure that there is a valid token seed on the session.
+    $ignored_token = $this->generator->get();
+
     $this->generator->validate($token, $value);
   }