From 78fb9287a2f0487c39288070cb0493a5265f1789 Mon Sep 17 00:00:00 2001
From: Oliver Hader <oliver@typo3.org>
Date: Tue, 13 Feb 2024 10:05:25 +0100
Subject: [PATCH] [!!!][SECURITY] Enforce absolute path checks in FAL local
driver
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/+/82945
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
---
.../Classes/Resource/Driver/LocalDriver.php | 15 +++++++
.../core/Classes/Utility/GeneralUtility.php | 3 +-
.../core/Classes/Utility/PathUtility.php | 29 ++++++++++++++
.../DefaultConfigurationDescription.yaml | 4 +-
...ePathsToMatchProjectRootOrLockRootPath.rst | 39 +++++++++++++++++++
.../Tests/Unit/Utility/PathUtilityTest.php | 37 ++++++++++++++++++
.../Classes/Controller/FileListController.php | 3 +-
.../Language/locallang_mod_file_list.xlf | 4 +-
8 files changed, 127 insertions(+), 7 deletions(-)
create mode 100644 typo3/sysext/core/Documentation/Changelog/11.5.x/Important-102800-FileAbstractionLayerEnforcesAbsolutePathsToMatchProjectRootOrLockRootPath.rst
diff --git a/typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php b/typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php
index c01d7f1e3e5f..82d68b7bac40 100644
--- a/typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php
+++ b/typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php
@@ -165,6 +165,12 @@ class LocalDriver extends AbstractHierarchicalFilesystemDriver implements Stream
}
$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.',
@@ -1464,4 +1470,13 @@ class LocalDriver extends AbstractHierarchicalFilesystemDriver implements Stream
return '';
}
+
+ /**
+ * Wrapper for `GeneralUtility::isAllowedAbsPath`, which implicitly invokes
+ * `GeneralUtility::validPathStr` (like in `parent::isPathValid`).
+ */
+ protected function isAllowedAbsolutePath(string $path): bool
+ {
+ return GeneralUtility::isAllowedAbsPath($path);
+ }
}
diff --git a/typo3/sysext/core/Classes/Utility/GeneralUtility.php b/typo3/sysext/core/Classes/Utility/GeneralUtility.php
index 2aae2f2b43be..fa63fbdedc7a 100644
--- a/typo3/sysext/core/Classes/Utility/GeneralUtility.php
+++ b/typo3/sysext/core/Classes/Utility/GeneralUtility.php
@@ -2659,12 +2659,11 @@ class GeneralUtility
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)
);
}
diff --git a/typo3/sysext/core/Classes/Utility/PathUtility.php b/typo3/sysext/core/Classes/Utility/PathUtility.php
index 31a01cfb41af..d676ea04274c 100644
--- a/typo3/sysext/core/Classes/Utility/PathUtility.php
+++ b/typo3/sysext/core/Classes/Utility/PathUtility.php
@@ -446,4 +446,33 @@ class PathUtility
{
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;
+ }
}
diff --git a/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml b/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
index 869dac0a84a2..a6ae8acf1cc2 100644
--- a/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
+++ b/typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml
@@ -246,8 +246,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!'
diff --git a/typo3/sysext/core/Documentation/Changelog/11.5.x/Important-102800-FileAbstractionLayerEnforcesAbsolutePathsToMatchProjectRootOrLockRootPath.rst b/typo3/sysext/core/Documentation/Changelog/11.5.x/Important-102800-FileAbstractionLayerEnforcesAbsolutePathsToMatchProjectRootOrLockRootPath.rst
new file mode 100644
index 000000000000..0eff41bb18d8
--- /dev/null
+++ b/typo3/sysext/core/Documentation/Changelog/11.5.x/Important-102800-FileAbstractionLayerEnforcesAbsolutePathsToMatchProjectRootOrLockRootPath.rst
@@ -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
diff --git a/typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php b/typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php
index 8eb78ff01070..5e977c4b650f 100644
--- a/typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php
+++ b/typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php
@@ -560,4 +560,41 @@ final class PathUtilityTest extends UnitTestCase
{
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));
+ }
}
diff --git a/typo3/sysext/filelist/Classes/Controller/FileListController.php b/typo3/sysext/filelist/Classes/Controller/FileListController.php
index ed4370f040af..fd0bd0840082 100644
--- a/typo3/sysext/filelist/Classes/Controller/FileListController.php
+++ b/typo3/sysext/filelist/Classes/Controller/FileListController.php
@@ -43,6 +43,7 @@ use TYPO3\CMS\Core\Page\JavaScriptModuleInstruction;
use TYPO3\CMS\Core\Page\PageRenderer;
use TYPO3\CMS\Core\Resource\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;
@@ -148,7 +149,7 @@ class FileListController implements LoggerAwareInterface
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
diff --git a/typo3/sysext/filelist/Resources/Private/Language/locallang_mod_file_list.xlf b/typo3/sysext/filelist/Resources/Private/Language/locallang_mod_file_list.xlf
index 46d366724133..c40683387b0a 100644
--- a/typo3/sysext/filelist/Resources/Private/Language/locallang_mod_file_list.xlf
+++ b/typo3/sysext/filelist/Resources/Private/Language/locallang_mod_file_list.xlf
@@ -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>
--
GitLab