feat(RiskAcc-Eng): Clean RiskAcc-Eng mapping + Allow correct handling by API

[kiblik]

This PR is trying to fix "not the best" implementation of mapping between Engagements and RiskAcc.
Until now, it was set as ManyToManyField in Engagements, and from RiskAcc, it was just using the first possible match. This implementation is eventually a one-to-many relation, which should be solved via ForeignKey in RiskAcc.

[kiblik]

@mtesauro & @Maffooch, this PR will need more work. But I prepared an initial commit to open discussion: Was there any specific reason why this was implemented as ManyToManyField? It just consumes (a bit) more resources and performance, + adds more operations. Have I missed sth?

[valentijnscholten]

I think the initial idea was that a Risk Acceptance could apply to more than just one engagement. For example accept the risk of a CVE that occurs in multiple branches. I think via the UI you cannot actually do this, but it might be possible via the API?
There's a risk we kill someone's current use case, or our own future use cases. All in all changing it to a 1-to-many relationship doesn't really provide huge savings or performance gains, so I think it should be left as is.

[kiblik]

I think the initial idea was that a Risk Acceptance could apply to more than just one engagement. For example accept the risk of a CVE that occurs in multiple branches. I think via the UI you cannot actually do this, but it might be possible via the API? There's a risk we kill someone's current use case, or our own future use cases. All in all changing it to a 1-to-many relationship doesn't really provide huge savings or performance gains, so I think it should be left as is.

API would not be broken; there is a checker correctly (see below). In fact, API needs to be fixed - you can create RiskAcc (via POST /api/v2/risk_acceptance/), but it is not possible to map it to Eng (there is a missing endpoint for ManyToManyField) so it is not possible to see it in UI (which is bug).
Ok, performance overhead is probably not that large.
But still, it is good to keep the code clean and mainly to be consistent.

• If we want to have RiskAcc, which applies across multiple Eng
  • We should list them on a different page (not in <dd>/engagement/{id})
    

    django-DefectDojo/dojo/templates/dojo/view_eng.html

    Line 407 in 477583e

    {% for risk_acceptance in risks_accepted %}

  • We should not limit grouping accepted findings only to one Eng
    

    django-DefectDojo/dojo/api_v2/serializers.py

    Lines 1589 to 1594 in 477583e

    	def validate(self, data):
    	def validate_findings_have_same_engagement(finding_objects: list[Finding]):
    	engagements = finding_objects.values_list("test__engagement__id", flat=True).distinct().count()
    	if engagements > 1:
    	msg = "You are not permitted to add findings from multiple engagements"
    	raise PermissionDenied(msg)

• If we want to have n:1 mapping between RiskAcc:Eng, we should do it properly (as I'm trying to offer in this PR).

I'm not strictly to one or another option. Both might make sense. It is the reason why I didn't continue with the implementation, but raised the question.
I incline towards n:1 on Risk:Prod level. But if there was a reason in history to design it towards another model, I would like to ask for reasoning, and we can add it to the documentation so it will be available to users as well.

Regarding breaking the current setup - I do not think so because we would just extend current abilities (but please correct me if I'm wrong). It might not be true about future use cases. But it is the reason why I'm asking about original intentions.

[kiblik]

@mtesauro & @Maffooch, can we have your opinion on which way we should move forward? Thank you.

[dryrunsecurity]

This pull request introduces potential risks in database migration and model relationships, including null reference scenarios, incomplete migration logic, unintended data deletion through CASCADE strategy, and changes to data relationships that could impact existing data tracking between Engagement and Risk_Acceptance models.

💭 Unconfirmed Findings (4)

Vulnerability	Potential Null Reference Risk
Description	In database migration file, null foreign key could lead to unexpected behavior and might cause null reference scenarios if engagement relationship is assumed to always exist.

Vulnerability	Placeholder Migration with No Actual Implementation
Description	In migration file 0230, contains an empty function with no implementation, suggesting incomplete migration logic that might not perform intended data transformations.

Vulnerability	Potential Unintended Data Deletion Risk
Description	In migration file 0231, uses Django's CASCADE deletion strategy which means if an engagement is deleted, all associated risk acceptances will be automatically deleted, potentially causing unintended data loss.

Vulnerability	Potential Data Integrity Risk in Relationship Modeling
Description	In models.py, changes database relationship between Engagement and Risk_Acceptance models by removing ManyToManyField and adding direct ForeignKey, which could impact existing data relationships and cause challenges in tracking risk acceptances with engagements.

---

All finding details can be found in the DryRun Security Dashboard.

[Maffooch]

Hi @kiblik, thank you for pausing on this feature before continuing any further. Risk Exceptions are one of those features that is used a little differently by everyone. As Val pointed out, we don't want to break any current use cases, or point ourselves into a corner for future use cases. Minimizing changes to the risk exception object would be the best move we could make at this time

[kiblik]

Hi @kiblik, thank you for pausing on this feature before continuing any further. Risk Exceptions are one of those features that is used a little differently by everyone. As Val pointed out, we don't want to break any current use cases, or point ourselves into a corner for future use cases. Minimizing changes to the risk exception object would be the best move we could make at this time

Thank you @Maffooch. I will prepare the implementation, which will:

• Behave the same as you can experience in UI
• Enable full management via API (it is not possible right now)
• Simplify data structure
• Keep the door open to change it to "less strict" implementation in the future

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[vini-ppro]

Hi all, I am the author of the bug that triggered this discussion.

@valentijnscholten - as for your question "it might be possible via the API?" I tried that, but I got an error that is not possible to have findings from different engagements under the same risk acceptance, so there is a validation somewhere.

As for the general use case for Risk Acceptances. It is quite common for us to accept the risk for a certain CVE across the whole business. In our view, it should be possible to have a single Risk Acceptance comprising many findings across many engagements. I would like to think this is a very common requirement for many companies

Because it is not possible to have a single Risk Acceptance across engagements, we had to implement workarounds. We created a script that creates one Risk Acceptance per finding. It could happen that we create more than 1k Risk Acceptances for a single CVE, this is not ideal, we may clog the DB at some point.

[kiblik]

Hi all, I am the author of the bug that triggered this discussion.

@valentijnscholten - as for your question "it might be possible via the API?" I tried that, but I got an error that is not possible to have findings from different engagements under the same risk acceptance, so there is a validation somewhere.

As for the general use case for Risk Acceptances. It is quite common for us to accept the risk for a certain CVE across the whole business. In our view, it should be possible to have a single Risk Acceptance comprising many findings across many engagements. I would like to think this is a very common requirement for many companies

Because it is not possible to have a single Risk Acceptance across engagements, we had to implement workarounds. We created a script that creates one Risk Acceptance per finding. It could happen that we create more than 1k Risk Acceptances for a single CVE, this is not ideal, we may clog the DB at some point.

I hope you are not the author of the bug, but only the author of the bug report 😄

You are writing at the best time. I almost finished this PR (only one validation statement and a fix of unittests are missing). I wrote it in a way that reflects current behaviour (allows mapping RiskAcc only to engagements), but correctly handled in case of API processing.
I went to this road because "we don't want to break any current use cases, or point ourselves into a corner for future use cases. Minimizing changes to the risk exception object would be the best move we could make at this time".

On the other hand, I agree that if there is no specific use case (and I do not know it), I suppose it might be a much better idea to move RiskAcc from the Engagement level to the Product level. Cross-product level would not be the best because I'm not sure how we would handle permissions there (e.g., Can I see all RAs? Because if I want to add my findings to some pre-created RA, I need to be able to see it).

@Maffooch, @valentijnscholten, @mtesauro, can I ask you for reconsideration? Change from the Engagement level to the Product level would not break any correct implementation; it would just open the door for a more flexible definition/handling.

[Arthur-DTAG]

We also have this use case:

... it should be possible to have a single Risk Acceptance comprising many findings across many engagements...

E.g. you track multiple versions of a product in different engagement (within the same product or multiple). Then a new CVE gets reported. You investigate and see that it has no effect to you. Therefor We would like to accept it for all installed Versions (so multiple engagements) as single Risk Acceptance since it has been evaluated together. Depending on the Application/System, this can affect many CVE (e.g. because it is not exposed).

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[kiblik]

@Maffooch, @valentijnscholten, @mtesauro, can I ask you for reconsideration? Change from the Engagement level to the Product level would not break any correct implementation; it would just open the door for a more flexible definition/handling.

@Maffooch, @valentijnscholten, @mtesauro, can I know your opinion?

[dryrunsecurity]

This pull request introduces three potential security vulnerabilities: an authorization bypass risk in risk acceptance functions, an information disclosure issue through API filtering of 'notes' fields, and an information leakage vulnerability in validation error handling that exposes internal finding IDs.

Authorization Bypass via Sole Decorator Reliance in dojo/engagement/views.py

Vulnerability

Authorization Bypass via Sole Decorator Reliance

Description

The expire_risk_acceptance, reinstate_risk_acceptance, and delete_risk_acceptance functions rely exclusively on the @user_is_authorized decorator for access control. There are no additional in-function authorization checks. This single point of failure means that if the decorator is bypassed, misconfigured, or contains a vulnerability, an unauthorized user could potentially perform these sensitive operations.

django-DefectDojo/dojo/engagement/views.py

Lines 1433 to 1464 in 12930cb

	})
	@user_is_authorized(Risk_Acceptance, Permissions.Risk_Acceptance, "raid")
	def expire_risk_acceptance(request, raid):
	risk_acceptance = get_object_or_404(prefetch_for_expiration(Risk_Acceptance.objects.all()), pk=raid)
	ra_helper.expire_now(risk_acceptance)
	return redirect_to_return_url_or_else(request, reverse("view_risk_acceptance", args=(raid, )))
	@user_is_authorized(Risk_Acceptance, Permissions.Risk_Acceptance, "raid")
	def reinstate_risk_acceptance(request, raid):
	risk_acceptance = get_object_or_404(prefetch_for_expiration(Risk_Acceptance.objects.all()), pk=raid)
	eng = risk_acceptance.engagement
	if not eng.product.enable_full_risk_acceptance:
	raise PermissionDenied
	ra_helper.reinstate(risk_acceptance, risk_acceptance.expiration_date)
	return redirect_to_return_url_or_else(request, reverse("view_risk_acceptance", args=(raid, )))
	@user_is_authorized(Risk_Acceptance, Permissions.Risk_Acceptance, "raid")
	def delete_risk_acceptance(request, raid):
	risk_acceptance = get_object_or_404(Risk_Acceptance, pk=raid)
	eng = risk_acceptance.engagement
	ra_helper.delete(eng, risk_acceptance)

Information Disclosure via 'notes' field exposure in dojo/filters.py

Vulnerability

Information Disclosure via 'notes' field exposure

Description

The patch adds the 'notes' field to the RiskAcceptanceFilter, making it filterable via the API. This, combined with the API view prefetching the 'notes' field, creates a potential for information disclosure. Even with object-level permissions, a user could use the filter to infer sensitive information from notes of risk acceptances they are not authorized to directly view, by observing filter results (blind information disclosure).

django-DefectDojo/dojo/filters.py

Lines 2800 to 2806 in 12930cb

	"name", "accepted_findings", "recommendation", "recommendation_details",
	"decision", "decision_details", "accepted_by", "owner", "expiration_date",
	"expiration_date_warned", "expiration_date_handled", "reactivate_expired",
	"restart_sla_expired", "notes", "engagement",
	]

Information Disclosure via ValidationError in dojo/models.py

Vulnerability

Information Disclosure via ValidationError

Description

The Risk_Acceptance.clean() method and the validate_findings_engagement helper function (called by RiskAcceptanceSerializer) raise a ValidationError that includes the internal database IDs of 'Finding' objects that do not belong to the specified engagement. When this validation error is triggered via the API, these internal IDs are exposed in the API response. This allows an attacker to confirm the existence and internal IDs of findings they may not otherwise be authorized to view, leading to information disclosure.

django-DefectDojo/dojo/models.py

Lines 3685 to 3700 in 12930cb

	def __str__(self):
	return str(self.name)
	def clean(self):
	super().clean()
	if self.pk:
	# Get all findings that do NOT belong to this engagement
	problematic_findings = self.accepted_findings.exclude(test__engagement=self.engagement)
	if problematic_findings.exists():
	problematic_ids = list(problematic_findings.values_list("id", flat=True))
	msg = f"Findings with IDs {problematic_ids} do not belong to engagement {self.engagement_id}."
	raise ValidationError(msg)
	def filename(self):
	# logger.debug('path: "%s"', self.path)
	if not self.path:

---

All finding details can be found in the DryRun Security Dashboard.