Skip to content
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.

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

Report CRITICAL (CVSSv3) or HIGH (CVSSv2) when highest-ranking CVSS-scored and unscored vulnerabilities are both present #4116

Merged
merged 5 commits into from
Apr 4, 2022

Conversation

aikebah
Copy link
Collaborator

@aikebah aikebah commented Feb 26, 2022

Fixes Issue #4112, Fixes Issue #4186

Description of Change

Draft status

Draft proposal, still work-in-progress with only the HTML report updated to have a base for further discussion on the approach.

Change the vulnerability natural order to take severity into account, with any unscored issue considered to be at the top of the second-highest rating when comparing to CVSS-scored vulnerabilities (so that they only are considered 'more sever' if the CVSS-score is not in the highest severity category.
This prevents a scenario where the highest scoring vulnerabilty is reported as 'unknown' while there are known vulnerabilities of the highest category.

It influences the sorting in the reports (which was up to now purely alphabetical on vulnerability name), so it should land in a new major version release as we know that there are teams out there relying on the sorting to perform a balanced-read comparison between builds to not re-alert on already alerted vulnerabilities for a subsequent build.

Have test cases been added to cover the new functionality?

not yet, intend to add tests for the compareTo of Vulnerability before finalizing the PR

@aikebah aikebah marked this pull request as draft February 26, 2022 16:51
@aikebah aikebah changed the title Report CRITICAL CVSSv3 or HIGH (CVSSv2) when CSSV-scored and unscored vulnerabilities are both present Report CRITICAL CVSSv3 or HIGH (CVSSv2) when CVSS-scored and unscored vulnerabilities are both present Feb 26, 2022
@boring-cyborg boring-cyborg bot added the core changes to core label Feb 26, 2022
@aikebah aikebah changed the title Report CRITICAL CVSSv3 or HIGH (CVSSv2) when CVSS-scored and unscored vulnerabilities are both present Report CRITICAL (CVSSv3) or HIGH (CVSSv2) when highest-ranking CVSS-scored and unscored vulnerabilities are both present Feb 26, 2022
@marcelstoer
Copy link
Contributor

Looks good to me, thanks for your effort!

It influences the sorting in the reports [...] so it should land in a new major version

Under SemVer this is correct if you consider the HTML report to be part of your public API.

there are teams out there relying on the sorting to perform a balanced-read comparison between builds

Oh, sounds interesting, can you elaborate on this? English obviously isn't my mother tongue...what is a "balanced-read comparison"?

@aikebah
Copy link
Collaborator Author

aikebah commented Feb 26, 2022

@marcelstoer Balanced read is an algorithm to find the matches and differences in two sorted lists by reading them in parallel from top to bottom. When you encounter an item that's not on the other list you keep reading the list with the lowest value (from sort perspective) until you either get back to a match or you pass the value on the other list. The items read from that list are the 'extra items' on that list. You continue travelling both lists in this way and record the matches and differences along the way. This way with one pass through both lists in parallel you have the full overview of matches and the uniques per list.

We had request to try and make sure that the sequence and bundling was stable. Their scripts were mainly set off by the bundling analyzer that did not do a stable job of picking the 'main' library that other related libraries were linked to. So they frequently got false positive 'new vulnerabilities' because their script saw vulnerabilities for a different dependency (e.g. spring-framework being reported in one report for 'spring-core' with 'spring-web' bundled in as a 'releated dependency' and another time 'spring-web' was picked first by the bundling analyzer and 'spring-core' reported as a related dependency.

@aikebah
Copy link
Collaborator Author

aikebah commented Feb 27, 2022

Needs some more thought... current resulting sort order is unstable and dependent on the order of addition to sorted collections. CVSSv2-only high scores can be less severe than CVSSv3-only high scores, but CVSSv2 high scores are considered more severe than unscored values while CVSSv3-only high scores are considered less severe than unscored values.

So addition to a TreeSet of

  • CVSSv2-high-7.0 > unscored > CVSSv3-high-8.9 yields { CVSSv3-high-8.9, CVSSv2-high-7.0, unscored } => HIGH
  • unscored > CVSSv2-high-7.0 > CVSSv3-high-8.9 yields { CVSSv2-high-7.0, unscored, CVSSv3-high-8.9 } => Unknown
  • CVSSv3-high-8.9 > unscored > CVSSv2-high-7.0 yields { unscored, CVSSv3-high-8.9, CVSSv2-high-7.0 } => HIGH

Leading to 2 different outcomes for 'highest severity' for the same set of 3 vulnerabilities depending on the order of addition during vulnerability analysis. And unpredictive sorting of the report as a whole when more vulnerabilities come into play.

aikebah added 2 commits March 23, 2022 18:07
…ting regardless

of the sequence in which items are entered into a sorted collection.
@boring-cyborg boring-cyborg bot added the tests test cases label Mar 30, 2022
@aikebah aikebah marked this pull request as ready for review March 30, 2022 16:07
@jeremylong jeremylong merged commit fa14f64 into main Apr 4, 2022
@jeremylong jeremylong deleted the issue-4112 branch April 4, 2022 10:32
@aikebah aikebah added this to the 7.1.0 milestone Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core changes to core tests test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

highest serverity wrong Highest Severity in report summary reported as "Unknown"
3 participants