Skip to content

Commit

Permalink
*Permission.get_scopes: don't tolerate unknown actions
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 committed Sep 10, 2024
1 parent 0fafb79 commit c95fa3f
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 c95fa3f

Please sign in to comment.