SA-CORE-2023-005 by benjifisher, Heine, cmlara, mlhess, larowlan, David_Rothstein, xjm, Wim Leers, DamienMcKenna, effulgentsia, pwolanin, mcdruid, poker10, jenlampton, longwave, kim.pepper, alexpott, drumm

core/assets/scaffold/files/default.settings.php

@@ -531,6 +531,25 @@ $settings['update_free_access'] = FALSE;
  */
 # $settings['file_additional_public_schemes'] = ['example'];
 
+/**
+ * File schemes whose paths should not be normalized:
+ *
+ * Normally, Drupal normalizes '/./' and '/../' segments in file URIs in order
+ * to prevent unintended file access. For example, 'private://css/../image.png'
+ * is normalized to 'private://image.png' before checking access to the file.
+ *
+ * On Windows, Drupal also replaces '\' with '/' in URIs for the local
+ * filesystem.
+ *
+ * If file URIs with one or more scheme should not be normalized like this, then
+ * list the schemes here. For example, if 'porcelain://china/./plate.png' should
+ * not be normalized to 'porcelain://china/plate.png', then add 'porcelain' to
+ * this array. In this case, make sure that the module providing the 'porcelain'
+ * scheme does not allow unintended file access when using '/../' to move up the
+ * directory tree.
+ */
+# $settings['file_sa_core_2023_005_schemes'] = ['porcelain'];
+
 /**
  * Private file path:
  *

core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Core\StreamWrapper;
 
+use Drupal\Core\Site\Settings;
 use Symfony\Component\DependencyInjection\ContainerAwareInterface;
 use Symfony\Component\DependencyInjection\ContainerAwareTrait;
 
@@ -242,6 +243,36 @@ class StreamWrapperManager implements ContainerAwareInterface, StreamWrapperMana
       $target = $this->getTarget($uri);
 
       if ($target !== FALSE) {
+
+        if (!in_array($scheme, Settings::get('file_sa_core_2023_005_schemes', []))) {
+          $class = $this->getClass($scheme);
+          $is_local = is_subclass_of($class, LocalStream::class);
+          if ($is_local) {
+            $target = str_replace(DIRECTORY_SEPARATOR, '/', $target);
+          }
+
+          $parts = explode('/', $target);
+          $normalized_parts = [];
+          while ($parts) {
+            $part = array_shift($parts);
+            if ($part === '' || $part === '.') {
+              continue;
+            }
+            elseif ($part === '..' && $is_local && $normalized_parts === []) {
+              $normalized_parts[] = $part;
+              break;
+            }
+            elseif ($part === '..') {
+              array_pop($normalized_parts);
+            }
+            else {
+              $normalized_parts[] = $part;
+            }
+          }
+
+          $target = implode('/', array_merge($normalized_parts, $parts));
+        }
+
         $uri = $scheme . '://' . $target;
       }
     }

core/modules/image/src/Controller/ImageStyleDownloadController.php

@@ -109,6 +109,19 @@ class ImageStyleDownloadController extends FileDownloadController {
   public function deliver(Request $request, $scheme, ImageStyleInterface $image_style) {
     $target = $request->query->get('file');
     $image_uri = $scheme . '://' . $target;
+    $image_uri = $this->streamWrapperManager->normalizeUri($image_uri);
+
+    if ($this->streamWrapperManager->isValidScheme($scheme)) {
+      $normalized_target = $this->streamWrapperManager->getTarget($image_uri);
+      if ($normalized_target !== FALSE) {
+        if (!in_array($scheme, Settings::get('file_sa_core_2023_005_schemes', []))) {
+          $parts = explode('/', $normalized_target);
+          if (array_intersect($parts, ['.', '..'])) {
+            throw new NotFoundHttpException();
+          }
+        }
+      }
+    }
 
     // Check that the style is defined and the scheme is valid.
     $valid = !empty($image_style) && $this->streamWrapperManager->isValidScheme($scheme);
@@ -124,7 +137,8 @@ class ImageStyleDownloadController extends FileDownloadController {
     // styles/<style_name>/... as structure, so we check if the $target variable
     // starts with styles/.
     $token = $request->query->get(IMAGE_DERIVATIVE_TOKEN, '');
-    $token_is_valid = hash_equals($image_style->getPathToken($image_uri), $token);
+    $token_is_valid = hash_equals($image_style->getPathToken($image_uri), $token)
+      || hash_equals($image_style->getPathToken($scheme . '://' . $target), $token);
     if (!$this->config('image.settings')->get('allow_insecure_derivatives') || strpos(ltrim($target, '\/'), 'styles/') === 0) {
       $valid = $valid && $token_is_valid;
     }

core/modules/image/src/Entity/ImageStyle.php

@@ -205,11 +205,11 @@ class ImageStyle extends ConfigEntityBase implements ImageStyleInterface, Entity
    * {@inheritdoc}
    */
   public function buildUrl($path, $clean_urls = NULL) {
-    $uri = $this->buildUri($path);
-
     /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
     $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
 
+    $uri = $stream_wrapper_manager->normalizeUri($this->buildUri($path));
+
     // The token query is added even if the
     // 'image.settings:allow_insecure_derivatives' configuration is TRUE, so
     // that the emitted links remain valid if it is changed back to the default

core/modules/system/src/FileDownloadController.php

@@ -69,7 +69,7 @@ class FileDownloadController extends ControllerBase {
   public function download(Request $request, $scheme = 'private') {
     $target = $request->query->get('file');
     // Merge remaining path arguments into relative file path.
-    $uri = $scheme . '://' . $target;
+    $uri = $this->streamWrapperManager->normalizeUri($scheme . '://' . $target);
 
     if ($this->streamWrapperManager->isValidScheme($scheme) && is_file($uri)) {
       // Let other modules provide headers and controls access to the file.

core/modules/system/system.module

@@ -28,6 +28,7 @@ use Drupal\Core\Queue\QueueGarbageCollectionInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Routing\StackedRouteMatchInterface;
 use Drupal\Core\Site\Settings;
+use Drupal\Core\StreamWrapper\LocalStream;
 use Drupal\Core\StreamWrapper\StreamWrapperManager;
 use Drupal\Core\Url;
 use GuzzleHttp\Exception\TransferException;
@@ -1376,6 +1377,26 @@ function system_page_top() {
  * Implements hook_file_download().
  */
 function system_file_download($uri) {
+  $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
+  $scheme = $stream_wrapper_manager->getScheme($uri);
+  if ($stream_wrapper_manager->isValidScheme($scheme)) {
+    $target = $stream_wrapper_manager->getTarget($uri);
+    if ($target !== FALSE) {
+      if (!in_array($scheme, Settings::get('file_sa_core_2023_005_schemes', []))) {
+        if (DIRECTORY_SEPARATOR !== '/') {
+          $class = $stream_wrapper_manager->getClass($scheme);
+          if (is_subclass_of($class, LocalStream::class)) {
+            $target = str_replace(DIRECTORY_SEPARATOR, '/', $target);
+          }
+        }
+        $parts = explode('/', $target);
+        if (array_intersect($parts, ['.', '..'])) {
+          return -1;
+        }
+      }
+    }
+  }
+
   $core_schemes = ['public', 'private', 'temporary'];
   $additional_public_schemes = array_diff(Settings::get('file_additional_public_schemes', []), $core_schemes);
   if ($additional_public_schemes) {

sites/default/default.settings.php

@@ -531,6 +531,25 @@ $settings['update_free_access'] = FALSE;
  */
 # $settings['file_additional_public_schemes'] = ['example'];
 
+/**
+ * File schemes whose paths should not be normalized:
+ *
+ * Normally, Drupal normalizes '/./' and '/../' segments in file URIs in order
+ * to prevent unintended file access. For example, 'private://css/../image.png'
+ * is normalized to 'private://image.png' before checking access to the file.
+ *
+ * On Windows, Drupal also replaces '\' with '/' in URIs for the local
+ * filesystem.
+ *
+ * If file URIs with one or more scheme should not be normalized like this, then
+ * list the schemes here. For example, if 'porcelain://china/./plate.png' should
+ * not be normalized to 'porcelain://china/plate.png', then add 'porcelain' to
+ * this array. In this case, make sure that the module providing the 'porcelain'
+ * scheme does not allow unintended file access when using '/../' to move up the
+ * directory tree.
+ */
+# $settings['file_sa_core_2023_005_schemes'] = ['porcelain'];
+
 /**
  * Private file path:
  *