Skip to content

Commit accf537

Browse files
ohaderohader
committed
[!!!][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/+/82939 Reviewed-by: Oliver Hader <[email protected]> Tested-by: Oliver Hader <[email protected]>
1 parent 3f3ae9e commit accf537

File tree

8 files changed

+129
-7
lines changed

8 files changed

+129
-7
lines changed

‎typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,12 @@ protected function calculateBasePath(array $configuration)
169169
}
170170
$absoluteBasePath = $this->canonicalizeAndCheckFilePath($absoluteBasePath);
171171
$absoluteBasePath = rtrim($absoluteBasePath, '/') . '/';
172+
if (!$this->isAllowedAbsolutePath($absoluteBasePath)) {
173+
throw new InvalidConfigurationException(
174+
'Base path "' . $absoluteBasePath . '" is not within the allowed project root path or allowed lockRootPath.',
175+
1704807715
176+
);
177+
}
172178
if (!is_dir($absoluteBasePath)) {
173179
throw new InvalidConfigurationException(
174180
'Base path "' . $absoluteBasePath . '" does not exist or is no directory.',
@@ -1473,4 +1479,13 @@ protected function getRecycleDirectory($path)
14731479

14741480
return '';
14751481
}
1482+
1483+
/**
1484+
* Wrapper for `GeneralUtility::isAllowedAbsPath`, which implicitly invokes
1485+
* `GeneralUtility::validPathStr` (like in `parent::isPathValid`).
1486+
*/
1487+
protected function isAllowedAbsolutePath(string $path): bool
1488+
{
1489+
return GeneralUtility::isAllowedAbsPath($path);
1490+
}
14761491
}

‎typo3/sysext/core/Classes/Utility/GeneralUtility.php

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2826,12 +2826,11 @@ public static function isAllowedAbsPath($path)
28262826
if (substr($path, 0, 6) === 'vfs://') {
28272827
return true;
28282828
}
2829-
$lockRootPath = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] ?? '';
28302829
return PathUtility::isAbsolutePath($path) && static::validPathStr($path)
28312830
&& (
28322831
str_starts_with($path, Environment::getProjectPath())
28332832
|| str_starts_with($path, Environment::getPublicPath())
2834-
|| ($lockRootPath && str_starts_with($path, $lockRootPath))
2833+
|| PathUtility::isAllowedAdditionalPath($path)
28352834
);
28362835
}
28372836

‎typo3/sysext/core/Classes/Utility/PathUtility.php

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,4 +464,33 @@ public static function hasProtocolAndScheme(string $path): bool
464464
{
465465
return strpos($path, '//') === 0 || strpos($path, '://') > 0;
466466
}
467+
468+
/**
469+
* Evaluates a given path against the optional settings in `$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']`.
470+
* Albeit the name `BE/lockRootPath` is misleading, this setting was and is used in general and is not limited
471+
* to the backend-scope. The setting actually allows defining additional paths, besides the project root path.
472+
*
473+
* @param string $path Absolute path to a file or directory
474+
*/
475+
public static function isAllowedAdditionalPath(string $path): bool
476+
{
477+
// ensure the submitted path ends with a string, even for a file
478+
$path = self::sanitizeTrailingSeparator($path);
479+
$allowedPaths = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] ?? [];
480+
if (is_string($allowedPaths)) {
481+
// The setting was a string before and is now an array
482+
// For compatibility reasons, we cast a string to an array here for now
483+
$allowedPaths = [$allowedPaths];
484+
}
485+
if (!is_array($allowedPaths)) {
486+
throw new \RuntimeException('$GLOBALS[\'TYPO3_CONF_VARS\'][\'BE\'][\'lockRootPath\'] is expected to be an array.', 1707408379);
487+
}
488+
foreach ($allowedPaths as $allowedPath) {
489+
$allowedPath = trim($allowedPath);
490+
if ($allowedPath !== '' && str_starts_with($path, self::sanitizeTrailingSeparator($allowedPath))) {
491+
return true;
492+
}
493+
}
494+
return false;
495+
}
467496
}

‎typo3/sysext/core/Configuration/DefaultConfigurationDescription.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,8 @@ BE:
255255
type: text
256256
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>'
257257
lockRootPath:
258-
type: text
259-
description: 'This path is used to evaluate if paths outside of public web path should be allowed. Ending slash required!'
258+
type: array
259+
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!'
260260
userHomePath:
261261
type: text
262262
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!'

‎typo3/sysext/core/Documentation/Changelog/11.5.x/Important-102800-FileAbstractionLayerEnforcesAbsolutePathsToMatchProjectRootOrLockRootPath.rst

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
.. include:: /Includes.rst.txt
2+
3+
.. _important-102800-1707409544:
4+
5+
=========================================================================================================
6+
Important: #102800 - File Abstraction Layer enforces absolute paths to match project root or lockRootPath
7+
=========================================================================================================
8+
9+
See :issue:`102800`
10+
11+
Description
12+
===========
13+
14+
15+
The File Abstraction Layer Local Driver has been adapted to verify whether a
16+
given absolute file path is allowed in order to prevent access to files outside
17+
the project root or to the additional root path restrictions defined in
18+
:php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']`.
19+
20+
The option :php:`$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath']` has been
21+
extended to support an array of root path prefixes to allow for multiple storages
22+
to be listed. Beware that trailing slashes are enforced automatically.
23+
24+
It is suggested to use the new array-based syntax, which will be applied automatically
25+
once this setting is updated via Install Tool Configuration Wizard:
26+
27+
.. code-block:: php
28+
29+
// Before
30+
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = '/var/extra-storage';
31+
32+
// After
33+
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = [
34+
'/var/extra-storage1/',
35+
'/var/extra-storage2/',
36+
];
37+
38+
39+
.. index:: FAL, LocalConfiguration, ext:core

‎typo3/sysext/core/Tests/Unit/Utility/PathUtilityTest.php

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -588,4 +588,43 @@ public function hasProtocolAndScheme(string $url, bool $result): void
588588
{
589589
self::assertSame($result, PathUtility::hasProtocolAndScheme($url));
590590
}
591+
592+
public static function allowedAdditionalPathsAreEvaluatedDataProvider(): \Generator
593+
{
594+
// empty settings
595+
yield [null, '/var/shared', false];
596+
yield ['', '/var/shared', false];
597+
yield [' ', '/var/shared', false];
598+
yield [[], '/var/shared', false];
599+
yield [[''], '/var/shared', false];
600+
yield [[' '], '/var/shared', false];
601+
// string settings
602+
yield ['/var', '/var/shared', true];
603+
yield ['/var/shared/', '/var/shared', true];
604+
yield ['/var/shared', '/var/shared/', true];
605+
yield ['/var/shared/', '/var/shared/', true];
606+
yield ['/var/shared/', '/var/shared/file.png', true];
607+
yield ['/var/shared/', '/var/shared-secret', false];
608+
yield ['/var/shared/', '/var', false];
609+
// array settings
610+
yield [['/var'], '/var/shared', true];
611+
yield [['/var/shared/'], '/var/shared', true];
612+
yield [['/var/shared'], '/var/shared/', true];
613+
yield [['/var/shared/'], '/var/shared/', true];
614+
yield [['/var/shared/'], '/var/shared/file.png', true];
615+
yield [['/var/shared/'], '/var/shared-secret', false];
616+
yield [['/var/shared/'], '/var', false];
617+
}
618+
619+
/**
620+
* @param mixed $lockRootPath
621+
*
622+
* @test
623+
* @dataProvider allowedAdditionalPathsAreEvaluatedDataProvider
624+
*/
625+
public function allowedAdditionalPathsAreEvaluated($lockRootPath, string $path, bool $expectation): void
626+
{
627+
$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = $lockRootPath;
628+
self::assertSame($expectation, PathUtility::isAllowedAdditionalPath($path));
629+
}
591630
}

‎typo3/sysext/filelist/Classes/Controller/FileListController.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use TYPO3\CMS\Core\Page\PageRenderer;
3838
use TYPO3\CMS\Core\Resource\DuplicationBehavior;
3939
use TYPO3\CMS\Core\Resource\Exception;
40+
use TYPO3\CMS\Core\Resource\Exception\FolderDoesNotExistException;
4041
use TYPO3\CMS\Core\Resource\Exception\InsufficientFolderAccessPermissionsException;
4142
use TYPO3\CMS\Core\Resource\Folder;
4243
use TYPO3\CMS\Core\Resource\ResourceFactory;
@@ -151,7 +152,7 @@ public function handleRequest(ServerRequestInterface $request): ResponseInterfac
151152
if ($this->folderObject && !$this->folderObject->getStorage()->isWithinFileMountBoundaries($this->folderObject)) {
152153
throw new \RuntimeException('Folder not accessible.', 1430409089);
153154
}
154-
} catch (InsufficientFolderAccessPermissionsException $permissionException) {
155+
} catch (FolderDoesNotExistException|InsufficientFolderAccessPermissionsException $permissionException) {
155156
$this->folderObject = null;
156157
if ($storage !== null && $storage->getDriverType() === 'Local' && !$storage->isOnline()) {
157158
// If the base folder for a local storage does not exists, the storage is marked as offline and the

‎typo3/sysext/filelist/Resources/Private/Language/locallang_mod_file_list.xlf

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,10 @@
7676
<source>You have no access to the folder "%s".</source>
7777
</trans-unit>
7878
<trans-unit id="localStorageOfflineTitle" resname="localStorageOfflineTitle">
79-
<source>Base folder for local storage missing</source>
79+
<source>Base folder for local storage missing or not allowed</source>
8080
</trans-unit>
8181
<trans-unit id="localStorageOfflineMessage" resname="localStorageOfflineMessage">
82-
<source>Base folder for local storage does not exists. Verify that the base folder for "%s" exists.</source>
82+
<source>Verify that the base folder for the storage "%s" exists and is allowed to be accessed.</source>
8383
</trans-unit>
8484
<trans-unit id="folderNotFoundTitle" resname="folderNotFoundTitle">
8585
<source>Folder not found.</source>

0 commit comments

Comments
 (0)