CVE-2021-4119: Other (NVD-CWE-Other)
value: bookstack is vulnerable to Improper Access Control
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 9.8
Published: 2021-12-15T20:15Z
Repository: https://github.com/bookstackapp/bookstack
Language: PHP
Files changed: app/Auth/UserRepo.php, app/Http/Controllers/UserSearchController.php, resources/views/users/delete.blade.php, tests/User/UserManagementTest.php, tests/User/UserSearchTest.php
Commit date: 2021-12-14 18:47:22 +0000
Commit message: Addressed user detail harvesting issue

Altered access & usage of the /search/users/select endpoint with the
following changes:
- Removed searching of email address to prevent email detail discovery
  via hunting via search queries.
- Required the user to be logged in and have permission to manage users
  or manage permissions on items in some way.
- Removed the user migration option on user delete unless they have
  permission to manage users.

For #3108
Reported in https://huntr.dev/bounties/135f2d7d-ab0b-4351-99b9-889efac46fca/
Reported by @haxatron

Diff: diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php
index 6d48f12402..84002b7f7a 100644
--- a/app/Auth/UserRepo.php
+++ b/app/Auth/UserRepo.php
@@ -61,17 +61,20 @@ class UserRepo
         return User::query()->with('roles', 'avatar')->orderBy('name', 'asc')->get();
     }
 
     /**
      * Get all the users with their permissions in a paginated format.
+     * Note: Due to the use of email search this should only be used when
+     * user is assumed to be trusted. (Admin users).
+     * Email search can be abused to extract email addresses.
      */
     public function getAllUsersPaginatedAndSorted(int $count, array $sortData): LengthAwarePaginator
     {
         $sort = $sortData['sort'];
 
         $query = User::query()->select(['*'])
-            ->withLastActivityAt()
+            ->scopes(['withLastActivityAt'])
             ->with(['roles', 'avatar'])
             ->withCount('mfaValues')
             ->orderBy($sort, $sortData['order']);
 
         if ($sortData['search']) {
diff --git a/app/Http/Controllers/UserSearchController.php b/app/Http/Controllers/UserSearchController.php
index f7ed9e57a9..1a37ca491d 100644
--- a/app/Http/Controllers/UserSearchController.php
+++ b/app/Http/Controllers/UserSearchController.php
@@ -1,32 +1,39 @@
 <?php
 
 namespace BookStack\Http\Controllers;
 
 use BookStack\Auth\User;
-use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Http\Request;
 
 class UserSearchController extends Controller
 {
     /**
      * Search users in the system, with the response formatted
      * for use in a select-style list.
      */
     public function forSelect(Request $request)
     {
+        $hasPermission = signedInUser() && (
+                   userCan('users-manage')
+                || userCan('restrictions-manage-own')
+                || userCan('restrictions-manage-all')
+        );
+
+        if (!$hasPermission) {
+            $this->showPermissionError();
+        }
+
         $search = $request->get('search', '');
-        $query = User::query()->orderBy('name', 'desc')
+        $query = User::query()
+            ->orderBy('name', 'asc')
             ->take(20);
 
         if (!empty($search)) {
-            $query->where(function (Builder $query) use ($search) {
-                $query->where('email', 'like', '%' . $search . '%')
-                    ->orWhere('name', 'like', '%' . $search . '%');
-            });
+            $query->where('name', 'like', '%' . $search . '%');
         }
 
-        $users = $query->get();
-
-        return view('form.user-select-list', compact('users'));
+        return view('form.user-select-list', [
+            'users' => $query->get(),
+        ]);
     }
 }
diff --git a/resources/views/users/delete.blade.php b/resources/views/users/delete.blade.php
index aea4d79545..490e9d6c5a 100644
--- a/resources/views/users/delete.blade.php
+++ b/resources/views/users/delete.blade.php
@@ -10,21 +10,23 @@
         <div class="card content-wrap auto-height">
             <h1 class="list-heading">{{ trans('settings.users_delete') }}</h1>
 
             <p>{{ trans('settings.users_delete_warning', ['userName' => $user->name]) }}</p>
 
-            <hr class="my-l">
-
-            <div class="grid half gap-xl v-center">
-                <div>
-                    <label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
-                    <p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
+            @if(userCan('users-manage'))
+                <hr class="my-l">
+
+                <div class="grid half gap-xl v-center">
+                    <div>
+                        <label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
+                        <p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
+                    </div>
+                    <div>
+                        @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
+                    </div>
                 </div>
-                <div>
-                    @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
-                </div>
-            </div>
+            @endif
 
             <hr class="my-l">
 
             <div class="grid half">
                 <p class="text-neg"><strong>{{ trans('settings.users_delete_confirm') }}</strong></p>
diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php
index 5a36b85df6..94970df4fe 100644
--- a/tests/User/UserManagementTest.php
+++ b/tests/User/UserManagementTest.php
@@ -128,10 +128,25 @@ class UserManagementTest extends TestCase
         $resp = $this->asAdmin()->get("settings/users/{$editor->id}/delete");
         $resp->assertSee('Migrate Ownership');
         $resp->assertSee('new_owner_id');
     }
 
+    public function test_migrate_option_hidden_if_user_cannot_manage_users()
+    {
+        $editor = $this->getEditor();
+
+        $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
+        $resp->assertDontSee('Migrate Ownership');
+        $resp->assertDontSee('new_owner_id');
+
+        $this->giveUserPermissions($editor, ['users-manage']);
+
+        $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
+        $resp->assertSee('Migrate Ownership');
+        $resp->assertSee('new_owner_id');
+    }
+
     public function test_delete_with_new_owner_id_changes_ownership()
     {
         $page = Page::query()->first();
         $owner = $page->ownedBy;
         $newOwner = User::query()->where('id', '!=', $owner->id)->first();
diff --git a/tests/User/UserSearchTest.php b/tests/User/UserSearchTest.php
new file mode 100644
index 0000000000..1bf3f2d296
--- /dev/null
+++ b/tests/User/UserSearchTest.php
@@ -0,0 +1,68 @@
+<?php
+
+namespace Tests\User;
+
+use BookStack\Auth\User;
+use Tests\TestCase;
+
+class UserSearchTest extends TestCase
+{
+
+    public function test_select_search_matches_by_name()
+    {
+        $viewer = $this->getViewer();
+        $admin = $this->getAdmin();
+        $resp = $this->actingAs($admin)->get('/search/users/select?search=' . urlencode($viewer->name));
+
+        $resp->assertOk();
+        $resp->assertSee($viewer->name);
+        $resp->assertDontSee($admin->name);
+    }
+
+    public function test_select_search_shows_first_by_name_without_search()
+    {
+        /** @var User $firstUser */
+        $firstUser = User::query()->orderBy('name', 'desc')->first();
+        $resp = $this->asAdmin()->get('/search/users/select');
+
+        $resp->assertOk();
+        $resp->assertSee($firstUser->name);
+    }
+
+    public function test_select_search_does_not_match_by_email()
+    {
+        $viewer = $this->getViewer();
+        $editor = $this->getEditor();
+        $resp = $this->actingAs($editor)->get('/search/users/select?search=' . urlencode($viewer->email));
+
+        $resp->assertDontSee($viewer->name);
+    }
+
+    public function test_select_requires_right_permission()
+    {
+        $permissions = ['users-manage', 'restrictions-manage-own', 'restrictions-manage-all'];
+        $user = $this->getViewer();
+
+        foreach ($permissions as $permission) {
+            $resp = $this->actingAs($user)->get('/search/users/select?search=a');
+            $this->assertPermissionError($resp);
+
+            $this->giveUserPermissions($user, [$permission]);
+            $resp = $this->actingAs($user)->get('/search/users/select?search=a');
+            $resp->assertOk();
+            $user->roles()->delete();
+            $user->clearPermissionCache();
+        }
+    }
+
+    public function test_select_requires_logged_in_user()
+    {
+        $this->setSettings(['app-public' => true]);
+        $defaultUser = User::getDefault();
+        $this->giveUserPermissions($defaultUser, ['users-manage']);
+
+        $resp = $this->get('/search/users/select?search=a');
+        $this->assertPermissionError($resp);
+    }
+
+}
\ No newline at end of file

Vulnerable code:             ->withLastActivityAt()
use Illuminate\Database\Eloquent\Builder;
        $query = User::query()->orderBy('name', 'desc')
            $query->where(function (Builder $query) use ($search) {
                $query->where('email', 'like', '%' . $search . '%')
                    ->orWhere('name', 'like', '%' . $search . '%');
            });
        $users = $query->get();

        return view('form.user-select-list', compact('users'));
            <hr class="my-l">

            <div class="grid half gap-xl v-center">
                <div>
                    <label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
                    <p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
                <div>
                    @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
                </div>
            </div>
Fixed code:      * Note: Due to the use of email search this should only be used when
     * user is assumed to be trusted. (Admin users).
     * Email search can be abused to extract email addresses.
            ->scopes(['withLastActivityAt'])
        $hasPermission = signedInUser() && (
                   userCan('users-manage')
                || userCan('restrictions-manage-own')
                || userCan('restrictions-manage-all')
        );

        if (!$hasPermission) {
            $this->showPermissionError();
        }

        $query = User::query()
            ->orderBy('name', 'asc')
            $query->where('name', 'like', '%' . $search . '%');
        return view('form.user-select-list', [
            'users' => $query->get(),
        ]);
            @if(userCan('users-manage'))
                <hr class="my-l">

                <div class="grid half gap-xl v-center">
                    <div>
                        <label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
                        <p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
                    </div>
                    <div>
                        @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
                    </div>
            @endif
    public function test_migrate_option_hidden_if_user_cannot_manage_users()
    {
        $editor = $this->getEditor();

        $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
        $resp->assertDontSee('Migrate Ownership');
        $resp->assertDontSee('new_owner_id');

        $this->giveUserPermissions($editor, ['users-manage']);

        $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
        $resp->assertSee('Migrate Ownership');
        $resp->assertSee('new_owner_id');
    }

<?php

namespace Tests\User;

use BookStack\Auth\User;
use Tests\TestCase;

class UserSearchTest extends TestCase
{

    public function test_select_search_matches_by_name()
    {
        $viewer = $this->getViewer();
        $admin = $this->getAdmin();
        $resp = $this->actingAs($admin)->get('/search/users/select?search=' . urlencode($viewer->name));

        $resp->assertOk();
        $resp->assertSee($viewer->name);
        $resp->assertDontSee($admin->name);
    }

    public function test_select_search_shows_first_by_name_without_search()
    {
        /** @var User $firstUser */
        $firstUser = User::query()->orderBy('name', 'desc')->first();
        $resp = $this->asAdmin()->get('/search/users/select');

        $resp->assertOk();
        $resp->assertSee($firstUser->name);
    }

    public function test_select_search_does_not_match_by_email()
    {
        $viewer = $this->getViewer();
        $editor = $this->getEditor();
        $resp = $this->actingAs($editor)->get('/search/users/select?search=' . urlencode($viewer->email));

        $resp->assertDontSee($viewer->name);
    }

    public function test_select_requires_right_permission()
    {
        $permissions = ['users-manage', 'restrictions-manage-own', 'restrictions-manage-all'];
        $user = $this->getViewer();

        foreach ($permissions as $permission) {
            $resp = $this->actingAs($user)->get('/search/users/select?search=a');
            $this->assertPermissionError($resp);

            $this->giveUserPermissions($user, [$permission]);
            $resp = $this->actingAs($user)->get('/search/users/select?search=a');
            $resp->assertOk();
            $user->roles()->delete();
            $user->clearPermissionCache();
        }
    }

    public function test_select_requires_logged_in_user()
    {
        $this->setSettings(['app-public' => true]);
        $defaultUser = User::getDefault();
        $this->giveUserPermissions($defaultUser, ['users-manage']);

        $resp = $this->get('/search/users/select?search=a');
        $this->assertPermissionError($resp);
    }

}