Skip to content

Instantly share code, notes, and snippets.

@popstas
Created April 26, 2018 05:50
Show Gist options
  • Save popstas/93c9c5c1a61c479d564deb0b1bc380c4 to your computer and use it in GitHub Desktop.
Save popstas/93c9c5c1a61c479d564deb0b1bc380c4 to your computer and use it in GitHub Desktop.
SA-CORE-2018-004 for Drupal 7.34 or older
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,31 @@ function _drupal_bootstrap_variables() {
// Load bootstrap modules.
require_once DRUPAL_ROOT . '/includes/module.inc';
module_load_all(TRUE);
+
+ // Sanitize the destination parameter (which is often used for redirects) to
+ // prevent open redirect attacks leading to other domains. Sanitize both
+ // $_GET['destination'] and $_REQUEST['destination'] to protect code that
+ // relies on either, but do not sanitize $_POST to avoid interfering with
+ // unrelated form submissions. The sanitization happens here because
+ // url_is_external() requires the variable system to be available.
+ if (isset($_GET['destination']) || isset($_REQUEST['destination'])) {
+ require_once DRUPAL_ROOT . '/includes/common.inc';
+ // If the destination is an external URL, remove it.
+ if (isset($_GET['destination']) && url_is_external($_GET['destination'])) {
+ 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'])) {
+ unset($_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');
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment