Skip to content

Commit

Permalink
[!!!][SECURITY] Enforce absolute path checks in FAL local driver
Browse files Browse the repository at this point in the history
The File Abstraction Layer Local Driver did not verify whether
a given absolute file path is allowed, and made it possible to
access files outside of the project path, and to by-pass the
setting in $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'].

In case lockRootPath is not set, any local file path must be
at least located in the base directory of the current project.

The lockRootPath setting now supports array values as well.

The trailing slash is enforced automatically. Example:
* instead of 'lockRootPath=/var/spe' previously matching
  the paths '/var/specs/'  and '/var/specials/,
* now both paths need to be declared explicitly, since
  'lockRootPath=/var/spe' is evaluated as '/var/spe/'

Resolves: #102800
Releases: main, 13.0, 12.4, 11.5
Change-Id: I6561df562c5dbaff1f77d33db24d5f1c6358b198
Security-Bulletin: TYPO3-CORE-SA-2024-001
Security-References: CVE-2023-30451
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/82951
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
  • Loading branch information
ohader committed Feb 13, 2024
1 parent 44d84f6 commit 205115c
Show file tree
Hide file tree
Showing 8 changed files with 127 additions and 7 deletions.
15 changes: 15 additions & 0 deletions typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php
Expand Up @@ -157,6 +157,12 @@ protected function calculateBasePath(array $configuration): string
}
$absoluteBasePath = $this->canonicalizeAndCheckFilePath($absoluteBasePath);
$absoluteBasePath = rtrim($absoluteBasePath, '/') . '/';
if (!$this->isAllowedAbsolutePath($absoluteBasePath)) {
throw new InvalidConfigurationException(
'Base path "' . $absoluteBasePath . '" is not within the allowed project root path or allowed lockRootPath.',
1704807715
);
}
if (!is_dir($absoluteBasePath)) {
throw new InvalidConfigurationException(
'Base path "' . $absoluteBasePath . '" does not exist or is no directory.',
Expand Down Expand Up @@ -1360,4 +1366,13 @@ protected function getRecycleDirectory(string $path): string

return '';
}

/**
* Wrapper for `GeneralUtility::isAllowedAbsPath`, which implicitly invokes
* `GeneralUtility::validPathStr` (like in `parent::isPathValid`).
*/
protected function isAllowedAbsolutePath(string $path): bool
{
return GeneralUtility::isAllowedAbsPath($path);
}
}
3 changes: 1 addition & 2 deletions typo3/sysext/core/Classes/Utility/GeneralUtility.php
Expand Up @@ -2478,12 +2478,11 @@ public static function isAllowedAbsPath(string $path): bool
if (substr($path, 0, 6) === 'vfs://') {
return true;
}
$lockRootPath = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] ?? '';
return PathUtility::isAbsolutePath($path) && static::validPathStr($path)
&& (
str_starts_with($path, Environment::getProjectPath())
|| str_starts_with($path, Environment::getPublicPath())
|| ($lockRootPath && str_starts_with($path, $lockRootPath))
|| PathUtility::isAllowedAdditionalPath($path)
);
}

Expand Down
29 changes: 29 additions & 0 deletions typo3/sysext/core/Classes/Utility/PathUtility.php
Expand Up @@ -446,4 +446,33 @@ public static function hasProtocolAndScheme(string $path): bool
{
return str_starts_with($path, '//') || strpos($path, '://') > 0;
}

/**
* Evaluates a given path against the optional settings in `$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']`.
* Albeit the name `BE/lockRootPath` is misleading, this setting was and is used in general and is not limited
* to the backend-scope. The setting actually allows defining additional paths, besides the project root path.
*
* @param string $path Absolute path to a file or directory
*/
public static function isAllowedAdditionalPath(string $path): bool
{
// ensure the submitted path ends with a string, even for a file
$path = self::sanitizeTrailingSeparator($path);
$allowedPaths = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] ?? [];
if (is_string($allowedPaths)) {
// The setting was a string before and is now an array
// For compatibility reasons, we cast a string to an array here for now
$allowedPaths = [$allowedPaths];
}
if (!is_array($allowedPaths)) {
throw new \RuntimeException('$GLOBALS[\'TYPO3_CONF_VARS\'][\'BE\'][\'lockRootPath\'] is expected to be an array.', 1707408379);
}
foreach ($allowedPaths as $allowedPath) {
$allowedPath = trim($allowedPath);
if ($allowedPath !== '' && str_starts_with($path, self::sanitizeTrailingSeparator($allowedPath))) {
return true;
}
}
return false;
}
}
Expand Up @@ -226,8 +226,8 @@ BE:
type: text
description: 'Path to the primary directory of files for editors. This is relative to the public web dir, DefaultStorage will be created with that configuration, do not access manually but via <code>\TYPO3\CMS\Core\Resource\ResourceFactory::getDefaultStorage().</code>'
lockRootPath:
type: text
description: 'This path is used to evaluate if paths outside of public web path should be allowed. Ending slash required!'
type: array
description: 'List of absolute root path prefixes to be allowed for file operations (including FAL storages). The project root path is allowed in any case and does not need to be defined here. Ending slashes are enforced!'
userHomePath:
type: text
description: 'Combined folder identifier of the directory where TYPO3 backend-users have their home-dirs. A combined folder identifier looks like this: [storageUid]:[folderIdentifier]. Eg. <code>2:users/</code>. A home for backend user 2 would be: <code>2:users/2/</code>. Ending slash required!'
Expand Down
@@ -0,0 +1,39 @@
.. include:: /Includes.rst.txt

.. _important-102800-1707409544:

=========================================================================================================
Important: #102800 - File Abstraction Layer enforces absolute paths to match project root or lockRootPath
=========================================================================================================

See :issue:`102800`

Description
===========


The File Abstraction Layer Local Driver has been adapted to verify whether a
given absolute file path is allowed in order to prevent access to files outside
the project root or to the additional root path restrictions defined in
:php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']`.

The option :php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']` has been
extended to support an array of root path prefixes to allow for multiple storages
to be listed. Beware that trailing slashes are enforced automatically.

It is suggested to use the new array-based syntax, which will be applied automatically
once this setting is updated via Install Tool Configuration Wizard:

.. code-block:: php
// Before
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = '/var/extra-storage';
// After
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = [
'/var/extra-storage1/',
'/var/extra-storage2/',
];
.. index:: FAL, LocalConfiguration, ext:core
37 changes: 37 additions & 0 deletions typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php
Expand Up @@ -560,4 +560,41 @@ public function hasProtocolAndScheme(string $url, bool $result): void
{
self::assertSame($result, PathUtility::hasProtocolAndScheme($url));
}

public static function allowedAdditionalPathsAreEvaluatedDataProvider(): \Generator
{
// empty settings
yield [null, '/var/shared', false];
yield ['', '/var/shared', false];
yield [' ', '/var/shared', false];
yield [[], '/var/shared', false];
yield [[''], '/var/shared', false];
yield [[' '], '/var/shared', false];
// string settings
yield ['/var', '/var/shared', true];
yield ['/var/shared/', '/var/shared', true];
yield ['/var/shared', '/var/shared/', true];
yield ['/var/shared/', '/var/shared/', true];
yield ['/var/shared/', '/var/shared/file.png', true];
yield ['/var/shared/', '/var/shared-secret', false];
yield ['/var/shared/', '/var', false];
// array settings
yield [['/var'], '/var/shared', true];
yield [['/var/shared/'], '/var/shared', true];
yield [['/var/shared'], '/var/shared/', true];
yield [['/var/shared/'], '/var/shared/', true];
yield [['/var/shared/'], '/var/shared/file.png', true];
yield [['/var/shared/'], '/var/shared-secret', false];
yield [['/var/shared/'], '/var', false];
}

/**
* @test
* @dataProvider allowedAdditionalPathsAreEvaluatedDataProvider
*/
public function allowedAdditionalPathsAreEvaluated(mixed $lockRootPath, string $path, bool $expectation): void
{
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = $lockRootPath;
self::assertSame($expectation, PathUtility::isAllowedAdditionalPath($path));
}
}
Expand Up @@ -44,6 +44,7 @@
use TYPO3\CMS\Core\Page\PageRenderer;
use TYPO3\CMS\Core\Resource\Enum\DuplicationBehavior;
use TYPO3\CMS\Core\Resource\Exception;
use TYPO3\CMS\Core\Resource\Exception\FolderDoesNotExistException;
use TYPO3\CMS\Core\Resource\Exception\InsufficientFolderAccessPermissionsException;
use TYPO3\CMS\Core\Resource\Folder;
use TYPO3\CMS\Core\Resource\ResourceFactory;
Expand Down Expand Up @@ -150,7 +151,7 @@ public function handleRequest(ServerRequestInterface $request): ResponseInterfac
if ($this->folderObject && !$this->folderObject->getStorage()->isWithinFileMountBoundaries($this->folderObject)) {
throw new \RuntimeException('Folder not accessible.', 1430409089);
}
} catch (InsufficientFolderAccessPermissionsException $permissionException) {
} catch (FolderDoesNotExistException|InsufficientFolderAccessPermissionsException $permissionException) {
$this->folderObject = null;
if ($storage !== null && $storage->getDriverType() === 'Local' && !$storage->isOnline()) {
// If the base folder for a local storage does not exists, the storage is marked as offline and the
Expand Down
Expand Up @@ -73,10 +73,10 @@
<source>You have no access to the folder "%s".</source>
</trans-unit>
<trans-unit id="localStorageOfflineTitle" resname="localStorageOfflineTitle">
<source>Base folder for local storage missing</source>
<source>Base folder for local storage missing or not allowed</source>
</trans-unit>
<trans-unit id="localStorageOfflineMessage" resname="localStorageOfflineMessage">
<source>Base folder for local storage does not exists. Verify that the base folder for "%s" exists.</source>
<source>Verify that the base folder for the storage "%s" exists and is allowed to be accessed.</source>
</trans-unit>
<trans-unit id="folderNotFoundTitle" resname="folderNotFoundTitle">
<source>Folder not found.</source>
Expand Down

0 comments on commit 205115c

Please sign in to comment.