#575280 follow-up by mfb, carlos8f: Empty session IDs break Drupal. (courtesty of BADCamp 2010 woo)

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
@@ -223,6 +223,30 @@ class SessionTestCase extends DrupalWebTestCase {
   }
 
   /**
+   * 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.
    *
    * @param $uid User id to set as the active session.
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;
 }
@@ -104,6 +116,13 @@ function _session_test_id() {
 }
 
 /**
+ * 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.
  */
 function _session_test_set_message() {
@@ -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();
 }
 
 /**