feat(advisor): Centrally normalize vulnerability data

There are more advisors than VulnerableCode that report "MODERATE" instead of "MEDIUM" as a severity. To address this, generally normalize vulnerability data independently of the advisor. This guarantees uniform data for more convenient use in rules and other places that implement conditions based on e.g. the severity, and results in a more consistent experience when displaying results in a UI. Signed-off-by: Sebastian Schuberth <[email protected]>
oss-review-toolkit · Dec 5, 2024 · b5cc0ea · b5cc0ea
1 parent 254809a
commit b5cc0ea
Show file tree

Hide file tree

Showing 10 changed files with 79 additions and 45 deletions.
diff --git a/advisor/build.gradle.kts b/advisor/build.gradle.kts
@@ -29,6 +29,7 @@ dependencies {
 
     implementation(projects.utils.ortUtils)
 
+    implementation(libs.aeSecurity)
     implementation(libs.kotlinx.coroutines)
 
     testImplementation(libs.mockk)

diff --git a/advisor/src/main/kotlin/Advisor.kt b/advisor/src/main/kotlin/Advisor.kt
@@ -27,12 +27,16 @@ import kotlinx.coroutines.withContext
 
 import org.apache.logging.log4j.kotlin.logger
 
+import org.metaeffekt.core.security.cvss.CvssVector
+
 import org.ossreviewtoolkit.model.AdvisorResult
 import org.ossreviewtoolkit.model.AdvisorRun
 import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.OrtResult
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.config.AdvisorConfiguration
+import org.ossreviewtoolkit.model.vulnerabilities.Vulnerability
+import org.ossreviewtoolkit.model.vulnerabilities.VulnerabilityReference
 import org.ossreviewtoolkit.plugins.api.PluginConfig
 import org.ossreviewtoolkit.utils.ort.Environment
 
@@ -101,7 +105,9 @@ class Advisor(
                     // Merge results from different providers into a single map keyed by the package ID. The original
                     // provider is still maintained as part of the AdvisorResult's AdvisorDetails.
                     providerResults.await().forEach { (pkg, advisorResults) ->
-                        results.merge(pkg.id, listOf(advisorResults)) { existingResults, additionalResults ->
+                        val normalizedResults = advisorResults.normalizeVulnerabilityData()
+
+                        results.merge(pkg.id, listOf(normalizedResults)) { existingResults, additionalResults ->
                             existingResults + additionalResults
                         }
                     }
@@ -113,3 +119,37 @@ class Advisor(
             AdvisorRun(startTime, endTime, Environment(), config, results)
         }
 }
+
+fun AdvisorResult.normalizeVulnerabilityData(): AdvisorResult =
+    copy(vulnerabilities = vulnerabilities.normalizeVulnerabilityData())
+
+fun List<Vulnerability>.normalizeVulnerabilityData(): List<Vulnerability> =
+    map { vulnerability ->
+        val normalizedReferences = vulnerability.references.map { reference ->
+            reference
+                .run {
+                    // Treat "MODERATE" as an alias for "MEDIUM" independently of the scoring system.
+                    if (severity == "MODERATE") copy(severity = "MEDIUM") else this
+                }
+                .run {
+                    // Reconstruct the base score from the vector if possible.
+                    if (score == null && vector != null) {
+                        val score = CvssVector.parseVector(vector)?.baseScore?.toFloat()
+                        copy(score = score)
+                    } else {
+                        this
+                    }
+                }
+                .run {
+                    // Reconstruct the severity from the scoring system and score if possible.
+                    if (severity == null && scoringSystem != null && score != null) {
+                        val severity = VulnerabilityReference.getQualitativeRating(scoringSystem, score)?.name
+                        copy(severity = severity)
+                    } else {
+                        this
+                    }
+                }
+        }
+
+        vulnerability.copy(references = normalizedReferences)
+    }
diff --git a/advisor/src/test/kotlin/AdvisorTest.kt b/advisor/src/test/kotlin/AdvisorTest.kt
@@ -154,4 +154,5 @@ private fun mockkAdviceProvider(): AdviceProvider =
 private fun mockkAdvisorResult(): AdvisorResult =
     mockk<AdvisorResult>().apply {
         every { vulnerabilities } returns emptyList()
+        every { copy(vulnerabilities = any()) } returns this
     }
diff --git a/plugins/advisors/osv/build.gradle.kts b/plugins/advisors/osv/build.gradle.kts
@@ -30,7 +30,6 @@ dependencies {
     implementation(projects.utils.commonUtils)
     implementation(projects.utils.ortUtils)
 
-    implementation(libs.aeSecurity)
     implementation(libs.kotlinx.serialization.core)
     implementation(libs.kotlinx.serialization.json)
 

diff --git a/plugins/advisors/osv/src/funTest/assets/retrieve-package-findings-expected-result.json b/plugins/advisors/osv/src/funTest/assets/retrieve-package-findings-expected-result.json
@@ -15,25 +15,25 @@
       "references" : [ {
         "url" : "https://nvd.nist.gov/vuln/detail/CVE-2020-7764",
         "scoring_system" : "CVSS_V3",
-        "severity" : "MODERATE",
+        "severity" : "MEDIUM",
         "score" : 5.9,
         "vector" : "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H"
       }, {
         "url" : "https://github.com/delvedor/find-my-way/commit/ab408354690e6b9cf3c4724befb3b3fa4bb90aac",
         "scoring_system" : "CVSS_V3",
-        "severity" : "MODERATE",
+        "severity" : "MEDIUM",
         "score" : 5.9,
         "vector" : "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H"
       }, {
         "url" : "https://snyk.io/vuln/SNYK-JS-FINDMYWAY-1038269",
         "scoring_system" : "CVSS_V3",
-        "severity" : "MODERATE",
+        "severity" : "MEDIUM",
         "score" : 5.9,
         "vector" : "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H"
       }, {
         "url" : "https://www.npmjs.com/package/find-my-way",
         "scoring_system" : "CVSS_V3",
-        "severity" : "MODERATE",
+        "severity" : "MEDIUM",
         "score" : 5.9,
         "vector" : "CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H"
       } ]

diff --git a/plugins/advisors/osv/src/funTest/kotlin/OsvFunTest.kt b/plugins/advisors/osv/src/funTest/kotlin/OsvFunTest.kt
@@ -28,6 +28,7 @@ import io.kotest.matchers.shouldNot
 
 import java.time.Instant
 
+import org.ossreviewtoolkit.advisor.normalizeVulnerabilityData
 import org.ossreviewtoolkit.model.AdvisorResult
 import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.Package
@@ -94,7 +95,7 @@ private fun createOsv(): Osv = OsvFactory.create()
 
 private fun Map<Identifier, AdvisorResult>.patchTimes(): Map<Identifier, AdvisorResult> =
     mapValues { (_, advisorResult) ->
-        advisorResult.copy(
+        advisorResult.normalizeVulnerabilityData().copy(
             summary = advisorResult.summary.copy(
                 startTime = Instant.EPOCH,
                 endTime = Instant.EPOCH

diff --git a/plugins/advisors/osv/src/main/kotlin/Osv.kt b/plugins/advisors/osv/src/main/kotlin/Osv.kt
@@ -26,8 +26,6 @@ import kotlinx.serialization.json.contentOrNull
 
 import org.apache.logging.log4j.kotlin.logger
 
-import org.metaeffekt.core.security.cvss.CvssVector
-
 import org.ossreviewtoolkit.advisor.AdviceProvider
 import org.ossreviewtoolkit.advisor.AdviceProviderFactory
 import org.ossreviewtoolkit.clients.osv.OsvServiceWrapper
@@ -167,17 +165,10 @@ private fun Vulnerability.toOrtVulnerability(): org.ossreviewtoolkit.model.vulne
                 // Use the 'severity' property of the unspecified 'databaseSpecific' object.
                 // See also https://github.com/google/osv.dev/issues/484.
                 val specificSeverity = databaseSpecific?.get("severity")
-
-                val baseScore = runCatching {
-                    CvssVector.parseVector(vector)?.baseScore?.toFloat()
-                }.onFailure {
-                    logger.debug { "Unable to parse CVSS vector '$vector': ${it.collectMessages()}." }
-                }.getOrNull()
-
                 val severityRating = (specificSeverity as? JsonPrimitive)?.contentOrNull
-                    ?: VulnerabilityReference.getQualitativeRating(scoringSystem, baseScore)?.name
 
-                VulnerabilityReference(it, scoringSystem, severityRating, baseScore, vector)
+                // OSV never provides the numeric base score as it can be calculated from the vector string.
+                VulnerabilityReference(it, scoringSystem, severityRating, null, vector)
             }.getOrNull()
         }
     }

diff --git a/plugins/advisors/vulnerable-code/src/funTest/kotlin/VulnerableCodeFunTest.kt b/plugins/advisors/vulnerable-code/src/funTest/kotlin/VulnerableCodeFunTest.kt
@@ -26,6 +26,7 @@ import io.kotest.matchers.nulls.shouldNotBeNull
 import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 
+import org.ossreviewtoolkit.advisor.normalizeVulnerabilityData
 import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.utils.toPurl
@@ -37,10 +38,10 @@ class VulnerableCodeFunTest : WordSpec({
             val id = Identifier("Go::github.com/quic-go/quic-go:0.40.0")
             val pkg = Package.EMPTY.copy(id, purl = id.toPurl())
 
-            val findings = vc.retrievePackageFindings(setOf(pkg))
+            val results = vc.retrievePackageFindings(setOf(pkg)).values.map { it.normalizeVulnerabilityData() }
 
-            findings.values.flatMap { it.summary.issues } should beEmpty()
-            with(findings.values.flatMap { it.vulnerabilities }.associateBy { it.id }) {
+            results.flatMap { it.summary.issues } should beEmpty()
+            with(results.flatMap { it.vulnerabilities }.associateBy { it.id }) {
                 keys shouldContainAll setOf(
                     "CVE-2023-49295"
                 )
@@ -63,10 +64,10 @@ class VulnerableCodeFunTest : WordSpec({
             val id = Identifier("Maven:com.google.guava:guava:19.0")
             val pkg = Package.EMPTY.copy(id, purl = id.toPurl())
 
-            val findings = vc.retrievePackageFindings(setOf(pkg))
+            val results = vc.retrievePackageFindings(setOf(pkg)).values.map { it.normalizeVulnerabilityData() }
 
-            findings.values.flatMap { it.summary.issues } should beEmpty()
-            with(findings.values.flatMap { it.vulnerabilities }.associateBy { it.id }) {
+            results.flatMap { it.summary.issues } should beEmpty()
+            with(results.flatMap { it.vulnerabilities }.associateBy { it.id }) {
                 keys shouldContainAll setOf(
                     "CVE-2018-10237",
                     "CVE-2020-8908",
@@ -89,10 +90,10 @@ class VulnerableCodeFunTest : WordSpec({
             val id = Identifier("Maven:org.apache.commons:commons-compress:1.23.0")
             val pkg = Package.EMPTY.copy(id, purl = id.toPurl())
 
-            val findings = vc.retrievePackageFindings(setOf(pkg))
+            val results = vc.retrievePackageFindings(setOf(pkg)).values.map { it.normalizeVulnerabilityData() }
 
-            findings.values.flatMap { it.summary.issues } should beEmpty()
-            with(findings.values.flatMap { it.vulnerabilities }.associateBy { it.id }) {
+            results.flatMap { it.summary.issues } should beEmpty()
+            with(results.flatMap { it.vulnerabilities }.associateBy { it.id }) {
                 keys shouldContainAll setOf(
                     "CVE-2023-42503"
                 )
@@ -115,10 +116,10 @@ class VulnerableCodeFunTest : WordSpec({
             val id = Identifier("NPM::elliptic:6.5.7")
             val pkg = Package.EMPTY.copy(id, purl = id.toPurl())
 
-            val findings = vc.retrievePackageFindings(setOf(pkg))
+            val results = vc.retrievePackageFindings(setOf(pkg)).values.map { it.normalizeVulnerabilityData() }
 
-            findings.values.flatMap { it.summary.issues } should beEmpty()
-            with(findings.values.flatMap { it.vulnerabilities }.associateBy { it.id }) {
+            results.flatMap { it.summary.issues } should beEmpty()
+            with(results.flatMap { it.vulnerabilities }.associateBy { it.id }) {
                 keys shouldContainAll setOf(
                     "CVE-2024-48948"
                 )

diff --git a/plugins/advisors/vulnerable-code/src/main/kotlin/VulnerableCode.kt b/plugins/advisors/vulnerable-code/src/main/kotlin/VulnerableCode.kt
@@ -155,18 +155,14 @@ class VulnerableCode(override val descriptor: PluginDescriptor, config: Vulnerab
 
             if (scores.isEmpty()) return listOf(VulnerabilityReference(sourceUri, null, null, null, null))
 
-            return scores.map { (scoringSystem, scoringElements, value) ->
-                val score = value.toFloatOrNull()
-
-                val severity = if (score != null) {
-                    VulnerabilityReference.getQualitativeRating(scoringSystem, score)?.name
-                } else {
-                    // VulnerableCode returns "MODERATE" instead of "MEDIUM" in case of the "cvssv3.1_qr" scoring
-                    // system, see https://github.com/aboutcode-org/vulnerablecode/issues/1186.
-                    value.takeUnless { it == "MODERATE" && scoringSystem == "cvssv3.1_qr" } ?: "MEDIUM"
-                }
+            return scores.map {
+                // In VulnerableCode's data model, a Score class's value is either a numeric score or a severity string.
+                val score = it.value.toFloatOrNull()
+                val severity = it.value.takeUnless { score != null }
+
+                val vector = it.scoringElements?.ifEmpty { null }
 
-                VulnerabilityReference(sourceUri, scoringSystem, severity, score, scoringElements?.ifEmpty { null })
+                VulnerabilityReference(sourceUri, it.scoringSystem, severity, score, vector)
             }
         }.onFailure {
             issues += createAndLogIssue(

diff --git a/plugins/advisors/vulnerable-code/src/test/kotlin/VulnerableCodeTest.kt b/plugins/advisors/vulnerable-code/src/test/kotlin/VulnerableCodeTest.kt
@@ -40,6 +40,7 @@ import io.kotest.matchers.shouldBe
 import java.io.File
 import java.net.URI
 
+import org.ossreviewtoolkit.advisor.normalizeVulnerabilityData
 import org.ossreviewtoolkit.model.AdvisorCapability
 import org.ossreviewtoolkit.model.AdvisorDetails
 import org.ossreviewtoolkit.model.Identifier
@@ -111,7 +112,7 @@ class VulnerableCodeTest : WordSpec({
                     )
                 )
             )
-            langResult.vulnerabilities should containExactly(expLangVulnerability)
+            langResult.vulnerabilities.normalizeVulnerabilityData() should containExactly(expLangVulnerability)
 
             val strutsResult = result.getValue(idStruts)
             strutsResult.advisor shouldBe vulnerableCode.details
@@ -149,7 +150,8 @@ class VulnerableCodeTest : WordSpec({
                     )
                 )
             )
-            strutsResult.vulnerabilities should containExactlyInAnyOrder(expStrutsVulnerabilities)
+            strutsResult.vulnerabilities.normalizeVulnerabilityData() should
+                containExactlyInAnyOrder(expStrutsVulnerabilities)
         }
 
         "extract the CVE ID from an alias" {
@@ -178,7 +180,8 @@ class VulnerableCodeTest : WordSpec({
                     )
                 )
             )
-            result.getValue(idJUnit).vulnerabilities should containExactly(expJunitVulnerability)
+            result.getValue(idJUnit).vulnerabilities.normalizeVulnerabilityData() should
+                containExactly(expJunitVulnerability)
         }
 
         "extract other official identifiers from aliases" {
@@ -214,7 +217,8 @@ class VulnerableCodeTest : WordSpec({
                     )
                 )
             )
-            result.getValue(idLog4j).vulnerabilities should containExactlyInAnyOrder(expLog4jVulnerabilities)
+            result.getValue(idLog4j).vulnerabilities.normalizeVulnerabilityData() should
+                containExactlyInAnyOrder(expLog4jVulnerabilities)
         }
 
         "handle a failure response from the server" {