CVE-2024-5685: Insufficient Information (NVD-CWE-noinfo)
value: Users with "User:edit" and "Self:api" permissions can promote or demote themselves or other users by performing changes to the group's memberships via API call.This issue affects snipe-it: from v4.6.17 through v6.4.1.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-06-14T10:15Z
Repository: https://github.com/snipe/snipe-it
Language: PHP
Files changed: app/Http/Controllers/Api/UsersController.php
Commit date: 2024-05-21 10:00:49 +0100
Commit message: Re-order gating and refactor group syncing

Signed-off-by: snipe <snipe@snipe.net>

Diff: diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php
index 0faa541243a..006b6de9a6c 100644
--- a/app/Http/Controllers/Api/UsersController.php
+++ b/app/Http/Controllers/Api/UsersController.php
@@ -473,11 +473,11 @@ class UsersController extends Controller
         // here because we need to overwrite permissions
         // if someone needs to null them out
         if ($request->has('permissions')) {
             $permissions_array = $request->input('permissions');
 
-            // Strip out the superuser permission if the API user isn't a superadmin
+            // Strip out the individual superuser permission if the API user isn't a superadmin
             if (! Auth::user()->isSuperUser()) {
                 unset($permissions_array['superuser']);
             }
 
             $user->permissions = $permissions_array;
@@ -491,36 +491,24 @@ class UsersController extends Controller
         
         app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar');
           
         if ($user->save()) {
 
-            // Check if the request has groups passed and has a value
-            if ($request->filled('groups')) {
+            // Check if the request has groups passed and has a value, AND that the user us a superuser
+            if (($request->has('groups')) && (Auth::user()->isSuperUser())) {
 
                 $validator = Validator::make($request->all(), [
                     'groups.*' => 'integer|exists:permission_groups,id',
                 ]);
-                
-                if ($validator->fails()){
-                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
-                }
 
-                // Only save groups if the user is a superuser
-                if (Auth::user()->isSuperUser()) {
-                    $user->groups()->sync($request->input('groups'));
-                }
+                $user->groups()->sync($request->input('groups'));
 
-            // The groups field has been passed but it is null, so we should blank it out
-            } elseif ($request->has('groups')) {
-                
-                // Only save groups if the user is a superuser
-                if (Auth::user()->isSuperUser()) {
-                    $user->groups()->sync($request->input('groups'));
+                if ($validator->fails()) {
+                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
                 }
-            }
-
 
+            }
             return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
         }
 
         return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
     }

Vulnerable code:             // Strip out the superuser permission if the API user isn't a superadmin
            // Check if the request has groups passed and has a value
            if ($request->filled('groups')) {
                
                if ($validator->fails()){
                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
                }
                // Only save groups if the user is a superuser
                if (Auth::user()->isSuperUser()) {
                    $user->groups()->sync($request->input('groups'));
                }
            // The groups field has been passed but it is null, so we should blank it out
            } elseif ($request->has('groups')) {
                
                // Only save groups if the user is a superuser
                if (Auth::user()->isSuperUser()) {
                    $user->groups()->sync($request->input('groups'));
            }

Fixed code:             // Strip out the individual superuser permission if the API user isn't a superadmin
            // Check if the request has groups passed and has a value, AND that the user us a superuser
            if (($request->has('groups')) && (Auth::user()->isSuperUser())) {
                $user->groups()->sync($request->input('groups'));
                if ($validator->fails()) {
                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
            }