Merge commit from fork

* Fix security issue when anyone could obtain project|task|job|membership details

* Check additional permissions when trying to update task/project target/source storage

changelog.d/20240920_124259_maria_fix_patch_security_issue.md

@@ -0,0 +1,4 @@
+### Security
+
+- Fixed a security issue that occurred in PATCH requests to projects|tasks|jobs|memberships
+  (<https://github.com/cvat-ai/cvat/security/advisories/GHSA-gxhm-hg65-5gh2>)

cvat/apps/engine/permissions.py

@@ -210,6 +210,7 @@ class Scopes(StrEnum):
         UPDATE_ASSIGNEE = 'update:assignee'
         UPDATE_DESC = 'update:desc'
         UPDATE_ORG = 'update:organization'
+        UPDATE_ASSOCIATED_STORAGE = 'update:associated_storage'
         VIEW = 'view'
         IMPORT_DATASET = 'import:dataset'
         EXPORT_ANNOTATIONS = 'export:annotations'
@@ -285,6 +286,9 @@ def get_scopes(request, view, obj):
 
         scopes = []
         if scope == Scopes.UPDATE:
+            # user should have permissions to view a project
+            scopes.append(Scopes.VIEW)
+
             if any(k in request.data for k in ('owner_id', 'owner')):
                 owner_id = request.data.get('owner_id') or request.data.get('owner')
                 if owner_id != getattr(obj.owner, 'id', None):
@@ -299,6 +303,9 @@ def get_scopes(request, view, obj):
                     break
             if 'organization' in request.data:
                 scopes.append(Scopes.UPDATE_ORG)
+
+            if {'source_storage', 'target_storage'} & request.data.keys():
+                scopes.append(Scopes.UPDATE_ASSOCIATED_STORAGE)
         else:
             scopes.append(scope)
 
@@ -376,6 +383,7 @@ class Scopes(StrEnum):
         UPDATE_ASSIGNEE = 'update:assignee'
         UPDATE_PROJECT = 'update:project'
         UPDATE_OWNER = 'update:owner'
+        UPDATE_ASSOCIATED_STORAGE = 'update:associated_storage'
         DELETE = 'delete'
         VIEW_ANNOTATIONS = 'view:annotations'
         UPDATE_ANNOTATIONS = 'update:annotations'
@@ -509,6 +517,8 @@ def get_scopes(request, view, obj) -> List[Scopes]:
             scopes.append(scope)
 
         elif scope == Scopes.UPDATE:
+            # user should have permissions to view a task
+            scopes.append(Scopes.VIEW)
             if any(k in request.data for k in ('owner_id', 'owner')):
                 owner_id = request.data.get('owner_id') or request.data.get('owner')
                 if owner_id != getattr(obj.owner, 'id', None):
@@ -530,6 +540,9 @@ def get_scopes(request, view, obj) -> List[Scopes]:
             if request.data.get('organization'):
                 scopes.append(Scopes.UPDATE_ORGANIZATION)
 
+            if {'source_storage', 'target_storage'} & request.data.keys():
+                scopes.append(Scopes.UPDATE_ASSOCIATED_STORAGE)
+
         elif scope == Scopes.VIEW_ANNOTATIONS:
             if 'format' in request.query_params:
                 scope = Scopes.EXPORT_ANNOTATIONS
@@ -614,11 +627,8 @@ class Scopes(StrEnum):
         VIEW = 'view'
         UPDATE = 'update'
         UPDATE_ASSIGNEE = 'update:assignee'
-        UPDATE_OWNER = 'update:owner'
-        UPDATE_PROJECT = 'update:project'
         UPDATE_STAGE = 'update:stage'
         UPDATE_STATE = 'update:state'
-        UPDATE_DESC = 'update:desc'
         DELETE = 'delete'
         VIEW_ANNOTATIONS = 'view:annotations'
         UPDATE_ANNOTATIONS = 'update:annotations'
@@ -728,25 +738,19 @@ def get_scopes(request, view, obj):
 
         scopes = []
         if scope == Scopes.UPDATE:
-            if any(k in request.data for k in ('owner_id', 'owner')):
-                owner_id = request.data.get('owner_id') or request.data.get('owner')
-                if owner_id != getattr(obj.owner, 'id', None):
-                    scopes.append(Scopes.UPDATE_OWNER)
+            # user should have permissions to view a job
+            scopes.append(Scopes.VIEW)
+
             if any(k in request.data for k in ('assignee_id', 'assignee')):
                 assignee_id = request.data.get('assignee_id') or request.data.get('assignee')
                 if assignee_id != getattr(obj.assignee, 'id', None):
                     scopes.append(Scopes.UPDATE_ASSIGNEE)
-            if any(k in request.data for k in ('project_id', 'project')):
-                project_id = request.data.get('project_id') or request.data.get('project')
-                if project_id != getattr(obj.project, 'id', None):
-                    scopes.append(Scopes.UPDATE_PROJECT)
+
             if 'stage' in request.data:
                 scopes.append(Scopes.UPDATE_STAGE)
             if 'state' in request.data:
                 scopes.append(Scopes.UPDATE_STATE)
 
-            if any(k in request.data for k in ('name', 'labels', 'bug_tracker', 'subset')):
-                scopes.append(Scopes.UPDATE_DESC)
         elif scope == Scopes.VIEW_ANNOTATIONS:
             if 'format' in request.query_params:
                 scope = Scopes.EXPORT_ANNOTATIONS

cvat/apps/engine/rules/projects.rego

@@ -7,7 +7,7 @@ import data.organizations
 
 # input: {
 #     "scope": <"create"|"list"|"update:desc"|"update:owner"|"update:assignee"|
-#               "view"|"delete"|"export:dataset"|"export:annotations"|
+#               "update:associated_storage"|"view"|"delete"|"export:dataset"|"export:annotations"|
 #               "import:dataset"> or null,
 #     "auth": {
 #         "user": {
@@ -127,22 +127,22 @@ allow if {
 
 
 allow if {
-    input.scope in {utils.DELETE, utils.UPDATE_ORG}
+    input.scope in {utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE}
     utils.is_sandbox
     utils.has_perm(utils.WORKER)
     utils.is_resource_owner
 }
 
 allow if {
-    input.scope in {utils.DELETE, utils.UPDATE_ORG}
+    input.scope in {utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE}
     input.auth.organization.id == input.resource.organization.id
     utils.has_perm(utils.WORKER)
     organizations.is_member
     utils.is_resource_owner
 }
 
 allow if {
-    input.scope in {utils.DELETE, utils.UPDATE_ORG}
+    input.scope in {utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE}
     input.auth.organization.id == input.resource.organization.id
     utils.has_perm(utils.USER)
     organizations.is_staff

cvat/apps/engine/rules/tasks.rego

@@ -7,8 +7,8 @@ import data.organizations
 
 # input: {
 #     "scope": <"create"|"create@project"|"view"|"list"|"update:desc"|
-#         "update:owner"|"update:assignee"|"update:project"|"delete"|
-#         "view:annotations"|"update:annotations"|"delete:annotations"|
+#         "update:owner"|"update:assignee"|"update:project"|"update:associated_storage"|
+#         "delete"|"view:annotations"|"update:annotations"|"delete:annotations"|
 #         "export:dataset"|"view:data"|"upload:data"|"export:annotations"> or null,
 #     "auth": {
 #         "user": {
@@ -250,10 +250,17 @@ allow if {
     utils.has_perm(utils.WORKER)
 }
 
+allow if {
+    input.scope == utils.UPDATE_ASSOCIATED_STORAGE
+    utils.is_sandbox
+    is_project_owner
+    utils.has_perm(utils.WORKER)
+}
+
 allow if {
     input.scope in {
         utils.UPDATE_OWNER, utils.UPDATE_ASSIGNEE, utils.UPDATE_PROJECT,
-        utils.DELETE, utils.UPDATE_ORG
+        utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE
     }
     utils.is_sandbox
     is_task_owner
@@ -263,7 +270,7 @@ allow if {
 allow if {
     input.scope in {
         utils.UPDATE_OWNER, utils.UPDATE_ASSIGNEE, utils.UPDATE_PROJECT,
-        utils.DELETE, utils.UPDATE_ORG
+        utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE
     }
     input.auth.organization.id == input.resource.organization.id
     utils.has_perm(utils.USER)
@@ -273,13 +280,20 @@ allow if {
 allow if {
     input.scope in {
         utils.UPDATE_OWNER, utils.UPDATE_ASSIGNEE, utils.UPDATE_PROJECT,
-        utils.DELETE, utils.UPDATE_ORG
+        utils.DELETE, utils.UPDATE_ORG, utils.UPDATE_ASSOCIATED_STORAGE
     }
     input.auth.organization.id == input.resource.organization.id
     utils.has_perm(utils.WORKER)
     organizations.has_perm(organizations.WORKER)
     is_task_owner
 }
+allow if {
+    input.scope == utils.UPDATE_ASSOCIATED_STORAGE
+    input.auth.organization.id == input.resource.organization.id
+    utils.has_perm(utils.WORKER)
+    organizations.has_perm(organizations.WORKER)
+    is_project_owner
+}
 
 allow if {
     input.scope in {

cvat/apps/engine/rules/tests/configs/projects.csv

@@ -43,4 +43,9 @@ export:backup,Project,Organization,None,,GET,/projects/{id}/backup,User,Maintain
 update:organization,"Project, Organization",Sandbox,"None, Assignee",,PATCH,/projects/{id},Admin,N/A
 update:organization,"Project, Organization",Sandbox,Owner,,PATCH,/projects/{id},Worker,N/A
 update:organization,"Project, Organization",Organization,"None, Assignee",,PATCH,/projects/{id},User,Maintainer
-update:organization,"Project, Organization",Organization,Owner,,PATCH,/projects/{id},Worker,Worker
+update:organization,"Project, Organization",Organization,Owner,,PATCH,/projects/{id},Worker,Worker
+update:associated_storage,Project,Sandbox,None,,PATCH,/projects/{id},Admin,N/A
+update:associated_storage,Project,Sandbox,Owner,,PATCH,/projects/{id},Worker,N/A
+update:associated_storage,Project,Organization,None,,PATCH,/projects/{id},Admin,N/A
+update:associated_storage,Project,Organization,"None, Assignee",,PATCH,/projects/{id},User,Maintainer
+update:associated_storage,Project,Organization,Owner,,PATCH,/projects/{id},Worker,Worker

cvat/apps/engine/rules/tests/configs/tasks.csv

@@ -29,6 +29,11 @@ update:project,"Task, Project",Sandbox,"None, Assignee",,PATCH,/tasks/{id},Admin
 update:project,"Task, Project",Sandbox,"Owner, Project:owner, Project:assignee",,PATCH,/tasks/{id},Worker,N/A
 update:project,"Task, Project",Organization,"None, Assignee",,PATCH,/tasks/{id},User,Maintainer
 update:project,"Task, Project",Organization,"Owner, Project:owner, Project:assignee",,PATCH,/tasks/{id},Worker,Worker
+update:associated_storage,Task,Sandbox,None,,PATCH,/tasks/{id},Admin,N/A
+update:associated_storage,Task,Sandbox,"Owner, Project:owner",,PATCH,/tasks/{id},Worker,N/A
+update:associated_storage,Task,Organization,None,,PATCH,/tasks/{id},Admin,N/A
+update:associated_storage,Task,Organization,"None, Assignee, Project:assignee",,PATCH,/tasks/{id},User,Maintainer
+update:associated_storage,Task,Organization,"Owner, Project:owner",,PATCH,/tasks/{id},Worker,Worker
 delete,Task,Sandbox,"None, Assignee",,DELETE,/tasks/{id},Admin,N/A
 delete,Task,Sandbox,"Owner, Project:owner, Project:assignee",,DELETE,/tasks/{id},Worker,N/A
 delete,Task,Organization,"None, Assignee",,DELETE,/tasks/{id},User,Maintainer

cvat/apps/iam/rules/utils.rego

@@ -36,6 +36,7 @@ RESEND := "resend"
 UPDATE_DESC := "update:desc"
 UPDATE_ASSIGNEE := "update:assignee"
 UPDATE_OWNER := "update:owner"
+UPDATE_ASSOCIATED_STORAGE := "update:associated_storage"
 EXPORT_ANNOTATIONS := "export:annotations"
 EXPORT_DATASET := "export:dataset"
 CREATE_IN_PROJECT := "create@project"

cvat/apps/organizations/permissions.py

@@ -3,8 +3,6 @@
 #
 # SPDX-License-Identifier: MIT
 
-from typing import cast
-
 from django.conf import settings
 
 from cvat.apps.iam.permissions import OpenPolicyAgentPermission, StrEnum
@@ -175,7 +173,10 @@ def get_scopes(request, view, obj):
         }[view.action]
 
         if scope == Scopes.UPDATE:
-            if request.data.get('role') != cast(Membership, obj).role:
+            # user should have permissions to view a membership
+            scopes.append(Scopes.VIEW)
+
+            if 'role' in request.data.keys():
                 scopes.append(Scopes.UPDATE_ROLE)
         else:
             scopes.append(scope)

tests/python/rest_api/test_jobs.py

@@ -18,6 +18,7 @@
 import pytest
 from cvat_sdk import models
 from cvat_sdk.api_client.api_client import ApiClient, Endpoint
+from cvat_sdk.api_client.exceptions import ForbiddenException
 from cvat_sdk.core.helpers import get_paginated_collection
 from deepdiff import DeepDiff
 from PIL import Image
@@ -1275,6 +1276,15 @@ def test_can_update_assignee_updated_date_on_assignee_updates(
             else:
                 assert updated_job.assignee is None
 
+    def test_malefactor_cannot_obtain_job_details_via_empty_partial_update_request(
+        self, regular_lonely_user, jobs
+    ):
+        job = next(iter(jobs))
+
+        with make_api_client(regular_lonely_user) as api_client:
+            with pytest.raises(ForbiddenException):
+                api_client.jobs_api.partial_update(job["id"])
+
 
 def _check_coco_job_annotations(content, values_to_be_checked):
     exported_annotations = json.loads(content)

tests/python/rest_api/test_memberships.py

@@ -8,6 +8,7 @@
 
 import pytest
 from cvat_sdk.api_client.api_client import ApiClient, Endpoint
+from cvat_sdk.api_client.exceptions import ForbiddenException
 from deepdiff import DeepDiff
 
 from shared.utils.config import get_method, make_api_client, patch_method
@@ -137,6 +138,15 @@ def test_user_cannot_change_self_role(self, who: str, find_users):
             user["username"], user["membership_id"], self.ROLES[abs(self.ROLES.index(who) - 1)]
         )
 
+    def test_malefactor_cannot_obtain_membership_details_via_empty_partial_update_request(
+        self, regular_lonely_user, memberships
+    ):
+        membership = next(iter(memberships))
+
+        with make_api_client(regular_lonely_user) as api_client:
+            with pytest.raises(ForbiddenException):
+                api_client.memberships_api.partial_update(membership["id"])
+
 
 @pytest.mark.usefixtures("restore_db_per_function")
 class TestDeleteMemberships:

tests/python/rest_api/test_projects.py

@@ -1398,3 +1398,12 @@ def test_can_update_assignee_updated_date_on_assignee_updates(
                 assert updated_project.assignee.id == new_assignee_id
             else:
                 assert updated_project.assignee is None
+
+    def test_malefactor_cannot_obtain_project_details_via_empty_partial_update_request(
+        self, regular_lonely_user, projects
+    ):
+        project = next(iter(projects))
+
+        with make_api_client(regular_lonely_user) as api_client:
+            with pytest.raises(ForbiddenException):
+                api_client.projects_api.partial_update(project["id"])

tests/python/rest_api/test_tasks.py

@@ -24,6 +24,7 @@
 from cvat_sdk import Client, Config, exceptions
 from cvat_sdk.api_client import models
 from cvat_sdk.api_client.api_client import ApiClient, ApiException, Endpoint
+from cvat_sdk.api_client.exceptions import ForbiddenException
 from cvat_sdk.core.helpers import get_paginated_collection
 from cvat_sdk.core.progress import NullProgressReporter
 from cvat_sdk.core.proxies.tasks import ResourceType, Task
@@ -2778,6 +2779,15 @@ def test_user_cannot_update_task_with_cloud_storage_without_access(
             )
         assert response.status == HTTPStatus.FORBIDDEN
 
+    def test_malefactor_cannot_obtain_task_details_via_empty_partial_update_request(
+        self, regular_lonely_user, tasks
+    ):
+        task = next(iter(tasks))
+
+        with make_api_client(regular_lonely_user) as api_client:
+            with pytest.raises(ForbiddenException):
+                api_client.tasks_api.partial_update(task["id"])
+
     @pytest.mark.parametrize("has_old_assignee", [False, True])
     @pytest.mark.parametrize("new_assignee", [None, "same", "different"])
     def test_can_update_assignee_updated_date_on_assignee_updates(