diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index 06acf935e013b1f0977412170eb43b5f3b61c84f..d5963a0aff7e117618c4836862cc53100ac47cbe 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -2778,6 +2778,11 @@ function _drupal_bootstrap_variables() { unset($_GET['destination']); unset($_REQUEST['destination']); } + // Use the DrupalRequestSanitizer to ensure that the destination's query + // parameters are not dangerous. + if (isset($_GET['destination'])) { + DrupalRequestSanitizer::cleanDestination(); + } // If there's still something in $_REQUEST['destination'] that didn't come // from $_GET, check it too. if (isset($_REQUEST['destination']) && (!isset($_GET['destination']) || $_REQUEST['destination'] != $_GET['destination']) && url_is_external($_REQUEST['destination'])) { diff --git a/includes/common.inc b/includes/common.inc index d7dc47f229ea75d375fd35ddab2a17aad26cd4ca..f61d1eb0f2496a4afd186715765773eb20ddfc33 100644 --- a/includes/common.inc +++ b/includes/common.inc @@ -611,8 +611,9 @@ function drupal_parse_url($url) { } // The 'q' parameter contains the path of the current page if clean URLs are // disabled. It overrides the 'path' of the URL when present, even if clean - // URLs are enabled, due to how Apache rewriting rules work. - if (isset($options['query']['q'])) { + // URLs are enabled, due to how Apache rewriting rules work. The path + // parameter must be a string. + if (isset($options['query']['q']) && is_string($options['query']['q'])) { $options['path'] = $options['query']['q']; unset($options['query']['q']); } diff --git a/includes/request-sanitizer.inc b/includes/request-sanitizer.inc index 1daa6b53485d76715541e5d4ce6ddffa90293a43..7214436b8ad18e04626cd6a401d2d37d1279769d 100644 --- a/includes/request-sanitizer.inc +++ b/includes/request-sanitizer.inc @@ -51,6 +51,38 @@ public static function sanitize() { } } + /** + * Removes the destination if it is dangerous. + * + * Note this can only be called after common.inc has been included. + * + * @return bool + * TRUE if the destination has been removed from $_GET, FALSE if not. + */ + public static function cleanDestination() { + $dangerous_keys = array(); + $log_sanitized_keys = variable_get('sanitize_input_logging', FALSE); + + $parts = drupal_parse_url($_GET['destination']); + // If there is a query string, check its query parameters. + if (!empty($parts['query'])) { + $whitelist = variable_get('sanitize_input_whitelist', array()); + + self::stripDangerousValues($parts['query'], $whitelist, $dangerous_keys); + if (!empty($dangerous_keys)) { + // The destination is removed rather than sanitized to mirror the + // handling of external destinations. + unset($_GET['destination']); + unset($_REQUEST['destination']); + if ($log_sanitized_keys) { + trigger_error(format_string('Potentially unsafe destination removed from query string parameters (GET) because it contained the following keys: @keys', array('@keys' => implode(', ', $dangerous_keys)))); + } + return TRUE; + } + } + return FALSE; + } + /** * Strips dangerous keys from the provided input. * diff --git a/modules/file/file.module b/modules/file/file.module index 1e98f11bd7caa49fa79be7962347ce497c66962a..eea58470fab2deb007c9ba99b3bf3afbcaf4acb2 100644 --- a/modules/file/file.module +++ b/modules/file/file.module @@ -239,6 +239,9 @@ function file_ajax_upload() { $form_parents = func_get_args(); $form_build_id = (string) array_pop($form_parents); + // Sanitize form parents before using them. + $form_parents = array_filter($form_parents, 'element_child'); + if (empty($_POST['form_build_id']) || $form_build_id != $_POST['form_build_id']) { // Invalid request. drupal_set_message(t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', array('@size' => format_size(file_upload_max_size()))), 'error');