From 706b0006c5c43d0d42a1d81c62de81ee7905a97a Mon Sep 17 00:00:00 2001 From: xjm Date: Wed, 19 Apr 2023 11:18:37 -0500 Subject: [PATCH] =?UTF-8?q?SA-CORE-2023-005=20by=20benjifisher,=20Heine,?= =?UTF-8?q?=20cmlara,=20mlhess,=C2=A0larowlan,=C2=A0David=5FRothstein,?= =?UTF-8?q?=C2=A0xjm,=C2=A0Wim=20Leers,=C2=A0DamienMcKenna,=C2=A0effulgent?= =?UTF-8?q?sia,=C2=A0pwolanin,=C2=A0mcdruid,=C2=A0poker10,=20jenlampton,?= =?UTF-8?q?=C2=A0longwave,=C2=A0kim.pepper,=C2=A0alexpott,=20drumm?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../scaffold/files/default.settings.php | 19 ++++++++++++ .../StreamWrapper/StreamWrapperManager.php | 31 +++++++++++++++++++ .../ImageStyleDownloadController.php | 16 +++++++++- core/modules/image/src/Entity/ImageStyle.php | 4 +-- .../system/src/FileDownloadController.php | 2 +- core/modules/system/system.module | 21 +++++++++++++ sites/default/default.settings.php | 19 ++++++++++++ 7 files changed, 108 insertions(+), 4 deletions(-) diff --git a/core/assets/scaffold/files/default.settings.php b/core/assets/scaffold/files/default.settings.php index 5f88a44c164..81a6dbf3d0b 100644 --- a/core/assets/scaffold/files/default.settings.php +++ b/core/assets/scaffold/files/default.settings.php @@ -541,6 +541,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: * diff --git a/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php index 4799a378b57..1e1bd5f178c 100644 --- a/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php +++ b/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; } } diff --git a/core/modules/image/src/Controller/ImageStyleDownloadController.php b/core/modules/image/src/Controller/ImageStyleDownloadController.php index b077c93336a..1df6ef1d42f 100644 --- a/core/modules/image/src/Controller/ImageStyleDownloadController.php +++ b/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//... 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') || str_starts_with(ltrim($target, '\/'), 'styles/')) { $valid = $valid && $token_is_valid; } diff --git a/core/modules/image/src/Entity/ImageStyle.php b/core/modules/image/src/Entity/ImageStyle.php index 5f6d1ebe96a..964823934ed 100644 --- a/core/modules/image/src/Entity/ImageStyle.php +++ b/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 diff --git a/core/modules/system/src/FileDownloadController.php b/core/modules/system/src/FileDownloadController.php index cb9071bc171..e2f9132e246 100644 --- a/core/modules/system/src/FileDownloadController.php +++ b/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. diff --git a/core/modules/system/system.module b/core/modules/system/system.module index c821d3b2bfc..65b85da0c70 100644 --- a/core/modules/system/system.module +++ b/core/modules/system/system.module @@ -31,6 +31,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; @@ -1354,6 +1355,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) { diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php index 5f88a44c164..81a6dbf3d0b 100644 --- a/sites/default/default.settings.php +++ b/sites/default/default.settings.php @@ -541,6 +541,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: *