Skip to content

Commit

Permalink
fix: security vulnerability when file downloading is not permit
Browse files Browse the repository at this point in the history
  • Loading branch information
@rivexe
rivexe committed Feb 26, 2024
1 parent f8b9c0f commit b0b87fc
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 20 deletions.
40 changes: 26 additions & 14 deletions controller/callbackcontroller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -613,20 +625,20 @@ 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 {
$folder = !$template ? $this->root->getUserFolder($userId) : TemplateManager::getGlobalTemplateDir();
$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];
Expand All @@ -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];
}
}

Expand All @@ -667,7 +679,7 @@ private function getFile($userId, $fileId, $filePath = null, $version = 0, $temp
}
}

return [$file, null];
return [$file, null, null];
}

/**
Expand All @@ -683,26 +695,26 @@ 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) {
try {
$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 {
Expand All @@ -722,7 +734,7 @@ private function getFileByToken($fileId, $shareToken, $version = 0) {
}
}

return [$file, null];
return [$file, null, $share];
}

/**
Expand Down
7 changes: 2 additions & 5 deletions controller/editorapicontroller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
19 changes: 18 additions & 1 deletion controller/editorcontroller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")];
}
Expand Down Expand Up @@ -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);
Expand Down
38 changes: 38 additions & 0 deletions lib/fileutility.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}

0 comments on commit b0b87fc

Please sign in to comment.