diff options
-rw-r--r-- | includes/session.inc | 3 | ||||
-rw-r--r-- | modules/simpletest/tests/session.test | 52 | ||||
-rw-r--r-- | modules/simpletest/tests/session_test.module | 25 | ||||
-rw-r--r-- | modules/user/user.module | 5 |
4 files changed, 82 insertions, 3 deletions
diff --git a/includes/session.inc b/includes/session.inc index c9113982f..aae3f29ff 100644 --- a/includes/session.inc +++ b/includes/session.inc @@ -163,6 +163,9 @@ function _sess_write($key, $value) { */ function drupal_session_regenerate() { $old_session_id = session_id(); + extract(session_get_cookie_params()); + // Set "httponly" to TRUE to reduce the risk of session stealing via XSS. + session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE); session_regenerate_id(); db_update('sessions') ->fields(array( 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); } /** |