fix: fix severity lookup in Qualys parser

[nv-pipo]

Description

Fixes a bug in the Qualys parser where the severity lookup was not correctly handling integer values. This caused incorrect severity values to be returned. The fix updates the severity lookup to convert the severity value to an integer before performing the lookup.

Test results

Ideally you extend the test suite in tests/ and dojo/unittests to cover the changed in this PR.
Alternatively, describe what you have and haven't tested.

Documentation

Please update any documentation when needed in the documentation folder)

Checklist

This checklist is for your information.

• Make sure to rebase your PR against the very latest dev.
• Features/Changes should be submitted against the dev.
• Bugfixes should be submitted against the bugfix branch.
• Give a meaningful name to your PR, as it may end up being used in the release notes.
• Your code is flake8 compliant.
• Your code is python 3.11 compliant.
• If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
• Model changes must include the necessary migrations in the dojo/db_migrations folder.
• Add applicable tests to the unit tests.
• Add the proper label to categorize your PR.

Extra information

Please clear everything below when submitting your pull request, it's here purely for your information.

Moderators: Labels currently accepted for PRs:

• Import Scans (for new scanners/importers)
• enhancement
• performance
• feature
• bugfix
• maintenance (a.k.a chores)
• dependencies
• New Migration (when the PR introduces a DB migration)
• settings_changes (when the PR introduces changes or new settings in settings.dist.py)

Contributors: Git Tips

Rebase on dev branch

If the dev branch has changed since you started working on it, please rebase your work after the current dev.

On your working branch mybranch:

git rebase dev mybranch

In case of conflict:

 git mergetool
 git rebase --continue

When everything's fine on your local branch, force push to your myOrigin remote:

git push myOrigin --force-with-lease

To cancel everything:

git rebase --abort

Squashing commits

git rebase -i origin/dev

• Replace pick by fixup on the commits you want squashed out
• Replace pick by reword on the first commit if you want to change the commit message
• Save the file and quit your editor

Force push to your myOrigin remote:

git push myOrigin --force-with-lease

[dryrunsecurity]

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

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

Note

🟢 Risk threshold not exceeded.

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 accuracy and reliability of the Qualys vulnerability data parsing in both the unittests and the Dojo application security tool. The key changes include:

1. Severity Mapping: The code now uses a more comprehensive severity mapping system, which first looks up the severity value in a legacy dictionary and then in a non-legacy dictionary, ensuring accurate severity classification.

2. CVSS Parsing: The code has been updated to handle CVSS3 and CVSS2 scores more robustly, using a dedicated split_cvss function to extract the CVSS score and vector from the XML data.

3. Vulnerability Details: The code now extracts more detailed information about the vulnerabilities, such as the vulnerability type, category, QID, port, result evidence, and timestamps for when the vulnerability was first and last found.

These changes improve the quality and accuracy of the vulnerability data imported from Qualys, ensuring that the findings are correctly classified and prioritized within the application security management system. The increased level of detail about the vulnerabilities also provides security teams with more context to understand and address the identified issues.

Files Changed:

1. unittests/tools/test_qualys_parser.py: The changes in this file are focused on improving the parsing of CVSS information and severity levels, including a bug fix for incorrectly classifying a "High" severity finding as "Critical". The changes are well-tested with a comprehensive set of unit tests.

2. dojo/tools/qualys/parser.py: The changes in this file enhance the Qualys vulnerability scanner parser in the Dojo application security tool. The improvements include a more comprehensive severity mapping system, robust CVSS parsing, and the extraction of additional vulnerability details, all of which contribute to the accurate representation of Qualys scan data within the Dojo application.

Powered by DryRun Security

[cneill]

Would you mind adding a unit test here with an example file that triggers the bug you mention, in order to validate this fix and ensure we don't have a regression?

[nv-pipo]

Would you mind adding a unit test here with an example file that triggers the bug you mention, in order to validate this fix and ensure we don't have a regression?

Should I close this PR and open a new one with the UNITTEST?

[mtesauro]

Approved

[mtesauro]

Would you mind adding a unit test here with an example file that triggers the bug you mention, in order to validate this fix and ensure we don't have a regression?

Should I close this PR and open a new one with the UNITTEST?

You can push a new commit to your branch with the unit test and it will update this PR automatically. You're welcome to create a new PR if you want, but it's not strictly necessary.

[nv-pipo]

@mtesauro
I added the unit test.

Additional changes follow the logic of #6248 (comment). Pull severity from 'SEVERITY' field in source XML, if not available, try to extract it from CVSS_VALUE field, if neither are available: default to Informational and log a warning. Before this PR: it was the other way around: first 'CVSS_VALUE' then 'SEVERITY'.

[mtesauro]

@mtesauro I added the unit test.

Additional changes follow the logic of #6248 (comment). Pull severity from 'SEVERITY' field in source XML, if not available, try to extract it from CVSS_VALUE field, if neither are available: default to Informational and log a warning. Before this PR: it was the other way around: first 'CVSS_VALUE' then 'SEVERITY'.

@nv-pipo
This makes sense to me - be warned we've see 2 instances where the CVSS score from a vendor's tool does not match the "word" severity reported in the same scan export e.g. Report says "High" but the CVSS score is a "Low" when you calculate it from the CVSS vector.
This is the reason we generally use the 'word' severity and then fall back to the CVSS score if it's not available.

You'd like to think vendors could make their 'word' score match CVSS but 🤷

[cneill]

@mtesauro I added the unit test.
Additional changes follow the logic of #6248 (comment). Pull severity from 'SEVERITY' field in source XML, if not available, try to extract it from CVSS_VALUE field, if neither are available: default to Informational and log a warning. Before this PR: it was the other way around: first 'CVSS_VALUE' then 'SEVERITY'.

@nv-pipo This makes sense to me - be warned we've see 2 instances where the CVSS score from a vendor's tool does not match the "word" severity reported in the same scan export e.g. Report says "High" but the CVSS score is a "Low" when you calculate it from the CVSS vector. This is the reason we generally use the 'word' severity and then fall back to the CVSS score if it's not available.

You'd like to think vendors could make their 'word' score match CVSS but 🤷

It may be a case where they get CVSS info from NVD or similar but then decide based on their own analysis that the implied severity is wrong. In that case, I expect they probably don't update the CVSS score/vector.

[Maffooch]

The need for this line to change tells me that this will be unexpected behavior for qualys users. What used to be Critical will now to be demoted to a High...

I looked at your first commit, and see that it is fixing the issue described in the pull request. The move to using CVSS to attempt to be "smarter than the vendor" is a slippery path that has bit us before as @mtesauro has mentioned. I think reverting to your first commit, and then adding a unit test for that change would be the best path to success with this PR

[nv-pipo]

@Maffooch

This line had to change because for that vulnerability the Finding severity didn't match the CVSS severity. So again, the question is, what should DD use first?

• The severity explicitly given by the vendor, or
• The mapped value from the CVSS score to severity.

In other words, that line changed because this PR changes the priority in how to populate the finding severity: before it was "CVSS Severity" then "real severity", this PR uses: "real severity" and if not available fallback to "CVSS Severity".

In the past, DD went for this PR's approach: #6248 (comment). If you think this is a mistake, maybe the Veracode SCA parser also needs to be changed?

For context:
Qualys calls this the "CVSS Severity" (https://blog.qualys.com/qualys-insights/2022/10/10/in-depth-look-into-data-driven-science-behind-qualys-trurisk) and it might not always match the finding severity (https://success.qualys.com/support/s/article/000002759). Extracts:

[nv-pipo]

@Maffooch
I think there is a misunderstanding: the current version of the Qualys parser is the one that tries to be "smarter than the vendor", as it uses the CVSS, whereas this PR is simply attempts to use qualys' severity. In other words, we all agree with @mtesauro: DD should use the already available 'Severity' field by the vendor, before trying CVSS to fill DD's 'severity' field. Similar to other parsers in DD: #6248 (comment).... TBH, I've never seen a qualys vulnerability without 'Severity' field, so I'd be happy to remove the fallback to CVSS altogether, as it will probably never be used.

FYI:
The change of severity in the other unittest, is because of this mismatch between Severity and CVSS Severity:

A 'CVSSv3 severity' translates to 'Critical' (old code), but the 'vendor provided severity' is 4 in a scale 1-5, so it translates to 'High' (this PR code).

[Maffooch]

You're right. I was confusing the CSV parser and the XML one. I agree with removing the CVSS fallback altogether. That way it would match the CSV parser

@mtesauro what do you think about that?

[mtesauro]

@Maffooch @nv-pipo
I agree on removing the CVSS fallback completely on this PR given what's been said on this thread (including what Qualys says about how they handle severity / CVSS - it seems the most sensible way forward.

[nv-pipo]

@Maffooch and @mtesauro,
As requested, I've updated the code to remove the fallback to cvss_value. (code looks cleaner, simpler now 👍 )

There seem to be some build problems, but I doubt it has anything to do with this PR (ran the Qualys unit_tests locally without problems). Please advise how to fix.
Thanks!

[mtesauro]

@nv-pipo

There seem to be some build problems, but I doubt it has anything to do with this PR (ran the Qualys unit_tests locally without problems). Please advise how to fix. Thanks!

There was a commit in Dev that broke the tests that require building a container. The best way to fix is to rebase against what's currently in the Dev branch (the fix to what broke tests has been merged already)

[nv-pipo]

@mtesauro
Great! all tests passed.

@Maffooch, could you change your approval status?