Add support for Calculating Risk in VulnerableCode

[ziadhany]

issue: #1543

[DennisClark]

it would be really great to see a review of this PR soon!

[TG1999]

@ziadhany this generally looks good to me, can we have some logs for the pipeline and some UI and API screenshots so we can merge it.

[keshav-space]

Thanks @ziadhany, some nits for your consideration.
Also, please use LoopProgress while iterating over large records, as we do here

vulnerablecode/vulnerabilities/pipelines/enhance_with_exploitdb.py

Line 61 in 590c91a

progress = LoopProgress(total_iterations=fetched_exploit_count, logger=self.log)

[keshav-space]

And please let me know if anyone has any other ideas, and feel free to edit the weight configuration list as needed so I can use it in the code.

@ziadhany this looks good to start.

[keshav-space]

@ziadhany please update the changelog. Also, IMO we should use weight_config.yml instead of JSON file for configuration, like we do in our other projects.

[DennisClark]

Re: "IMO we should use weight_config.yml instead of JSON file for configuration, like we do in our other projects." I am fine with that alternative.

[pombredanne]

Re: "IMO we should use weight_config.yml instead of JSON file for configuration, like we do in our other projects." I am fine with that alternative.

But why having a config file? IMHO this should just be some Python data in a .py file. This is not meant to be updated by anyone but us for now.

[DennisClark]

I think that someone running their own installation of VulnerableCode should be able to adjust the weight assignments based on their own experience. The idea of a config file has been in the design document for quite a while now and we need to move things forward to get this done.

[DennisClark]

ok, let's go with .py if that is the simplest

[mjherzog]

We should keep local configuration in the plan but custom configuration would add unnecessary complexity at this early stage.

[keshav-space]

Thanks @ziadhany, merging this now!

[DennisClark]

@ziadhany or @keshav-space could you please post some screenshots here that show the display of the new values in VCIO? Thanks! Also please confirm that all 3 values will be available in the V2 API.

• Weighted Severity
• Exploitability
• Risk

[ziadhany]

@DennisClark We store the value of risk_score in a column and currently display it in the API. However, Exploitability and Weighted Severity are still missing. We already have the logic to calculate these metrics (Exploitability, Weighted Severity, and Risk), and we can easily implement it. However, we need to decide whether to store exploitability and weighted_severity in the database or calculate them dynamically with each API call. If we choose to store them, we must also determine whether to store them at the Vulnerability level or the Package level.

[DennisClark]

@ziadhany I think your should store the values of exploitability and weighted_severity at the same level as risk_score.

[tdruez]

@ziadhany @DennisClark

If we choose to store them, we must also determine whether to store them at the Vulnerability level or the Package level.

Looking at https://github.com/aboutcode-org/vulnerablecode/pull/1593/files

I see 4 values being computed:

• get_weighted_severity
• get_exploitability_level
• compute_vulnerability_risk
• compute_package_risk

We need to clarify if those values are computed/defined for a Vulnerability entry of for a Package (combination of Vulnerabilities).

This matters a lot in the presentation of the data, depending on the context, one may view it through 1 Package, or through 1 vulnerability that affects many Packages.

At the moment, only the Package.risk_score is stored and displayed (Package model and API).
I would suggest that the Vulnerability.risk_score should also be stored and available in the API so the value can be used when looking at the list of Vulnerabilities.

Now, for weighted_severity and exploitability_level, looking at the code or at the design document, it is not clear if those values are qualifying a Package, a Vulnerability, or both.

[ziadhany]

@DennisClark @tdruez

Yes, we can store a vulnerability risk. However, weighted_severity and exploitability_level are specific to each vulnerability and are computed using the exploits, references, and severities associated with it.

If we plan to store the risk_score for each vulnerability while also storing and displaying the weighted_severity and exploitability_level, it will lead to data duplication.

I suggest storing only the weighted_severity and exploitability_level for each vulnerability. This way, calculating the vulnerability risk and package risk becomes a straightforward mathematical operation.

However, if we don't mind some data duplication in exchange for better performance, we could store the following:
exploitability_level, weighted_severity, and vulnerability risk in the vulnerability model
package risk in the package model (already exists).

Please let me know exactly what needs to be done so we can quickly create a pull request for it.

[tdruez]

If we plan to store the risk_score for each vulnerability while also storing and displaying the weighted_severity and exploitability_level, it will lead to data duplication.

@ziadhany could you clarify the "data duplication", maybe with an example? I'm not sure to get what would be duplicated here.

[ziadhany]

@tdruez
Let's say we have two vulnerabilities, both affecting the package.

We are going to calculate the weighted_severity and exploitability_level for Vulnerability 1. We get values of 2 and 3, respectively.
We did the same for Vulnerability 2 and get 4 , 2.

The risk for Vulnerability 1: ( 2 * 3 ) = 6
The risk for Vulnerability 2: ( 4 * 2 ) = 8

The package risk is the maximum risk of the vulnerabilities affecting this package, which is 8.

If we store the weighted_severity and exploitability_level, there’s no need to store the vulnerability risk (as it would be redundant data). Similarly, there’s no need to store the package risk since it can be easily calculated as the maximum of the vulnerability risks.

[tdruez]

If we plan to store the risk_score for each vulnerability while also storing and displaying the weighted_severity and exploitability_level, it will lead to data duplication.

I still don't see the duplication storing the risk_score for each vulnerability.
The risk for Vulnerability 1: ( 2 * 3 ) = 6 and The risk for Vulnerability 2: ( 4 * 2 ) = 8 in your example: both have a different risk_score that is useful to order and filter a Vulnerability listing outside a Package context.

If we store the weighted_severity and exploitability_level, there’s no need to store the vulnerability risk (as it would be redundant data).

Could you clarify this? Why is it redundant?

[ziadhany]

@tdruez I mean, if we create two columns for weighted_severity and exploitability_level, there’s no need to have a vulnerability_risk column because it is the product of the two columns.

and there might be no need for a package risk column since it represents the maximum of the vulnerability risk value that affect this package .

But, as you mentioned, it can be useful for ordering and filtering a vulnerability listing, and it can lead to better performance.

[tdruez]

@ziadhany Alright, we should start by adding weighted_severity and exploitability_level as fields of the Vulnerability model in any case., right?

[ziadhany]

@ziadhany Alright, we should start by adding weighted_severity and exploitability_level as fields of the Vulnerability model in any case., right?

Alright, I'll create a quick PR for this.

[DennisClark]

@ziadhany I think that the suggestions from @tdruez make more sense than my previous remarks about putting all 3 fields at the same level. Thanks for all the analysis.