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

Move severity from VulnerabilityReference to Vulnerability and make it a list #5598

Open
fviernau opened this issue Jul 21, 2022 · 3 comments
Assignees
Labels
advisor About the advisor tool

Comments

@fviernau
Copy link
Member

fviernau commented Jul 21, 2022

Looking at all provider implementations it seems that having the severity as property of a reference is not necessary, because all
providers do create redundant entries:

  1. VulnerableCode
  • has multiple severities per violation but just one URL, so duplicates the URLs
  • private fun VulnerableCodeService.VulnerabilityReference.toModel(): List<VulnerabilityReference> {
    val sourceUri = URI(url)
    if (scores.isEmpty()) return listOf(VulnerabilityReference(sourceUri, null, null))
    return scores.map { VulnerabilityReference(sourceUri, it.scoringSystem, it.value) }
    }
  1. OssIndex
  • has multiple references but single severity, so duplicates the severities in order to provide the URLs
  • private fun OssIndexService.Vulnerability.toVulnerability(): Vulnerability {
    val reference = VulnerabilityReference(
    url = URI(reference),
    scoringSystem = cvssVector?.substringBefore('/'),
    severity = cvssScore.toString()
    )
    val references = mutableListOf(reference)
    externalReferences?.mapTo(references) { reference.copy(url = URI(it)) }
    return Vulnerability(cve ?: displayName ?: title, references)
    }
  1. Osv
  • has multiple URLs, but (currently) a single severity value. So duplicates the severity
  • val references = references.mapNotNull { reference ->
    val url = reference.url.trim().let { if (it.startsWith("://")) "https$it" else it }
    url.toUri().onFailure {
    log.debug { "Could not parse reference URL for vulnerability '$id': ${it.message}." }
    }.map {
    VulnerabilityReference(
    url = it,
    scoringSystem = scoringSystem,
    severity = severity,
    )
    }.getOrNull()
    }
  1. NexusIq
  • has either 2 or 1 URLs and one severity, So may duplicate severity
  • private fun NexusIqService.SecurityIssue.toVulnerability(): Vulnerability {
    val references = mutableListOf<VulnerabilityReference>()
    val browseUrl = URI("${nexusIqConfig.browseUrl}/assets/index.html#/vulnerabilities/$reference")
    val nexusIqReference = VulnerabilityReference(browseUrl, scoringSystem(), severity.toString())
    references += nexusIqReference
    url.takeIf { it != browseUrl }?.let { references += nexusIqReference.copy(url = it) }
    return Vulnerability(reference, references)
    }

So ORT's data structure isn't a good match for any of the above as in each case the mapping introduces redundancy.
Aligning ORT's data model with the one from OSV in this regard, would provide a good match in all of the four cases.
So I propose to align by doing the following:

  1. Move severity from VulnerabilityReference to Vulnerability
  2. Turn Vulnerability.severity (tuples) into an array / list with same semantics as in OSV: severity represented in different scoring systems.

Note: The current approach with severity next to URL was invented by [1]. The change in Vulnerabity / VulnereabilityReference is inspired by / aligns with the model of Vulnerable code.

[1] This is the origin of the current](#3823

@fviernau fviernau changed the title Move severity from VulnerabilityReference to Vulnerability (and make it a list) Move severity from VulnerabilityReference to Vulnerability and make it a list Jul 21, 2022
@fviernau fviernau self-assigned this Jul 21, 2022
@fviernau fviernau added the advisor About the advisor tool label Jul 21, 2022
@sschuberth
Copy link
Member

So I propose to align by doing the following:

  1. Move severity from VulnerabilityReference to Vulnerability

  2. Turn Vulnerability.severity (tuples) into an array / list with same semantics as in OSV: severity represented in different scoring systems.

I'm unsure whether that works out: So far my understanding was that different parties could assign different severities (for the same scoring system) to the same vulnerability, simply because the judgment of the impact of a vulnerability is subjective, after all. And that's what ORT's data model allows. IIRC I even saw that case in real-life once.

@fviernau
Copy link
Member Author

fviernau commented Jul 21, 2022

I'm unsure whether that works out: So far my understanding was that different parties could assign different severities (for the same scoring system) to the same vulnerability, simply because the judgment of the impact of a vulnerability is subjective, after all. And that's what ORT's data model allows. IIRC I even saw that case in real-life once.

If it's an edge case which happens rarely, is it maybe sufficiently addressed by the severity array in the Vulnerability class? So, multiple severities of the different parties can be added there.

edit: All in all I believe moving the severity to Vulnerability only negatively affects the use case of figuring out which severity corresponds to which URL. I believe loosing this information is a good trade, as we get a simpler model with less redundant data which still has all severities of all sources in one list, so they can be consumed by e.g. the policy rules.

@sschuberth
Copy link
Member

If it's an edge case which happens rarely, is it maybe sufficiently addressed by the severity array in the Vulnerability class?

I'm not sure. Maybe @oheger-bosch can report back if they're seeing that case on their side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advisor About the advisor tool
Projects
None yet
Development

No branches or pull requests

2 participants