Refactor engagement and risk acceptance permissions #14155
Conversation
|
This pull request introduces object-level authorization gaps allowing any authenticated user to manipulate and view sensitive findings and questionnaire answers: the set_finding_as_original action bypasses proper permission checks enabling IDOR (an attacker can mark arbitrary findings as duplicates within a product), and the QuestionnaireAnswerViewSet exposes all answers to any authenticated user by returning Answer.objects.all() without per-user filtering.
Insecure Direct Object Reference (IDOR) in
|
| Vulnerability | Insecure Direct Object Reference (IDOR) |
|---|---|
| Description | The set_finding_as_original action in FindingViewSet is vulnerable to IDOR (Broken Object Level Authorization). The action is decorated with UserHasFindingRelatedObjectPermission, which relies on object-level permission checks performed during self.get_object() or self.check_object_permissions(). However, the action fails to call either of these methods. Instead, it directly calls set_finding_as_original_internal, which uses get_object_or_404 to retrieve findings without any authorization check beyond ensuring they belong to the same product. Since UserHasFindingRelatedObjectPermission always returns True for the general has_permission check, any authenticated user can call this endpoint to manipulate findings across the entire system, even if they have no permissions for the relevant product. Specifically, an attacker can mark any finding as a duplicate of another in the same product, which results in the target finding being deactivated and closed. |
django-DefectDojo/dojo/api_v2/views.py
Lines 1324 to 1325 in a8fdafe
Broken Object Level Authorization (BOLA) in Questionnaire Answer ViewSet in dojo/api_v2/views.py
| Vulnerability | Broken Object Level Authorization (BOLA) in Questionnaire Answer ViewSet |
|---|---|
| Description | The QuestionnaireAnswerViewSet (and other questionnaire-related viewsets) allows any authenticated user to list all answers in the database. This is because the viewset's get_queryset method returns all objects (Answer.objects.all()) without any user-based filtering. While it uses DjangoModelPermissions and UserHasEngagementRelatedObjectPermission, these do not effectively restrict GET requests for the list action: DjangoModelPermissions allows GET by default for authenticated users, and UserHasEngagementRelatedObjectPermission's has_permission method explicitly returns True. This allows any user to access sensitive security questionnaire answers across the entire system, regardless of their assigned products or engagements. |
django-DefectDojo/dojo/api_v2/views.py
Lines 3163 to 3169 in a8fdafe
All finding details can be found in the DryRun Security Dashboard.
…ermission, UserHasRegulationPermission, and UserHasSLAPermission; update views accordingly
…on for unsupported request methods
Maffooch
requested review from
Jino-T,
blakeaowens,
dogboat and
valentijnscholten
dojo/api_v2/views.py
Outdated
| ) | ||
| @action(detail=True, methods=["get"]) | ||
| def download_proof(self, request, pk=None): | ||
| def download_proof(self, request, pk=None, permission_classes=(IsAuthenticated, permissions.UserHasRiskAcceptanceRelatedObjectPermission)): |
There was a problem hiding this comment.
Is there are reason why the permission_classes are passed as a parameter instead of in the decorator? Are they picked up like this?
There was a problem hiding this comment.
Good catch, this is supposed to be in the action decorator
valentijnscholten
left a comment
valentijnscholten
left a comment
There was a problem hiding this comment.
See comment. Also as far as I can see there are no tests covering this new mechanism? Would be good to at least have one to prove it's working. Would this be something you could add and then release in 2.55.0 in the dev branch?
Agreed - the passing of existing DRF tests felt like enough, but I will create some more tests to cover these new classes explicitly |
…ing permission_classes to the decorator
|
@valentijnscholten I have added new tests to cover the API endpoints that use these new base classes. This approach felt like a two-for-one deal to get more endpoint coverage while exercising the base classes do what the intend |
Introduce a base class for related object permissions to streamline permission checks for engagements, risk acceptances, and findings. Update existing permission classes to utilize the new structure, enhancing code maintainability and clarity.