From 2f54b101bf722849e456d859876b27b90ad7e479 Mon Sep 17 00:00:00 2001 From: David Rothstein Date: Wed, 24 Feb 2016 14:19:52 -0500 Subject: Drupal 7.43 --- 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 +++- 9 files changed, 294 insertions(+), 12 deletions(-) (limited to 'modules') diff --git a/modules/file/file.module b/modules/file/file.module index fbf8b81ec..9e091af03 100644 --- a/modules/file/file.module +++ b/modules/file/file.module @@ -529,14 +529,19 @@ 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)) { - $fid = $file->fid; + if (!in_array(file_uri_scheme($file->uri), variable_get('file_public_schema', array('public'))) && !file_download_access($file->uri)) { + $force_default = TRUE; } - // If the current user doesn't have access, don't let the file be - // changed. - else { + // 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 all checks pass, allow the file to be changed. + else { + $fid = $file->fid; + } } } } diff --git a/modules/file/tests/file.test b/modules/file/tests/file.test index 80433954b..6d7cb4bc4 100644 --- a/modules/file/tests/file.test +++ b/modules/file/tests/file.test @@ -218,6 +218,30 @@ 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; + } } /** @@ -526,6 +550,120 @@ 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 bf8557619..92aefe48f 100644 --- a/modules/simpletest/tests/common.test +++ b/modules/simpletest/tests/common.test @@ -372,6 +372,65 @@ 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(). */ @@ -1256,6 +1315,15 @@ 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 674a49446..2eb8cd5d2 100644 --- a/modules/simpletest/tests/common_test.module +++ b/modules/simpletest/tests/common_test.module @@ -92,6 +92,15 @@ 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 1a9ef2349..bb74f059b 100644 --- a/modules/simpletest/tests/xmlrpc.test +++ b/modules/simpletest/tests/xmlrpc.test @@ -246,4 +246,38 @@ 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 0f525c6cf..16c40d4d4 100644 --- a/modules/system/system.admin.inc +++ b/modules/system/system.admin.inc @@ -2202,6 +2202,11 @@ 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); } @@ -2875,13 +2880,14 @@ 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('admin/config/regional/date-time/formats/lookup'), + 'lookup' => url($ajax_path, array('query' => array('token' => drupal_get_token($ajax_path)))), ), ), ), diff --git a/modules/system/system.js b/modules/system/system.js index 910fb5d3d..c0e76d38e 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 + (/\?q=/.test(fieldSettings.lookup) ? '&format=' : '?format=') + encodeURIComponent(input.val()); + var url = fieldSettings.lookup + (/\?/.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 bc764dde5..95b43538b 100644 --- a/modules/system/system.test +++ b/modules/system/system.test @@ -1350,7 +1350,23 @@ 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 c33aa0982..d38de69b1 100644 --- a/modules/user/user.module +++ b/modules/user/user.module @@ -1308,10 +1308,12 @@ function user_user_presave(&$edit, $account, $category) { elseif (!empty($edit['picture_delete'])) { $edit['picture'] = NULL; } - // Prepare user roles. - if (isset($edit['roles'])) { - $edit['roles'] = array_filter($edit['roles']); - } + } + + // 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']); } // Move account cancellation information into $user->data. @@ -2227,7 +2229,11 @@ function user_login_final_validate($form, &$form_state) { } } else { - 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'])))))); + // 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))))); watchdog('user', 'Login attempt failed for %user.', array('%user' => $form_state['values']['name'])); } } -- cgit v1.2.3