Skip to content

Commit

Permalink
*Permission.get_scopes: don't tolerate unknown actions (cvat-ai#8426)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
SpecLad authored and Bradley Schultz committed Sep 12, 2024
1 parent b749b68 commit 395bfeb
Show file tree
Hide file tree
Showing 9 changed files with 29 additions and 32 deletions.
2 changes: 1 addition & 1 deletion cvat/apps/analytics_report/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
31 changes: 13 additions & 18 deletions cvat/apps/engine/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion cvat/apps/events/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion cvat/apps/lambda_manager/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion cvat/apps/log_viewer/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions cvat/apps/organizations/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions cvat/apps/quality_control/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -158,7 +158,7 @@ def get_scopes(request, view, obj):
return [
{
"list": Scopes.LIST,
}.get(view.action, None)
}[view.action]
]

def get_resource(self):
Expand Down Expand Up @@ -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):
Expand Down
4 changes: 2 additions & 2 deletions cvat/apps/webhooks/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ 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:
webhook_type = request.data.get('type')
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
Expand Down
4 changes: 3 additions & 1 deletion cvat/apps/webhooks/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 395bfeb

Please sign in to comment.