From b0b87fc177976cad6a0c382f5e566cad52a059f3 Mon Sep 17 00:00:00 2001 From: rivexe Date: Mon, 26 Feb 2024 13:13:59 +0300 Subject: [PATCH] fix: security vulnerability when file downloading is not permit --- controller/callbackcontroller.php | 40 +++++++++++++++++++----------- controller/editorapicontroller.php | 7 ++---- controller/editorcontroller.php | 19 +++++++++++++- lib/fileutility.php | 38 ++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 20 deletions(-) diff --git a/controller/callbackcontroller.php b/controller/callbackcontroller.php index 58992b79..7e9b5f1b 100644 --- a/controller/callbackcontroller.php +++ b/controller/callbackcontroller.php @@ -42,6 +42,7 @@ use OCA\Onlyoffice\Crypt; use OCA\Onlyoffice\DocumentService; use OCA\Onlyoffice\FileVersions; +use OCA\Onlyoffice\FileUtility; use OCA\Onlyoffice\VersionManager; use OCA\Onlyoffice\KeyManager; use OCA\Onlyoffice\RemoteInstance; @@ -242,13 +243,24 @@ public function download($doc) { } $shareToken = isset($hashData->shareToken) ? $hashData->shareToken : null; - list($file, $error) = empty($shareToken) ? $this->getFile($userId, $fileId, null, $changes ? null : $version, $template) : $this->getFileByToken($fileId, $shareToken, $changes ? null : $version); + list($file, $error, $share) = empty($shareToken) ? $this->getFile($userId, $fileId, null, $changes ? null : $version, $template) : $this->getFileByToken($fileId, $shareToken, $changes ? null : $version); if (isset($error)) { return $error; } - if ($this->userSession->isLoggedIn() && !$file->isReadable()) { + $canDownload = true; + + $fileStorage = $file->getStorage(); + if ($fileStorage->instanceOfStorage("\OCA\Files_Sharing\SharedStorage") || !empty($shareToken)) { + $share = empty($share) ? $fileStorage->getShare() : $share; + $canDownload = FileUtility::canShareDownload($share); + if (!$canDownload && !empty($this->config->getDocumentServerSecret())) { + $canDownload = true; + } + } + + if ($this->userSession->isLoggedIn() && !$file->isReadable() || !$canDownload) { $this->logger->error("Download without access right", ["app" => $this->appName]); return new JSONResponse(["message" => $this->trans->t("Access denied")], Http::STATUS_FORBIDDEN); } @@ -504,7 +516,7 @@ public function track($doc, $users, $key, $status, $url, $token, $history, $chan \OC_Util::setupFS($userId); } - list($file, $error) = empty($shareToken) ? $this->getFile($userId, $fileId, $filePath) : $this->getFileByToken($fileId, $shareToken); + list($file, $error, $share) = empty($shareToken) ? $this->getFile($userId, $fileId, $filePath) : $this->getFileByToken($fileId, $shareToken); if (isset($error)) { $this->logger->error("track error: $fileId " . json_encode($error->getData()), ["app" => $this->appName]); @@ -613,7 +625,7 @@ function () use ($file, $newData) { */ private function getFile($userId, $fileId, $filePath = null, $version = 0, $template = false) { if (empty($fileId)) { - return [null, new JSONResponse(["message" => $this->trans->t("FileId is empty")], Http::STATUS_BAD_REQUEST)]; + return [null, new JSONResponse(["message" => $this->trans->t("FileId is empty")], Http::STATUS_BAD_REQUEST), null]; } try { @@ -621,12 +633,12 @@ private function getFile($userId, $fileId, $filePath = null, $version = 0, $temp $files = $folder->getById($fileId); } catch (\Exception $e) { $this->logger->logException($e, ["message" => "getFile: $fileId", "app" => $this->appName]); - return [null, new JSONResponse(["message" => $this->trans->t("Invalid request")], Http::STATUS_BAD_REQUEST)]; + return [null, new JSONResponse(["message" => $this->trans->t("Invalid request")], Http::STATUS_BAD_REQUEST), null]; } if (empty($files)) { $this->logger->error("Files not found: $fileId", ["app" => $this->appName]); - return [null, new JSONResponse(["message" => $this->trans->t("Files not found")], Http::STATUS_NOT_FOUND)]; + return [null, new JSONResponse(["message" => $this->trans->t("Files not found")], Http::STATUS_NOT_FOUND), null]; } $file = $files[0]; @@ -651,10 +663,10 @@ private function getFile($userId, $fileId, $filePath = null, $version = 0, $temp if ($owner !== null) { if ($owner->getUID() !== $userId) { - list($file, $error) = $this->getFile($owner->getUID(), $file->getId()); + list($file, $error, $share) = $this->getFile($owner->getUID(), $file->getId()); if (isset($error)) { - return [null, $error]; + return [null, $error, null]; } } @@ -667,7 +679,7 @@ private function getFile($userId, $fileId, $filePath = null, $version = 0, $temp } } - return [$file, null]; + return [$file, null, null]; } /** @@ -683,14 +695,14 @@ private function getFileByToken($fileId, $shareToken, $version = 0) { list($share, $error) = $this->getShare($shareToken); if (isset($error)) { - return [null, $error]; + return [null, $error, null]; } try { $node = $share->getNode(); } catch (NotFoundException $e) { $this->logger->logException($e, ["message" => "getFileByToken error", "app" => $this->appName]); - return [null, new JSONResponse(["message" => $this->trans->t("File not found")], Http::STATUS_NOT_FOUND)]; + return [null, new JSONResponse(["message" => $this->trans->t("File not found")], Http::STATUS_NOT_FOUND), null]; } if ($node instanceof Folder) { @@ -698,11 +710,11 @@ private function getFileByToken($fileId, $shareToken, $version = 0) { $files = $node->getById($fileId); } catch (\Exception $e) { $this->logger->logException($e, ["message" => "getFileByToken: $fileId", "app" => $this->appName]); - return [null, new JSONResponse(["message" => $this->trans->t("Invalid request")], Http::STATUS_NOT_FOUND)]; + return [null, new JSONResponse(["message" => $this->trans->t("Invalid request")], Http::STATUS_NOT_FOUND), null]; } if (empty($files)) { - return [null, new JSONResponse(["message" => $this->trans->t("File not found")], Http::STATUS_NOT_FOUND)]; + return [null, new JSONResponse(["message" => $this->trans->t("File not found")], Http::STATUS_NOT_FOUND), null]; } $file = $files[0]; } else { @@ -722,7 +734,7 @@ private function getFileByToken($fileId, $shareToken, $version = 0) { } } - return [$file, null]; + return [$file, null, $share]; } /** diff --git a/controller/editorapicontroller.php b/controller/editorapicontroller.php index 91558b84..f0d0d852 100644 --- a/controller/editorapicontroller.php +++ b/controller/editorapicontroller.php @@ -325,11 +325,8 @@ public function config($fileId, $filePath = null, $shareToken = null, $version = $storageShare = $fileStorage->getShare(); if (method_exists($storageShare, "getAttributes")) { $attributes = $storageShare->getAttributes(); - - $permissionsDownload = $attributes->getAttribute("permissions", "download"); - if ($permissionsDownload !== null) { - $params["document"]["permissions"]["download"] = $params["document"]["permissions"]["print"] = $params["document"]["permissions"]["copy"] = $permissionsDownload === true; - } + $canDownload = FileUtility::canShareDownload($storageShare); + $params["document"]["permissions"]["download"] = $params["document"]["permissions"]["print"] = $params["document"]["permissions"]["copy"] = $canDownload === true; if (isset($format["review"]) && $format["review"]) { $permissionsReviewOnly = $attributes->getAttribute($this->appName, "review"); diff --git a/controller/editorcontroller.php b/controller/editorcontroller.php index db269095..ee42e442 100644 --- a/controller/editorcontroller.php +++ b/controller/editorcontroller.php @@ -1092,7 +1092,16 @@ public function url($filePath) { $this->logger->error("File for generate presigned url was not found: $dir", ["app" => $this->appName]); return ["error" => $this->trans->t("File not found")]; } - if (!$file->isReadable()) { + + $canDownload = true; + + $fileStorage = $file->getStorage(); + if ($fileStorage->instanceOfStorage("\OCA\Files_Sharing\SharedStorage")) { + $share = $fileStorage->getShare(); + $canDownload = FileUtility::canShareDownload($share); + } + + if (!$file->isReadable() || !$canDownload) { $this->logger->error("File without permission: $dir", ["app" => $this->appName]); return ["error" => $this->trans->t("You do not have enough permissions to view the file")]; } @@ -1162,6 +1171,14 @@ public function download($fileId, $toExtension = null, $template = false) { return $this->renderError($this->trans->t("Not permitted")); } + $fileStorage = $file->getStorage(); + if ($fileStorage->instanceOfStorage("\OCA\Files_Sharing\SharedStorage")) { + $share = empty($share) ? $fileStorage->getShare() : $share; + if (!FileUtility::canShareDownload($share)) { + return $this->renderError($this->trans->t("Not permitted")); + } + } + $fileName = $file->getName(); $ext = strtolower(pathinfo($fileName, PATHINFO_EXTENSION)); $toExtension = strtolower($toExtension); diff --git a/lib/fileutility.php b/lib/fileutility.php index 8f2b6dab..2d0782db 100644 --- a/lib/fileutility.php +++ b/lib/fileutility.php @@ -27,6 +27,7 @@ use OCP\ILogger; use OCP\ISession; use OCP\Share\IManager; +use OCP\Share\IShare; use OCA\Onlyoffice\AppConfig; use OCA\Onlyoffice\Version; @@ -297,4 +298,41 @@ public function getVersionKey($version) { return $key; } + + /** + * The method checks download permission + * + * @param IShare $share - share object + * + * @return bool + */ + public static function canShareDownload($share) { + $can = true; + + $downloadAttribute = self::getShareAttrubute($share, "download"); + if (isset($downloadAttribute)) { + $can = $downloadAttribute; + } + + return $can; + } + + /** + * The method extracts share attribute + * + * @param IShare $share - share object + * @param string $attribute - attribute name + * + * @return bool|null + */ + private static function getShareAttrubute($share, $attribute) { + $attributes = null; + if (method_exists(IShare::class, "getAttributes")) { + $attributes = $share->getAttributes(); + } + + $attribute = isset($attributes) ? $attributes->getAttribute("permissions", $attribute) : null; + + return $attribute; + } }