From b73b86b8aa8dfdff9897c61a321dda792606085d Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Wed, 30 Apr 2014 10:47:49 +0100 Subject: [PATCH] Issue #2245003 by znerol, YesCT, sun, damiankloip: Use a random seed instead of the session_id for CSRF token generation. --- .../Drupal/Core/Access/CsrfTokenGenerator.php | 35 ++++++++-- .../Drupal/Core/Session/SessionManager.php | 38 +++++++++- .../system/Tests/Session/SessionHttpsTest.php | 70 +++++++++++++++++++ .../session_test/Form/SessionTestForm.php | 51 ++++++++++++++ .../session_test/session_test.routing.yml | 8 +++ .../Core/Access/CsrfTokenGeneratorTest.php | 6 ++ 6 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php diff --git a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php index 9918610ae2c..7d68750714c 100644 --- a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php +++ b/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()); } } diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php index 1c37dc3264c..6a08c6d64a5 100644 --- a/core/lib/Drupal/Core/Session/SessionManager.php +++ b/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; + } + } diff --git a/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php index 02523d0c3b3..f1779f457ee 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php +++ b/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. * diff --git a/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php new file mode 100644 index 00000000000..706c79c0a8b --- /dev/null +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/Form/SessionTestForm.php @@ -0,0 +1,51 @@ + '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']))); + } + +} diff --git a/core/modules/system/tests/modules/session_test/session_test.routing.yml b/core/modules/system/tests/modules/session_test/session_test.routing.yml index b1dc48954f9..b044fc42025 100644 --- a/core/modules/system/tests/modules/session_test/session_test.routing.yml +++ b/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' diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php index 86073047534..81ac83d5202 100644 --- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php +++ b/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); }