summaryrefslogtreecommitdiff
path: root/modules
diff options
context:
space:
mode:
authorDavid Rothstein <drothstein@gmail.com>2016-02-24 14:19:52 -0500
committerDavid Rothstein <drothstein@gmail.com>2016-02-24 14:19:52 -0500
commit2f54b101bf722849e456d859876b27b90ad7e479 (patch)
tree9fdf1d34a03ec83b95a4fbcced22bb1b599f76d0 /modules
parentaaf2d59820d7daf70c3acdde20c0e13d618a4e07 (diff)
downloadbrdo-2f54b101bf722849e456d859876b27b90ad7e479.tar.gz
brdo-2f54b101bf722849e456d859876b27b90ad7e479.tar.bz2
Drupal 7.43
Diffstat (limited to 'modules')
-rw-r--r--modules/file/file.module15
-rw-r--r--modules/file/tests/file.test138
-rw-r--r--modules/simpletest/tests/common.test68
-rw-r--r--modules/simpletest/tests/common_test.module9
-rw-r--r--modules/simpletest/tests/xmlrpc.test34
-rw-r--r--modules/system/system.admin.inc8
-rw-r--r--modules/system/system.js2
-rw-r--r--modules/system/system.test16
-rw-r--r--modules/user/user.module16
9 files changed, 294 insertions, 12 deletions
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;
+ }
}
/**
@@ -527,6 +551,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.
*/
function testMultiValuedWidget() {
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
@@ -373,6 +373,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,
+ '<front>' => FALSE,
+ );
+ }
+}
+
+/**
* Tests for check_plain(), filter_xss(), format_string(), and check_url().
*/
class CommonXssUnitTest extends DrupalUnitTestCase {
@@ -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('<front>', 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
@@ -93,6 +93,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.
*/
function common_test_destination() {
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 + ': <em>' + data + '</em>');
});
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. <a href="@password">Have you forgotten your password?</a>', 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. <a href="@password">Have you forgotten your password?</a>', array('@password' => url('user/password', array('query' => $query)))));
watchdog('user', 'Login attempt failed for %user.', array('%user' => $form_state['values']['name']));
}
}