CVE-2021-4026: Incorrect Authorization (CWE-863)
value: bookstack is vulnerable to Improper Access Control
CWE description: The product performs an authorization check when an actor attempts to access a resource or perform an action, but it does not correctly perform the check. This allows attackers to bypass intended access restrictions.
CVSS v3 base score: 4.3
Published: 2021-11-30T20:15Z
Repository: https://github.com/bookstackapp/bookstack
Language: PHP
Files changed: app/Actions/ActivityService.php, app/Auth/Permissions/PermissionService.php, app/Exceptions/Handler.php, tests/Api/AttachmentsApiTest.php
Commit date: 2021-11-30 00:06:17 +0000
Commit message: Fixed related permissions query not considering drafts

Page-related items added on drafts could be visible in certain scenarios
since the applied permissions query filters would not consider
page draft visibility.
This commit alters queries on related items to apply such filtering.

Included test to cover API scenario.
Thanks to @haxatron for reporting.

Diff: diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php
index 983c1a603c..73dc76de00 100644
--- a/app/Actions/ActivityService.php
+++ b/app/Actions/ActivityService.php
@@ -131,11 +131,11 @@ class ActivityService
 
         return $this->filterSimilar($activity);
     }
 
     /**
-     * Get latest activity for a user, Filtering out similar items.
+     * Get the latest activity for a user, Filtering out similar items.
      */
     public function userActivity(User $user, int $count = 20, int $page = 0): array
     {
         $activityList = $this->permissionService
             ->filterRestrictedEntityRelations($this->activity->newQuery(), 'activities', 'entity_id', 'entity_type')
diff --git a/app/Auth/Permissions/PermissionService.php b/app/Auth/Permissions/PermissionService.php
index 1397253397..4214861c22 100644
--- a/app/Auth/Permissions/PermissionService.php
+++ b/app/Auth/Permissions/PermissionService.php
@@ -600,29 +600,39 @@ class PermissionService
         return $this->entityRestrictionQuery($query, $action);
     }
 
     /**
      * Filter items that have entities set as a polymorphic relation.
+     * For simplicity, this will not return results attached to draft pages.
+     * Draft pages should never really have related items though.
      *
      * @param Builder|QueryBuilder $query
      */
     public function filterRestrictedEntityRelations($query, string $tableName, string $entityIdColumn, string $entityTypeColumn, string $action = 'view')
     {
         $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn, 'entityTypeColumn' => $entityTypeColumn];
-
-        $q = $query->where(function ($query) use ($tableDetails, $action) {
-            $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) {
-                /** @var Builder $permissionQuery */
-                $permissionQuery->select(['role_id'])->from('joint_permissions')
-                    ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
-                    ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'])
-                    ->where('action', '=', $action)
-                    ->whereIn('role_id', $this->getCurrentUserRoles())
-                    ->where(function (QueryBuilder $query) {
-                        $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
-                    });
-            });
+        $pageMorphClass = (new Page())->getMorphClass();
+
+        $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) {
+            /** @var Builder $permissionQuery */
+            $permissionQuery->select(['role_id'])->from('joint_permissions')
+                ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
+                ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'])
+                ->where('joint_permissions.action', '=', $action)
+                ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles())
+                ->where(function (QueryBuilder $query) {
+                    $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
+                });
+        })->where(function ($query) use ($tableDetails, $pageMorphClass) {
+            /** @var Builder $query */
+            $query->where($tableDetails['entityTypeColumn'], '!=', $pageMorphClass)
+                ->orWhereExists(function(QueryBuilder $query) use ($tableDetails, $pageMorphClass) {
+                    $query->select('id')->from('pages')
+                        ->whereColumn('pages.id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
+                        ->where($tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'], '=', $pageMorphClass)
+                        ->where('pages.draft', '=', false);
+                });
         });
 
         $this->clean();
 
         return $q;
@@ -632,29 +642,43 @@ class PermissionService
      * Add conditions to a query to filter the selection to related entities
      * where view permissions are granted.
      */
     public function filterRelatedEntity(string $entityClass, Builder $query, string $tableName, string $entityIdColumn): Builder
     {
-        $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn];
-        $morphClass = app($entityClass)->getMorphClass();
-
-        $q = $query->where(function ($query) use ($tableDetails, $morphClass) {
-            $query->where(function ($query) use (&$tableDetails, $morphClass) {
-                $query->whereExists(function ($permissionQuery) use (&$tableDetails, $morphClass) {
-                    /** @var Builder $permissionQuery */
-                    $permissionQuery->select('id')->from('joint_permissions')
-                        ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
-                        ->where('entity_type', '=', $morphClass)
-                        ->where('action', '=', 'view')
-                        ->whereIn('role_id', $this->getCurrentUserRoles())
-                        ->where(function (QueryBuilder $query) {
-                            $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
-                        });
+        $fullEntityIdColumn = $tableName . '.' . $entityIdColumn;
+        $instance = new $entityClass;
+        $morphClass = $instance->getMorphClass();
+
+        $existsQuery = function($permissionQuery) use ($fullEntityIdColumn, $morphClass) {
+            /** @var Builder $permissionQuery */
+            $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions')
+                ->whereColumn('joint_permissions.entity_id', '=', $fullEntityIdColumn)
+                ->where('joint_permissions.entity_type', '=', $morphClass)
+                ->where('joint_permissions.action', '=', 'view')
+                ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles())
+                ->where(function (QueryBuilder $query) {
+                    $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
                 });
-            })->orWhere($tableDetails['entityIdColumn'], '=', 0);
+        };
+
+        $q = $query->where(function ($query) use ($existsQuery, $fullEntityIdColumn) {
+            $query->whereExists($existsQuery)
+                ->orWhere($fullEntityIdColumn, '=', 0);
         });
 
+        if ($instance instanceof Page) {
+            // Prevent visibility of non-owned draft pages
+            $q->whereExists(function(QueryBuilder $query) use ($fullEntityIdColumn) {
+                $query->select('id')->from('pages')
+                    ->whereColumn('pages.id', '=', $fullEntityIdColumn)
+                    ->where(function (QueryBuilder $query) {
+                        $query->where('pages.draft', '=', false)
+                            ->orWhere('pages.owned_by', '=', $this->currentUser()->id);
+                    });
+            });
+        }
+
         $this->clean();
 
         return $q;
     }
 
@@ -664,13 +688,13 @@ class PermissionService
      *
      * @param QueryBuilder|Builder $query
      */
     protected function addJointHasPermissionCheck($query, int $userIdToCheck)
     {
-        $query->where('has_permission', '=', true)->orWhere(function ($query) use ($userIdToCheck) {
-            $query->where('has_permission_own', '=', true)
-                ->where('owned_by', '=', $userIdToCheck);
+        $query->where('joint_permissions.has_permission', '=', true)->orWhere(function ($query) use ($userIdToCheck) {
+            $query->where('joint_permissions.has_permission_own', '=', true)
+                ->where('joint_permissions.owned_by', '=', $userIdToCheck);
         });
     }
 
     /**
      * Get the current user.
diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index 3b4ad4a4da..7ec5025250 100644
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -2,10 +2,11 @@
 
 namespace BookStack\Exceptions;
 
 use Exception;
 use Illuminate\Auth\AuthenticationException;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Request;
 use Illuminate\Validation\ValidationException;
 use Symfony\Component\HttpKernel\Exception\HttpException;
@@ -73,19 +74,24 @@ class Handler extends ExceptionHandler
     }
 
     /**
      * Render an exception when the API is in use.
      */
-    protected function renderApiException(Exception $e): JsonResponse
+    protected function renderApiException(Throwable $e): JsonResponse
     {
-        $code = $e->getCode() === 0 ? 500 : $e->getCode();
+        $code = 500;
         $headers = [];
+
         if ($e instanceof HttpException) {
             $code = $e->getStatusCode();
             $headers = $e->getHeaders();
         }
 
+        if ($e instanceof ModelNotFoundException) {
+            $code = 404;
+        }
+
         $responseData = [
             'error' => [
                 'message' => $e->getMessage(),
             ],
         ];
diff --git a/tests/Api/AttachmentsApiTest.php b/tests/Api/AttachmentsApiTest.php
index ceab5d49af..bfa47343e9 100644
--- a/tests/Api/AttachmentsApiTest.php
+++ b/tests/Api/AttachmentsApiTest.php
@@ -222,10 +222,33 @@ class AttachmentsApiTest extends TestCase
         ]);
 
         unlink(storage_path($attachment->path));
     }
 
+    public function test_attachment_not_visible_on_other_users_draft()
+    {
+        $this->actingAsApiAdmin();
+        $editor = $this->getEditor();
+
+        /** @var Page $page */
+        $page = Page::query()->first();
+        $page->draft = true;
+        $page->owned_by = $editor;
+        $page->save();
+        $this->regenEntityPermissions($page);
+
+        $attachment = $this->createAttachmentForPage($page, [
+            'name'  => 'my attachment',
+            'path'  => 'https://example.com',
+            'order' => 1,
+        ]);
+
+        $resp = $this->getJson("{$this->baseEndpoint}/{$attachment->id}");
+
+        $resp->assertStatus(404);
+    }
+
     public function test_update_endpoint()
     {
         $this->actingAsApiAdmin();
         /** @var Page $page */
         $page = Page::query()->first();

Vulnerable code:      * Get latest activity for a user, Filtering out similar items.

        $q = $query->where(function ($query) use ($tableDetails, $action) {
            $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) {
                /** @var Builder $permissionQuery */
                $permissionQuery->select(['role_id'])->from('joint_permissions')
                    ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
                    ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'])
                    ->where('action', '=', $action)
                    ->whereIn('role_id', $this->getCurrentUserRoles())
                    ->where(function (QueryBuilder $query) {
                        $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
                    });
            });
        $tableDetails = ['tableName' => $tableName, 'entityIdColumn' => $entityIdColumn];
        $morphClass = app($entityClass)->getMorphClass();

        $q = $query->where(function ($query) use ($tableDetails, $morphClass) {
            $query->where(function ($query) use (&$tableDetails, $morphClass) {
                $query->whereExists(function ($permissionQuery) use (&$tableDetails, $morphClass) {
                    /** @var Builder $permissionQuery */
                    $permissionQuery->select('id')->from('joint_permissions')
                        ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
                        ->where('entity_type', '=', $morphClass)
                        ->where('action', '=', 'view')
                        ->whereIn('role_id', $this->getCurrentUserRoles())
                        ->where(function (QueryBuilder $query) {
                            $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
                        });
            })->orWhere($tableDetails['entityIdColumn'], '=', 0);
        $query->where('has_permission', '=', true)->orWhere(function ($query) use ($userIdToCheck) {
            $query->where('has_permission_own', '=', true)
                ->where('owned_by', '=', $userIdToCheck);
    protected function renderApiException(Exception $e): JsonResponse
        $code = $e->getCode() === 0 ? 500 : $e->getCode();
Fixed code:      * Get the latest activity for a user, Filtering out similar items.
     * For simplicity, this will not return results attached to draft pages.
     * Draft pages should never really have related items though.
        $pageMorphClass = (new Page())->getMorphClass();

        $q = $query->whereExists(function ($permissionQuery) use (&$tableDetails, $action) {
            /** @var Builder $permissionQuery */
            $permissionQuery->select(['role_id'])->from('joint_permissions')
                ->whereColumn('joint_permissions.entity_id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
                ->whereColumn('joint_permissions.entity_type', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'])
                ->where('joint_permissions.action', '=', $action)
                ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles())
                ->where(function (QueryBuilder $query) {
                    $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
                });
        })->where(function ($query) use ($tableDetails, $pageMorphClass) {
            /** @var Builder $query */
            $query->where($tableDetails['entityTypeColumn'], '!=', $pageMorphClass)
                ->orWhereExists(function(QueryBuilder $query) use ($tableDetails, $pageMorphClass) {
                    $query->select('id')->from('pages')
                        ->whereColumn('pages.id', '=', $tableDetails['tableName'] . '.' . $tableDetails['entityIdColumn'])
                        ->where($tableDetails['tableName'] . '.' . $tableDetails['entityTypeColumn'], '=', $pageMorphClass)
                        ->where('pages.draft', '=', false);
                });
        $fullEntityIdColumn = $tableName . '.' . $entityIdColumn;
        $instance = new $entityClass;
        $morphClass = $instance->getMorphClass();

        $existsQuery = function($permissionQuery) use ($fullEntityIdColumn, $morphClass) {
            /** @var Builder $permissionQuery */
            $permissionQuery->select('joint_permissions.role_id')->from('joint_permissions')
                ->whereColumn('joint_permissions.entity_id', '=', $fullEntityIdColumn)
                ->where('joint_permissions.entity_type', '=', $morphClass)
                ->where('joint_permissions.action', '=', 'view')
                ->whereIn('joint_permissions.role_id', $this->getCurrentUserRoles())
                ->where(function (QueryBuilder $query) {
                    $this->addJointHasPermissionCheck($query, $this->currentUser()->id);
        };

        $q = $query->where(function ($query) use ($existsQuery, $fullEntityIdColumn) {
            $query->whereExists($existsQuery)
                ->orWhere($fullEntityIdColumn, '=', 0);
        if ($instance instanceof Page) {
            // Prevent visibility of non-owned draft pages
            $q->whereExists(function(QueryBuilder $query) use ($fullEntityIdColumn) {
                $query->select('id')->from('pages')
                    ->whereColumn('pages.id', '=', $fullEntityIdColumn)
                    ->where(function (QueryBuilder $query) {
                        $query->where('pages.draft', '=', false)
                            ->orWhere('pages.owned_by', '=', $this->currentUser()->id);
                    });
            });
        }

        $query->where('joint_permissions.has_permission', '=', true)->orWhere(function ($query) use ($userIdToCheck) {
            $query->where('joint_permissions.has_permission_own', '=', true)
                ->where('joint_permissions.owned_by', '=', $userIdToCheck);
use Illuminate\Database\Eloquent\ModelNotFoundException;
    protected function renderApiException(Throwable $e): JsonResponse
        $code = 500;

        if ($e instanceof ModelNotFoundException) {
            $code = 404;
        }

    public function test_attachment_not_visible_on_other_users_draft()
    {
        $this->actingAsApiAdmin();
        $editor = $this->getEditor();

        /** @var Page $page */
        $page = Page::query()->first();
        $page->draft = true;
        $page->owned_by = $editor;
        $page->save();
        $this->regenEntityPermissions($page);

        $attachment = $this->createAttachmentForPage($page, [
            'name'  => 'my attachment',
            'path'  => 'https://example.com',
            'order' => 1,
        ]);

        $resp = $this->getJson("{$this->baseEndpoint}/{$attachment->id}");

        $resp->assertStatus(404);
    }