From 5d3a0aecc373ea9169f2017fd3f8a3c7042c71fc Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 3 Jul 2024 16:33:40 +0200 Subject: [PATCH] feat: Add `IFilenameValidator` to have one consistent place for filename validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ferdinand Thiessen Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> Signed-off-by: Ferdinand Thiessen --- config/config.sample.php | 24 +- lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + lib/private/Files/FilenameValidator.php | 249 ++++++++++++++++++++ lib/private/Server.php | 2 + lib/public/Files/IFilenameValidator.php | 39 +++ tests/lib/Files/FilenameValidatorTest.php | 188 +++++++++++++++ 7 files changed, 500 insertions(+), 6 deletions(-) create mode 100644 lib/private/Files/FilenameValidator.php create mode 100644 lib/public/Files/IFilenameValidator.php create mode 100644 tests/lib/Files/FilenameValidatorTest.php diff --git a/config/config.sample.php b/config/config.sample.php index 77fd2490a1498..76b6532a19c46 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1976,26 +1976,38 @@ 'updatedirectory' => '', /** - * Blacklist a specific file or files and disallow the upload of files + * Block 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. * + * Note that this list is case-insensitive. + * * Defaults to ``array('.htaccess')`` */ -'blacklisted_files' => ['.htaccess'], +'forbidden_filenames' => ['.htaccess'], /** - * Blacklist characters from being used in filenames. This is useful if you + * Block 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()`` */ -'forbidden_chars' => [], +'forbidden_filename_characters' => [], + +/** + * Deny extensions from being used for filenames. + * + * The '.part' extension is always forbidden, as this is used internally by Nextcloud. + * + * Defaults to ``array('.filepart', '.part')`` + */ +'forbidden_filename_extensions' => ['.part', '.filepart'], /** * If you are applying a theme to Nextcloud, enter the name of the theme here. diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index f0addbcdaa8a1..9b6fd9c7e56cc 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -379,6 +379,7 @@ 'OCP\\Files\\ForbiddenException' => $baseDir . '/lib/public/Files/ForbiddenException.php', 'OCP\\Files\\GenericFileException' => $baseDir . '/lib/public/Files/GenericFileException.php', 'OCP\\Files\\IAppData' => $baseDir . '/lib/public/Files/IAppData.php', + 'OCP\\Files\\IFilenameValidator' => $baseDir . '/lib/public/Files/IFilenameValidator.php', 'OCP\\Files\\IHomeStorage' => $baseDir . '/lib/public/Files/IHomeStorage.php', 'OCP\\Files\\IMimeTypeDetector' => $baseDir . '/lib/public/Files/IMimeTypeDetector.php', 'OCP\\Files\\IMimeTypeLoader' => $baseDir . '/lib/public/Files/IMimeTypeLoader.php', @@ -1444,6 +1445,7 @@ 'OC\\Files\\Config\\UserMountCache' => $baseDir . '/lib/private/Files/Config/UserMountCache.php', 'OC\\Files\\Config\\UserMountCacheListener' => $baseDir . '/lib/private/Files/Config/UserMountCacheListener.php', 'OC\\Files\\FileInfo' => $baseDir . '/lib/private/Files/FileInfo.php', + 'OC\\Files\\FilenameValidator' => $baseDir . '/lib/private/Files/FilenameValidator.php', 'OC\\Files\\Filesystem' => $baseDir . '/lib/private/Files/Filesystem.php', 'OC\\Files\\Lock\\LockManager' => $baseDir . '/lib/private/Files/Lock/LockManager.php', 'OC\\Files\\Mount\\CacheMountProvider' => $baseDir . '/lib/private/Files/Mount/CacheMountProvider.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 51044d28a46eb..0f387cb6980a3 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -412,6 +412,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Files\\ForbiddenException' => __DIR__ . '/../../..' . '/lib/public/Files/ForbiddenException.php', 'OCP\\Files\\GenericFileException' => __DIR__ . '/../../..' . '/lib/public/Files/GenericFileException.php', 'OCP\\Files\\IAppData' => __DIR__ . '/../../..' . '/lib/public/Files/IAppData.php', + 'OCP\\Files\\IFilenameValidator' => __DIR__ . '/../../..' . '/lib/public/Files/IFilenameValidator.php', 'OCP\\Files\\IHomeStorage' => __DIR__ . '/../../..' . '/lib/public/Files/IHomeStorage.php', 'OCP\\Files\\IMimeTypeDetector' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeDetector.php', 'OCP\\Files\\IMimeTypeLoader' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeLoader.php', @@ -1477,6 +1478,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Files\\Config\\UserMountCache' => __DIR__ . '/../../..' . '/lib/private/Files/Config/UserMountCache.php', 'OC\\Files\\Config\\UserMountCacheListener' => __DIR__ . '/../../..' . '/lib/private/Files/Config/UserMountCacheListener.php', 'OC\\Files\\FileInfo' => __DIR__ . '/../../..' . '/lib/private/Files/FileInfo.php', + 'OC\\Files\\FilenameValidator' => __DIR__ . '/../../..' . '/lib/private/Files/FilenameValidator.php', 'OC\\Files\\Filesystem' => __DIR__ . '/../../..' . '/lib/private/Files/Filesystem.php', 'OC\\Files\\Lock\\LockManager' => __DIR__ . '/../../..' . '/lib/private/Files/Lock/LockManager.php', 'OC\\Files\\Mount\\CacheMountProvider' => __DIR__ . '/../../..' . '/lib/private/Files/Mount/CacheMountProvider.php', diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php new file mode 100644 index 0000000000000..170453141fd3a --- /dev/null +++ b/lib/private/Files/FilenameValidator.php @@ -0,0 +1,249 @@ + + */ + private array $forbiddenNames = []; + + /** + * @var list + */ + private array $forbiddenCharacters = []; + + /** + * @var list + */ + private array $forbiddenExtensions = []; + + public function __construct( + IFactory $l10nFactory, + private IConfig $config, + private LoggerInterface $logger, + ) { + $this->l10n = $l10nFactory->get('core'); + } + + /** + * Get a list of reserved filenames that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. + * @return list + * @since 30.0.0 + */ + public function getForbiddenExtensions(): array { + if (empty($this->forbiddenExtensions)) { + $forbiddenExtensions = $this->config->getSystemValue('forbidden_filename_extensions', ['.filepart']); + if (!is_array($forbiddenExtensions)) { + $this->logger->error('Invalid system config value for "forbidden_filename_extensions" is ignored.'); + $forbiddenExtensions = ['.filepart']; + } + + // Always forbid .part files as they are used internally + $forbiddenExtensions = array_merge($forbiddenExtensions, ['.part']); + + // The list is case insensitive so we provide it always lowercase + $forbiddenExtensions = array_map('mb_strtolower', $forbiddenExtensions); + $this->forbiddenExtensions = array_values($forbiddenExtensions); + } + return $this->forbiddenExtensions; + } + + /** + * Get a list of forbidden filename extensions that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. + * @return list + * @since 30.0.0 + */ + public function getForbiddenFilenames(): array { + if (empty($this->forbiddenNames)) { + $forbiddenNames = $this->config->getSystemValue('forbidden_filenames', ['.htaccess']); + if (!is_array($forbiddenNames)) { + $this->logger->error('Invalid system config value for "forbidden_filenames" is ignored.'); + $forbiddenNames = ['.htaccess']; + } + + // Handle legacy config option + // TODO: Drop with Nextcloud 34 + $legacyForbiddenNames = $this->config->getSystemValue('blacklisted_files', []); + if (!is_array($legacyForbiddenNames)) { + $this->logger->error('Invalid system config value for "blacklisted_files" is ignored.'); + $legacyForbiddenNames = []; + } + if (!empty($legacyForbiddenNames)) { + $this->logger->warning('System config option "blacklisted_files" is deprecated and will be removed in Nextcloud 34, use "forbidden_filenames" instead.'); + } + $forbiddenNames = array_merge($legacyForbiddenNames, $forbiddenNames); + + // The list is case insensitive so we provide it always lowercase + $forbiddenNames = array_map('mb_strtolower', $forbiddenNames); + $this->forbiddenNames = array_values($forbiddenNames); + } + return $this->forbiddenNames; + } + + /** + * Get a list of characters forbidden in filenames + * + * Note: Characters in the range [0-31] are always forbidden, + * even if not inside this list (see OCP\Files\Storage\IStorage::verifyPath). + * + * @return list + * @since 30.0.0 + */ + public function getForbiddenCharacters(): array { + if (empty($this->forbiddenCharacters)) { + // Get always forbidden characters + $forbiddenCharacters = str_split(\OCP\Constants::FILENAME_INVALID_CHARS); + if ($forbiddenCharacters === false) { + $forbiddenCharacters = []; + } + + // Get admin defined invalid characters + $additionalChars = $this->config->getSystemValue('forbidden_filename_characters', []); + if (!is_array($additionalChars)) { + $this->logger->error('Invalid system config value for "forbidden_chars" is ignored.'); + $additionalChars = []; + } + $forbiddenCharacters = array_merge($forbiddenCharacters, $additionalChars); + + // Handle legacy config option + // TODO: Drop with Nextcloud 34 + $legacyForbiddenCharacters = $this->config->getSystemValue('forbidden_chars', []); + if (!is_array($legacyForbiddenCharacters)) { + $this->logger->error('Invalid system config value for "forbidden_chars" is ignored.'); + $legacyForbiddenCharacters = []; + } + if (!empty($legacyForbiddenCharacters)) { + $this->logger->warning('System config option "forbidden_chars" is deprecated and will be removed in Nextcloud 34, use "forbidden_filename_characters" instead.'); + } + $forbiddenCharacters = array_merge($legacyForbiddenCharacters, $forbiddenCharacters); + + $this->forbiddenCharacters = array_values($forbiddenCharacters); + } + return $this->forbiddenCharacters; + } + + /** + * @inheritdoc + */ + public function isFilenameValid(string $filename): bool { + try { + $this->validateFilename($filename); + } catch (\OCP\Files\InvalidPathException) { + return false; + } + return true; + } + + /** + * @inheritdoc + */ + public function validateFilename(string $filename): void { + $trimmed = trim($filename); + if ($trimmed === '') { + throw new EmptyFileNameException(); + } + + // the special directories . and .. would cause never ending recursion + if ($trimmed === '.' || $trimmed === '..') { + throw new ReservedWordException(); + } + + // 255 characters is the limit on common file systems (ext/xfs) + // oc_filecache has a 250 char length limit for the filename + if (isset($filename[250])) { + throw new FileNameTooLongException(); + } + + if ($this->isForbidden($filename)) { + throw new ReservedWordException(); + } + + $this->checkForbiddenExtension($filename); + + $this->checkForbiddenCharacters($filename); + } + + /** + * Check if the filename is forbidden + * @param string $path Path to check the filename + * @return bool True if invalid name, False otherwise + */ + public function isForbidden(string $path): bool { + $filename = basename($path); + $filename = mb_strtolower($filename); + + if ($filename === '') { + return false; + } + + // The name part without extension + $basename = substr($filename, 0, strpos($filename, '.', 1) ?: null); + // Check for forbidden filenames + $forbiddenNames = $this->getForbiddenFilenames(); + if (in_array($basename, $forbiddenNames)) { + return true; + } + + // Filename is not forbidden + return false; + } + + /** + * Check if a filename contains any of the forbidden characters + * @param string $filename + * @throws InvalidCharacterInPathException + */ + protected function checkForbiddenCharacters(string $filename): void { + $sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW); + if ($sanitizedFileName !== $filename) { + throw new InvalidCharacterInPathException(); + } + + foreach ($this->getForbiddenCharacters() as $char) { + if (str_contains($filename, $char)) { + throw new InvalidCharacterInPathException($char); + } + } + } + + /** + * Check if a filename has a forbidden filename extension + * @param string $filename The filename to validate + * @throws InvalidPathException + */ + protected function checkForbiddenExtension(string $filename): void { + $filename = mb_strtolower($filename); + // Check for forbidden filename extengetForbiddenExtensions(); + foreach ($forbiddenExtensions as $extension) { + if (str_ends_with($filename, $extension)) { + throw new InvalidPathException($this->l10n->t('Invalid filename extension "%1$s"', [$extension])); + } + } + } +}; diff --git a/lib/private/Server.php b/lib/private/Server.php index bcdf482f02d0e..795d72e307629 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1371,6 +1371,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(\OCP\Files\AppData\IAppDataFactory::class, \OC\Files\AppData\Factory::class); + $this->registerAlias(\OCP\Files\IFilenameValidator::class, \OC\Files\FilenameValidator::class); + $this->registerAlias(IBinaryFinder::class, BinaryFinder::class); $this->registerAlias(\OCP\Share\IPublicShareTemplateFactory::class, \OC\Share20\PublicShareTemplateFactory::class); diff --git a/lib/public/Files/IFilenameValidator.php b/lib/public/Files/IFilenameValidator.php new file mode 100644 index 0000000000000..2bd3bb945dcab --- /dev/null +++ b/lib/public/Files/IFilenameValidator.php @@ -0,0 +1,39 @@ +createMock(IL10N::class); + $l10n->method('t') + ->willReturnCallback(fn ($string, $params) => sprintf($string, ...$params)); + $this->l10n = $this->createMock(IFactory::class); + $this->l10n + ->method('get') + ->with('core') + ->willReturn($l10n); + + $this->config = $this->createMock(IConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + } + + /** + * @dataProvider dataValidateFilename + */ + public function testValidateFilename( + string $filename, + array $forbiddenNames, + array $forbiddenExtensions, + array $forbiddenCharacters, + ?string $exception, + ): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenCharacters') + ->willReturn($forbiddenCharacters); + $validator->method('getForbiddenExtensions') + ->willReturn($forbiddenExtensions); + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + if ($exception !== null) { + $this->expectException($exception); + } else { + $this->expectNotToPerformAssertions(); + } + $validator->validateFilename($filename); + } + + /** + * @dataProvider dataValidateFilename + */ + public function testIsFilenameValid( + string $filename, + array $forbiddenNames, + array $forbiddenExtensions, + array $forbiddenCharacters, + ?string $exception, + ): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenCharacters') + ->willReturn($forbiddenCharacters); + $validator->method('getForbiddenExtensions') + ->willReturn($forbiddenExtensions); + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + + $this->assertEquals($exception === null, $validator->isFilenameValid($filename)); + } + + public function dataValidateFilename(): array { + return [ + 'valid name' => [ + 'a: b.txt', ['.htaccess'], [], [], null + ], + 'valid name with some more parameters' => [ + 'a: b.txt', ['.htaccess'], ['exe'], ['~'], null + ], + 'forbidden name' => [ + '.htaccess', ['.htaccess'], [], [], ReservedWordException::class + ], + 'forbidden name - name is case insensitive' => [ + 'COM1', ['.htaccess', 'com1'], [], [], ReservedWordException::class + ], + 'forbidden name - name checks the filename' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess', 'com1'], [], [], ReservedWordException::class + ], + 'invalid character' => [ + 'a: b.txt', ['.htaccess'], [], [':'], InvalidCharacterInPathException::class + ], + 'invalid path' => [ + '../../foo.bar', ['.htaccess'], [], ['/', '\\'], InvalidCharacterInPathException::class, + ], + 'invalid extension' => [ + 'a: b.txt', ['.htaccess'], ['.txt'], [], InvalidPathException::class + ], + 'empty filename' => [ + '', [], [], [], EmptyFileNameException::class + ], + 'reserved unix name "."' => [ + '.', [], [], [], InvalidPathException::class + ], + 'reserved unix name ".."' => [ + '..', [], [], [], ReservedWordException::class + ], + 'too long filename "."' => [ + str_repeat('a', 251), [], [], [], FileNameTooLongException::class + ], + // make sure to not split the list entries as they migh contain Unicode sequences + // in this example the "face in clouds" emoji contains the clouds emoji so only having clouds is ok + ['🌫️.txt', ['.htaccess'], [], ['😶‍🌫️'], null], + // This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji + ['😶‍🌫️.txt', ['.htaccess'], [], ['🌫️'], InvalidCharacterInPathException::class], + ]; + } + + /** + * @dataProvider dataIsForbidden + */ + public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenFilenames']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + + $this->assertEquals($expected, $validator->isFilenameValid($filename)); + } + + public function dataIsForbidden(): array { + return [ + 'valid name' => [ + 'a: b.txt', ['.htaccess'], true + ], + 'valid name with some more parameters' => [ + 'a: b.txt', ['.htaccess'], true + ], + 'forbidden name' => [ + '.htaccess', ['.htaccess'], false + ], + 'forbidden name - name is case insensitive' => [ + 'COM1', ['.htaccess', 'com1'], false + ], + 'forbidden name - name checks the filename' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess', 'com1'], false + ], + ]; + } +}