From b5cc0ea487a5151fa0d4830f94bb063f0d9bf520 Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Fri, 29 Nov 2024 15:56:21 +0100 Subject: [PATCH] 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 --- advisor/build.gradle.kts | 1 + advisor/src/main/kotlin/Advisor.kt | 42 ++++++++++++++++++- advisor/src/test/kotlin/AdvisorTest.kt | 1 + plugins/advisors/osv/build.gradle.kts | 1 - ...ieve-package-findings-expected-result.json | 8 ++-- .../osv/src/funTest/kotlin/OsvFunTest.kt | 3 +- plugins/advisors/osv/src/main/kotlin/Osv.kt | 13 +----- .../funTest/kotlin/VulnerableCodeFunTest.kt | 25 +++++------ .../src/main/kotlin/VulnerableCode.kt | 18 ++++---- .../src/test/kotlin/VulnerableCodeTest.kt | 12 ++++-- 10 files changed, 79 insertions(+), 45 deletions(-) diff --git a/advisor/build.gradle.kts b/advisor/build.gradle.kts index 6c75e86c9a4c2..afd94cd04e0ee 100644 --- 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 index 57f2bccd1df8a..b63a6eefdcbef 100644 --- 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.normalizeVulnerabilityData(): List = + 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 index 370db5c545a1e..c61010df2aa86 100644 --- 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().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 index f2860f558a5b9..1f44ee9292bc2 100644 --- 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 index 222c6cd8a37f2..f80fdf269a0f0 100644 --- 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 index 4a87b71dd6b23..8001467eb7b7a 100644 --- 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.patchTimes(): Map = 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 index c0fd0b1678276..fbd8702072a06 100644 --- 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 index e1ef63ca18a00..f6c76d052636c 100644 --- 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 index 79b2b09a762c5..3fd072c0145cb 100644 --- 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 index a020e7a52d7a1..078561140308e 100644 --- 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" {