Skip to content

Commit

Permalink
SA-CORE-2023-005 by benjifisher, Heine, cmlara, mlhess, larowlan, Dav…
Browse files Browse the repository at this point in the history
…id_Rothstein, xjm, Wim Leers, DamienMcKenna, effulgentsia, pwolanin, mcdruid, poker10, jenlampton, longwave, kim.pepper, alexpott, drumm
  • Loading branch information
xjm committed Apr 19, 2023
1 parent 8ad87c8 commit 8360ad0
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 4 deletions.
19 changes: 19 additions & 0 deletions assets/scaffold/files/default.settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,25 @@
*/
# $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:
*
Expand Down
31 changes: 31 additions & 0 deletions lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Drupal\Core\StreamWrapper;

use Drupal\Core\Site\Settings;
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerAwareTrait;

Expand Down Expand Up @@ -242,6 +243,36 @@ public function normalizeUri($uri) {
$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;
}
}
Expand Down
16 changes: 15 additions & 1 deletion modules/image/src/Controller/ImageStyleDownloadController.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,19 @@ public static function create(ContainerInterface $container) {
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);
Expand All @@ -129,7 +142,8 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
// 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;
}
Expand Down
4 changes: 2 additions & 2 deletions modules/image/src/Entity/ImageStyle.php
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,11 @@ public function buildUri($uri) {
* {@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
Expand Down
2 changes: 1 addition & 1 deletion modules/system/src/FileDownloadController.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public static function create(ContainerInterface $container) {
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.
Expand Down
21 changes: 21 additions & 0 deletions modules/system/system.module
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1392,6 +1393,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) {
Expand Down

0 comments on commit 8360ad0

Please sign in to comment.