fixes reimporter to ensure that risk accepted findings do not get mitigated

[lme-nca]

This tries to improve on the (incomplete) fix introduced here: https://github.com/DefectDojo/django-DefectDojo/pull/7447 Also see the discussion on Slack: https://owasp.slack.com/archives/C014H3ZV9U6/p1700819414409409 and related issue https://github.com/DefectDojo/django-DefectDojo/issues/8797

Previous behaviour:

IF existing findings (false positive, out of scope or risk accepted): - IGNORE ALL OTHER values of findings from scanner (even if its mitigated) so mitigated findings from scanner that are not mitigated in Defectdojo (but do have one of the above flags would never have been closed)

• IF existingFinding.false_p == new_finding.false_p and existingFinding.out_of_scope == new_finding.out_of_scope and existingFinding.risk_accepted == new_finding.risk_accepted) - Then: unchanged_items.append(finding) - comes from(https://github.com/DefectDojo/django-DefectDojo/pull/7447)

• ELSE Mitigate the finding incorrectly <--- THE BUG described in this ISSUE: -- caused by the fact the finding is not added to "unchanged_items.append(finding)" -- Therefore the following code adds it to the mitigation list to_mitigate = ( set(original_items) - set(reactivated_items) - set(unchanged_items) )

elIF existing finding IS mitigated ..... // not changed

else (existing finding is not mitigated) - IF new_finding: false positive, out of scope or risk accepted -- Close existing findings and take over values of scanner

New behaviour

First IF condition is completely removed, no special treatment needed for this case --> if existing findings (false positive, out of scope or risk accepted):

IF existing finding IS mitigated ..... // not changed

else (existing finding is not mitigated)

• simplified logic (removed unnecessary If statement) -- if not (finding.mitigated and finding.is_mitigated): this always resolves to TRUE in this else branch!!! -- this changed the indenting here

• IF existingFinding.false_p == new_finding.false_p and existingFinding.out_of_scope == new_finding.out_of_scope and existingFinding.risk_accepted == new_finding.risk_accepted) - Then dont touch the existing finding, this keeps (https://github.com/DefectDojo/django-DefectDojo/pull/7447) alive, however question is if we want this like that ?

• IF new_finding: false positive, out of scope or risk accepted -- Close existing findings and take over values of scanner

• ELSE Do not touch the finding

Overall this is some of the most complicated and unreadable code I have seen so-far in Defectdojo. I would gladly refactor this complete method ( it is way to long and the variable names are terrible), however I dont know if this would fit with the whole Defectdojo 3.0 work ? @mtesauro @Maffooch Let me know if you are open for this and I would gladly work on making this code more digestible.

@coheigea @Gby56 you also both worked on this code, I am open to any suggesting or improvements ?

[dryrunsecurity[bot]]

Contextual Security Analysis

As DryRun Security performs checks, we’ll summarize them here. You can always dive into the results in the section below for checks.

Status	DryRun Security Check
❌	Configured Sensitive Files Check
✅	AI-powered Sensitive Files Check

Chat with your AI-powered Security Buddy by typing /dryrunsec: (or /drs:) followed by your question. Example: /dryrunsec: From a security perspective, what are some sensitive files in an Express application?

Install and configure more repositories at DryRun Security

[Gby56]

I'm reading through the code changes, so far it makes sense to me, it removes a lot of unnecessary conditions I think

It made me think about the decision matrix overall, given the existing finding and the new uploaded item, there is probably a table looking like that that we should define no ? Started filling it, it helps thinking of all the possible cases, I think it's useful tbh. I'm just not sure if we can consider all the attributes for the parsed item, lots of parsers won't have the notion of risk accepted for sure

[lme-nca]

So one of the failing tests "test_import_reimport_keep_false_positive_and_out_of_scope" is interesting. Here it becomes obvious there was some kind of "special" treatment going on for existing_findings for which finding.false_p or finding.out_of_scope or finding.risk_accepted

This kind of worked in the cases of finding.false_p or finding.out_of_scope as these status also set "is_mitigated" to true, therefore they never ran into the problem described in the issue https://github.com/DefectDojo/django-DefectDojo/issues/8797 Risk_accepted keeps "is_mitigated" = False And therefore got set to is_mitigated=True incorrectly. (again this test did not find it because it manually sets is_mitigated=True with risk acceptance)

However, I always assumed it worked like this:

if "do_not_reactivate" == TRUE --> Then Defectdojo is source of truth and Scanner cannot reopen findings. (regardless of false positive, out of scope, risk acceptance) if "do_not_reactivate" == FALSE --> Then Scanner is source of truth and reopens/closes findings (regardless of false positive, out of scope, risk acceptance)

"do_not_reactivate" was introduced here: https://github.com/DefectDojo/django-DefectDojo/pull/6893 but as I read was mostly intended to work-around "triage-less scanners"

@Gby56 I agree that if we could define a complete decision table that this would make it easier.

[Gby56]

Good catch with the tests yes, and definitely it's the special case of risk acceptance that highlighted this unclear decision matrix (and poor code reability, but we already knew about that 😄 )

Maybe do_not_reactivate could be renamed to an even more broad term afterwards, I agree. It definitely has its use so I'm glad I implemented it, but the rest of the logic is so hard to predict/understand, it would even be a good idea to integrate an audit trace to really explain each change of a finding's attributes (so far I think traces are a bit everywhere, in the finding history, or in notes, but not in clear details)

So my guess, for risk accepted, is:

• if it's RA in dojo, and upload has no RA in the item, don't touch anything
• if it's RA in dojo, and the upload has RA=true in the item... default says to take the scanner input, otherwise, use the do_not_reactivate ?
• if it's not RA in dojo, take the value from the parser item obviously, if there is one

This is pretty much the same treatment as false positive I'd say, but don't touch the is_mitigated yep

[lme-nca]

The problem here is this code

https://github.com/DefectDojo/django-DefectDojo/pull/9050/files#diff-bddc671a2a14e2e86ce08a054df3119895196fa892d84ab71fdd8ca1b50caadaR259

Currently the default behaviour is to just "mitigate" a finding if the scanner sets risk acceptance (or false_p or out of scope). It even sets risk_accepted regardless if "simple risk acceptance" is active it seems. And it does not set mitigated=true (even though Defectdojo always does this in case of " false_p or out of scope"

I Think I can come up with a workable structure but need a bit of time to put it to paper, will try and come back with a hierarchy

[Gby56]

I think a decision tree might be better, I realized that a matrix might be a bit overwhelming to navigate it

[lme-nca]

dont have a nice way to create this here in this text, so just a screenshot. This is roughly how the logic works now, in: https://github.com/DefectDojo/django-DefectDojo/pull/9050/commits/d3d918b7375c43322d2971e36ff6944b5c8d1d35

[lme-nca]

@coheigea @Gby56 @mtesauro @blakeaowens : this pull request is now ready for review, would appreciate some feedback

[lme-nca]

Reminder to: @Gby56 @mtesauro @blakeaowens this pull request is still here, we have been running it on our fork for the last 3 weeks and it seems stable.

[lme-nca]

Just giving this another bump.... @mtesauro @blakeaowens @Maffooch

[github-actions[bot]]

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

[lme-nca]

Hey everyone, this is still here.... @mtesauro @blakeaowens @Maffooch Willing to fix the conflict if there is still interest to merge this ?

[mtesauro]

@lme-nca There's some work happening around this part of DefectDojo e.g. #10011 which may address this issue. That work is of a broader scope than this PR. Let that one get merged and let's see where things are at after.

[gietschess]

@lme-nca There's some work happening around this part of DefectDojo e.g. #10011 which may address this issue. That work is of a broader scope than this PR. Let that one get merged and let's see where things are at after.

hey there, i've tested the behaviour of issue #8797 again with defectdojo version 2.34.2 and risk accepted findings are still mitigated after reimporting them even if they're still active in the uploaded trivy json file

[github-actions[bot]]

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

[dryrunsecurity[bot]]

Hi there :wave:, @dryrunsecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
Configured Codepaths Analyzer	:x:	1 finding
Sensitive Files Analyzer	:white_check_mark:	0 findings
AppSec Analyzer	:white_check_mark:	0 findings
Authn/Authz Analyzer	:white_check_mark:	0 findings
Secrets Analyzer	:white_check_mark:	0 findings

[!Note] :red_circle: Risk threshold exceeded. Adding a reviewer if one is configured in .dryrunsecurity.yaml.

notification list: @mtesauro @grendel513

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy :robot:. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request focus on improving the handling of security scan report imports and reimports within the application. The key changes include:

1. Ensuring that risk-accepted findings in Defect Dojo are not accidentally mitigated during the reimport process. This helps maintain the intended state of risk-accepted findings.

2. Enhancing the import and reimport functionality to support a wide variety of security scan report formats, including ZAP, Acunetix, Anchore, Veracode, SonarQube, and GitLab Dependency Scanning.

3. Implementing robust algorithms to accurately match and update findings between imported scans and existing findings in the application, even when the unique identifier of a finding may change.

4. Providing flexible control over the active, verified, and mitigated status of findings, as well as the ability to mark them as false positive, out of scope, or risk-accepted.

5. Handling the management of endpoints associated with findings, ensuring that they are properly linked and their status is updated accordingly.

6. Accurately capturing the scan date and maintaining a timeline of security issues and their remediation.

7. Delivering a comprehensive reimport functionality that can handle cases where findings have been added, removed, or modified in the new scan report, and updating the status of existing findings accordingly.

8. Generating detailed statistics and reporting on the imported and reimported findings, providing valuable insights for the security team.

These changes demonstrate a well-designed and feature-rich implementation of security scan report handling, which is a crucial component of a robust application security program.

Files Changed:

1. dojo/importers/default_reimporter.py: The changes in this file focus on ensuring that risk-accepted findings in Defect Dojo are not accidentally mitigated during the reimport process. This is an important security enhancement to maintain the intended state of risk-accepted findings.

2. unittests/test_import_reimport.py: This file contains changes related to the import and reimport functionality of various types of security scan reports. The code supports a wide range of scan report formats, provides robust algorithms for matching and updating findings, and offers flexible control over the status of findings. It also handles endpoint management, scan date handling, and delivers comprehensive statistics and reporting.

Powered by DryRun Security

[lme-nca]

@mtesauro @Gby56 @coheigea @Maffooch : I have rebased this change on the latest state and introduced a similar fix. If an existing finding is risk accepted then we avoid it being mitigated and closed if its still in an existing scan.

A quick review would be much appreciated :) .

[mtesauro]

@lme-nca Sorry that this one has gotten lost in the shuffle for so long.

Would you mind rebasing this so we can review and approve it?

[dryrunsecurity[bot]]

DryRun Security Summary

The pull request focuses on improving the reliability and consistency of the reimport process in the DefectDojo application, including handling risk-accepted and inactive findings, and updating the process to handle the "do not reactivate" option, ensuring the accuracy and completeness of the system.

Expand for full summary

Summary:

The changes in this pull request focus on improving the reliability and consistency of the reimport process in the DefectDojo application. The key changes include:

1. Handling the case where an existing finding is risk-accepted and inactive in DefectDojo. In this scenario, the finding is added to the unchanged_items list, and the method returns the existing finding without exiting the loop, ensuring that the endpoints are still updated.

2. Updating the process_matched_mitigated_finding method to handle the case where the user has set the do_not_reactivate option. If this option is set, the method skips reactivating the finding and adds a note to the finding instead.

These changes help make the reimport process more robust and flexible, allowing the application to handle complex scenarios that may arise during the import of security scan reports. The changes also ensure that the endpoints associated with the findings are still updated, even if the finding status is not synced from the scanner, which helps maintain the accuracy and completeness of the DefectDojo system.

Files Changed:

1. dojo/importers/default_reimporter.py: This file contains the DefaultReImporter class, which is responsible for processing and importing scan reports into the DefectDojo system. The changes in this file improve the handling of risk-accepted and inactive findings, as well as the option to skip reactivating findings.

2. unittests/test_import_reimport.py: This file contains the unit tests related to the import and re-import functionality of different types of security scan reports in the application. The changes in this file demonstrate the comprehensive approach to handling various scenarios, such as existing findings, scan dates, endpoints, vulnerability IDs, severity thresholds, and statistics.

Overall, these code changes contribute to the overall security and reliability of the DefectDojo application, making it more capable of handling complex scenarios during the reimport of scan reports.

Code Analysis

We ran 9 analyzers against 2 files and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer	Findings
Configured Codepaths Analyzer	1 finding

Riskiness

:red_circle: Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

[lme-nca]

@lme-nca Sorry that this one has gotten lost in the shuffle for so long.

Would you mind rebasing this so we can review and approve it?

Hi @mtesauro I rebased it. Another pull request of mine that has been stuck for a long time is: https://github.com/DefectDojo/django-DefectDojo/pull/9787 would appreciate a nudge there as well ?