From b24fcf26db76af97ff6854209e2a9c05d7f60c43 Mon Sep 17 00:00:00 2001 From: Marlene Williams Date: Wed, 25 Apr 2018 10:03:57 -0700 Subject: [PATCH] Applying security patch SA-CORE-2018-004. --- includes/bootstrap.inc | 5 ++ includes/common.inc | 5 +- includes/request-sanitizer.inc | 32 +++++++++ modules/file/file.module | 3 + sites/all/patches/sa-core-2018-004.patch | 89 ++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 sites/all/patches/sa-core-2018-004.patch diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index dc62280c..5cf78dcf 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -2499,6 +2499,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 9a9e763a..472485f7 100644 --- a/includes/common.inc +++ b/includes/common.inc @@ -612,8 +612,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 1daa6b53..7214436b 100644 --- a/includes/request-sanitizer.inc +++ b/includes/request-sanitizer.inc @@ -51,6 +51,38 @@ class DrupalRequestSanitizer { } } + /** + * 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 ae452a68..c24d768e 100644 --- a/modules/file/file.module +++ b/modules/file/file.module @@ -238,6 +238,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'); diff --git a/sites/all/patches/sa-core-2018-004.patch b/sites/all/patches/sa-core-2018-004.patch new file mode 100644 index 00000000..de1dffba --- /dev/null +++ b/sites/all/patches/sa-core-2018-004.patch @@ -0,0 +1,89 @@ +diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc +index 06acf93..d5963a0 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 d7dc47f..f61d1eb 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 1daa6b5..7214436 100644 +--- a/includes/request-sanitizer.inc ++++ b/includes/request-sanitizer.inc +@@ -52,6 +52,38 @@ class DrupalRequestSanitizer { + } + + /** ++ * 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. + * + * @param mixed $input +diff --git a/modules/file/file.module b/modules/file/file.module +index 1e98f11..eea5847 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');