Skip to content

Commit

Permalink
fix: Allow rename and delete of invalid filenames - just forbid creat…
Browse files Browse the repository at this point in the history 
…e and write

Signed-off-by: Ferdinand Thiessen <[email protected]>
  • Loading branch information
@susnux
susnux committed Aug 14, 2024
1 parent 29a6db7 commit c5db8c2
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 82 deletions.
2 changes: 1 addition & 1 deletion apps/dav/lib/Connector/Sabre/Directory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
208 changes: 127 additions & 81 deletions lib/private/Files/View.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class View {
private bool $lockingEnabled;
private bool $updaterEnabled = true;
private UserManager $userManager;
private FilenameValidator $filenameValidator;
private LoggerInterface $logger;

/**
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1439,17 +1457,26 @@ 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();
}, $contents);
/**
* @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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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'));
Expand Down

0 comments on commit c5db8c2

Please sign in to comment.