feat(advisor)!: Rework VulnerabilityReference semantics

Previously, the semantics of the `severity` property were too loaded as the string could have also represented a numeric value. Introduce a dedicated `score` for the latter and clarify that `severity` from now on is supposed to exclusively contain a semantic rating like "LOW" or "HIGH". Additionally, start to maintain the individual metrics as usually encoded into a vector string. This allows for more sophisticated visualizations e.g. via [1]. Adjust tests accordingly and make some smaller improvements along the way. [1]: https://github.com/org-metaeffekt/metaeffekt-universal-cvss-calculator Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
oss-review-toolkit · Sep 18, 2024 · 60ef7c9 · 60ef7c9
1 parent d1fa1f2
commit 60ef7c9
Show file tree

Hide file tree

Showing 26 changed files with 215 additions and 177 deletions.
diff --git a/cli/src/funTest/assets/semver4j-ort-result.yml b/cli/src/funTest/assets/semver4j-ort-result.yml
@@ -394,8 +394,8 @@ advisor:
         references:
         - url: "http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-15250"
           scoring_system: "CVSS2"
-          severity: "5.5"
-          severity_rating: "MEDIUM"
+          severity: "MEDIUM"
+          score: 5.5
 evaluator: null
 resolved_configuration:
   package_curations:

diff --git a/evaluator/src/main/kotlin/PackageRule.kt b/evaluator/src/main/kotlin/PackageRule.kt
@@ -29,6 +29,9 @@ import org.ossreviewtoolkit.model.config.Excludes
 import org.ossreviewtoolkit.model.licenses.LicenseView
 import org.ossreviewtoolkit.model.licenses.ResolvedLicense
 import org.ossreviewtoolkit.model.licenses.ResolvedLicenseInfo
+import org.ossreviewtoolkit.model.vulnerabilities.Cvss2Rating
+import org.ossreviewtoolkit.model.vulnerabilities.Cvss3Rating
+import org.ossreviewtoolkit.model.vulnerabilities.Cvss4Rating
 import org.ossreviewtoolkit.model.vulnerabilities.Vulnerability
 import org.ossreviewtoolkit.model.vulnerabilities.VulnerabilityReference
 import org.ossreviewtoolkit.utils.spdx.SpdxExpression
@@ -83,21 +86,40 @@ open class PackageRule(
 
     /**
      * A [RuleMatcher] that checks whether any vulnerability for the [package][pkg] has a
-     * [reference][Vulnerability.references] with a [severity][VulnerabilityReference.severity] that equals or is
-     * greater than [threshold] according to the [scoringSystem] and the belonging [severityComparator].
+     * [reference][Vulnerability.references] with a [score][VulnerabilityReference.score] that equals or is
+     * greater than [scoreThreshold] according to the [scoringSystem]. If the reference provides no score but a
+     * [severity][VulnerabilityReference.severity], the threshold is mapped to a qualitative rating for comparison.
      */
-    fun hasVulnerability(threshold: String, scoringSystem: String, severityComparator: (String, String) -> Boolean) =
+    fun hasVulnerability(scoreThreshold: Float, scoringSystem: String) =
         object : RuleMatcher {
-            override val description = "hasVulnerability($threshold, $scoringSystem)"
+            override val description = "hasVulnerability($scoreThreshold, $scoringSystem)"
 
             override fun matches(): Boolean {
                 val run = ruleSet.ortResult.advisor ?: return false
-                return run.getVulnerabilities(pkg.metadata.id).asSequence()
+                val matchingSystems = run.getVulnerabilities(pkg.metadata.id).asSequence()
                     .filter { !ruleSet.resolutionProvider.isResolved(it) }
                     .flatMap { it.references }
                     .filter { it.scoringSystem == scoringSystem }
+
+                val scores = matchingSystems.mapNotNull { it.score }
+                if (scores.any()) return scores.any { it >= scoreThreshold }
+
+                // Fall back to a more coarse comparison of qualitative severity ratings if no scores are available.
+                val severityThreshold = VulnerabilityReference.getQualitativeRating(scoringSystem, scoreThreshold)
+                    ?: return false
+
+                val severities = matchingSystems
                     .mapNotNull { it.severity }
-                    .any { severityComparator(it, threshold) }
+                    .mapNotNull {
+                        when (scoringSystem.uppercase()) {
+                            in Cvss2Rating.NAMES -> enumValueOf<Cvss2Rating>(it)
+                            in Cvss3Rating.NAMES -> enumValueOf<Cvss3Rating>(it)
+                            in Cvss4Rating.NAMES -> enumValueOf<Cvss4Rating>(it)
+                            else -> null
+                        }
+                    }
+
+                return severities.any { it.ordinal >= severityThreshold.ordinal }
             }
         }
 

diff --git a/evaluator/src/test/kotlin/PackageRuleTest.kt b/evaluator/src/test/kotlin/PackageRuleTest.kt
@@ -215,45 +215,28 @@ class PackageRuleTest : WordSpec() {
 
             "return true if a severity of a vulnerability is higher than the threshold" {
                 val rule = createPackageRule(packageWithVulnerabilities)
-                val matcher = rule.hasVulnerability("8.9", "CVSS3") { value, threshold ->
-                    value.toFloat() >= threshold.toFloat()
-                }
+                val matcher = rule.hasVulnerability(8.9f, "CVSS3")
 
                 matcher.matches() shouldBe true
             }
 
             "return false if a severity of a vulnerability is lower than the threshold" {
                 val rule = createPackageRule(packageWithVulnerabilities)
-                val matcher = rule.hasVulnerability("9.1", "CVSS3") { value, threshold ->
-                    value.toFloat() >= threshold.toFloat()
-                }
+                val matcher = rule.hasVulnerability(9.1f, "CVSS3")
 
                 matcher.matches() shouldBe false
             }
 
             "return true if a severity of a vulnerability is the same as the threshold" {
                 val rule = createPackageRule(packageWithVulnerabilities)
-                val matcher = rule.hasVulnerability("9.0", "CVSS3") { value, threshold ->
-                    value.toFloat() >= threshold.toFloat()
-                }
-
-                matcher.matches() shouldBe true
-            }
-
-            "return true if a severity of a vulnerability is the same as the threshold without decimals" {
-                val rule = createPackageRule(packageWithVulnerabilities)
-                val matcher = rule.hasVulnerability("9", "CVSS3") { value, threshold ->
-                    value.toFloat() >= threshold.toFloat()
-                }
+                val matcher = rule.hasVulnerability(9.0f, "CVSS3")
 
                 matcher.matches() shouldBe true
             }
 
             "return false if no vulnerability is found for the scoringSystem" {
                 val rule = createPackageRule(packageWithVulnerabilities)
-                val matcher = rule.hasVulnerability("10.0", "fake-scoring-system") { value, threshold ->
-                    value.toFloat() >= threshold.toFloat()
-                }
+                val matcher = rule.hasVulnerability(10.0f, "fake-scoring-system")
 
                 matcher.matches() shouldBe false
             }

diff --git a/evaluator/src/test/kotlin/TestData.kt b/evaluator/src/test/kotlin/TestData.kt
@@ -232,7 +232,9 @@ val ortResult = OrtResult(
                                 VulnerabilityReference(
                                     url = URI("https://oss-review-toolkit.org"),
                                     scoringSystem = "CVSS3",
-                                    severity = "9.0"
+                                    severity = "CRITICAL",
+                                    score = 9.0f,
+                                    vector = null
                                 )
                             )
                         ),
@@ -242,7 +244,9 @@ val ortResult = OrtResult(
                                 VulnerabilityReference(
                                     url = URI("https://oss-review-toolkit.org"),
                                     scoringSystem = "CVSS3",
-                                    severity = "2.0"
+                                    severity = "LOW",
+                                    score = 2.0f,
+                                    vector = null
                                 )
                             )
                         )

diff --git a/examples/example.rules.kts b/examples/example.rules.kts
@@ -214,20 +214,18 @@ fun RuleSet.vulnerabilityInPackageRule() = packageRule("VULNERABILITY_IN_PACKAGE
 }
 
 fun RuleSet.highSeverityVulnerabilityInPackageRule() = packageRule("HIGH_SEVERITY_VULNERABILITY_IN_PACKAGE") {
-    val maxAcceptedSeverity = "5.0"
+    val scoreThreshold = 5.0f
     val scoringSystem = "CVSS2"
 
     require {
         -isExcluded()
-        +hasVulnerability(maxAcceptedSeverity, scoringSystem) { value, threshold ->
-            value.toFloat() >= threshold.toFloat()
-        }
+        +hasVulnerability(scoreThreshold, scoringSystem)
     }
 
     issue(
         Severity.ERROR,
         "The package ${pkg.metadata.id.toCoordinates()} has a vulnerability with $scoringSystem severity > " +
-                maxAcceptedSeverity,
+            "${scoreThreshold}",
         howToFixDefault()
     )
 }

diff --git a/model/src/main/kotlin/vulnerabilities/VulnerabilityReference.kt b/model/src/main/kotlin/vulnerabilities/VulnerabilityReference.kt
@@ -32,28 +32,37 @@ import java.net.URI
  * with a list of references; each reference points to the source of the information and has some detailed information
  * provided by this source.
  */
-@JsonIgnoreProperties(value = ["severity_rating"], allowGetters = true)
+@JsonIgnoreProperties(value = ["severity_rating"])
 data class VulnerabilityReference(
     /**
      * The URI pointing to details of the belonging vulnerability.
      */
     val url: URI,
 
     /**
-     * The name of the scoring system, if any, to interpret the [severity] by.
+     * The name of the scoring system, if any, as reported by the advice provider.
      */
     val scoringSystem: String?,
 
     /**
-     * The severity assigned by this reference to the belonging vulnerability. This is string meaning depends on the
-     * [scoringSystem]. It could be a number, but also a semantic expression like "LOW" or "HIGH". If this is `null`,
-     * it means that this reference does not contain any information about the severity.
+     * The severity, if any, this reference assigns to the belonging vulnerability. This string is supposed to be a
+     * qualitative rating like "LOW" or "HIGH".
      */
-    val severity: String?
+    val severity: String?,
+
+    /**
+     * The (base) score, if any, this reference assigns to the belonging vulnerability. The meaning of this number
+     * depends on the [scoringSystem].
+     */
+    val score: Float?,
+
+    /**
+     * The full CVSS vector, if any, this reference assigns to the belonging vulnerability. Note that while the vector
+     * usually contains the [scoringSystem], that is not the case for e.g. CVSS version 2.
+     */
+    val vector: String?
 ) {
     companion object {
-        private val CVSS3_SEVERITIES = Cvss3Rating.entries.map { it.name }
-
         /**
          * Return a qualitative rating that is determined based on the given [scoringSystem] and [score].
          */
@@ -64,18 +73,5 @@ data class VulnerabilityReference(
                 in Cvss4Rating.NAMES -> score?.let { Cvss4Rating.fromScore(it) }
                 else -> null
             }
-
-        /**
-         * Return a human-readable string that is determined based on the given [scoringSystem] and [severity].
-         */
-        fun getSeverityString(scoringSystem: String?, severity: String?): String =
-            severity?.toFloatOrNull()?.let { getQualitativeRating(scoringSystem, it)?.name }
-                ?: severity?.uppercase()?.takeIf { it in CVSS3_SEVERITIES }
-                ?: "UNKNOWN"
     }
-
-    /**
-     * Return a human-readable severity rating string.
-     */
-    val severityRating: String by lazy { getSeverityString(scoringSystem, severity) }
 }
diff --git a/model/src/test/kotlin/AdvisorResultTest.kt b/model/src/test/kotlin/AdvisorResultTest.kt
@@ -289,12 +289,13 @@ private fun createVulnerability(
     id: String,
     uriPrefix: String = "http://cve.mitre.org/cgi-bin/cvename.cgi?name=",
     scoringSystem: String = "cvssv3.1_qr",
-    severity: String = "MODERATE"
+    severity: String = "MEDIUM",
+    score: Float = 5.0f
 ): Vulnerability =
     Vulnerability(
         id = id,
         references = listOf(
-            VulnerabilityReference(URI("$uriPrefix$id"), scoringSystem, severity)
+            VulnerabilityReference(URI("$uriPrefix$id"), scoringSystem, severity, score, null)
         )
     )
 

diff --git a/model/src/test/kotlin/vulnerabilities/VulnerabilityReferenceTest.kt b/model/src/test/kotlin/vulnerabilities/VulnerabilityReferenceTest.kt
@@ -20,36 +20,35 @@
 package org.ossreviewtoolkit.model.vulnerabilities
 
 import io.kotest.core.spec.style.StringSpec
+import io.kotest.matchers.nulls.beNull
+import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 
 class VulnerabilityReferenceTest : StringSpec({
-    "The severity string should be correct for a given CVSS 2 score" {
-        VulnerabilityReference.getSeverityString("CVSS2", "1.0") shouldBe "LOW"
-        VulnerabilityReference.getSeverityString("CVSSV2", "5.0") shouldBe "MEDIUM"
-        VulnerabilityReference.getSeverityString("CVSS:2.0", "8.0") shouldBe "HIGH"
+    "The severity rating should be correct for a given CVSS 2 score" {
+        VulnerabilityReference.getQualitativeRating("CVSS2", 1.0f) shouldBe Cvss2Rating.LOW
+        VulnerabilityReference.getQualitativeRating("CVSSV2", 5.0f) shouldBe Cvss2Rating.MEDIUM
+        VulnerabilityReference.getQualitativeRating("CVSS:2.0", 8.0f) shouldBe Cvss2Rating.HIGH
     }
 
-    "The severity string should be correct for a given CVSS 3 score" {
-        VulnerabilityReference.getSeverityString("CVSS3", "0.0") shouldBe "NONE"
-        VulnerabilityReference.getSeverityString("CVSSV3", "1.0") shouldBe "LOW"
-        VulnerabilityReference.getSeverityString("CVSS:3.0", "5.0") shouldBe "MEDIUM"
-        VulnerabilityReference.getSeverityString("CVSS:3.1", "8.0") shouldBe "HIGH"
-        VulnerabilityReference.getSeverityString("CVSS3", "9.0") shouldBe "CRITICAL"
+    "The severity rating should be correct for a given CVSS 3 score" {
+        VulnerabilityReference.getQualitativeRating("CVSS3", 0.0f) shouldBe Cvss3Rating.NONE
+        VulnerabilityReference.getQualitativeRating("CVSSV3", 1.0f) shouldBe Cvss3Rating.LOW
+        VulnerabilityReference.getQualitativeRating("CVSS:3.0", 5.0f) shouldBe Cvss3Rating.MEDIUM
+        VulnerabilityReference.getQualitativeRating("CVSS:3.1", 8.0f) shouldBe Cvss3Rating.HIGH
+        VulnerabilityReference.getQualitativeRating("CVSS3", 9.0f) shouldBe Cvss3Rating.CRITICAL
     }
 
-    "The severity string should be correct for a given CVSS 4 score" {
-        VulnerabilityReference.getSeverityString("CVSS4", "0.0") shouldBe "NONE"
-        VulnerabilityReference.getSeverityString("CVSSV4", "1.0") shouldBe "LOW"
-        VulnerabilityReference.getSeverityString("CVSS:4.0", "5.0") shouldBe "MEDIUM"
-        VulnerabilityReference.getSeverityString("CVSS4", "8.0") shouldBe "HIGH"
-        VulnerabilityReference.getSeverityString("CVSS4", "9.0") shouldBe "CRITICAL"
+    "The severity rating should be correct for a given CVSS 4 score" {
+        VulnerabilityReference.getQualitativeRating("CVSS4", 0.0f) shouldBe Cvss4Rating.NONE
+        VulnerabilityReference.getQualitativeRating("CVSSV4", 1.0f) shouldBe Cvss4Rating.LOW
+        VulnerabilityReference.getQualitativeRating("CVSS:4.0", 5.0f) shouldBe Cvss4Rating.MEDIUM
+        VulnerabilityReference.getQualitativeRating("CVSS4", 8.0f) shouldBe Cvss4Rating.HIGH
+        VulnerabilityReference.getQualitativeRating("CVSS4", 9.0f) shouldBe Cvss4Rating.CRITICAL
     }
 
-    "The severity string should be correct for a given qualitative rating from an unknown scoring system" {
-        VulnerabilityReference.getSeverityString("", "NONE") shouldBe "NONE"
-        VulnerabilityReference.getSeverityString("", "LOW") shouldBe "LOW"
-        VulnerabilityReference.getSeverityString("", "MEDIUM") shouldBe "MEDIUM"
-        VulnerabilityReference.getSeverityString("", "HIGH") shouldBe "HIGH"
-        VulnerabilityReference.getSeverityString("", "CRITICAL") shouldBe "CRITICAL"
+    "The severity rating should be null if either the system or score is null" {
+        VulnerabilityReference.getQualitativeRating(null, 8.0f) should beNull()
+        VulnerabilityReference.getQualitativeRating("CVSS4", null) should beNull()
     }
 })
diff --git a/plugins/advisors/nexus-iq/src/main/kotlin/NexusIq.kt b/plugins/advisors/nexus-iq/src/main/kotlin/NexusIq.kt
@@ -157,7 +157,7 @@ class NexusIq(override val descriptor: PluginDescriptor, private val config: Nex
         val references = mutableListOf<VulnerabilityReference>()
 
         val browseUrl = URI("${config.browseUrl}/assets/index.html#/vulnerabilities/$reference")
-        val nexusIqReference = VulnerabilityReference(browseUrl, scoringSystem(), severity.toString())
+        val nexusIqReference = VulnerabilityReference(browseUrl, scoringSystem(), threatCategory, severity, null)
 
         references += nexusIqReference
         url.takeIf { it != browseUrl }?.let { references += nexusIqReference.copy(url = it) }

diff --git a/plugins/advisors/oss-index/src/main/kotlin/OssIndex.kt b/plugins/advisors/oss-index/src/main/kotlin/OssIndex.kt
@@ -36,6 +36,7 @@ import org.ossreviewtoolkit.model.AdvisorSummary
 import org.ossreviewtoolkit.model.Issue
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.config.PluginConfiguration
+import org.ossreviewtoolkit.model.vulnerabilities.Cvss2Rating
 import org.ossreviewtoolkit.model.vulnerabilities.Vulnerability
 import org.ossreviewtoolkit.model.vulnerabilities.VulnerabilityReference
 import org.ossreviewtoolkit.plugins.api.OrtPlugin
@@ -136,11 +137,11 @@ class OssIndex(override val descriptor: PluginDescriptor, config: OssIndexConfig
      * [OssIndexService.Vulnerability].
      */
     private fun OssIndexService.Vulnerability.toVulnerability(): Vulnerability {
-        val reference = VulnerabilityReference(
-            url = URI(reference),
-            scoringSystem = cvssVector?.substringBefore('/'),
-            severity = cvssScore.toString()
-        )
+        // Only CVSS version 2 vectors do not contain the "CVSS:" label and version prefix.
+        val scoringSystem = cvssVector?.substringBefore('/', Cvss2Rating.NAMES.first())
+
+        val severity = VulnerabilityReference.getQualitativeRating(scoringSystem, cvssScore)?.name
+        val reference = VulnerabilityReference(URI(reference), scoringSystem, severity, cvssScore, cvssVector)
 
         val references = mutableListOf(reference)
         externalReferences?.mapTo(references) { reference.copy(url = URI(it)) }

diff --git a/plugins/advisors/oss-index/src/test/kotlin/OssIndexTest.kt b/plugins/advisors/oss-index/src/test/kotlin/OssIndexTest.kt
@@ -97,12 +97,16 @@ class OssIndexTest : WordSpec({
                                         "&utm_content=5.0"
                                 ),
                                 scoringSystem = "CVSS:3.0",
-                                severity = "5.5"
+                                severity = "MEDIUM",
+                                score = 5.5f,
+                                vector = "CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:N/A:N"
                             ),
                             VulnerabilityReference(
                                 url = URI("https://nvd.nist.gov/vuln/detail/CVE-2020-15250"),
                                 scoringSystem = "CVSS:3.0",
-                                severity = "5.5"
+                                severity = "MEDIUM",
+                                score = 5.5f,
+                                vector = "CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:N/A:N"
                             )
                         )
                     )