From a3fab0edade68b9748d40b04bc7b48ee69b7fe3e Mon Sep 17 00:00:00 2001 From: Angie Byron Date: Sat, 13 Nov 2010 17:40:09 +0000 Subject: #575280 follow-up by mfb, carlos8f: Empty session IDs break Drupal. (courtesty of BADCamp 2010 woo) --- includes/session.inc | 9 ++++----- modules/simpletest/tests/session.test | 24 ++++++++++++++++++++++++ modules/simpletest/tests/session_test.module | 26 ++++++++++++++++++++++++++ modules/system/system.install | 5 ++++- 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/includes/session.inc b/includes/session.inc index c23c23e1c..23af6bd64 100644 --- a/includes/session.inc +++ b/includes/session.inc @@ -88,10 +88,7 @@ function _drupal_session_read($sid) { // a HTTPS session or we are about to log in so we check the sessions table // for an anonymous session with the non-HTTPS-only cookie. if ($is_https) { - // Ensure that an empty secure session ID cannot be selected. - if ($sid) { - $user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.ssid = :ssid", array(':ssid' => $sid))->fetchObject(); - } + $user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.ssid = :ssid", array(':ssid' => $sid))->fetchObject(); if (!$user) { if (isset($_COOKIE[$insecure_session_name])) { $user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid AND s.uid = 0", array( @@ -239,7 +236,9 @@ function drupal_session_initialize() { session_set_save_handler('_drupal_session_open', '_drupal_session_close', '_drupal_session_read', '_drupal_session_write', '_drupal_session_destroy', '_drupal_session_garbage_collection'); - if (isset($_COOKIE[session_name()]) || ($is_https && variable_get('https', FALSE) && isset($_COOKIE[substr(session_name(), 1)]))) { + // We use !empty() in the following check to ensure that blank session IDs + // are not valid. + if (!empty($_COOKIE[session_name()]) || ($is_https && variable_get('https', FALSE) && !empty($_COOKIE[substr(session_name(), 1)]))) { // If a session cookie exists, initialize the session. Otherwise the // session is only started on demand in drupal_session_commit(), making // anonymous users not use a session cookie unless something is stored in diff --git a/modules/simpletest/tests/session.test b/modules/simpletest/tests/session.test index f02cbef40..a42adcf36 100644 --- a/modules/simpletest/tests/session.test +++ b/modules/simpletest/tests/session.test @@ -222,6 +222,30 @@ class SessionTestCase extends DrupalWebTestCase { $this->assertNotEqual($times5->timestamp, $times4->timestamp, t('Sessions table was updated.')); } + /** + * Test that empty session IDs are not allowed. + */ + function testEmptySessionID() { + $user = $this->drupalCreateUser(array('access content')); + $this->drupalLogin($user); + $this->drupalGet('session-test/is-logged-in'); + $this->assertResponse(200, t('User is logged in.')); + + // Reset the sid in {sessions} to a blank string. This may exist in the + // wild in some cases, although we normally prevent it from happening. + db_query("UPDATE {sessions} SET sid = '' WHERE uid = :uid", array(':uid' => $user->uid)); + // Send a blank sid in the session cookie, and the session should no longer + // be valid. Closing the curl handler will stop the previous session ID + // from persisting. + $this->curlClose(); + $this->additionalCurlOptions[CURLOPT_COOKIE] = rawurlencode($this->session_name) . '=;'; + $this->drupalGet('session-test/id-from-cookie'); + $this->assertRaw("session_id:\n", t('Session ID is blank as sent from cookie header.')); + // Assert that we have an anonymous session now. + $this->drupalGet('session-test/is-logged-in'); + $this->assertResponse(403, t('An empty session ID is not allowed.')); + } + /** * Reset the cookie file so that it refers to the specified user. * diff --git a/modules/simpletest/tests/session_test.module b/modules/simpletest/tests/session_test.module index bf6c302e2..61aa3ae95 100644 --- a/modules/simpletest/tests/session_test.module +++ b/modules/simpletest/tests/session_test.module @@ -17,6 +17,12 @@ function session_test_menu() { 'access arguments' => array('access content'), 'type' => MENU_CALLBACK, ); + $items['session-test/id-from-cookie'] = array( + 'title' => 'Session ID from cookie', + 'page callback' => '_session_test_id_from_cookie', + 'access arguments' => array('access content'), + 'type' => MENU_CALLBACK, + ); $items['session-test/set/%'] = array( 'title' => 'Set session value', 'page callback' => '_session_test_set', @@ -49,6 +55,12 @@ function session_test_menu() { 'access arguments' => array('access content'), 'type' => MENU_CALLBACK, ); + $items['session-test/is-logged-in'] = array( + 'title' => 'Check if user is logged in', + 'page callback' => '_session_test_is_logged_in', + 'access callback' => 'user_is_logged_in', + 'type' => MENU_CALLBACK, + ); return $items; } @@ -103,6 +115,13 @@ function _session_test_id() { return 'session_id:' . session_id() . "\n"; } +/** + * Menu callback: print the current session ID as read from the cookie. + */ +function _session_test_id_from_cookie() { + return 'session_id:' . $_COOKIE[session_name()] . "\n"; +} + /** * Menu callback, sets a message to me displayed on the following page. */ @@ -165,3 +184,10 @@ function session_test_drupal_goto_alter(&$path, &$options, &$http_response_code) $path = $base_insecure_url . '/' . $path; } } + +/** + * Menu callback, only available if current user is logged in. + */ +function _session_test_is_logged_in() { + return t('User is logged in.'); +} diff --git a/modules/system/system.install b/modules/system/system.install index 94c0bb2ee..ad76e11be 100644 --- a/modules/system/system.install +++ b/modules/system/system.install @@ -2911,7 +2911,10 @@ function system_update_7065() { 'length' => 128, 'not null' => TRUE, ); - db_change_field('sessions', 'sid', 'sid', $spec); + db_drop_primary_key('sessions'); + db_change_field('sessions', 'sid', 'sid', $spec, array('primary key' => array('sid', 'ssid'))); + // Delete any sessions with empty session ID. + db_delete('sessions')->condition('sid', '')->execute(); } /** -- cgit v1.2.3