- Patch #280934 by pwolanin, swentel, et al: harden session regeneration. It took a while, but it comes with tests and extra features now.

3 files changed, 79 insertions, 3 deletions

diff --git a/modules/simpletest/tests/session.test b/modules/simpletest/tests/session.test
index 5e2533988..751917f12 100644
--- a/modules/simpletest/tests/session.test
+++ b/modules/simpletest/tests/session.test
@@ -7,6 +7,9 @@
  */
 
 class SessionTestCase extends DrupalWebTestCase {
+
+  protected $saved_cookie;
+
   /**
    * Implementation of getInfo().
    */
@@ -24,16 +27,61 @@ class SessionTestCase extends DrupalWebTestCase {
   function setUp() {
     parent::setUp('session_test');
   }
+  /**
+   * Implementation of curlHeaderCallback().
+   */
+  protected function curlHeaderCallback($ch, $header) {
+    // Look for a Set-Cookie header.
+    if (preg_match('/^Set-Cookie.+$/i', $header, $matches)) {
+      $this->saved_cookie = $header;
+    }
+
+    return parent::curlHeaderCallback($ch, $header);
+  }
 
   /**
-   * Tests for drupal_save_session().
+   * Tests for drupal_save_session() and drupal_session_regenerate().
    */
-  function testSessionSaveSession() {
+  function testSessionSaveRegenerate() {
     $this->assertTrue(drupal_save_session(), t('drupal_save_session() correctly returns TRUE when initially called with no arguments.'), t('Session'));
     $this->assertFalse(drupal_save_session(FALSE), t('drupal_save_session() correctly returns FALSE when called with FALSE.'), t('Session'));
     $this->assertFalse(drupal_save_session(), t('drupal_save_session() correctly returns FALSE when saving has been disabled.'), t('Session'));
     $this->assertTrue(drupal_save_session(TRUE), t('drupal_save_session() correctly returns TRUE when called with TRUE.'), t('Session'));
     $this->assertTrue(drupal_save_session(), t('drupal_save_session() correctly returns TRUE when saving has been enabled.'), t('Session'));
+
+    // Test session hardening code from SA-2008-044.
+    $user = $this->drupalCreateUser(array('access content'));
+    // Enable sessions.
+    $this->sessionReset($user->uid);
+    // Make sure the session cookie is set as HttpOnly.
+    $this->drupalLogin($user);
+    $this->assertTrue(preg_match('/HttpOnly/i', $this->saved_cookie), t('Session cookie is set as HttpOnly.'));
+    $this->drupalLogout();
+    // Verify that the session is regenerated if a module calls exit
+    // in hook_user_login().
+    user_save($user, array('name' => 'session_test_user'));
+    $user->name = 'session_test_user';
+    $this->drupalGet('session-test/id');
+    $matches = array();
+    preg_match('/\s*session_id:(.*)\n/', $this->drupalGetContent(), $matches);
+    $this->assertTrue(!empty($matches[1]) , t('Found session ID before logging in.'));
+    $original_session = $matches[1];
+    // We cannot use $this->drupalLogin($user); because we exit in
+    // session_test_user_login() which breaks a normal assertion.
+    $edit = array(
+      'name' => $user->name,
+      'pass' => $user->pass_raw
+    );
+    $this->drupalPost('user', $edit, t('Log in'));
+    $this->drupalGet('node');
+    $pass = $this->assertText($user->name, t('Found name: %name', array('%name' => $user->name)), t('User login'));
+    $this->_logged_in = $pass;
+
+    $this->drupalGet('session-test/id');
+    $matches = array();
+    preg_match('/\s*session_id:(.*)\n/', $this->drupalGetContent(), $matches);
+    $this->assertTrue(!empty($matches[1]) , t('Found session ID after logging in.'));
+    $this->assertTrue($matches[1] != $original_session, t('Session ID changed after login.'));
   }
 
   /**
diff --git a/modules/simpletest/tests/session_test.module b/modules/simpletest/tests/session_test.module
index b183ec38c..842e8f014 100644
--- a/modules/simpletest/tests/session_test.module
+++ b/modules/simpletest/tests/session_test.module
@@ -11,6 +11,12 @@ function session_test_menu() {
     'access arguments' => array('access content'),
     'type' => MENU_CALLBACK,
   );
+  $items['session-test/id'] = array(
+    'title' => t('Session ID value'),
+    'page callback' => '_session_test_id',
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
   $items['session-test/set/%'] = array(
     'title' => t('Set Session value'),
     'page callback' => '_session_test_set',
@@ -58,3 +64,22 @@ function _session_test_no_set($value) {
   _session_test_set($value);
   return t('session saving was disabled, and then %val was set', array('%val' => $value));
 }
+
+/**
+ * Menu callback: print the current session ID.
+ */
+function _session_test_id() {
+  return 'session_id:' . session_id() . "\n";
+}
+
+/**
+ * Implementation of hook_user().
+ */
+function session_test_user_login($edit = array(), $user = NULL) {
+  if ($edit['name'] == 'session_test_user') {
+    // Exit so we can verify that the session was regenerated
+    // before hook_user() was called.
+    exit;
+  }
+}
+
diff --git a/modules/user/user.module b/modules/user/user.module
index 6e8b83a80..cdb912d92 100644
--- a/modules/user/user.module
+++ b/modules/user/user.module
@@ -1361,8 +1361,11 @@ function user_authenticate_finalize(&$edit) {
   // This is also used to invalidate one-time login links.
   $user->login = REQUEST_TIME;
   db_query("UPDATE {users} SET login = %d WHERE uid = %d", $user->login, $user->uid);
-  user_module_invoke('login', $edit, $user);
+  // Regenerate the session ID to prevent against session fixation attacks.
+  // This is called before hook_user in case one of those functions fails
+  // or incorrectly does a redirect which would leave the old session in place.
   drupal_session_regenerate();
+  user_module_invoke('login', $edit, $user);
 }
 
 /**