Skip to content

Dependencytrack default severity #9370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mtesauro merged 2 commits into from
Feb 6, 2024
Merged

Dependencytrack default severity #9370

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fb70fc
:bug: fix defaulting severity, see last comments in #8778
manuel-sommer Jan 19, 2024
68af3f8
Merge branch 'dev' of github.com:DefectDojo/django-DefectDojo into de…
Maffooch Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions dojo/tools/dependency_track/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ def _convert_dependency_track_finding_to_dojo_finding(self, dependency_track_fin
dependency_track_severity = dependency_track_finding['vulnerability']['severity']
vulnerability_severity = self._convert_dependency_track_severity_to_dojo_severity(dependency_track_severity)
if vulnerability_severity is None:
logger.warning("Detected severity of %s that could not be mapped for %s. Defaulting to Critical!", dependency_track_severity, title)
vulnerability_severity = "Critical"
logger.warning("Detected severity of %s that could not be mapped for %s. Defaulting to Informational!", dependency_track_severity, title)
Copy link
Copy Markdown
Contributor Author

@manuel-sommer manuel-sommer Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maffooch and @mtesauro:

The problem we face with DependencyTrack findings is that single findings are not patched if the severity changes (e.g. if the severity is not yet classified).

Thus, we can deal with this in two ways:

  • The default severity for findings is Critical (which till now is "Critical, see this PR) Then, severities are not deduplicated and in general Teams will have a higher load of critical findings and they have to deal with them. This puts more pressure on the teams then necessary. Also, unclassified findings often don't have a patch available yet. On the other hand, switching severities are not deleted. Thus, it is easier to keep track on vulnerabilities.

  • The default severity for findings is Informational and we activate the deduplication in settings.dist.py. Then, these findings are closed automatically, but have the real severity once classified right. In this case, findings are closed and a new finding is generated as soon as the severity changes. Keeping track on vulnerabilities is harder.

Both ways are not the 100% right way as the capabilities are still limited due to the missing patch functionality of finding's severities from DependencyTrack to DefectDojo.
Maybe as an interim solution, we could implement both ways and choose the right one with #9351. We could implement a small logic in DependencyTrack to switch between the two options via parser_custom_setting.

What are your opinions and did I forget something?

Copy link
Copy Markdown
Contributor

@Maffooch Maffooch Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR and #9371 make sense to go together. This is a tricky problem to solve that is not just limited to dependency track.

Copy link
Copy Markdown
Contributor Author

@manuel-sommer manuel-sommer Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now, if me merge both PRs, we have another problem: Info findings will stay with severity "info" eventhough the severity cfhanges. So, info findings would have to be analyzed.

Copy link
Copy Markdown
Contributor

@kepten kepten Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @manuel-sommer that merging the two would "hide" real CVEs (most ppl/companies ignore info severity findings) if they were created and synced to DD before their real severity was assigned.

Let me collect my thoughts on this issue and then brainstorm on solution ideas:

  • CVE severities can change and this is a totally valid case (e.g. withdrawn or created with unassigned severity initially then set to the real one, etc) which both DT and DD should be prepared to handle. AFAIK DT updates the severities upon each sync with CVE DBs, so they are up-to-date in the import data, so it is DD which doesn't handle it properly yet.
  • AFAIK hash_code calculation is used to find duplicates. I think duplicate issues are the ones which are about the same vuln but might be coming from different sources (e.g https://nvd.nist.gov/vuln/detail/CVE-2023-52079 and GHSA-7hpj-7hhx-2fgx are the same vuln) and therefore potentially can have different metadata (like severity, references, etc). Therefore I think including severity into the hash_code is a bad idea since it'd break deduplication (e.g. the above two CVEs would have ended up as two different findings in DD when the severity was already updated in GHSA to medium but still unassigned in CVE). Therefore I think Revert adding severity to Dependency Track hash_code calculation #9371 should be merged asap.
  • Regarding this PR: I agree with it, I think unassigned CVEs should fall back to Info severity rather than Critical. However, unless severity patching is implemented in DD, this change would "hide" those findings which is I think worse than the current Critical one, so I'd only merge this, once there is another PR which solves severity patching
  • And now we arrive to the root cause which needs a solution: how to handle severity (and potentially other metadata "patching" in DD). This is not a trivial problem indeed since duplicated findings can have different metadata as described above. AFAIK DD stores all duplicated findings as separate records in the DB and then a scheduled task might remove duplicates according to the setting here. This means that if two findings which are duplicates have different metadata in the import, those metadata would be stored separately in their respective DD record, so nothing would be overridden. This should mean that simply overriding the metadata of existing records at import time would be a trivial solution and should work. However, that would cause issues if such metadata gets updated/patched which is used for hash calculation (e.g vulnerability_ids in the case of Dependency Track imports) because then the hash_code would need to be updated as well which can result in new findings to become duplicates of others and existing ones not to be duplicates anymore. This would result in an inconsistent state which could be resolved if after any hash_code updates, some tasks would update all the findings in the DB which either have the old or the new hash_code values and "fix" the duplicate connections (I think duplicate_finding_id field). This is my best idea so far but I might be missing something or misunderstood the internals of DD so feel free to correct me if I'm wrong or you have better alternative.

Copy link
Copy Markdown
Contributor Author

@manuel-sommer manuel-sommer Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking this issue also: #6774

vulnerability_severity = "Informational"

# Get the cvss score of the vulnerabililty
cvss_score = dependency_track_finding['vulnerability'].get("cvssV3BaseScore")
Expand Down