From 242b04ea534c3fb5936c9952a3677082e6b8dfa7 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 1 May 2024 00:36:27 +0200 Subject: [PATCH 1/7] feat: Add `getForbiddenFilenames` to `OCP\Util` and ensure forbidden files are enforced * Provide filename blacklist in public `\OCP\Util` * Use filename blacklist and forbidden characters also in `Filesystem::isFileBlacklisted` * Drop unused and deprecated `\OC_Util::isValidFileName` Signed-off-by: Ferdinand Thiessen --- config/config.sample.php | 9 +++++---- lib/private/Files/Filesystem.php | 19 ++++++++++++------- lib/private/legacy/OC_Util.php | 29 ----------------------------- lib/public/Util.php | 27 ++++++++++++++++++++++++++- tests/lib/UtilTest.php | 8 ++------ 5 files changed, 45 insertions(+), 47 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index f45e7dcc5e0f6..cb8e0342edae7 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1961,8 +1961,9 @@ 'updatedirectory' => '', /** - * Blacklist a specific file or files and disallow the upload of files + * Deny a specific file or files and disallow the upload of files * with this name. ``.htaccess`` is blocked by default. + * * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING. * * Defaults to ``array('.htaccess')`` @@ -1970,12 +1971,12 @@ 'blacklisted_files' => ['.htaccess'], /** - * Blacklist characters from being used in filenames. This is useful if you + * Deny characters from being used in filenames. This is useful if you * have a filesystem or OS which does not support certain characters like windows. * - * The '/' and '\' characters are always forbidden. + * The '/' and '\' characters are always forbidden, as well as all characters in the ASCII range [0-31]. * - * Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"', chr(0), "\n", "\r")`` + * Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"')`` * see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits * * Defaults to ``array()`` diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index c6a5513d5b7c7..6a92a08410162 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -59,9 +59,6 @@ class Filesystem { private static ?CappedMemoryCache $normalizedPathCache = null; - /** @var string[]|null */ - private static ?array $blacklist = null; - /** * classname which used for hooks handling * used as signalclass in OC_Hooks::emit() @@ -464,13 +461,21 @@ public static function isValidPath($path) { */ public static function isFileBlacklisted($filename) { $filename = self::normalizePath($filename); + $filename = basename($filename); - if (self::$blacklist === null) { - self::$blacklist = \OC::$server->getConfig()->getSystemValue('blacklisted_files', ['.htaccess']); + if ($filename === '') { + return false; + } + + $forbiddenChars = \OCP\Util::getForbiddenFileNameChars(); + foreach($forbiddenChars as $char) { + if (str_contains($filename, $char)) { + return false; + } } - $filename = strtolower(basename($filename)); - return in_array($filename, self::$blacklist); + $forbiddenNames = \OCP\Util::getForbiddenFilenames(); + return in_array($filename, $forbiddenNames); } /** diff --git a/lib/private/legacy/OC_Util.php b/lib/private/legacy/OC_Util.php index 86472f1b5e569..746c1d2db31bc 100644 --- a/lib/private/legacy/OC_Util.php +++ b/lib/private/legacy/OC_Util.php @@ -1092,35 +1092,6 @@ public static function getHumanVersion() { return $version; } - /** - * Returns whether the given file name is valid - * - * @param string $file file name to check - * @return bool true if the file name is valid, false otherwise - * @deprecated use \OC\Files\View::verifyPath() - */ - public static function isValidFileName($file) { - $trimmed = trim($file); - if ($trimmed === '') { - return false; - } - if (\OC\Files\Filesystem::isIgnoredDir($trimmed)) { - return false; - } - - // detect part files - if (preg_match('/' . \OCP\Files\FileInfo::BLACKLIST_FILES_REGEX . '/', $trimmed) !== 0) { - return false; - } - - foreach (\OCP\Util::getForbiddenFileNameChars() as $char) { - if (str_contains($trimmed, $char)) { - return false; - } - } - return true; - } - /** * Check whether the instance needs to perform an upgrade, * either when the core version is higher or any app requires diff --git a/lib/public/Util.php b/lib/public/Util.php index 9ab9895acb87f..ae32d51e90a71 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -521,8 +521,28 @@ public static function uploadLimit(): int|float { return \OC_Helper::uploadLimit(); } + /** + * Get a list of reserved file names that must not be used + * @return string[] + * @since 30.0.0 + */ + public static function getForbiddenFilenames(): array { + $config = \OCP\Server::get(IConfig::class); + $invalidFilenames = $config->getSystemValue('blacklisted_files', ['.htaccess']); + if (!is_array($invalidFilenames)) { + \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.'); + $invalidFilenames = []; + } + return $invalidFilenames; + + } + /** * Get a list of characters forbidden in file names + * + * Note: Characters in the range [0-31] are always forbidden, + * even if not inside this list (see OCP\Files\Storage\IStorage::verifyPath). + * * @return string[] * @since 29.0.0 */ @@ -551,7 +571,12 @@ public static function getForbiddenFileNameChars(): array { * @suppress PhanDeprecatedFunction */ public static function isValidFileName($file) { - return \OC_Util::isValidFileName($file); + $trimmed = trim($file); + if ($trimmed === '' || \OC\Files\Filesystem::isIgnoredDir($trimmed)) { + return false; + } + + return \OC\Files\Filesystem::isFileBlacklisted($file); } /** diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 568f535e55752..290f009ae2be6 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -126,8 +126,6 @@ public function testGetInstanceIdGeneratesValidId() { * @dataProvider filenameValidationProvider */ public function testFilenameValidation($file, $valid) { - // private API - $this->assertEquals($valid, \OC_Util::isValidFileName($file)); // public API $this->assertEquals($valid, \OCP\Util::isValidFileName($file)); } @@ -169,10 +167,8 @@ public function filenameValidationProvider() { ['. ', false], [' .', false], - // part files not allowed - ['.part', false], - ['notallowed.part', false], - ['neither.filepart', false], + // htaccess files not allowed + ['.htaccess', false], // part in the middle is ok ['super movie part one.mkv', true], From 55932592216018b2e4c3cfbf895fafee9aa0c3eb Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 1 May 2024 00:41:31 +0200 Subject: [PATCH 2/7] feat: Cache filename blacklist and forbidden filename characters Signed-off-by: Ferdinand Thiessen --- .../unit/Connector/Sabre/DirectoryTest.php | 9 ++ .../tests/unit/Connector/Sabre/FileTest.php | 9 +- lib/public/Util.php | 49 +++++--- tests/lib/Files/PathVerificationTest.php | 11 ++ tests/lib/Files/Storage/CommonTest.php | 19 ++- tests/lib/UtilTest.php | 116 +++++++++++++++--- 6 files changed, 171 insertions(+), 42 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 0f41ff97cc6c5..8b9fd7ba2a6a7 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -100,6 +100,12 @@ protected function setUp(): void { ->willReturn(Constants::PERMISSION_READ); } + protected function tearDown(): void { + // Reset invalid chars as we touched this during the tests + self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]); + parent::tearDown(); + } + private function getDir($path = '/') { $this->view->expects($this->once()) ->method('getRelativePath') @@ -411,6 +417,9 @@ public function testMoveSuccess($source, $destination, $updatables, $deletables) * @dataProvider moveFailedInvalidCharsProvider */ public function testMoveFailedInvalidChars($source, $destination, $updatables, $deletables): void { + // Enforce * as an invalid character + self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]); + $this->expectException(\OCA\DAV\Connector\Sabre\Exception\InvalidPath::class); $this->moveTest($source, $destination, $updatables, $deletables); diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index a858c57b952a6..ce39ffa996f0a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -91,6 +91,8 @@ protected function tearDown(): void { $userManager = \OCP\Server::get(IUserManager::class); $userManager->get($this->user)->delete(); + // Reset invalid chars as we touched this during the tests + self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]); parent::tearDown(); } @@ -823,7 +825,8 @@ public function testChunkedPutFailsFinalRename(): void { * Test put file with invalid chars */ public function testSimplePutInvalidChars(): void { - // setup + // Enforce * as an invalid character + self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]); /** @var View|MockObject */ $view = $this->getMockBuilder(View::class) ->onlyMethods(['getRelativePath']) @@ -858,12 +861,12 @@ public function testSimplePutInvalidChars(): void { /** * Test setting name with setName() with invalid chars - * */ public function testSetNameInvalidChars(): void { $this->expectException(\OCA\DAV\Connector\Sabre\Exception\InvalidPath::class); - // setup + // Enforce * as an invalid character + self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]); /** @var View|MockObject */ $view = $this->getMockBuilder(View::class) ->onlyMethods(['getRelativePath']) diff --git a/lib/public/Util.php b/lib/public/Util.php index ae32d51e90a71..c94e5876f0f9d 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -61,6 +61,10 @@ class Util { private static ?IManager $shareManager = null; + /** @var string[] */ + private static array $invalidChars = []; + /** @var string[] */ + private static array $invalidFilenames = []; private static array $scriptsInit = []; private static array $scripts = []; private static array $scriptDeps = []; @@ -527,14 +531,17 @@ public static function uploadLimit(): int|float { * @since 30.0.0 */ public static function getForbiddenFilenames(): array { - $config = \OCP\Server::get(IConfig::class); - $invalidFilenames = $config->getSystemValue('blacklisted_files', ['.htaccess']); - if (!is_array($invalidFilenames)) { - \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.'); - $invalidFilenames = []; + if (empty(self::$invalidFilenames)) { + $config = \OCP\Server::get(IConfig::class); + + $invalidFilenames = $config->getSystemValue('blacklisted_files', ['.htaccess']); + if (!is_array($invalidFilenames)) { + \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.'); + $invalidFilenames = ['.htaccess']; + } + self::$invalidFilenames = $invalidFilenames; } - return $invalidFilenames; - + return self::$invalidFilenames; } /** @@ -547,19 +554,25 @@ public static function getForbiddenFilenames(): array { * @since 29.0.0 */ public static function getForbiddenFileNameChars(): array { - // Get always forbidden characters - $invalidChars = str_split(\OCP\Constants::FILENAME_INVALID_CHARS); - if ($invalidChars === false) { - $invalidChars = []; + if (empty(self::$invalidChars)) { + $config = \OCP\Server::get(IConfig::class); + + // Get always forbidden characters + $invalidChars = str_split(\OCP\Constants::FILENAME_INVALID_CHARS); + if ($invalidChars === false) { + $invalidChars = []; + } + + // Get admin defined invalid characters + $additionalChars = $config->getSystemValue('forbidden_chars', []); + if (!is_array($additionalChars)) { + \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "forbidden_chars" is ignored.'); + $additionalChars = []; + } + self::$invalidChars = array_merge($invalidChars, $additionalChars); } - // Get admin defined invalid characters - $additionalChars = \OCP\Server::get(IConfig::class)->getSystemValue('forbidden_chars', []); - if (!is_array($additionalChars)) { - \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "forbidden_chars" is ignored.'); - $additionalChars = []; - } - return array_merge($invalidChars, $additionalChars); + return self::$invalidChars; } /** diff --git a/tests/lib/Files/PathVerificationTest.php b/tests/lib/Files/PathVerificationTest.php index ededab14d0bfd..1d03572f9fc4f 100644 --- a/tests/lib/Files/PathVerificationTest.php +++ b/tests/lib/Files/PathVerificationTest.php @@ -27,8 +27,19 @@ class PathVerificationTest extends \Test\TestCase { protected function setUp(): void { parent::setUp(); $this->view = new View(); + self::resetOCPUtil(); } + protected function tearDown(): void { + parent::tearDown(); + self::resetOCPUtil(); + } + + protected static function resetOCPUtil(): void { + // Reset util cache + self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]); + self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]); + } public function testPathVerificationFileNameTooLong() { $this->expectException(\OCP\Files\InvalidPathException::class); diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php index 3740289df95be..cfad962182ee4 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -25,6 +25,8 @@ use OC\Files\Storage\Wrapper\Jail; use OC\Files\Storage\Wrapper\Wrapper; use OCP\Files\InvalidPathException; +use OCP\IConfig; +use OCP\ITempManager; use PHPUnit\Framework\MockObject\MockObject; /** @@ -44,16 +46,25 @@ class CommonTest extends Storage { protected function setUp(): void { parent::setUp(); + self::resetOCPUtil(); - $this->tmpDir = \OC::$server->getTempManager()->getTemporaryFolder(); + $this->tmpDir = \OCP\Server::get(ITempManager::class)->getTemporaryFolder(); $this->instance = new \OC\Files\Storage\CommonTest(['datadir' => $this->tmpDir]); - $this->invalidCharsBackup = \OC::$server->getConfig()->getSystemValue('forbidden_chars', []); + $this->invalidCharsBackup = \OCP\Server::get(IConfig::class)->getSystemValue('forbidden_chars', []); } protected function tearDown(): void { \OC_Helper::rmdirr($this->tmpDir); - \OC::$server->getConfig()->setSystemValue('forbidden_chars', $this->invalidCharsBackup); + \OCP\Server::get(IConfig::class)->setSystemValue('forbidden_chars', $this->invalidCharsBackup); + parent::tearDown(); + self::resetOCPUtil(); + } + + protected static function resetOCPUtil(): void { + // Reset util cache as we do not want to leak our test values into other tests + self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]); + self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]); } /** @@ -68,7 +79,7 @@ public function testVerifyPath(string $filename, array $additionalChars, bool $t $instance->method('copyFromStorage') ->willThrowException(new \Exception('copy')); - \OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars); + \OCP\Server::get(IConfig::class)->setSystemValue('forbidden_chars', $additionalChars); if ($throws) { $this->expectException(InvalidPathException::class); diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 290f009ae2be6..1207c633f2bf7 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -9,6 +9,7 @@ namespace Test; use OC_Util; +use OCP\IConfig; /** * Class UtilTest @@ -17,6 +18,25 @@ * @group DB */ class UtilTest extends \Test\TestCase { + protected function setUp(): void { + parent::setUp(); + self::resetOCPUtil(); + } + + protected function tearDown(): void { + parent::tearDown(); + self::resetOCPUtil(); + } + + protected static function resetOCPUtil() { + \OC_Util::$scripts = []; + \OC_Util::$styles = []; + self::invokePrivate(\OCP\Util::class, 'scripts', [[]]); + self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]); + self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]); + self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]); + } + public function testGetVersion() { $version = \OCP\Util::getVersion(); $this->assertTrue(is_array($version)); @@ -122,6 +142,85 @@ public function testGetInstanceIdGeneratesValidId() { $this->assertSame(1, $matchesRegex); } + public function testGetForbiddenCharacters() { + $config = \OCP\Server::get(IConfig::class); + $backup = $config->getSystemValue('forbidden_chars', []); + + try { + // Fake config + $config->setSystemValue('forbidden_chars', ['*', '-']); + $this->assertEqualsCanonicalizing( + [ + // Added by the test + '*', + '-', + // Always added + '/', + '\\', + ], + \OCP\Util::getForbiddenFileNameChars(), + ); + } finally { + // Reset config + $config->setSystemValue('forbidden_chars', $backup); + } + } + + public function testGetForbiddenCharactersInvalidConfig() { + $config = \OCP\Server::get(IConfig::class); + $backup = $config->getSystemValue('forbidden_chars', []); + + try { + // Fake config + $config->setSystemValue('forbidden_chars', 'not an array'); + $this->assertEqualsCanonicalizing( + [ + // Always added + '/', + '\\', + ], + \OCP\Util::getForbiddenFileNameChars(), + ); + } finally { + // Reset config + $config->setSystemValue('forbidden_chars', $backup); + } + } + + public function testGetForbiddenFilenames() { + $config = \OCP\Server::get(IConfig::class); + $backup = $config->getSystemValue('blacklisted_files', ['.htaccess']); + + try { + // Fake config + $config->setSystemValue('blacklisted_files', ['.htaccess', 'foo-bar']); + $this->assertEqualsCanonicalizing( + ['.htaccess', 'foo-bar'], + \OCP\Util::getForbiddenFilenames(), + ); + } finally { + // Reset config + $config->setSystemValue('blacklisted_files', $backup); + } + } + + public function testGetForbiddenFilenamesInvalidConfig() { + $config = \OCP\Server::get(IConfig::class); + $backup = $config->getSystemValue('blacklisted_files', ['.htaccess']); + + try { + // Fake config + $config->setSystemValue('blacklisted_files', 'not an array'); + $this->assertEqualsCanonicalizing( + ['.htaccess'], + \OCP\Util::getForbiddenFilenames(), + ); + } finally { + // Reset config + $config->setSystemValue('blacklisted_files', $backup); + } + } + /** * @dataProvider filenameValidationProvider */ @@ -216,23 +315,6 @@ public function testCheckDataDirectoryValidity() { $this->assertNotEmpty($errors); } - protected function setUp(): void { - parent::setUp(); - - \OC_Util::$scripts = []; - \OC_Util::$styles = []; - self::invokePrivate(\OCP\Util::class, 'scripts', [[]]); - self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]); - } - protected function tearDown(): void { - parent::tearDown(); - - \OC_Util::$scripts = []; - \OC_Util::$styles = []; - self::invokePrivate(\OCP\Util::class, 'scripts', [[]]); - self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]); - } - public function testAddScript() { \OCP\Util::addScript('first', 'myFirstJSFile'); \OCP\Util::addScript('core', 'myFancyJSFile1'); From 8b857740e4b01412ee340df9c41db08b056b85e2 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 1 May 2024 14:44:30 +0200 Subject: [PATCH 3/7] fix: Move filename validation to one common place Signed-off-by: Ferdinand Thiessen --- lib/private/Files/Filesystem.php | 28 +++++++++++----- lib/private/Files/Storage/Common.php | 33 ++----------------- lib/public/Util.php | 6 +++- tests/lib/Files/FilesystemTest.php | 42 +++++++++++++++++++++++- tests/lib/Files/PathVerificationTest.php | 8 +++-- tests/lib/Files/Storage/CommonTest.php | 1 + 6 files changed, 75 insertions(+), 43 deletions(-) diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 6a92a08410162..3bc240cb5b9f9 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -459,7 +459,26 @@ public static function isValidPath($path) { * @param string $filename * @return bool */ - public static function isFileBlacklisted($filename) { + public static function hasFilenameInvalidCharacters(string $filename): bool { + $invalidChars = \OCP\Util::getForbiddenFileNameChars(); + foreach ($invalidChars as $char) { + if (str_contains($filename, $char)) { + return true; + } + } + + $sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW); + if ($sanitizedFileName !== $filename) { + return true; + } + return false; + } + + /** + * @param string $filename + * @return bool + */ + public static function isFileBlacklisted(string $filename): bool { $filename = self::normalizePath($filename); $filename = basename($filename); @@ -467,13 +486,6 @@ public static function isFileBlacklisted($filename) { return false; } - $forbiddenChars = \OCP\Util::getForbiddenFileNameChars(); - foreach($forbiddenChars as $char) { - if (str_contains($filename, $char)) { - return false; - } - } - $forbiddenNames = \OCP\Util::getForbiddenFilenames(); return in_array($filename, $forbiddenNames); } diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index fb4aa0a7c3c58..d2962008e404c 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -558,41 +558,14 @@ public function verifyPath($path, $fileName) { throw new FileNameTooLongException(); } - // NOTE: $path will remain unverified for now - $this->verifyPosixPath($fileName); - } - - /** - * @param string $fileName - * @throws InvalidPathException - */ - protected function verifyPosixPath($fileName) { - $invalidChars = \OCP\Util::getForbiddenFileNameChars(); - $this->scanForInvalidCharacters($fileName, $invalidChars); - - $fileName = trim($fileName); - $reservedNames = ['*']; - if (in_array($fileName, $reservedNames)) { + if (\OC\Files\Filesystem::isFileBlacklisted($fileName)) { throw new ReservedWordException(); } - } - - /** - * @param string $fileName - * @param string[] $invalidChars - * @throws InvalidPathException - */ - private function scanForInvalidCharacters(string $fileName, array $invalidChars) { - foreach ($invalidChars as $char) { - if (str_contains($fileName, $char)) { - throw new InvalidCharacterInPathException(); - } - } - $sanitizedFileName = filter_var($fileName, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW); - if ($sanitizedFileName !== $fileName) { + if (\OC\Files\Filesystem::hasFilenameInvalidCharacters($fileName)) { throw new InvalidCharacterInPathException(); } + // NOTE: $path will remain unverified for now } /** diff --git a/lib/public/Util.php b/lib/public/Util.php index c94e5876f0f9d..666f440056e29 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -589,7 +589,11 @@ public static function isValidFileName($file) { return false; } - return \OC\Files\Filesystem::isFileBlacklisted($file); + if (\OC\Files\Filesystem::isFileBlacklisted($file)) { + return false; + } + + return !\OC\Files\Filesystem::hasFilenameInvalidCharacters($file); } /** diff --git a/tests/lib/Files/FilesystemTest.php b/tests/lib/Files/FilesystemTest.php index 5473f164cef52..5662a550773c8 100644 --- a/tests/lib/Files/FilesystemTest.php +++ b/tests/lib/Files/FilesystemTest.php @@ -284,7 +284,7 @@ public function isFileBlacklistedData() { ['/etc/foo\bar/.htaccess/', true], ['/etc/foo\bar/.htaccess/foo', false], ['//foo//bar/\.htaccess/', true], - ['\foo\bar\.HTAccess', true], + ['\foo\bar\.htaccess', true], ]; } @@ -295,6 +295,46 @@ public function testIsFileBlacklisted($path, $expected) { $this->assertSame($expected, \OC\Files\Filesystem::isFileBlacklisted($path)); } + /** + * @dataProvider hasFilenameInvalidCharactersData + */ + public function testHasFilenameInvalidCharacters($filename, $expected) { + $this->assertSame($expected, \OC\Files\Filesystem::hasFilenameInvalidCharacters($filename)); + } + + public function hasFilenameInvalidCharactersData(): array { + return array_merge( + [ + // slash and backslash are always forbidden + ['foobar/txt', true], + ['foobar\\txt', true], + // some valid special characters + ['foobar txt', false], + ['foobar-txt', false], + ['foobar_txt', false], + // Also unicode is allowed by default + ['šŸ˜¶ā€šŸŒ«ļø.txt', false], + ], + // Always block ascii 0-31 + array_map(fn (int $i) => ['foo' . chr($i) . 'txt', true], range(0, 31)), + ); + } + + public static function hasFilenameInvalidCharacters(string $filename): bool { + $invalidChars = \OCP\Util::getForbiddenFileNameChars(); + foreach ($invalidChars as $char) { + if (str_contains($filename, $char)) { + return true; + } + } + + $sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW); + if ($sanitizedFileName !== $filename) { + return true; + } + return false; + } + public function testNormalizePathUTF8() { if (!class_exists('Patchwork\PHP\Shim\Normalizer')) { $this->markTestSkipped('UTF8 normalizer Patchwork was not found'); diff --git a/tests/lib/Files/PathVerificationTest.php b/tests/lib/Files/PathVerificationTest.php index 1d03572f9fc4f..01e832bc4b753 100644 --- a/tests/lib/Files/PathVerificationTest.php +++ b/tests/lib/Files/PathVerificationTest.php @@ -126,12 +126,16 @@ public function testPathVerificationInvalidCharsPosix($fileName) { $storage = new Local(['datadir' => '']); $fileName = " 123{$fileName}456 "; - self::invokePrivate($storage, 'verifyPosixPath', [$fileName]); + $storage->verifyPath('', $fileName); } public function providesInvalidCharsPosix() { return [ + // posix forbidden [\chr(0)], + ['/'], + ['\\'], + // We restrict also ascii 1-31 [\chr(1)], [\chr(2)], [\chr(3)], @@ -163,8 +167,6 @@ public function providesInvalidCharsPosix() { [\chr(29)], [\chr(30)], [\chr(31)], - ['/'], - ['\\'], ]; } diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php index cfad962182ee4..5b561e79b0c17 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -35,6 +35,7 @@ * @group DB * * @package Test\Files\Storage + * @backupStaticAttributes enabled */ class CommonTest extends Storage { /** From 3f0657f67b61a36d13effd8043803a3722fb7fd9 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 2 May 2024 12:17:40 +0200 Subject: [PATCH 4/7] fix: Deprecate `\OCP\Files\FileInfo::BLACKLIST_FILES_REGEX` as this is unused Signed-off-by: Ferdinand Thiessen --- lib/public/Files/FileInfo.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/public/Files/FileInfo.php b/lib/public/Files/FileInfo.php index 817b03dfc65f1..779439b6024ba 100644 --- a/lib/public/Files/FileInfo.php +++ b/lib/public/Files/FileInfo.php @@ -70,6 +70,7 @@ interface FileInfo { /** * @const \OCP\Files\FileInfo::BLACKLIST_FILES_REGEX Return regular expression to test filenames against (blacklisting) * @since 12.0.0 + * @deprecated 30.0.0 Use \OCP\Util::getForbiddenFilenames() instead */ public const BLACKLIST_FILES_REGEX = '\.(part|filepart)$'; From cc77c890569aec5f41b98d6230f3625646853c64 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 2 May 2024 13:04:37 +0200 Subject: [PATCH 5/7] fix(JSConfig): Add `forbidden_filenames` and `forbidden_filename_characters` to the `oc_config` Signed-off-by: Ferdinand Thiessen --- lib/private/Template/JSConfigHelper.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index d2cd536fa9f90..a817843056745 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -168,7 +168,10 @@ public function getConfig(): string { $config = [ 'auto_logout' => $this->config->getSystemValue('auto_logout', false), + // deprecated 'blacklist_files_regex' => FileInfo::BLACKLIST_FILES_REGEX, + 'forbidden_filenames' => \OCP\Util::getForbiddenFilenames(), + 'forbidden_filename_characters' => \OCP\Util::getForbiddenFileNameChars(), 'loglevel' => $this->config->getSystemValue('loglevel_frontend', $this->config->getSystemValue('loglevel', ILogger::WARN) ), From 60838bf5f439f7ff2f7d4bc6fb1362f320feac32 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 2 May 2024 20:25:09 +0200 Subject: [PATCH 6/7] fix: Make forbidden filename list case insensitive again It previously was, even if not documented, case insensitive. Signed-off-by: Ferdinand Thiessen --- config/config.sample.php | 2 ++ lib/private/Files/Filesystem.php | 2 +- lib/public/Util.php | 3 ++- tests/lib/Files/FilesystemTest.php | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index cb8e0342edae7..ab582ba79df6c 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1966,6 +1966,8 @@ * * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING. * + * Note that this list is case-insensitive. + * * Defaults to ``array('.htaccess')`` */ 'blacklisted_files' => ['.htaccess'], diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 3bc240cb5b9f9..0049143135b7a 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -487,7 +487,7 @@ public static function isFileBlacklisted(string $filename): bool { } $forbiddenNames = \OCP\Util::getForbiddenFilenames(); - return in_array($filename, $forbiddenNames); + return in_array(mb_strtolower($filename), $forbiddenNames); } /** diff --git a/lib/public/Util.php b/lib/public/Util.php index 666f440056e29..6e172e68d71a1 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -527,6 +527,7 @@ public static function uploadLimit(): int|float { /** * Get a list of reserved file names that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. * @return string[] * @since 30.0.0 */ @@ -539,7 +540,7 @@ public static function getForbiddenFilenames(): array { \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.'); $invalidFilenames = ['.htaccess']; } - self::$invalidFilenames = $invalidFilenames; + self::$invalidFilenames = array_map('mb_strtolower', $invalidFilenames); } return self::$invalidFilenames; } diff --git a/tests/lib/Files/FilesystemTest.php b/tests/lib/Files/FilesystemTest.php index 5662a550773c8..5b2c68df48680 100644 --- a/tests/lib/Files/FilesystemTest.php +++ b/tests/lib/Files/FilesystemTest.php @@ -284,7 +284,7 @@ public function isFileBlacklistedData() { ['/etc/foo\bar/.htaccess/', true], ['/etc/foo\bar/.htaccess/foo', false], ['//foo//bar/\.htaccess/', true], - ['\foo\bar\.htaccess', true], + ['\foo\bar\.HTAccess', true], ]; } From 30a981bc5c4d51c810bd8cb6f66663ffefca5a70 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 2 May 2024 20:32:58 +0200 Subject: [PATCH 7/7] feat: Allow to enforce Windows compatible file and folder names This will: * Deny characters forbidden on Windows * Deny files with names which are reserved on Windows * Deny trailing dot or space * Deny files or folders which are not case-insensitive unique in a folder Signed-off-by: Ferdinand Thiessen --- config/config.sample.php | 15 + lib/private/Files/Storage/Common.php | 29 +- lib/private/Files/View.php | 565 +++++++++++++-------------- lib/public/Util.php | 25 ++ 4 files changed, 348 insertions(+), 286 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index ab582ba79df6c..b2a5cd15aa7c4 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1960,6 +1960,21 @@ */ 'updatedirectory' => '', +/** + * Allow to enforce Windows compatible file and folder names. + * Nextcloud by default supports all files valid on Linux, + * but as Windows has some stricter filename rules this can lead to sync errors when using Windows clients. + * + * To enforce only Windows compatible filenames, also on the webui, set this value to ``true``. + * + * This will deny filenames with characters not valid on Windows, as well as some reserved filenames and nameing rules (no trailing dot or space). + * Additionally this will enforce files to be case in-sensitivly unique in a folder. + * + * Defaults to ``false`` + * + */ +'enforce_windows_compatibility' => false, + /** * Deny a specific file or files and disallow the upload of files * with this name. ``.htaccess`` is blocked by default. diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index d2962008e404c..818a08ac5ecff 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -63,6 +63,7 @@ use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; use OCP\Files\StorageNotAvailableException; +use OCP\IConfig; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Server; @@ -375,7 +376,7 @@ public function getWatcher($path = '', $storage = null) { } if (!isset($this->watcher)) { $this->watcher = new Watcher($storage); - $globalPolicy = \OC::$server->getConfig()->getSystemValue('filesystem_check_changes', Watcher::CHECK_NEVER); + $globalPolicy = \OCP\Server::get(IConfig::class)->getSystemValue('filesystem_check_changes', Watcher::CHECK_NEVER); $this->watcher->setPolicy((int)$this->getMountOption('filesystem_check_changes', $globalPolicy)); } return $this->watcher; @@ -565,6 +566,32 @@ public function verifyPath($path, $fileName) { if (\OC\Files\Filesystem::hasFilenameInvalidCharacters($fileName)) { throw new InvalidCharacterInPathException(); } + + $config = \OCP\Server::get(IConfig::class); + if ($config->getSystemValueBool('enforce_windows_compatibility', false)) { + // Windows does not allow filenames to end with a trailing dot or space + if (str_ends_with($fileName, '.') || str_ends_with($fileName, ' ')) { + throw new InvalidCharacterInPathException('Filenames must not end with a dot or space'); + } + + // Windows has path namespaces so e.g. `NUL` is a reserved word, + // but `NUL.txt` or `NUL.tar.gz` is considered the same and thus also reserved. + $basename = substr($fileName, 0, strpos($fileName, '.') ?: null); + if (\OC\Files\Filesystem::isFileBlacklisted($basename)) { + throw new ReservedWordException(); + } + + // Some windows systems are case insensitive, + // so to guarantee files can be synced we need to enfore case insensitivity + $content = $this->getDirectoryContent(dirname($path)); + $fileName = strtolower($fileName); + foreach ($content as $subPath) { + if (strtolower($subPath['name']) === $fileName) { + throw new InvalidPathException('Filename is not case insensitivly unique'); + } + } + } + // NOTE: $path will remain unverified for now } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 98b597dbd4ded..b1089d6acf443 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -621,65 +621,61 @@ protected function emit_file_hooks_post(bool $exists, string $path): void { * @param string|resource $data * @return bool|mixed * @throws LockedException + * @throws InvalidPathException When path is not within the expected root */ public function file_put_contents($path, $data) { - if (is_resource($data)) { //not having to deal with streams in file_put_contents makes life easier + if (!is_resource($data)) { + //not having to deal with streams in file_put_contents makes life easier + $hooks = $this->file_exists($path) ? ['update', 'write'] : ['create', 'write']; + return $this->basicOperation('file_put_contents', $path, $hooks, $data); + } else { $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); - if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) - ) { - $path = $this->getRelativePath($absolutePath); - if ($path === null) { - throw new InvalidPathException("Path $absolutePath is not in the expected root"); - } + $path = $this->getRelativePath($absolutePath); + if ($path === null) { + throw new InvalidPathException("Path $absolutePath is not in the expected root"); + } + $this->verifyPath(dirname($path), basename($path)); - $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $this->lockFile($path, ILockingProvider::LOCK_SHARED); - $exists = $this->file_exists($path); - $run = true; - if ($this->shouldEmitHooks($path)) { - $this->emit_file_hooks_pre($exists, $path, $run); - } - if (!$run) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return false; - } + $exists = $this->file_exists($path); + $run = true; + if ($this->shouldEmitHooks($path)) { + $this->emit_file_hooks_pre($exists, $path, $run); + } + if (!$run) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + return false; + } - try { - $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); - } catch (\Exception $e) { - // Release the shared lock before throwing. - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - throw $e; - } + try { + $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); + } catch (\Exception $e) { + // Release the shared lock before throwing. + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + throw $e; + } - /** @var Storage $storage */ - [$storage, $internalPath] = $this->resolvePath($path); - $target = $storage->fopen($internalPath, 'w'); - if ($target) { - [, $result] = \OC_Helper::streamCopy($data, $target); - fclose($target); - fclose($data); + /** @var Storage $storage */ + [$storage, $internalPath] = $this->resolvePath($path); + $target = $storage->fopen($internalPath, 'w'); + if ($target) { + [, $result] = \OC_Helper::streamCopy($data, $target); + fclose($target); + fclose($data); - $this->writeUpdate($storage, $internalPath); + $this->writeUpdate($storage, $internalPath); - $this->changeLock($path, ILockingProvider::LOCK_SHARED); + $this->changeLock($path, ILockingProvider::LOCK_SHARED); - if ($this->shouldEmitHooks($path) && $result !== false) { - $this->emit_file_hooks_post($exists, $path); - } - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return $result; - } else { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - return false; + if ($this->shouldEmitHooks($path) && $result !== false) { + $this->emit_file_hooks_post($exists, $path); } + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + return $result; } else { return false; } - } else { - $hooks = $this->file_exists($path) ? ['update', 'write'] : ['create', 'write']; - return $this->basicOperation('file_put_contents', $path, $hooks, $data); } } @@ -735,127 +731,125 @@ public function rename($source, $target) { $absolutePath2 = Filesystem::normalizePath($this->getAbsolutePath($target)); $targetParts = explode('/', $absolutePath2); $targetUser = $targetParts[1] ?? null; - $result = false; - if ( - Filesystem::isValidPath($target) - && Filesystem::isValidPath($source) - && !Filesystem::isFileBlacklisted($target) - ) { - $source = $this->getRelativePath($absolutePath1); - $target = $this->getRelativePath($absolutePath2); - $exists = $this->file_exists($target); - if ($source == null || $target == null) { - return false; - } + if (!Filesystem::isValidPath($source) || !Filesystem::isValidPath($target)) { + return false; + } - $this->lockFile($source, ILockingProvider::LOCK_SHARED, true); - try { - $this->lockFile($target, ILockingProvider::LOCK_SHARED, true); - - $run = true; - if ($this->shouldEmitHooks($source) && (Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target))) { - // if it was a rename from a part file to a regular file it was a write and not a rename operation - $this->emit_file_hooks_pre($exists, $target, $run); - } elseif ($this->shouldEmitHooks($source)) { - $sourcePath = $this->getHookPath($source); - $targetPath = $this->getHookPath($target); - if ($sourcePath !== null && $targetPath !== null) { - \OC_Hook::emit( - Filesystem::CLASSNAME, Filesystem::signal_rename, - [ - Filesystem::signal_param_oldpath => $sourcePath, - Filesystem::signal_param_newpath => $targetPath, - Filesystem::signal_param_run => &$run - ] - ); - } + $source = $this->getRelativePath($absolutePath1); + $target = $this->getRelativePath($absolutePath2); + $exists = $this->file_exists($target); + if ($source == null || $target == null) { + return false; + } + + $this->verifyPath(dirname($target), basename($target)); + + $result = false; + $this->lockFile($source, ILockingProvider::LOCK_SHARED, true); + try { + $this->lockFile($target, ILockingProvider::LOCK_SHARED, true); + + $run = true; + if ($this->shouldEmitHooks($source) && (Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target))) { + // if it was a rename from a part file to a regular file it was a write and not a rename operation + $this->emit_file_hooks_pre($exists, $target, $run); + } elseif ($this->shouldEmitHooks($source)) { + $sourcePath = $this->getHookPath($source); + $targetPath = $this->getHookPath($target); + if ($sourcePath !== null && $targetPath !== null) { + \OC_Hook::emit( + Filesystem::CLASSNAME, Filesystem::signal_rename, + [ + Filesystem::signal_param_oldpath => $sourcePath, + Filesystem::signal_param_newpath => $targetPath, + Filesystem::signal_param_run => &$run + ] + ); } - if ($run) { - $this->verifyPath(dirname($target), basename($target)); - - $manager = Filesystem::getMountManager(); - $mount1 = $this->getMount($source); - $mount2 = $this->getMount($target); - $storage1 = $mount1->getStorage(); - $storage2 = $mount2->getStorage(); - $internalPath1 = $mount1->getInternalPath($absolutePath1); - $internalPath2 = $mount2->getInternalPath($absolutePath2); - - $this->changeLock($source, ILockingProvider::LOCK_EXCLUSIVE, true); - try { - $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true); - - if ($internalPath1 === '') { - if ($mount1 instanceof MoveableMount) { - $sourceParentMount = $this->getMount(dirname($source)); - if ($sourceParentMount === $mount2 && $this->targetIsNotShared($targetUser, $absolutePath2)) { - /** - * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 - */ - $sourceMountPoint = $mount1->getMountPoint(); - $result = $mount1->moveMount($absolutePath2); - $manager->moveMount($sourceMountPoint, $mount1->getMountPoint()); - } else { - $result = false; - } - } else { - $result = false; - } - // moving a file/folder within the same mount point - } elseif ($storage1 === $storage2) { - if ($storage1) { - $result = $storage1->rename($internalPath1, $internalPath2); + } + if ($run) { + $manager = Filesystem::getMountManager(); + $mount1 = $this->getMount($source); + $mount2 = $this->getMount($target); + $storage1 = $mount1->getStorage(); + $storage2 = $mount2->getStorage(); + $internalPath1 = $mount1->getInternalPath($absolutePath1); + $internalPath2 = $mount2->getInternalPath($absolutePath2); + + $this->changeLock($source, ILockingProvider::LOCK_EXCLUSIVE, true); + try { + $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true); + + if ($internalPath1 === '') { + if ($mount1 instanceof MoveableMount) { + $sourceParentMount = $this->getMount(dirname($source)); + if ($sourceParentMount === $mount2 && $this->targetIsNotShared($targetUser, $absolutePath2)) { + /** + * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 + */ + $sourceMountPoint = $mount1->getMountPoint(); + $result = $mount1->moveMount($absolutePath2); + $manager->moveMount($sourceMountPoint, $mount1->getMountPoint()); } else { $result = false; } - // moving a file/folder between storages (from $storage1 to $storage2) } else { - $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); + $result = false; } - - if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) { - // if it was a rename from a part file to a regular file it was a write and not a rename operation - $this->writeUpdate($storage2, $internalPath2); - } elseif ($result) { - if ($internalPath1 !== '') { // don't do a cache update for moved mounts - $this->renameUpdate($storage1, $storage2, $internalPath1, $internalPath2); - } + // moving a file/folder within the same mount point + } elseif ($storage1 === $storage2) { + if ($storage1) { + $result = $storage1->rename($internalPath1, $internalPath2); + } else { + $result = false; } - } catch (\Exception $e) { - throw $e; - } finally { - $this->changeLock($source, ILockingProvider::LOCK_SHARED, true); - $this->changeLock($target, ILockingProvider::LOCK_SHARED, true); + // moving a file/folder between storages (from $storage1 to $storage2) + } else { + $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) { - if ($this->shouldEmitHooks()) { - $this->emit_file_hooks_post($exists, $target); - } + // if it was a rename from a part file to a regular file it was a write and not a rename operation + $this->writeUpdate($storage2, $internalPath2); } elseif ($result) { - if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) { - $sourcePath = $this->getHookPath($source); - $targetPath = $this->getHookPath($target); - if ($sourcePath !== null && $targetPath !== null) { - \OC_Hook::emit( - Filesystem::CLASSNAME, - Filesystem::signal_post_rename, - [ - Filesystem::signal_param_oldpath => $sourcePath, - Filesystem::signal_param_newpath => $targetPath, - ] - ); - } + if ($internalPath1 !== '') { // don't do a cache update for moved mounts + $this->renameUpdate($storage1, $storage2, $internalPath1, $internalPath2); + } + } + } catch (\Exception $e) { + throw $e; + } finally { + $this->changeLock($source, ILockingProvider::LOCK_SHARED, true); + $this->changeLock($target, ILockingProvider::LOCK_SHARED, true); + } + + if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) { + if ($this->shouldEmitHooks()) { + $this->emit_file_hooks_post($exists, $target); + } + } elseif ($result) { + if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) { + $sourcePath = $this->getHookPath($source); + $targetPath = $this->getHookPath($target); + if ($sourcePath !== null && $targetPath !== null) { + \OC_Hook::emit( + Filesystem::CLASSNAME, + Filesystem::signal_post_rename, + [ + Filesystem::signal_param_oldpath => $sourcePath, + Filesystem::signal_param_newpath => $targetPath, + ] + ); } } } - } catch (\Exception $e) { - throw $e; - } finally { - $this->unlockFile($source, ILockingProvider::LOCK_SHARED, true); - $this->unlockFile($target, ILockingProvider::LOCK_SHARED, true); } + } catch (\Exception $e) { + throw $e; + } finally { + $this->unlockFile($source, ILockingProvider::LOCK_SHARED, true); + $this->unlockFile($target, ILockingProvider::LOCK_SHARED, true); } return $result; } @@ -872,83 +866,81 @@ public function rename($source, $target) { public function copy($source, $target, $preserveMtime = false) { $absolutePath1 = Filesystem::normalizePath($this->getAbsolutePath($source)); $absolutePath2 = Filesystem::normalizePath($this->getAbsolutePath($target)); + + if (!Filesystem::isValidPath($source) || !Filesystem::isValidPath($target)) { + return false; + } + + $source = $this->getRelativePath($absolutePath1); + $target = $this->getRelativePath($absolutePath2); + if ($source == null || $target == null) { + return false; + } + + $this->verifyPath(dirname($target), basename($target)); + + $this->lockFile($target, ILockingProvider::LOCK_SHARED); + $this->lockFile($source, ILockingProvider::LOCK_SHARED); + $lockTypePath1 = ILockingProvider::LOCK_SHARED; + $lockTypePath2 = ILockingProvider::LOCK_SHARED; + $result = false; - if ( - Filesystem::isValidPath($target) - && Filesystem::isValidPath($source) - && !Filesystem::isFileBlacklisted($target) - ) { - $source = $this->getRelativePath($absolutePath1); - $target = $this->getRelativePath($absolutePath2); - - if ($source == null || $target == null) { - return false; - } + try { $run = true; + $exists = $this->file_exists($target); + if ($this->shouldEmitHooks()) { + \OC_Hook::emit( + Filesystem::CLASSNAME, + Filesystem::signal_copy, + [ + Filesystem::signal_param_oldpath => $this->getHookPath($source), + Filesystem::signal_param_newpath => $this->getHookPath($target), + Filesystem::signal_param_run => &$run + ] + ); + $this->emit_file_hooks_pre($exists, $target, $run); + } + if ($run) { + $mount1 = $this->getMount($source); + $mount2 = $this->getMount($target); + $storage1 = $mount1->getStorage(); + $internalPath1 = $mount1->getInternalPath($absolutePath1); + $storage2 = $mount2->getStorage(); + $internalPath2 = $mount2->getInternalPath($absolutePath2); + + $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE); + $lockTypePath2 = ILockingProvider::LOCK_EXCLUSIVE; + + if ($mount1->getMountPoint() == $mount2->getMountPoint()) { + if ($storage1) { + $result = $storage1->copy($internalPath1, $internalPath2); + } else { + $result = false; + } + } else { + $result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2); + } - $this->lockFile($target, ILockingProvider::LOCK_SHARED); - $this->lockFile($source, ILockingProvider::LOCK_SHARED); - $lockTypePath1 = ILockingProvider::LOCK_SHARED; - $lockTypePath2 = ILockingProvider::LOCK_SHARED; + $this->writeUpdate($storage2, $internalPath2); - try { - $exists = $this->file_exists($target); - if ($this->shouldEmitHooks()) { + $this->changeLock($target, ILockingProvider::LOCK_SHARED); + $lockTypePath2 = ILockingProvider::LOCK_SHARED; + + if ($this->shouldEmitHooks() && $result !== false) { \OC_Hook::emit( Filesystem::CLASSNAME, - Filesystem::signal_copy, + Filesystem::signal_post_copy, [ Filesystem::signal_param_oldpath => $this->getHookPath($source), - Filesystem::signal_param_newpath => $this->getHookPath($target), - Filesystem::signal_param_run => &$run + Filesystem::signal_param_newpath => $this->getHookPath($target) ] ); - $this->emit_file_hooks_pre($exists, $target, $run); - } - if ($run) { - $mount1 = $this->getMount($source); - $mount2 = $this->getMount($target); - $storage1 = $mount1->getStorage(); - $internalPath1 = $mount1->getInternalPath($absolutePath1); - $storage2 = $mount2->getStorage(); - $internalPath2 = $mount2->getInternalPath($absolutePath2); - - $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE); - $lockTypePath2 = ILockingProvider::LOCK_EXCLUSIVE; - - if ($mount1->getMountPoint() == $mount2->getMountPoint()) { - if ($storage1) { - $result = $storage1->copy($internalPath1, $internalPath2); - } else { - $result = false; - } - } else { - $result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2); - } - - $this->writeUpdate($storage2, $internalPath2); - - $this->changeLock($target, ILockingProvider::LOCK_SHARED); - $lockTypePath2 = ILockingProvider::LOCK_SHARED; - - if ($this->shouldEmitHooks() && $result !== false) { - \OC_Hook::emit( - Filesystem::CLASSNAME, - Filesystem::signal_post_copy, - [ - Filesystem::signal_param_oldpath => $this->getHookPath($source), - Filesystem::signal_param_newpath => $this->getHookPath($target) - ] - ); - $this->emit_file_hooks_post($exists, $target); - } + $this->emit_file_hooks_post($exists, $target); } - } catch (\Exception $e) { - $this->unlockFile($target, $lockTypePath2); - $this->unlockFile($source, $lockTypePath1); - throw $e; } - + } catch (\Exception $e) { + throw $e; + } finally { $this->unlockFile($target, $lockTypePath2); $this->unlockFile($source, $lockTypePath1); } @@ -1132,92 +1124,95 @@ public function free_space($path = '/') { private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null) { $postFix = (substr($path, -1) === '/') ? '/' : ''; $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); - if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) - ) { - $path = $this->getRelativePath($absolutePath); - if ($path == null) { - return false; - } - if (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) { - // always a shared lock during pre-hooks so the hook can read the file - $this->lockFile($path, ILockingProvider::LOCK_SHARED); - } + if (!Filesystem::isValidPath($path)) { + return false; + } - $run = $this->runHooks($hooks, $path); - [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); - if ($run && $storage) { - /** @var Storage $storage */ - if (in_array('write', $hooks) || in_array('delete', $hooks)) { - try { - $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); - } catch (LockedException $e) { - // release the shared lock we acquired before quitting - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - throw $e; - } - } + $path = $this->getRelativePath($absolutePath); + if ($path == null) { + return false; + } + + $this->verifyPath(dirname($path), basename($path)); + + if (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) { + // always a shared lock during pre-hooks so the hook can read the file + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + } + + $run = $this->runHooks($hooks, $path); + [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); + if ($run && $storage) { + /** @var Storage $storage */ + if (in_array('write', $hooks) || in_array('delete', $hooks)) { try { - if (!is_null($extraParam)) { - $result = $storage->$operation($internalPath, $extraParam); - } else { - $result = $storage->$operation($internalPath); - } - } catch (\Exception $e) { - if (in_array('write', $hooks) || in_array('delete', $hooks)) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif (in_array('read', $hooks)) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } + $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); + } catch (LockedException $e) { + // release the shared lock we acquired before quitting + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); throw $e; } - - if ($result !== false && in_array('delete', $hooks)) { - $this->removeUpdate($storage, $internalPath); - } - if ($result !== false && in_array('write', $hooks, true) && $operation !== 'fopen' && $operation !== 'touch') { - $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && in_array('create', $hooks, true)); - $sizeDifference = $operation === 'mkdir' ? 0 : $result; - $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); + } + try { + if (!is_null($extraParam)) { + $result = $storage->$operation($internalPath, $extraParam); + } else { + $result = $storage->$operation($internalPath); } - if ($result !== false && in_array('touch', $hooks)) { - $this->writeUpdate($storage, $internalPath, $extraParam); + } catch (\Exception $e) { + if (in_array('write', $hooks) || in_array('delete', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } elseif (in_array('read', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } + throw $e; + } - if ((in_array('write', $hooks) || in_array('delete', $hooks)) && ($operation !== 'fopen' || $result === false)) { - $this->changeLock($path, ILockingProvider::LOCK_SHARED); - } + if ($result !== false && in_array('delete', $hooks)) { + $this->removeUpdate($storage, $internalPath); + } + if ($result !== false && in_array('write', $hooks, true) && $operation !== 'fopen' && $operation !== 'touch') { + $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && in_array('create', $hooks, true)); + $sizeDifference = $operation === 'mkdir' ? 0 : $result; + $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); + } + if ($result !== false && in_array('touch', $hooks)) { + $this->writeUpdate($storage, $internalPath, $extraParam); + } - $unlockLater = false; - if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { - $unlockLater = true; - // make sure our unlocking callback will still be called if connection is aborted - ignore_user_abort(true); - $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path) { - if (in_array('write', $hooks)) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif (in_array('read', $hooks)) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } - }); - } + if ((in_array('write', $hooks) || in_array('delete', $hooks)) && ($operation !== 'fopen' || $result === false)) { + $this->changeLock($path, ILockingProvider::LOCK_SHARED); + } - if ($this->shouldEmitHooks($path) && $result !== false) { - if ($operation != 'fopen') { //no post hooks for fopen, the file stream is still open - $this->runHooks($hooks, $path, true); + $unlockLater = false; + if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { + $unlockLater = true; + // make sure our unlocking callback will still be called if connection is aborted + ignore_user_abort(true); + $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path) { + if (in_array('write', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } elseif (in_array('read', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } - } + }); + } - if (!$unlockLater - && (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) - ) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + if ($this->shouldEmitHooks($path) && $result !== false) { + if ($operation != 'fopen') { //no post hooks for fopen, the file stream is still open + $this->runHooks($hooks, $path, true); } - return $result; - } else { + } + + if (!$unlockLater + && (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) + ) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } + return $result; + } else { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } return null; } diff --git a/lib/public/Util.php b/lib/public/Util.php index 6e172e68d71a1..bdd2dd25f01f1 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -540,6 +540,20 @@ public static function getForbiddenFilenames(): array { \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.'); $invalidFilenames = ['.htaccess']; } + + if ($config->getSystemValueBool('enforce_windows_compatibility', false)) { + $invalidFilenames = array_merge( + $invalidFilenames, + [ + "CON", "PRN", "AUX", "NUL", "COM0", + "COM1", "COM2", "COM3", "COM4", "COM5", + "COM6", "COM7", "COM8", "COM9", "COMĀ¹", + "COMĀ²", "COMĀ³", "LPT0", "LPT1", "LPT2", + "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", + "LPT8", "LPT9", "LPTĀ¹", "LPTĀ²", "LPTĀ³", + ], + ); + } self::$invalidFilenames = array_map('mb_strtolower', $invalidFilenames); } return self::$invalidFilenames; @@ -564,6 +578,17 @@ public static function getForbiddenFileNameChars(): array { $invalidChars = []; } + // If windows compatibility is enabled we also forbidd reserved win32 API characters + if ($config->getSystemValueBool('enforce_windows_compatibility', false)) { + $invalidChars = array_merge( + $invalidChars, + // reserved characters of the win32 API + ['<', '>', ':', '"', '|', '?', '*'], + // character 0-31 are also forbiden on windows but already filtered + // see IStorage::verifyPath + ); + } + // Get admin defined invalid characters $additionalChars = $config->getSystemValue('forbidden_chars', []); if (!is_array($additionalChars)) {