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

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

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.
    *

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.');
+}

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();
 }
 
 /**