From 395bfebe3fa4a38fe40a4d6c8841d4922bbf76fc Mon Sep 17 00:00:00 2001 From: Roman Donchenko Date: Tue, 10 Sep 2024 17:24:52 +0300 Subject: [PATCH] `*Permission.get_scopes`: don't tolerate unknown actions (#8426) With almost all of the `get_scopes` methods, an unknown (action, method) combination will result in an array like `[None]` being returned (sometimes with other elements as well). If that happens, the OPA input will then have `"scope": null`, and so the policy evaluation will fail, unless the user is an admin. Because of this, it's really easy to accidentally make a view admin-only, by forgetting to add/update an entry in `get_scopes` when making changes. `TaskPermission`, `MembershipPermission` and `WebhookPermission` are even worse, because they will just return an empty list of scopes, which will later translate to an empty list of permissions, which means that everyone will be permitted to perform the action. This can lead to vulnerabilities like CVE-2024-45393. Fix this by replacing all `.get` calls with indexing, which will cause a crash if the (action, method) combo is unknown. This breaks one endpoint (`/api/webhooks/events`), which is supposed to be publicly accessible; fix that by disabling authorization for it. --- cvat/apps/analytics_report/permissions.py | 2 +- cvat/apps/engine/permissions.py | 31 ++++++++++------------- cvat/apps/events/permissions.py | 2 +- cvat/apps/lambda_manager/permissions.py | 2 +- cvat/apps/log_viewer/permissions.py | 2 +- cvat/apps/organizations/permissions.py | 8 +++--- cvat/apps/quality_control/permissions.py | 6 ++--- cvat/apps/webhooks/permissions.py | 4 +-- cvat/apps/webhooks/views.py | 4 ++- 9 files changed, 29 insertions(+), 32 deletions(-) diff --git a/cvat/apps/analytics_report/permissions.py b/cvat/apps/analytics_report/permissions.py index e6177b1d4df8..d93418398902 100644 --- a/cvat/apps/analytics_report/permissions.py +++ b/cvat/apps/analytics_report/permissions.py @@ -70,7 +70,7 @@ def get_scopes(request, view, obj): { "list": Scopes.LIST, "create": Scopes.CREATE, - }.get(view.action, None) + }[view.action] ] def get_resource(self): diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index 92d1b107b747..70c147a4661f 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -65,7 +65,7 @@ def get_scopes(request, view, obj): ('about', 'GET'): Scopes.VIEW, ('plugins', 'GET'): Scopes.VIEW, ('share', 'GET'): Scopes.LIST_CONTENT, - }.get((view.action, request.method))] + }[(view.action, request.method)]] def get_resource(self): return None @@ -100,7 +100,7 @@ def get_scopes(request, view, obj): 'retrieve': Scopes.VIEW, 'partial_update': Scopes.UPDATE, 'destroy': Scopes.DELETE, - }.get(view.action)] + }[view.action]] @classmethod def create_scope_view(cls, iam_context, user_id): @@ -178,7 +178,7 @@ def get_scopes(request, view, obj): 'preview': Scopes.VIEW, 'status': Scopes.VIEW, 'actions': Scopes.VIEW, - }.get(view.action)] + }[view.action]] def get_resource(self): data = None @@ -281,7 +281,7 @@ def get_scopes(request, view, obj): ('append_backup_chunk', 'PATCH'): Scopes.IMPORT_BACKUP, ('append_backup_chunk', 'HEAD'): Scopes.IMPORT_BACKUP, ('preview', 'GET'): Scopes.VIEW, - }.get((view.action, request.method)) + }[(view.action, request.method)] scopes = [] if scope == Scopes.UPDATE: @@ -498,7 +498,7 @@ def get_scopes(request, view, obj) -> List[Scopes]: ('export_backup', 'GET'): Scopes.EXPORT_BACKUP, ('export_backup_v2', 'POST'): Scopes.EXPORT_BACKUP, ('preview', 'GET'): Scopes.VIEW, - }.get((view.action, request.method)) + }[(view.action, request.method)] scopes = [] if scope == Scopes.CREATE: @@ -542,13 +542,8 @@ def get_scopes(request, view, obj) -> List[Scopes]: scopes.append(scope) - elif scope is not None: - scopes.append(scope) - else: - # TODO: think if we can protect from missing endpoints - # assert False, "Unknown scope" - pass + scopes.append(scope) return scopes @@ -729,7 +724,7 @@ def get_scopes(request, view, obj): ('dataset_export', 'GET'): Scopes.EXPORT_DATASET, ('export_dataset_v2', 'POST'): Scopes.EXPORT_DATASET if is_dataset_export(request) else Scopes.EXPORT_ANNOTATIONS, ('preview', 'GET'): Scopes.VIEW, - }.get((view.action, request.method)) + }[(view.action, request.method)] scopes = [] if scope == Scopes.UPDATE: @@ -849,7 +844,7 @@ def get_scopes(request, view, obj): 'destroy': Scopes.DELETE, 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): data = None @@ -941,7 +936,7 @@ def get_scopes(request, view, obj): 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, 'comments': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): data = None @@ -1065,7 +1060,7 @@ def get_scopes(request, view, obj): 'destroy': Scopes.DELETE, 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): data = None @@ -1127,7 +1122,7 @@ def get_scopes(request, view, obj): 'destroy': Scopes.DELETE, 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): data = {} @@ -1205,7 +1200,7 @@ def get_scopes(request, view, obj): 'create': Scopes.CREATE, 'destroy': Scopes.DELETE, 'retrieve': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] class RequestPermission(OpenPolicyAgentPermission): @@ -1237,7 +1232,7 @@ def get_scopes(request, view, obj) -> List[Scopes]: ('list', 'GET'): Scopes.LIST, ('retrieve', 'GET'): Scopes.VIEW, ('cancel', 'POST'): Scopes.CANCEL, - }.get((view.action, request.method))] + }[(view.action, request.method)]] def get_resource(self): diff --git a/cvat/apps/events/permissions.py b/cvat/apps/events/permissions.py index 6f8ef8c4735c..a1b049cbbd4b 100644 --- a/cvat/apps/events/permissions.py +++ b/cvat/apps/events/permissions.py @@ -50,7 +50,7 @@ def get_scopes(request, view, obj): return [{ ('create', 'POST'): Scopes.SEND_EVENTS, ('list', 'GET'): Scopes.DUMP_EVENTS, - }.get((view.action, request.method))] + }[(view.action, request.method)]] def get_resource(self): return None diff --git a/cvat/apps/lambda_manager/permissions.py b/cvat/apps/lambda_manager/permissions.py index e1f78ce58c49..94800f0edd5d 100644 --- a/cvat/apps/lambda_manager/permissions.py +++ b/cvat/apps/lambda_manager/permissions.py @@ -49,7 +49,7 @@ def get_scopes(request, view, obj): ('lambda_request', 'list'): Scopes.LIST_OFFLINE, ('lambda_request', 'retrieve'): Scopes.CALL_OFFLINE, ('lambda_request', 'destroy'): Scopes.CALL_OFFLINE, - }.get((view.basename, view.action), None)] + }[(view.basename, view.action)]] def get_resource(self): return None diff --git a/cvat/apps/log_viewer/permissions.py b/cvat/apps/log_viewer/permissions.py index cc92dd92c2eb..2d9500a29ea2 100644 --- a/cvat/apps/log_viewer/permissions.py +++ b/cvat/apps/log_viewer/permissions.py @@ -30,7 +30,7 @@ def get_scopes(request, view, obj): Scopes = __class__.Scopes return [{ 'list': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): return { diff --git a/cvat/apps/organizations/permissions.py b/cvat/apps/organizations/permissions.py index e0c3aa0b44df..3ee36fba1481 100644 --- a/cvat/apps/organizations/permissions.py +++ b/cvat/apps/organizations/permissions.py @@ -42,7 +42,7 @@ def get_scopes(request, view, obj): 'destroy': Scopes.DELETE, 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, - }.get(view.action, None)] + }[view.action]] def get_resource(self): if self.obj: @@ -109,7 +109,7 @@ def get_scopes(request, view, obj): 'accept': Scopes.ACCEPT, 'decline': Scopes.DECLINE, 'resend': Scopes.RESEND, - }.get(view.action)] + }[view.action]] def get_resource(self): data = None @@ -172,12 +172,12 @@ def get_scopes(request, view, obj): 'partial_update': Scopes.UPDATE, 'retrieve': Scopes.VIEW, 'destroy': Scopes.DELETE, - }.get(view.action) + }[view.action] if scope == Scopes.UPDATE: if request.data.get('role') != cast(Membership, obj).role: scopes.append(Scopes.UPDATE_ROLE) - elif scope: + else: scopes.append(scope) return scopes diff --git a/cvat/apps/quality_control/permissions.py b/cvat/apps/quality_control/permissions.py index 0396adb3c490..0060a4d10bbf 100644 --- a/cvat/apps/quality_control/permissions.py +++ b/cvat/apps/quality_control/permissions.py @@ -85,7 +85,7 @@ def get_scopes(request, view, obj): "create": Scopes.CREATE, "retrieve": Scopes.VIEW, "data": Scopes.VIEW, - }.get(view.action, None) + }[view.action] ] def get_resource(self): @@ -158,7 +158,7 @@ def get_scopes(request, view, obj): return [ { "list": Scopes.LIST, - }.get(view.action, None) + }[view.action] ] def get_resource(self): @@ -225,7 +225,7 @@ def get_scopes(request, view, obj): "list": Scopes.LIST, "retrieve": Scopes.VIEW, "partial_update": Scopes.UPDATE, - }.get(view.action, None) + }[view.action] ] def get_resource(self): diff --git a/cvat/apps/webhooks/permissions.py b/cvat/apps/webhooks/permissions.py index e350605116f3..e5d132c55de6 100644 --- a/cvat/apps/webhooks/permissions.py +++ b/cvat/apps/webhooks/permissions.py @@ -62,7 +62,7 @@ def get_scopes(request, view, obj): ('deliveries', 'GET'): Scopes.VIEW, ('retrieve_delivery', 'GET'): Scopes.VIEW, ('redelivery', 'POST'): Scopes.UPDATE, - }.get((view.action, request.method)) + }[(view.action, request.method)] scopes = [] if scope == Scopes.CREATE: @@ -70,7 +70,7 @@ def get_scopes(request, view, obj): if webhook_type in [m.value for m in WebhookTypeChoice]: scope = Scopes(str(scope) + f'@{webhook_type}') scopes.append(scope) - elif scope in [Scopes.UPDATE, Scopes.DELETE, Scopes.LIST, Scopes.VIEW]: + else: scopes.append(scope) return scopes diff --git a/cvat/apps/webhooks/views.py b/cvat/apps/webhooks/views.py index 31de7a85d6af..66529bc6a7bd 100644 --- a/cvat/apps/webhooks/views.py +++ b/cvat/apps/webhooks/views.py @@ -109,7 +109,9 @@ def perform_create(self, serializer): ], responses={"200": OpenApiResponse(EventsSerializer)}, ) - @action(detail=False, methods=["GET"], serializer_class=EventsSerializer) + @action(detail=False, methods=["GET"], serializer_class=EventsSerializer, + permission_classes=[], + ) def events(self, request): webhook_type = request.query_params.get("type", "all") events = None