From e920fe34ef16d30af0f4fb8e33b565e572ab30c8 Mon Sep 17 00:00:00 2001 From: Dries Buytaert Date: Fri, 5 Nov 2010 19:05:02 +0000 Subject: [PATCH] - Patch #575280 by mfb, carlos8f, chx, bleen18: impersonation when an https session exists. --- includes/bootstrap.inc | 65 +++++++++++-------- includes/session.inc | 29 +++++---- includes/update.inc | 2 +- modules/simpletest/simpletest.test | 42 ++++++++++-- modules/simpletest/tests/http.php | 33 ++++++++++ modules/simpletest/tests/https.php | 24 ++++--- modules/simpletest/tests/session.test | 61 +++++++++++++++-- modules/simpletest/tests/upgrade/upgrade.test | 5 ++ modules/system/system.install | 18 ++++- 9 files changed, 214 insertions(+), 65 deletions(-) create mode 100644 modules/simpletest/tests/http.php diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index 6f695814adf..e83832d5b32 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -2134,15 +2134,7 @@ function _drupal_bootstrap_database() { // The user agent header is used to pass a database prefix in the request when // running tests. However, for security reasons, it is imperative that we // validate we ourselves made the request. - if (isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^(simpletest\d+);/", $_SERVER['HTTP_USER_AGENT'], $matches)) { - if (!drupal_valid_test_ua($_SERVER['HTTP_USER_AGENT'])) { - header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden'); - exit; - } - - // The first part of the user agent is the prefix itself. - $test_prefix = $matches[1]; - + if ($test_prefix = drupal_valid_test_ua()) { // Set the test run id for use in other parts of Drupal. $test_info = &$GLOBALS['drupal_test_info']; $test_info['test_run_id'] = $test_prefix; @@ -2221,22 +2213,40 @@ function drupal_get_bootstrap_phase() { } /** - * Validate the HMAC and timestamp of a user agent header from simpletest. + * Checks the current User-Agent string to see if this is an internal request + * from SimpleTest. If so, returns the test prefix for this test. + * + * @return + * Either the simpletest prefix (the string "simpletest" followed by any + * number of digits) or FALSE if the user agent does not contain a valid + * HMAC and timestamp. */ -function drupal_valid_test_ua($user_agent) { +function drupal_valid_test_ua() { global $drupal_hash_salt; + // No reason to reset this. + static $test_prefix; - list($prefix, $time, $salt, $hmac) = explode(';', $user_agent); - $check_string = $prefix . ';' . $time . ';' . $salt; - // We use the salt from settings.php to make the HMAC key, since - // the database is not yet initialized and we can't access any Drupal variables. - // The file properties add more entropy not easily accessible to others. - $filepath = DRUPAL_ROOT . '/includes/bootstrap.inc'; - $key = $drupal_hash_salt . filectime($filepath) . fileinode($filepath); - $time_diff = REQUEST_TIME - $time; - // Since we are making a local request a 5 second time window is allowed, - // and the HMAC must match. - return ($time_diff >= 0) && ($time_diff <= 5) && ($hmac == drupal_hmac_base64($check_string, $key)); + if (isset($test_prefix)) { + return $test_prefix; + } + + if (isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $_SERVER['HTTP_USER_AGENT'], $matches)) { + list(, $prefix, $time, $salt, $hmac) = $matches; + $check_string = $prefix . ';' . $time . ';' . $salt; + // We use the salt from settings.php to make the HMAC key, since + // the database is not yet initialized and we can't access any Drupal variables. + // The file properties add more entropy not easily accessible to others. + $key = $drupal_hash_salt . filectime(__FILE__) . fileinode(__FILE__); + $time_diff = REQUEST_TIME - $time; + // Since we are making a local request a 5 second time window is allowed, + // and the HMAC must match. + if ($time_diff >= 0 && $time_diff <= 5 && $hmac == drupal_hmac_base64($check_string, $key)) { + $test_prefix = $prefix; + return $test_prefix; + } + } + + return FALSE; } /** @@ -2250,13 +2260,12 @@ function drupal_generate_test_ua($prefix) { // We use the salt from settings.php to make the HMAC key, since // the database is not yet initialized and we can't access any Drupal variables. // The file properties add more entropy not easily accessible to others. - $filepath = DRUPAL_ROOT . '/includes/bootstrap.inc'; - $key = $drupal_hash_salt . filectime($filepath) . fileinode($filepath); + $key = $drupal_hash_salt . filectime(__FILE__) . fileinode(__FILE__); } - // Generate a moderately secure HMAC based on the database credentials. - $salt = uniqid('', TRUE); - $check_string = $prefix . ';' . time() . ';' . $salt; - return $check_string . ';' . drupal_hmac_base64($check_string, $key); + // Generate a moderately secure HMAC based on the database credentials. + $salt = uniqid('', TRUE); + $check_string = $prefix . ';' . time() . ';' . $salt; + return $check_string . ';' . drupal_hmac_base64($check_string, $key); } /** diff --git a/includes/session.inc b/includes/session.inc index 412db118ac8..c23c23e1cc3 100644 --- a/includes/session.inc +++ b/includes/session.inc @@ -88,7 +88,10 @@ 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) { - $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(); + // 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(); + } 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( @@ -180,21 +183,23 @@ function _drupal_session_write($sid, $value) { 'timestamp' => REQUEST_TIME, ); - // The "secure pages" setting allows a site to simultaneously use both - // secure and insecure session cookies. If enabled and both cookies are - // presented then use both keys. If not enabled but on HTTPS then use the - // PHP session id as 'ssid'. If on HTTP then use the PHP session id as - // 'sid'. + // Use the session ID as 'sid' and an empty string as 'ssid' by default. + // _drupal_session_read() does not allow empty strings so that's a safe + // default. + $key = array('sid' => $sid, 'ssid' => ''); + // On HTTPS connections, use the session ID as both 'sid' and 'ssid'. if ($is_https) { $key['ssid'] = $sid; - $insecure_session_name = substr(session_name(), 1); - if (variable_get('https', FALSE) && isset($_COOKIE[$insecure_session_name])) { - $key['sid'] = $_COOKIE[$insecure_session_name]; + // The "secure pages" setting allows a site to simultaneously use both + // secure and insecure session cookies. If enabled and both cookies are + // presented then use both keys. + if (variable_get('https', FALSE)) { + $insecure_session_name = substr(session_name(), 1); + if (isset($_COOKIE[$insecure_session_name])) { + $key['sid'] = $_COOKIE[$insecure_session_name]; + } } } - else { - $key['sid'] = $sid; - } db_merge('sessions') ->key($key) diff --git a/includes/update.inc b/includes/update.inc index c352cf8a3bb..5fdc1698155 100644 --- a/includes/update.inc +++ b/includes/update.inc @@ -701,7 +701,7 @@ function update_fix_d7_requirements() { } // Add ssid column and index. - db_add_field('sessions', 'ssid', array('description' => "Secure session ID. The value is generated by PHP's Session API.", 'type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => '')); + db_add_field('sessions', 'ssid', array('description' => "Secure session ID. The value is generated by Drupal's session handlers.", 'type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => '')); db_add_index('sessions', 'ssid', array('ssid')); // Drop existing primary key. db_drop_primary_key('sessions'); diff --git a/modules/simpletest/simpletest.test b/modules/simpletest/simpletest.test index a457d132946..dbed3676006 100644 --- a/modules/simpletest/simpletest.test +++ b/modules/simpletest/simpletest.test @@ -77,6 +77,43 @@ class SimpleTestFunctionalTest extends DrupalWebTestCase { } } + /** + * Test validation of the User-Agent header we use to perform test requests. + */ + function testUserAgentValidation() { + if (!$this->inCURL()) { + global $base_url; + $simpletest_path = $base_url . '/' . drupal_get_path('module', 'simpletest'); + $HTTP_path = $simpletest_path .'/tests/http.php?q=node'; + $https_path = $simpletest_path .'/tests/https.php?q=node'; + // Generate a valid simpletest User-Agent to pass validation. + $this->assertTrue(preg_match('/simpletest\d+/', $this->databasePrefix, $matches), t('Database prefix contains simpletest prefix.')); + $test_ua = drupal_generate_test_ua($matches[0]); + $this->additionalCurlOptions = array(CURLOPT_USERAGENT => $test_ua); + + // Test pages only available for testing. + $this->drupalGet($HTTP_path); + $this->assertResponse(200, t('Requesting http.php with a legitimate simpletest User-Agent returns OK.')); + $this->drupalGet($https_path); + $this->assertResponse(200, t('Requesting https.php with a legitimate simpletest User-Agent returns OK.')); + + // Now slightly modify the HMAC on the header, which should not validate. + $this->additionalCurlOptions = array(CURLOPT_USERAGENT => $test_ua . 'X'); + $this->drupalGet($HTTP_path); + $this->assertResponse(403, t('Requesting http.php with a bad simpletest User-Agent fails.')); + $this->drupalGet($https_path); + $this->assertResponse(403, t('Requesting https.php with a bad simpletest User-Agent fails.')); + + // Use a real User-Agent and verify that the special files http.php and + // https.php can't be accessed. + $this->additionalCurlOptions = array(CURLOPT_USERAGENT => 'Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'); + $this->drupalGet($HTTP_path); + $this->assertResponse(403, t('Requesting http.php with a normal User-Agent fails.')); + $this->drupalGet($https_path); + $this->assertResponse(403, t('Requesting https.php with a normal User-Agent fails.')); + } + } + /** * Make sure that tests selected through the web interface are run and * that the results are displayed correctly. @@ -274,10 +311,7 @@ class SimpleTestFunctionalTest extends DrupalWebTestCase { * Check if the test is being run from inside a CURL request. */ function inCURL() { - // We cannot rely on drupal_static('drupal_test_info') here, because - // 'in_child_site' would be FALSE for the parent site when we are - // executing the tests. Default to direct detection of the HTTP headers. - return isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^simpletest\d+/", $_SERVER['HTTP_USER_AGENT']); + return (bool) drupal_valid_test_ua(); } } diff --git a/modules/simpletest/tests/http.php b/modules/simpletest/tests/http.php new file mode 100644 index 00000000000..0c5f1eb7893 --- /dev/null +++ b/modules/simpletest/tests/http.php @@ -0,0 +1,33 @@ + $value) { + $_SERVER[$key] = str_replace('modules/simpletest/tests/http.php', 'index.php', $value); + $_SERVER[$key] = str_replace('https://', 'http://', $_SERVER[$key]); +} + +// Change current directory to the Drupal root. +chdir('../../..'); +define('DRUPAL_ROOT', getcwd()); +require_once DRUPAL_ROOT . '/includes/bootstrap.inc'; + +// Make sure this file can only be used by simpletest. +drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION); +if (!drupal_valid_test_ua()) { + header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden'); + exit; +} + +drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); +menu_execute_active_handler(); diff --git a/modules/simpletest/tests/https.php b/modules/simpletest/tests/https.php index 121e4ee178f..ba618c151ae 100644 --- a/modules/simpletest/tests/https.php +++ b/modules/simpletest/tests/https.php @@ -6,23 +6,27 @@ * Fake an https request, for use during testing. */ -// Negated copy of the condition in _drupal_bootstrap(). If the user agent is -// not from simpletest then disallow access. -if (!(isset($_SERVER['HTTP_USER_AGENT']) && (strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE))) { - exit; -} - // Set a global variable to indicate a mock HTTPS request. $is_https_mock = empty($_SERVER['HTTPS']); // Change to https. $_SERVER['HTTPS'] = 'on'; - -// Change to index.php. -chdir('../../..'); foreach ($_SERVER as $key => $value) { $_SERVER[$key] = str_replace('modules/simpletest/tests/https.php', 'index.php', $value); $_SERVER[$key] = str_replace('http://', 'https://', $_SERVER[$key]); } -require_once 'index.php'; +// Change current directory to the Drupal root. +chdir('../../..'); +define('DRUPAL_ROOT', getcwd()); +require_once DRUPAL_ROOT . '/includes/bootstrap.inc'; + +// Make sure this file can only be used by simpletest. +drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION); +if (!drupal_valid_test_ua()) { + header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden'); + exit; +} + +drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); +menu_execute_active_handler(); diff --git a/modules/simpletest/tests/session.test b/modules/simpletest/tests/session.test index 88931a8ebd6..f02cbef40fe 100644 --- a/modules/simpletest/tests/session.test +++ b/modules/simpletest/tests/session.test @@ -316,7 +316,7 @@ class SessionHttpsTestCase extends DrupalWebTestCase { // Check insecure cookie is not set. $this->assertFalse(isset($this->cookies[$insecure_session_name])); $ssid = $this->cookies[$secure_session_name]['value']; - $this->assertSessionIds('', $ssid, 'Session has NULL for SID and a correct secure SID.'); + $this->assertSessionIds($ssid, $ssid, 'Session has a non-empty SID and a correct secure SID.'); $cookie = $secure_session_name . '=' . $ssid; // Verify that user is logged in on secure URL. @@ -326,12 +326,36 @@ class SessionHttpsTestCase extends DrupalWebTestCase { $this->assertResponse(200); // Verify that user is not logged in on non-secure URL. - if (!$is_https) { - $this->curlClose(); - $this->drupalGet('admin/config', array(), array('Cookie: ' . $cookie)); - $this->assertNoText(t('Configuration')); - $this->assertResponse(403); - } + $this->curlClose(); + $this->drupalGet($this->httpUrl('admin/config'), array(), array('Cookie: ' . $cookie)); + $this->assertNoText(t('Configuration')); + $this->assertResponse(403); + + // Verify that empty SID cannot be used on the non-secure site. + $this->curlClose(); + $cookie = $insecure_session_name . '='; + $this->drupalGet($this->httpUrl('admin/config'), array(), array('Cookie: ' . $cookie)); + $this->assertResponse(403); + + // Test HTTP session handling by altering the form action to submit the + // login form through http.php, which creates a mock HTTP request on HTTPS + // test environments. + $this->curlClose(); + $this->drupalGet('user'); + $form = $this->xpath('//form[@id="user-login"]'); + $form[0]['action'] = $this->httpUrl('user'); + $edit = array('name' => $user->name, 'pass' => $user->pass_raw); + $this->drupalPost(NULL, $edit, t('Log in')); + $this->drupalGet($this->httpUrl('admin/config')); + $this->assertResponse(200); + $sid = $this->cookies[$insecure_session_name]['value']; + $this->assertSessionIds($sid, '', 'Session has the correct SID and an empty secure SID.'); + + // Verify that empty secure SID cannot be used on the secure site. + $this->curlClose(); + $cookie = $secure_session_name . '='; + $this->drupalGet($this->httpsUrl('admin/config'), array(), array('Cookie: ' . $cookie)); + $this->assertResponse(403); // Clear browser cookie jar. $this->cookies = array(); @@ -458,9 +482,32 @@ class SessionHttpsTestCase extends DrupalWebTestCase { return $this->assertTrue(db_query('SELECT timestamp FROM {sessions} WHERE sid = :sid AND ssid = :ssid', $args)->fetchField(), $assertion_text); } + /** + * Builds a URL for submitting a mock HTTPS request to HTTP test environments. + * + * @param $url + * A Drupal path such as 'user'. + * + * @return + * An absolute URL. + */ protected function httpsUrl($url) { global $base_url; return $base_url . '/modules/simpletest/tests/https.php?q=' . $url; } + + /** + * Builds a URL for submitting a mock HTTP request to HTTPS test environments. + * + * @param $url + * A Drupal path such as 'user'. + * + * @return + * An absolute URL. + */ + protected function httpUrl($url) { + global $base_url; + return $base_url . '/modules/simpletest/tests/http.php?q=' . $url; + } } diff --git a/modules/simpletest/tests/upgrade/upgrade.test b/modules/simpletest/tests/upgrade/upgrade.test index 4220faebbf7..8ea93deba13 100644 --- a/modules/simpletest/tests/upgrade/upgrade.test +++ b/modules/simpletest/tests/upgrade/upgrade.test @@ -113,7 +113,12 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase { // Force our way into the session of the child site. drupal_save_session(TRUE); + // A session cannot be written without the ssid column which is missing on + // Drupal 6 sites. + db_add_field('sessions', 'ssid', array('description' => "Secure session ID. The value is generated by Drupal's session handlers.", 'type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => '')); _drupal_session_write($sid, ''); + // Remove the temporarily added ssid column. + db_drop_field('sessions', 'ssid'); drupal_save_session(FALSE); // Restore necessary variables. diff --git a/modules/system/system.install b/modules/system/system.install index ca98e390bc8..f56ef405533 100644 --- a/modules/system/system.install +++ b/modules/system/system.install @@ -1469,14 +1469,13 @@ function system_schema() { 'not null' => TRUE, ), 'sid' => array( - 'description' => "A session ID. The value is generated by PHP's Session API.", + 'description' => "A session ID. The value is generated by Drupal's session handlers.", 'type' => 'varchar', 'length' => 128, 'not null' => TRUE, - 'default' => '', ), 'ssid' => array( - 'description' => "Secure session ID. The value is generated by PHP's Session API.", + 'description' => "Secure session ID. The value is generated by Drupal's session handlers.", 'type' => 'varchar', 'length' => 128, 'not null' => TRUE, @@ -2901,6 +2900,19 @@ function system_update_7064() { db_drop_field('menu_router', 'block_callback'); } +/** + * Remove the default value for sid. + */ +function system_update_7065() { + $spec = array( + 'description' => "A session ID. The value is generated by Drupal's session handlers.", + 'type' => 'varchar', + 'length' => 128, + 'not null' => TRUE, + ); + db_change_field('sessions', 'sid', 'sid', $spec); +} + /** * @} End of "defgroup updates-6.x-to-7.x" * The next series of updates should start at 8000.