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

[lme-nca]

This tries to improve on the (incomplete) fix introduced here: #7447
Also see the discussion on Slack: https://owasp.slack.com/archives/C014H3ZV9U6/p1700819414409409
and related issue #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(Add the old finding in the reimporter to the list of unchanged findin… #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 (Add the old finding in the reimporter to the list of unchanged findin… #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]

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 #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: #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: d3d918b

[lme-nca]

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

[coheigea]

It looks OK to me overall, the logic looks a lot cleaner, before the class was a mess. I don't have the scope to test it right now, but once the patch is applied and released I can test it more thorougjly

[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.

[Gby56]

Overall this looks good to me, renaming "items" also helped a whole lot for readability! Thank you for taking the time and testing it out

[Gby56]

Just wondering if we should log something here for a warning... or if a loop is needed to process all the matched existing findings instead of just the first one ?

[lme-nca]

Yes I was wondering something similar... For now we could write a debug/log statement. The purpose of this pull request was not to significantly change the behaviour of the reimporter (which this might do). Although I agree that probably this case should be handled somehow, I think this should be handled in a seperate ticket/issue.

[lme-nca]

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

[github-actions]

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]

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

[dryrunsecurity]

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
Configured Codepaths Analyzer	❌	1 finding
Sensitive Files Analyzer	✅	0 findings
AppSec Analyzer	✅	0 findings
Authn/Authz Analyzer	✅	0 findings
Secrets Analyzer	✅	0 findings

Note

🔴 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 🤖. 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]

DryRun Security Summary

The pull request focuses on improving the handling of risk-accepted findings during the reimport process and introducing a comprehensive set of unit tests for the import and reimport functionality to ensure the security and integrity of the data being imported.

Expand for full summary

Summary:

The code changes in this pull request focus on improving the handling of risk-accepted findings during the reimport process in the default_reimporter.py file, as well as introducing a comprehensive set of unit tests for the import and reimport functionality in the test_import_reimport.py file.

The changes to the default_reimporter.py file ensure that risk-accepted findings are not incorrectly set to "mitigated" during the reimport process, as DefectDojo does not automatically mitigate risk-accepted findings. This is an important security-related enhancement, as it preserves the accurate representation of the security posture and allows users to make informed decisions about managing these risks.

The unit tests in the test_import_reimport.py file cover a wide range of scenarios related to the import and reimport functionality, including the handling of false positives, out-of-scope, and risk-accepted findings, vulnerability IDs, scan dates, endpoints, severity changes, import history tracking, and various report formats. These tests help ensure the security and integrity of the data being imported, which is crucial for maintaining the application's security posture.

Files Changed:

1. dojo/importers/default_reimporter.py:

   • The changes in this file ensure that risk-accepted findings are not set to "mitigated" during the reimport process, preserving their correct status.
   • The code also ensures that the endpoints associated with risk-accepted findings are still updated, even though the finding itself is not modified.

2. unittests/test_import_reimport.py:

   • The changes in this file introduce a comprehensive set of unit tests for the import and reimport functionality of the application.
   • The tests cover various scenarios related to the handling of false positives, out-of-scope, and risk-accepted findings, vulnerability IDs, scan dates, endpoints, severity changes, import history tracking, and different report formats.
   • These tests help ensure the security and integrity of the data being imported, which is crucial for maintaining the application's security posture.

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

🔴 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: #9787 would appreciate a nudge there as well ?