Dependency-Track Hash Code computation: Add severity in the hash code calculation of the finding

[kmouzoul23]

Problem description
At the moment severity is not part of the Unique Hash code for findings found via Dependency Track. This configuration leads to misleading information within Defect Dojo when findings CVSS score changes.
DefectDojo/django-DefectDojo/tree/master/dojo/settings)
/settings.dist.py:
'Dependency Track Finding Packaging Format (FPF) Export': ['component_name', 'component_version', 'vulnerability_ids']

Steps to reproduce the behavior:

• Upload a Dependency-Track Report
• Change the severity of a finding included in the report
• Reupload this Dependency-Track file to the same engagement
• View the result (finding with changed severity will be marked as Inactive, Duplicate)

Expected behavior
The duplicate finding (with changed severity) should be considered the active one in Defect Dojo

Deployment method

• Docker Compose
• Kubernetes
• GoDojo

[quirinziessler]

@kmouzoul23 The hash code is without the severity on purpose. Otherwise it would cause a lot of false truth in DD. If you want to have this feature I would recommend you to open up a issue to the DependencyTrack project or even better create a PR for this there. Resturcturing the sync to use patch methods for existing findings will probably be the most easiest & effective way.

[manuel-sommer]

#9117

I guess this can be closed @mtesauro

[kepten]

I agree with @quirinziessler that this should have been addressed in DT and not in DD. CVE severities are sometimes unassigned at first or get recalculated afterwards but they should still be considered the same finding in DD.
I'm still investigating but this change seems to result in existing DT findings with old hash_code to be closed and then re-created with the new hash_code which totally messed up our SLA tracking.
@manuel-sommer I think #9117 should be reverted because I'd imagine this will cause unnecessary trouble for lots of users once they happen to upgrade to DD v2.30.x.

[manuel-sommer]

@kepten and @quirinziessler I had a quick look at the code.
Do findings with unassigned severity get a critical severity? Maybe we should change this behavior in a way that they will become an info finding. Then, this makes more sense. Maybe I had it wrong in my mind.
I don't know if the severity issue can be fixed on DT in an easy way as there is probably no trigger mechanism inside DT when the severity of a finding changes. Then, this mechanism could be used to implement a method to patch the finding's severity on DefectDojo side. Feel free to open up a new issue if you still have problems or you still have the opinion that this should be reverted.

[kepten]

I think there are two distinct problems which need two distinct solutions and neither of them should be to add severity to hash_code (which got implemented so far):

1. The problem statement in this issue by @kmouzoul23 is that DD severity is not updated if the severity in DT changes. Since DD right now works in a way that it never updates imported findings (except some metadata like active and mitigation_date), that should be done differently, e.g DT updating the findings via DD API when their metadata (e.g. severity) changes.
2. @manuel-sommer in Filter out rejected findings to not sync with DefectDojo DependencyTrack/dependency-track#1915 described the problem that rejected CVE findings are not automatically closed in DD. To solve this, no code change is necessary in DD. DT should either not send rejected findings to DD in the first place or set their severity to Info so DD would ignore them at import time (thanks to minimum_severity set to Info).

There is a 3rd topic raised in the previous comment:

Do findings with unassigned severity get a critical severity?

The answer is yes. I'd be okay changing it to Info, I think it would make much more sense than treating a finding with unknown severity as critical. Although making this change would solve DependencyTrack/dependency-track#1915, we should rather consider it as a conceptual question, i.e how to treat "unknown" severity findings in DD. Should we treat them with the utmost care (because they might turn out to be critical indeed) or should they be "ignored" unless their real severity is determined? Personally, I'd be fine with both but might prefer the latter.

Why I think #9117 should be reverted asap?

Because changing the hash_code calculation results in all existing DT findings in DD to be closed and then re-created with the new hash_code upon the next import. That has several drawbacks:

• The SLA of the new findings will start from the import day, losing the original "age" of the finding stored in the previous finding. This means SLAs for DT findings in DD will be all incorrect and this crucial information (at least in a corporate/compliance-sensitive environment) will be lost/messed up. That will impact everyone with existing DT findings once they upgrade to DD v2.30.x
• If the previous finding was already triaged (e.g. set to false positive in DD) then this information will be "lost" and won't be visible in the newly created finding.
• I checked and none of the other dependency checkers seem to include severity in their hash_code, I guess for a reason...

[manuel-sommer]

I have an additional idea: There should be a mechanism to automatically update the finding's hashcode as soon as the hash code settings changes. https://defectdojo.github.io/django-DefectDojo/usage/features/#hashcode-generation--regeneration

By the way, this is the solution that not all findings will be destroyed and recreated. But this has to be done when you upgrade to v2.30.0

[kepten]

@manuel-sommer Yes, this could be a solution for not recreating the findings. But it still doesn't solve the issue that if the severity of a CVE is changed (e.g in the NVD database) it'd destroy and create a new finding with the new severity, practically "resetting" the age attribute of the finding, losing any previous triage state, etc which doesn't make sense to me. If a CVE is created (e.g. with critical initial severity) and then it gets lowered (e.g. to low), the "age" of the finding should not change just the corresponding SLA in DD. Similarly if a finding was out of scope with some severity, it'll still be out of scope even if its severity changes. Therefore I still think that severity should not be added to the hash_code because it's not a unique attribute of it which should be used for deduplication but a dynamic field which can change during its lifecycle.

[manuel-sommer]

@kepten, I opened up a followup PR.

[kepten]

@manuel-sommer If you mean #9370, that is a good change but it's still not reverting #9117 which is the root cause of my problems (and eventually other's as well once they upgrade to v2.30.x).

[manuel-sommer]

Feel free to open a PR.