From 7121189f584ee4132f312e9900b059d27a7a9b9a Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 12 Aug 2024 18:12:23 +0200 Subject: [PATCH] fix: Allow rename and delete of invalid filenames - just forbid create and write Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/Directory.php | 2 +- lib/private/Files/View.php | 208 +++++++++++++-------- 2 files changed, 128 insertions(+), 82 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 427ec59bc3160..d56f56890cc7f 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -173,7 +173,7 @@ public function getChild($name, $info = null, ?IRequest $request = null, ?IL10N $path = $this->path . '/' . $name; if (is_null($info)) { try { - $this->fileView->verifyPath($this->path, $name); + $this->fileView->verifyPath($this->path, $name, true); $info = $this->fileView->getFileInfo($path); } catch (\OCP\Files\StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage(), 0, $e); diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 0e5e433ccb689..7cfb6e24a8ad4 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -57,6 +57,7 @@ class View { private bool $lockingEnabled; private bool $updaterEnabled = true; private UserManager $userManager; + private FilenameValidator $filenameValidator; private LoggerInterface $logger; /** @@ -71,6 +72,7 @@ public function __construct(string $root = '') { $this->lockingProvider = \OC::$server->get(ILockingProvider::class); $this->lockingEnabled = !($this->lockingProvider instanceof \OC\Lock\NoopLockingProvider); $this->userManager = \OC::$server->getUserManager(); + $this->filenameValidator = \OCP\Server::get(FilenameValidator::class); $this->logger = \OC::$server->get(LoggerInterface::class); } @@ -1097,98 +1099,108 @@ public function free_space($path = '/') { * @throws LockedException * * This method takes requests for basic filesystem functions (e.g. reading & writing - * files), processes hooks and proxies, sanitises paths, and finally passes them on to + * files), processes hooks and proxies, sanitizes paths, and finally passes them on to * \OC\Files\Storage\Storage for delegation to a storage backend for execution */ 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); + // Verify that the operation is allowed + try { + $parent = dirname($path); + $parentName = basename($parent); + $this->verifyPath($parent, basename($path), !in_array('write', $hooks)); + // If this is a create operation also verify the parent folder name + if (in_array('create', $hooks) && $parentName !== '') { + $this->verifyPath(dirname($parent), $parentName); } + } catch (InvalidPathException) { + 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; + } + + 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, 0); + } 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, 0); + } - $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; } @@ -1355,11 +1367,17 @@ public function getFileInfo($path, $includeMountPoints = true) { return false; } + if ($internalPath === '' && $data['name']) { + $data['name'] = basename($path); + } if ($mount instanceof MoveableMount && $internalPath === '') { $data['permissions'] |= \OCP\Constants::PERMISSION_DELETE; } - if ($internalPath === '' && $data['name']) { - $data['name'] = basename($path); + // Ensure that parent path is valid (so it is writable) + try { + $this->verifyPath(dirname($path), basename($path)); + } catch (InvalidPathException) { + $data['permissions'] &= ~(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); } $ownerId = $storage->getOwner($internalPath); @@ -1439,6 +1457,12 @@ public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Fil $contents = $cache->getFolderContentsById($folderId); //TODO: mimetype_filter $sharingDisabled = \OCP\Util::isSharingDisabledForUser(); + $folderIsReadonly = false; + try { + $this->verifyPath(dirname($directory), basename($directory)); + } catch (InvalidPathException) { + $folderIsReadonly = true; + } $fileNames = array_map(function (ICacheEntry $content) { return $content->getName(); @@ -1446,10 +1470,13 @@ public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Fil /** * @var \OC\Files\FileInfo[] $fileInfos */ - $fileInfos = array_map(function (ICacheEntry $content) use ($path, $storage, $mount, $sharingDisabled) { + $fileInfos = array_map(function (ICacheEntry $content) use ($path, $storage, $mount, $sharingDisabled, $folderIsReadonly) { if ($sharingDisabled) { $content['permissions'] = $content['permissions'] & ~\OCP\Constants::PERMISSION_SHARE; } + if ($folderIsReadonly) { + $content['permissions'] = $content['permissions'] & ~(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); + } $owner = $this->getUserObjectForOwner($storage->getOwner($content['path'])); return new FileInfo($path . '/' . $content['name'], $storage, $content['path'], $content, $mount, $owner); }, $contents); @@ -1517,6 +1544,9 @@ public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Fil if ($sharingDisabled) { $rootEntry['permissions'] = $rootEntry['permissions'] & ~\OCP\Constants::PERMISSION_SHARE; } + if ($folderIsReadonly) { + $rootEntry['permissions'] = $rootEntry['permissions'] & ~(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); + } $owner = $this->getUserObjectForOwner($subStorage->getOwner('')); $files[$rootEntry->getName()] = new FileInfo($path . '/' . $rootEntry['name'], $subStorage, '', $rootEntry, $mount, $owner); @@ -1826,28 +1856,44 @@ private function getPartFileInfo(string $path): \OC\Files\FileInfo { /** * @param string $path * @param string $fileName + * @param bool $readonly If the path should be verified for read-only operations * @throws InvalidPathException */ - public function verifyPath($path, $fileName): void { + public function verifyPath($path, $fileName, bool $readonly = false): void { // All of the view's functions disallow '..' in the path so we can short cut if the path is invalid if (!Filesystem::isValidPath($path ?: '/')) { $l = \OCP\Util::getL10N('lib'); throw new InvalidPathException($l->t('Path contains invalid segments')); } + // Handle filename validation for read-only operations, as we still allow to rename invalid files. + if ($readonly) { + // There is one exception: Forbidden files like `.htaccess` must not be accessible + if ($this->filenameValidator->isForbidden($fileName)) { + $l = \OCP\Util::getL10N('lib'); + throw new InvalidPathException($l->t('Filename is a reserved word')); + } + return; + } + try { - /** @type \OCP\Files\Storage $storage */ - [$storage, $internalPath] = $this->resolvePath($path); - $storage->verifyPath($internalPath, $fileName); + do { + /** @type \OCP\Files\Storage $storage */ + [$storage, $internalPath] = $this->resolvePath($path); + $storage->verifyPath($internalPath, $fileName); + + $fileName = basename($path); + $path = dirname($path); + } while ($fileName !== '' && $path !== '.'); } catch (ReservedWordException $ex) { $l = \OCP\Util::getL10N('lib'); - throw new InvalidPathException($l->t('File name is a reserved word')); + throw new InvalidPathException($ex->getMessage() ?: $l->t('Filename is a reserved word')); } catch (InvalidCharacterInPathException $ex) { $l = \OCP\Util::getL10N('lib'); - throw new InvalidPathException($l->t('File name contains at least one invalid character')); + throw new InvalidPathException($ex->getMessage() ?: $l->t('Filename contains at least one invalid character')); } catch (FileNameTooLongException $ex) { $l = \OCP\Util::getL10N('lib'); - throw new InvalidPathException($l->t('File name is too long')); + throw new InvalidPathException($l->t('Filename is too long')); } catch (InvalidDirectoryException $ex) { $l = \OCP\Util::getL10N('lib'); throw new InvalidPathException($l->t('Dot files are not allowed'));