#575280 follow-up by mfb and chx: Fixed impersonation attack when an https session exists.

5 files changed, 90 insertions, 24 deletions

diff --git a/includes/session.inc b/includes/session.inc
index 60d5d54a4..51e40ac75 100644
--- a/includes/session.inc
+++ b/includes/session.inc
@@ -151,12 +151,19 @@ function _drupal_session_write($sid, $value) {
     'session' => $value,
     'timestamp' => REQUEST_TIME,
   );
-  $insecure_session_name = substr(session_name(), 1);
-  if ($is_https && isset($_COOKIE[$insecure_session_name])) {
-    $fields['sid'] = $_COOKIE[$insecure_session_name];
+  $key = array('sid' => $sid);
+  if ($is_https) {
+    $key['ssid'] = $sid;
+    $insecure_session_name = substr(session_name(), 1);
+    // The "secure pages" setting allows a site to simultaneously use both
+    // secure and insecure session cookies. If enabled, use the insecure session
+    // identifier as the sid.
+    if (variable_get('https', FALSE) && isset($_COOKIE[$insecure_session_name])) {
+      $key['sid'] = $_COOKIE[$insecure_session_name];
+    }
   }
   db_merge('sessions')
-    ->key(array($is_https ? 'ssid' : 'sid' => $sid))
+    ->key($key)
     ->fields($fields)
     ->execute();
 
diff --git a/modules/simpletest/tests/common.test b/modules/simpletest/tests/common.test
index 5672961f3..483a671a0 100644
--- a/modules/simpletest/tests/common.test
+++ b/modules/simpletest/tests/common.test
@@ -760,6 +760,8 @@ class DrupalHTTPRequestTestCase extends DrupalWebTestCase {
   }
 
   function testDrupalHTTPRequest() {
+    global $is_https;
+
     // Parse URL schema.
     $missing_scheme = drupal_http_request('example.com/path');
     $this->assertEqual($missing_scheme->code, -1002, t('Returned with "-1002" error code.'));
@@ -781,18 +783,23 @@ class DrupalHTTPRequestTestCase extends DrupalWebTestCase {
     $this->assertEqual($result->code, '404', t('Result code is 404'));
     $this->assertEqual($result->status_message, 'Not Found', t('Result status message is "Not Found"'));
 
-    // Test that timeout is respected. The test machine is expected to be able
-    // to make the connection (i.e. complete the fsockopen()) in 2 seconds and
-    // return within a total of 5 seconds. If the test machine is extremely
-    // slow, the test will fail. fsockopen() has been seen to time out in
-    // slightly less than the specified timeout, so allow a little slack on the
-    // minimum expected time (i.e. 1.8 instead of 2).
-    timer_start(__METHOD__);
-    $result = drupal_http_request(url('system-test/sleep/10', array('absolute' => TRUE)), array('timeout' => 2));
-    $time = timer_read(__METHOD__) / 1000;
-    $this->assertTrue(1.8 < $time && $time < 5, t('Request timed out (%time seconds).', array('%time' => $time)));
-    $this->assertTrue($result->error, t('An error message was returned.'));
-    $this->assertEqual($result->code, HTTP_REQUEST_TIMEOUT, t('Proper error code was returned.'));
+    // Skip the timeout tests when the testing environment is HTTPS because
+    // stream_set_timeout() does not work for SSL connections.
+    // @link http://bugs.php.net/bug.php?id=47929
+    if (!$is_https) {
+      // Test that timeout is respected. The test machine is expected to be able
+      // to make the connection (i.e. complete the fsockopen()) in 2 seconds and
+      // return within a total of 5 seconds. If the test machine is extremely
+      // slow, the test will fail. fsockopen() has been seen to time out in
+      // slightly less than the specified timeout, so allow a little slack on
+      // the minimum expected time (i.e. 1.8 instead of 2).
+      timer_start(__METHOD__);
+      $result = drupal_http_request(url('system-test/sleep/10', array('absolute' => TRUE)), array('timeout' => 2));
+      $time = timer_read(__METHOD__) / 1000;
+      $this->assertTrue(1.8 < $time && $time < 5, t('Request timed out (%time seconds).', array('%time' => $time)));
+      $this->assertTrue($result->error, t('An error message was returned.'));
+      $this->assertEqual($result->code, HTTP_REQUEST_TIMEOUT, t('Proper error code was returned.'));
+    }
   }
 
   function testDrupalHTTPRequestBasicAuth() {
@@ -800,7 +807,7 @@ class DrupalHTTPRequestTestCase extends DrupalWebTestCase {
     $password = $this->randomName();
     $url = url('system-test/auth', array('absolute' => TRUE));
 
-    $auth = str_replace('http://', 'http://' . $username . ':' . $password . '@', $url);
+    $auth = str_replace('://', '://' . $username . ':' . $password . '@', $url);
     $result = drupal_http_request($auth);
 
     $this->drupalSetContent($result->data);
diff --git a/modules/simpletest/tests/https.php b/modules/simpletest/tests/https.php
index bc473875d..121e4ee17 100644
--- a/modules/simpletest/tests/https.php
+++ b/modules/simpletest/tests/https.php
@@ -12,6 +12,9 @@ if (!(isset($_SERVER['HTTP_USER_AGENT']) && (strpos($_SERVER['HTTP_USER_AGENT'],
   exit;
 }
 
+// Set a global variable to indicate a mock HTTPS request.
+$is_https_mock = empty($_SERVER['HTTPS']);
+
 // Change to https.
 $_SERVER['HTTPS'] = 'on';
 
diff --git a/modules/simpletest/tests/session.test b/modules/simpletest/tests/session.test
index 72648656c..379b82733 100644
--- a/modules/simpletest/tests/session.test
+++ b/modules/simpletest/tests/session.test
@@ -272,18 +272,62 @@ class SessionHttpsTestCase extends DrupalWebTestCase {
     global $is_https;
 
     if ($is_https) {
+      $secure_session_name = session_name();
+      $insecure_session_name = substr(session_name(), 1);
+    }
+    else {
+      $secure_session_name = 'S' . session_name();
+      $insecure_session_name = session_name();
+    }
+
+    $user = $this->drupalCreateUser(array('access administration pages'));
+
+    // Test HTTPS session handling by altering the form action to submit the
+    // login form through https.php, which creates a mock HTTPS request.
+    $this->drupalGet('user');
+    $form = $this->xpath('//form[@id="user-login"]');
+    $form[0]['action'] = $this->httpsUrl('user');
+    $edit = array('name' => $user->name, 'pass' => $user->pass_raw);
+    $this->drupalPost(NULL, $edit, t('Log in'));
+
+    // Test a second concurrent session.
+    $this->curlClose();
+    $this->drupalGet('user');
+    $form = $this->xpath('//form[@id="user-login"]');
+    $form[0]['action'] = $this->httpsUrl('user');
+    $this->drupalPost(NULL, $edit, t('Log in'));
+
+    // Check secure cookie on secure page.
+    $this->assertTrue($this->cookies[$secure_session_name]['secure'], 'The secure cookie has the secure attribute');
+    // Check insecure cookie is not set.
+    $this->assertFalse(isset($this->cookies[$insecure_session_name]));
+    $args = array_fill_keys(array(':sid', ':ssid'), $this->cookies[$secure_session_name]['value']);
+    $this->assertTrue(db_query('SELECT sid FROM {sessions} WHERE sid = :sid AND ssid = :ssid', $args)->fetchField(), 'Session has both SIDs');
+    $cookie = $secure_session_name . '=' . $args[':ssid'];
+
+    // Verify that user is logged in on secure URL.
+    $this->curlClose();
+    $this->drupalGet($this->httpsUrl('admin'), array(), array('Cookie: ' . $cookie));
+    $this->assertText(t('Administer'));
+
+    // Verify that user is not logged in on non-secure URL.
+    if (!$is_https) {
+      $this->curlClose();
+      $this->drupalGet('admin', array(), array('Cookie: ' . $cookie));
+      $this->assertNoText(t('Administer'));
+    }
+
+    // Clear browser cookie jar.
+    $this->cookies = array();
+
+    if ($is_https) {
       // The functionality does not make sense when running on https.
       return;
     }
 
-    $insecure_session_name = session_name();
-    $secure_session_name = "S$insecure_session_name";
-
     // Enable secure pages.
     variable_set('https', TRUE);
 
-    $user = $this->drupalCreateUser(array('access administration pages'));
-
     $this->curlClose();
     $this->drupalGet('session-test/set/1');
     // Check secure cookie on insecure page.
diff --git a/modules/simpletest/tests/session_test.module b/modules/simpletest/tests/session_test.module
index 01a9d3225..d479aba71 100644
--- a/modules/simpletest/tests/session_test.module
+++ b/modules/simpletest/tests/session_test.module
@@ -157,6 +157,11 @@ function session_test_form_user_login_alter(&$form) {
  * page through https.php.
  */
 function session_test_drupal_goto_alter(&$path, &$options, &$http_response_code) {
-  global $base_insecure_url;
-  $path = $base_insecure_url . '/' . $path;
+  global $base_insecure_url, $is_https_mock;
+  // Alter the redirect to use HTTP when using a mock HTTPS request through
+  // https.php because form submissions would otherwise redirect to a
+  // non-existent HTTPS site.
+  if (!empty($is_https_mock)) {
+    $path = $base_insecure_url . '/' . $path;
+  }
 }