From b8d9c44f83eca57039f648a0edb0f369f8d3e6b4 Mon Sep 17 00:00:00 2001 From: David Rothstein Date: Wed, 24 Feb 2016 14:25:49 -0500 Subject: Revert "Drupal 7.43" This reverts commit 2f54b101bf722849e456d859876b27b90ad7e479. --- CHANGELOG.txt | 3 +- includes/bootstrap.inc | 2 +- includes/common.inc | 37 ++++---- includes/path.inc | 3 +- includes/xmlrpcs.inc | 8 -- modules/file/file.module | 15 +-- modules/file/tests/file.test | 138 ---------------------------- modules/simpletest/tests/common.test | 68 -------------- modules/simpletest/tests/common_test.module | 9 -- modules/simpletest/tests/xmlrpc.test | 34 ------- modules/system/system.admin.inc | 8 +- modules/system/system.js | 2 +- modules/system/system.test | 16 ---- modules/user/user.module | 16 +--- 14 files changed, 33 insertions(+), 326 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 72d9d8fc9..42e5628e1 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,7 +1,6 @@ -Drupal 7.43, 2016-02-24 +Drupal 7.43, xxxx-xx-xx (development version) ----------------------- -- Fixed security issues (multiple vulnerabilities). See SA-CORE-2016-001. Drupal 7.42, 2016-02-03 ----------------------- diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index 0428bd362..3dde2740c 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -8,7 +8,7 @@ /** * The current system version. */ -define('VERSION', '7.43'); +define('VERSION', '7.43-dev'); /** * Core API compatibility. diff --git a/includes/common.inc b/includes/common.inc index c6303efad..34fa9b962 100644 --- a/includes/common.inc +++ b/includes/common.inc @@ -688,13 +688,6 @@ function drupal_goto($path = '', array $options = array(), $http_response_code = $options['fragment'] = $destination['fragment']; } - // In some cases modules call drupal_goto(current_path()). We need to ensure - // that such a redirect is not to an external URL. - if ($path === current_path() && empty($options['external']) && url_is_external($path)) { - // Force url() to generate a non-external URL. - $options['external'] = FALSE; - } - drupal_alter('drupal_goto', $path, $options, $http_response_code); // The 'Location' HTTP header must be absolute. @@ -2227,8 +2220,20 @@ function url($path = NULL, array $options = array()) { 'prefix' => '' ); + // A duplicate of the code from url_is_external() to avoid needing another + // function call, since performance inside url() is critical. if (!isset($options['external'])) { - $options['external'] = url_is_external($path); + // Return an external link if $path contains an allowed absolute URL. Avoid + // calling drupal_strip_dangerous_protocols() if there is any slash (/), + // hash (#) or question_mark (?) before the colon (:) occurrence - if any - + // as this would clearly mean it is not a URL. If the path starts with 2 + // slashes then it is always considered an external URL without an explicit + // protocol part. + $colonpos = strpos($path, ':'); + $options['external'] = (strpos($path, '//') === 0) + || ($colonpos !== FALSE + && !preg_match('![/?#]!', substr($path, 0, $colonpos)) + && drupal_strip_dangerous_protocols($path) == $path); } // Preserve the original path before altering or aliasing. @@ -2348,18 +2353,12 @@ function url($path = NULL, array $options = array()) { */ function url_is_external($path) { $colonpos = strpos($path, ':'); - // Some browsers treat \ as / so normalize to forward slashes. - $path = str_replace('\\', '/', $path); - // If the path starts with 2 slashes then it is always considered an external - // URL without an explicit protocol part. + // Avoid calling drupal_strip_dangerous_protocols() if there is any slash (/), + // hash (#) or question_mark (?) before the colon (:) occurrence - if any - as + // this would clearly mean it is not a URL. If the path starts with 2 slashes + // then it is always considered an external URL without an explicit protocol + // part. return (strpos($path, '//') === 0) - // Leading control characters may be ignored or mishandled by browsers, so - // assume such a path may lead to an external location. The \p{C} character - // class matches all UTF-8 control, unassigned, and private characters. - || (preg_match('/^\p{C}/u', $path) !== 0) - // Avoid calling drupal_strip_dangerous_protocols() if there is any slash - // (/), hash (#) or question_mark (?) before the colon (:) occurrence - if - // any - as this would clearly mean it is not a URL. || ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && drupal_strip_dangerous_protocols($path) == $path); diff --git a/includes/path.inc b/includes/path.inc index 6bd48d306..2e3571114 100644 --- a/includes/path.inc +++ b/includes/path.inc @@ -347,8 +347,7 @@ function drupal_match_path($path, $patterns) { * drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL) makes this function available. * * @return - * The current Drupal URL path. The path is untrusted user input and must be - * treated as such. + * The current Drupal URL path. * * @see request_path() */ diff --git a/includes/xmlrpcs.inc b/includes/xmlrpcs.inc index c334de159..8655c05b0 100644 --- a/includes/xmlrpcs.inc +++ b/includes/xmlrpcs.inc @@ -264,10 +264,6 @@ function xmlrpc_server_call($xmlrpc_server, $methodname, $args) { */ function xmlrpc_server_multicall($methodcalls) { // See http://www.xmlrpc.com/discuss/msgReader$1208 - // To avoid multicall expansion attacks, limit the number of duplicate method - // calls allowed with a default of 1. Set to -1 for unlimited. - $duplicate_method_limit = variable_get('xmlrpc_multicall_duplicate_method_limit', 1); - $method_count = array(); $return = array(); $xmlrpc_server = xmlrpc_server_get(); foreach ($methodcalls as $call) { @@ -277,14 +273,10 @@ function xmlrpc_server_multicall($methodcalls) { $ok = FALSE; } $method = $call['methodName']; - $method_count[$method] = isset($method_count[$method]) ? $method_count[$method] + 1 : 1; $params = $call['params']; if ($method == 'system.multicall') { $result = xmlrpc_error(-32600, t('Recursive calls to system.multicall are forbidden.')); } - elseif ($duplicate_method_limit > 0 && $method_count[$method] > $duplicate_method_limit) { - $result = xmlrpc_error(-156579, t('Too many duplicate method calls in system.multicall.')); - } elseif ($ok) { $result = xmlrpc_server_call($xmlrpc_server, $method, $params); } diff --git a/modules/file/file.module b/modules/file/file.module index 9e091af03..fbf8b81ec 100644 --- a/modules/file/file.module +++ b/modules/file/file.module @@ -529,18 +529,13 @@ function file_managed_file_value(&$element, $input = FALSE, $form_state = NULL) // publicly accessible, with no download restrictions; for security // reasons all other schemes must go through the file_download_access() // check. - if (!in_array(file_uri_scheme($file->uri), variable_get('file_public_schema', array('public'))) && !file_download_access($file->uri)) { - $force_default = TRUE; - } - // Temporary files that belong to other users should never be allowed. - // Since file ownership can't be determined for anonymous users, they - // are not allowed to reuse temporary files at all. - elseif ($file->status != FILE_STATUS_PERMANENT && (!$GLOBALS['user']->uid || $file->uid != $GLOBALS['user']->uid)) { - $force_default = TRUE; + if (in_array(file_uri_scheme($file->uri), variable_get('file_public_schema', array('public'))) || file_download_access($file->uri)) { + $fid = $file->fid; } - // If all checks pass, allow the file to be changed. + // If the current user doesn't have access, don't let the file be + // changed. else { - $fid = $file->fid; + $force_default = TRUE; } } } diff --git a/modules/file/tests/file.test b/modules/file/tests/file.test index 6d7cb4bc4..80433954b 100644 --- a/modules/file/tests/file.test +++ b/modules/file/tests/file.test @@ -218,30 +218,6 @@ class FileFieldTestCase extends DrupalWebTestCase { $message = isset($message) ? $message : format_string('File %file is permanent.', array('%file' => $file->uri)); $this->assertTrue($file->status == FILE_STATUS_PERMANENT, $message); } - - /** - * Creates a temporary file, for a specific user. - * - * @param string $data - * A string containing the contents of the file. - * @param int $uid - * The user ID of the file owner. - * - * @return object - * A file object, or FALSE on error. - */ - function createTemporaryFile($data, $uid = NULL) { - $file = file_save_data($data, NULL, NULL); - - if ($file) { - $file->uid = isset($uid) ? $uid : $this->admin_user->uid; - // Change the file status to be temporary. - $file->status = NULL; - return file_save($file); - } - - return $file; - } } /** @@ -550,120 +526,6 @@ class FileFieldWidgetTestCase extends FileFieldTestCase { } } - /** - * Tests exploiting the temporary file removal of another user using fid. - */ - function testTemporaryFileRemovalExploit() { - // Create a victim user. - $victim_user = $this->drupalCreateUser(); - - // Create an attacker user. - $attacker_user = $this->drupalCreateUser(array( - 'access content', - 'create page content', - 'edit any page content', - )); - - // Log in as the attacker user. - $this->drupalLogin($attacker_user); - - // Perform tests using the newly created users. - $this->doTestTemporaryFileRemovalExploit($victim_user->uid, $attacker_user->uid); - } - - /** - * Tests exploiting the temporary file removal for anonymous users using fid. - */ - public function testTemporaryFileRemovalExploitAnonymous() { - // Set up an anonymous victim user. - $victim_uid = 0; - - // Set up an anonymous attacker user. - $attacker_uid = 0; - - // Set up permissions for anonymous attacker user. - user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array( - 'access content' => TRUE, - 'create page content' => TRUE, - 'edit any page content' => TRUE, - )); - - // In order to simulate being the anonymous attacker user, we need to log - // out here since setUp() has logged in the admin. - $this->drupalLogout(); - - // Perform tests using the newly set up users. - $this->doTestTemporaryFileRemovalExploit($victim_uid, $attacker_uid); - } - - /** - * Helper for testing exploiting the temporary file removal using fid. - * - * @param int $victim_uid - * The victim user ID. - * @param int $attacker_uid - * The attacker user ID. - */ - protected function doTestTemporaryFileRemovalExploit($victim_uid, $attacker_uid) { - // Use 'page' instead of 'article', so that the 'article' image field does - // not conflict with this test. If in the future the 'page' type gets its - // own default file or image field, this test can be made more robust by - // using a custom node type. - $type_name = 'page'; - $field_name = 'test_file_field'; - $this->createFileField($field_name, $type_name); - - $test_file = $this->getTestFile('text'); - foreach (array('nojs', 'js') as $type) { - // Create a temporary file owned by the anonymous victim user. This will be - // as if they had uploaded the file, but not saved the node they were - // editing or creating. - $victim_tmp_file = $this->createTemporaryFile('some text', $victim_uid); - $victim_tmp_file = file_load($victim_tmp_file->fid); - $this->assertTrue($victim_tmp_file->status != FILE_STATUS_PERMANENT, 'New file saved to disk is temporary.'); - $this->assertFalse(empty($victim_tmp_file->fid), 'New file has a fid'); - $this->assertEqual($victim_uid, $victim_tmp_file->uid, 'New file belongs to the victim user'); - - // Have attacker create a new node with a different uploaded file and - // ensure it got uploaded successfully. - // @todo Can we test AJAX? See https://www.drupal.org/node/2538260 - $edit = array( - 'title' => $type . '-title', - ); - - // Attach a file to a node. - $langcode = LANGUAGE_NONE; - $edit['files[' . $field_name . '_' . $langcode . '_0]'] = drupal_realpath($test_file->uri); - $this->drupalPost("node/add/$type_name", $edit, 'Save'); - $node = $this->drupalGetNodeByTitle($edit['title']); - $node_file = file_load($node->{$field_name}[$langcode][0]['fid']); - $this->assertFileExists($node_file, 'New file saved to disk on node creation.'); - $this->assertEqual($attacker_uid, $node_file->uid, 'New file belongs to the attacker.'); - - // Ensure the file can be downloaded. - $this->drupalGet(file_create_url($node_file->uri)); - $this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.'); - - // "Click" the remove button (emulating either a nojs or js submission). - // In this POST request, the attacker "guesses" the fid of the victim's - // temporary file and uses that to remove this file. - $this->drupalGet('node/' . $node->nid . '/edit'); - switch ($type) { - case 'nojs': - $this->drupalPost(NULL, array("{$field_name}[$langcode][0][fid]" => (string) $victim_tmp_file->fid), 'Remove'); - break; - case 'js': - $button = $this->xpath('//input[@type="submit" and @value="Remove"]'); - $this->drupalPostAJAX(NULL, array("{$field_name}[$langcode][0][fid]" => (string) $victim_tmp_file->fid), array((string) $button[0]['name'] => (string) $button[0]['value'])); - break; - } - - // The victim's temporary file should not be removed by the attacker's - // POST request. - $this->assertFileExists($victim_tmp_file); - } - } - /** * Tests upload and remove buttons for multiple multi-valued File fields. */ diff --git a/modules/simpletest/tests/common.test b/modules/simpletest/tests/common.test index 92aefe48f..bf8557619 100644 --- a/modules/simpletest/tests/common.test +++ b/modules/simpletest/tests/common.test @@ -372,65 +372,6 @@ class CommonURLUnitTest extends DrupalWebTestCase { } } -/** - * Tests url_is_external(). - */ -class UrlIsExternalUnitTest extends DrupalUnitTestCase { - - public static function getInfo() { - return array( - 'name' => 'External URL checking', - 'description' => 'Performs tests on url_is_external().', - 'group' => 'System', - ); - } - - /** - * Tests if each URL is external or not. - */ - function testUrlIsExternal() { - foreach ($this->examples() as $path => $expected) { - $this->assertIdentical(url_is_external($path), $expected, $path); - } - } - - /** - * Provides data for testUrlIsExternal(). - * - * @return array - * An array of test data, keyed by a path, with the expected value where - * TRUE is external, and FALSE is not external. - */ - protected function examples() { - return array( - // Simple external URLs. - 'http://example.com' => TRUE, - 'https://example.com' => TRUE, - 'http://drupal.org/foo/bar?foo=bar&bar=baz&baz#foo' => TRUE, - '//drupal.org' => TRUE, - // Some browsers ignore or strip leading control characters. - "\x00//www.example.com" => TRUE, - "\x08//www.example.com" => TRUE, - "\x1F//www.example.com" => TRUE, - "\n//www.example.com" => TRUE, - // JSON supports decoding directly from UTF-8 code points. - json_decode('"\u00AD"') . "//www.example.com" => TRUE, - json_decode('"\u200E"') . "//www.example.com" => TRUE, - json_decode('"\uE0020"') . "//www.example.com" => TRUE, - json_decode('"\uE000"') . "//www.example.com" => TRUE, - // Backslashes should be normalized to forward. - '\\\\example.com' => TRUE, - // Local URLs. - 'node' => FALSE, - '/system/ajax' => FALSE, - '?q=foo:bar' => FALSE, - 'node/edit:me' => FALSE, - '/drupal.org' => FALSE, - '' => FALSE, - ); - } -} - /** * Tests for check_plain(), filter_xss(), format_string(), and check_url(). */ @@ -1315,15 +1256,6 @@ class DrupalGotoTest extends DrupalWebTestCase { $this->assertText('drupal_goto', 'Drupal goto redirect succeeded.'); $this->assertEqual($this->getUrl(), url('common-test/drupal_goto', array('query' => array('foo' => '123'), 'absolute' => TRUE)), 'Drupal goto redirected to expected URL.'); - // Test that calling drupal_goto() on the current path is not dangerous. - variable_set('common_test_redirect_current_path', TRUE); - $this->drupalGet('', array('query' => array('q' => 'http://www.example.com/'))); - $headers = $this->drupalGetHeaders(TRUE); - list(, $status) = explode(' ', $headers[0][':status'], 3); - $this->assertEqual($status, 302, 'Expected response code was sent.'); - $this->assertNotEqual($this->getUrl(), 'http://www.example.com/', 'Drupal goto did not redirect to external URL.'); - $this->assertTrue(strpos($this->getUrl(), url('', array('absolute' => TRUE))) === 0, 'Drupal redirected to itself.'); - variable_del('common_test_redirect_current_path'); // Test that drupal_goto() respects ?destination=xxx. Use an complicated URL // to test that the path is encoded and decoded properly. $destination = 'common-test/drupal_goto/destination?foo=%2525&bar=123'; diff --git a/modules/simpletest/tests/common_test.module b/modules/simpletest/tests/common_test.module index 2eb8cd5d2..674a49446 100644 --- a/modules/simpletest/tests/common_test.module +++ b/modules/simpletest/tests/common_test.module @@ -92,15 +92,6 @@ function common_test_drupal_goto_alter(&$path, &$options, &$http_response_code) } } -/** - * Implements hook_init(). - */ -function common_test_init() { - if (variable_get('common_test_redirect_current_path', FALSE)) { - drupal_goto(current_path()); - } -} - /** * Print destination query parameter. */ diff --git a/modules/simpletest/tests/xmlrpc.test b/modules/simpletest/tests/xmlrpc.test index bb74f059b..1a9ef2349 100644 --- a/modules/simpletest/tests/xmlrpc.test +++ b/modules/simpletest/tests/xmlrpc.test @@ -246,38 +246,4 @@ class XMLRPCMessagesTestCase extends DrupalWebTestCase { $this->assertEqual($removed, 'system.methodSignature', 'Hiding builting system.methodSignature with hook_xmlrpc_alter works'); } - /** - * Test limits on system.multicall that can prevent brute-force attacks. - */ - function testMulticallLimit() { - $url = url(NULL, array('absolute' => TRUE)) . 'xmlrpc.php'; - $multicall_args = array(); - $num_method_calls = 10; - for ($i = 0; $i < $num_method_calls; $i++) { - $struct = array('i' => $i); - $multicall_args[] = array('methodName' => 'validator1.echoStructTest', 'params' => array($struct)); - } - // Test limits of 1, 5, 9, 13. - for ($limit = 1; $limit < $num_method_calls + 4; $limit += 4) { - variable_set('xmlrpc_multicall_duplicate_method_limit', $limit); - $results = xmlrpc($url, array('system.multicall' => array($multicall_args))); - $this->assertEqual($num_method_calls, count($results)); - for ($i = 0; $i < min($limit, $num_method_calls); $i++) { - $x = array_shift($results); - $this->assertTrue(empty($x->is_error), "Result $i is not an error"); - $this->assertEqual($multicall_args[$i]['params'][0], $x); - } - for (; $i < $num_method_calls; $i++) { - $x = array_shift($results); - $this->assertFalse(empty($x->is_error), "Result $i is an error"); - $this->assertEqual(-156579, $x->code); - } - } - variable_set('xmlrpc_multicall_duplicate_method_limit', -1); - $results = xmlrpc($url, array('system.multicall' => array($multicall_args))); - $this->assertEqual($num_method_calls, count($results)); - foreach ($results as $i => $x) { - $this->assertTrue(empty($x->is_error), "Result $i is not an error"); - } - } } diff --git a/modules/system/system.admin.inc b/modules/system/system.admin.inc index 16c40d4d4..0f525c6cf 100644 --- a/modules/system/system.admin.inc +++ b/modules/system/system.admin.inc @@ -2202,11 +2202,6 @@ function system_add_date_format_type_form_submit($form, &$form_state) { * Return the date for a given format string via Ajax. */ function system_date_time_lookup() { - // This callback is protected with a CSRF token because user input from the - // query string is reflected in the output. - if (!isset($_GET['token']) || !drupal_valid_token($_GET['token'], 'admin/config/regional/date-time/formats/lookup')) { - return MENU_ACCESS_DENIED; - } $result = format_date(REQUEST_TIME, 'custom', $_GET['format']); drupal_json_output($result); } @@ -2880,14 +2875,13 @@ function system_date_time_formats() { * Allow users to add additional date formats. */ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) { - $ajax_path = 'admin/config/regional/date-time/formats/lookup'; $js_settings = array( 'type' => 'setting', 'data' => array( 'dateTime' => array( 'date-format' => array( 'text' => t('Displayed as'), - 'lookup' => url($ajax_path, array('query' => array('token' => drupal_get_token($ajax_path)))), + 'lookup' => url('admin/config/regional/date-time/formats/lookup'), ), ), ), diff --git a/modules/system/system.js b/modules/system/system.js index c0e76d38e..910fb5d3d 100644 --- a/modules/system/system.js +++ b/modules/system/system.js @@ -105,7 +105,7 @@ Drupal.behaviors.dateTime = { // Attach keyup handler to custom format inputs. $('input' + source, context).once('date-time').keyup(function () { var input = $(this); - var url = fieldSettings.lookup + (/\?/.test(fieldSettings.lookup) ? '&format=' : '?format=') + encodeURIComponent(input.val()); + var url = fieldSettings.lookup + (/\?q=/.test(fieldSettings.lookup) ? '&format=' : '?format=') + encodeURIComponent(input.val()); $.getJSON(url, function (data) { $(suffix).empty().append(' ' + fieldSettings.text + ': ' + data + ''); }); diff --git a/modules/system/system.test b/modules/system/system.test index 95b43538b..bc764dde5 100644 --- a/modules/system/system.test +++ b/modules/system/system.test @@ -1350,23 +1350,7 @@ class DateTimeFunctionalTest extends DrupalWebTestCase { $this->assertEqual($this->getUrl(), url('admin/config/regional/date-time/formats', array('absolute' => TRUE)), 'Correct page redirection.'); $this->assertText(t('Custom date format updated.'), 'Custom date format successfully updated.'); - // Check that ajax callback is protected by CSRF token. - $this->drupalGet('admin/config/regional/date-time/formats/lookup', array('query' => array('format' => 'Y m d'))); - $this->assertResponse(403, 'Access denied with no token'); - $this->drupalGet('admin/config/regional/date-time/formats/lookup', array('query' => array('token' => 'invalid', 'format' => 'Y m d'))); - $this->assertResponse(403, 'Access denied with invalid token'); - $this->drupalGet('admin/config/regional/date-time/formats'); - $this->clickLink(t('edit')); - $settings = $this->drupalGetSettings(); - $lookup_url = $settings['dateTime']['date-format']['lookup']; - preg_match('/token=([^&]+)/', $lookup_url, $matches); - $this->assertFalse(empty($matches[1]), 'Found token value'); - $this->drupalGet('admin/config/regional/date-time/formats/lookup', array('query' => array('token' => $matches[1], 'format' => 'Y m d'))); - $this->assertResponse(200, 'Access allowed with valid token'); - $this->assertText(format_date(time(), 'custom', 'Y m d')); - // Delete custom date format. - $this->drupalGet('admin/config/regional/date-time/formats'); $this->clickLink(t('delete')); $this->drupalPost($this->getUrl(), array(), t('Remove')); $this->assertEqual($this->getUrl(), url('admin/config/regional/date-time/formats', array('absolute' => TRUE)), 'Correct page redirection.'); diff --git a/modules/user/user.module b/modules/user/user.module index d38de69b1..c33aa0982 100644 --- a/modules/user/user.module +++ b/modules/user/user.module @@ -1308,12 +1308,10 @@ function user_user_presave(&$edit, $account, $category) { elseif (!empty($edit['picture_delete'])) { $edit['picture'] = NULL; } - } - - // Filter out roles with empty values to avoid granting extra roles when - // processing custom form submissions. - if (isset($edit['roles'])) { - $edit['roles'] = array_filter($edit['roles']); + // Prepare user roles. + if (isset($edit['roles'])) { + $edit['roles'] = array_filter($edit['roles']); + } } // Move account cancellation information into $user->data. @@ -2229,11 +2227,7 @@ function user_login_final_validate($form, &$form_state) { } } else { - // Use $form_state['input']['name'] here to guarantee that we send - // exactly what the user typed in. $form_state['values']['name'] may have - // been modified by validation handlers that ran earlier than this one. - $query = isset($form_state['input']['name']) ? array('name' => $form_state['input']['name']) : array(); - form_set_error('name', t('Sorry, unrecognized username or password. Have you forgotten your password?', array('@password' => url('user/password', array('query' => $query))))); + form_set_error('name', t('Sorry, unrecognized username or password. Have you forgotten your password?', array('@password' => url('user/password', array('query' => array('name' => $form_state['values']['name'])))))); watchdog('user', 'Login attempt failed for %user.', array('%user' => $form_state['values']['name'])); } } -- cgit v1.2.3