SA-CORE-2018-004 by David_Rothstein, alexpott, larowlan, Heine, Pere Orga, tim.plunkett, mlhess, xjm, Jasu_M, drumm, cashwilliams, quicksketch, dawehner, pwolanin, samuel.mortenson

Author: xjm

lib/Drupal/Core/Security/RequestSanitizer.php

@@ -2,6 +2,8 @@
 
 namespace Drupal\Core\Security;
 
+use Drupal\Component\Utility\UrlHelper;
+use Symfony\Component\HttpFoundation\ParameterBag;
 use Symfony\Component\HttpFoundation\Request;
 
 /**
@@ -39,33 +41,89 @@ class RequestSanitizer {
    */
   public static function sanitize(Request $request, $whitelist, $log_sanitized_keys = FALSE) {
     if (!$request->attributes->get(self::SANITIZED, FALSE)) {
-      // Process query string parameters.
-      $get_sanitized_keys = [];
-      $request->query->replace(static::stripDangerousValues($request->query->all(), $whitelist, $get_sanitized_keys));
-      if ($log_sanitized_keys && !empty($get_sanitized_keys)) {
-        trigger_error(sprintf('Potentially unsafe keys removed from query string parameters (GET): %s', implode(', ', $get_sanitized_keys)));
+      $update_globals = FALSE;
+      $bags = [
+        'query' => 'Potentially unsafe keys removed from query string parameters (GET): %s',
+        'request' => 'Potentially unsafe keys removed from request body parameters (POST): %s',
+        'cookies' => 'Potentially unsafe keys removed from cookie parameters: %s',
+      ];
+      foreach ($bags as $bag => $message) {
+        if (static::processParameterBag($request->$bag, $whitelist, $log_sanitized_keys, $bag, $message)) {
+          $update_globals = TRUE;
+        }
       }
-
-      // Request body parameters.
-      $post_sanitized_keys = [];
-      $request->request->replace(static::stripDangerousValues($request->request->all(), $whitelist, $post_sanitized_keys));
-      if ($log_sanitized_keys && !empty($post_sanitized_keys)) {
-        trigger_error(sprintf('Potentially unsafe keys removed from request body parameters (POST): %s', implode(', ', $post_sanitized_keys)));
+      if ($update_globals) {
+        $request->overrideGlobals();
       }
+      $request->attributes->set(self::SANITIZED, TRUE);
+    }
+    return $request;
+  }
 
-      // Cookie parameters.
-      $cookie_sanitized_keys = [];
-      $request->cookies->replace(static::stripDangerousValues($request->cookies->all(), $whitelist, $cookie_sanitized_keys));
-      if ($log_sanitized_keys && !empty($cookie_sanitized_keys)) {
-        trigger_error(sprintf('Potentially unsafe keys removed from cookie parameters: %s', implode(', ', $cookie_sanitized_keys)));
+  /**
+   * Processes a request parameter bag.
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $bag
+   *   The parameter bag to process.
+   * @param string[] $whitelist
+   *   An array of keys to whitelist as safe.
+   * @param bool $log_sanitized_keys
+   *   Set to TRUE to log keys that are sanitized.
+   * @param string $bag_name
+   *   The request parameter bag name. Either 'query', 'request' or 'cookies'.
+   * @param string $message
+   *   The message to log if the parameter bag contains keys that are removed.
+   *   If the message contains %s that is replaced by a list of removed keys.
+   *
+   * @return bool
+   *   TRUE if the parameter bag has been sanitized, FALSE if not.
+   */
+  protected static function processParameterBag(ParameterBag $bag, $whitelist, $log_sanitized_keys, $bag_name, $message) {
+    $sanitized = FALSE;
+    $sanitized_keys = [];
+    $bag->replace(static::stripDangerousValues($bag->all(), $whitelist, $sanitized_keys));
+    if (!empty($sanitized_keys)) {
+      $sanitized = TRUE;
+      if ($log_sanitized_keys) {
+        trigger_error(sprintf($message, implode(', ', $sanitized_keys)));
       }
+    }
 
-      if (!empty($get_sanitized_keys) || !empty($post_sanitized_keys) || !empty($cookie_sanitized_keys)) {
-        $request->overrideGlobals();
+    if ($bag->has('destination')) {
+      $destination_dangerous_keys = static::checkDestination($bag->get('destination'), $whitelist);
+      if (!empty($destination_dangerous_keys)) {
+        // The destination is removed rather than sanitized because the URL
+        // generator service is not available and this method is called very
+        // early in the bootstrap.
+        $bag->remove('destination');
+        $sanitized = TRUE;
+        if ($log_sanitized_keys) {
+          trigger_error(sprintf('Potentially unsafe destination removed from %s parameter bag because it contained the following keys: %s', $bag_name, implode(', ', $destination_dangerous_keys)));
+        }
       }
-      $request->attributes->set(self::SANITIZED, TRUE);
     }
-    return $request;
+    return $sanitized;
+  }
+
+  /**
+   * Checks a destination string to see if it is dangerous.
+   *
+   * @param string $destination
+   *   The destination string to check.
+   * @param array $whitelist
+   *   An array of keys to whitelist as safe.
+   *
+   * @return array
+   *   The dangerous keys found in the destination parameter.
+   */
+  protected static function checkDestination($destination, array $whitelist) {
+    $dangerous_keys = [];
+    $parts = UrlHelper::parse($destination);
+    // If there is a query string, check its query parameters.
+    if (!empty($parts['query'])) {
+      static::stripDangerousValues($parts['query'], $whitelist, $dangerous_keys);
+    }
+    return $dangerous_keys;
   }
 
   /**

modules/file/src/Element/ManagedFile.php

@@ -8,6 +8,7 @@
 use Drupal\Core\Ajax\AjaxResponse;
 use Drupal\Core\Ajax\ReplaceCommand;
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\Core\Render\Element;
 use Drupal\Core\Render\Element\FormElement;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\Url;
@@ -175,6 +176,9 @@ public static function uploadAjaxCallback(&$form, FormStateInterface &$form_stat
 
     $form_parents = explode('/', $request->query->get('element_parents'));
 
+    // Sanitize form parents before using them.
+    $form_parents = array_filter($form_parents, [Element::class, 'child']);
+
     // Retrieve the element to be rendered.
     $form = NestedArray::getValue($form, $form_parents);