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

Introduce vulnerability component #1295

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

trisch-me
Copy link
Contributor

Introduce a new security namespace Vulnerability.

Fields are taken from ECS Vulnerability namespace

Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.

Merge requirement checklist

@trisch-me trisch-me requested review from a team July 29, 2024 12:19
@adrielp
Copy link
Contributor

adrielp commented Jul 31, 2024

@trisch-me Following up on this PR from this weeks SemConv meeting.

In practice I've noticed that vendors don't always support the convention 1:1. In the OTEL collector we've had to do mappings of attributes for metrics. We currently define a git.repository.cve.count metric. This metric will probably eventually change to vcs.repository.cve.count (or maybe just cve.count with attribute of vcs.repository.url.full). cve.severity is the attribute I'm referring to that doesn't always map. We enum the attribute, but have to remap the value here.

Just wanted to provide this information for additional context. Love seeing these pr's come up.

| `vulnerability.score.base` | double | The base score of the vulnerability. [1] | `7.5` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| `vulnerability.score.environmental` | double | The environmental score of the vulnerability. [2] | `7.5` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| `vulnerability.score.temporal` | double | The temporal score of the vulnerability. [3] | `7.5` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| `vulnerability.score.version` | string | The version of the scoring system used to calculate the vulnerability score. | `3.0` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment here is whether or not it makes sense to have some of these be a CVSS specific attribute, particularly when I see things like score ranging from 0.0 to 10.0.

I like that this is based on the CVSS specification, what I can't tell as an outsider, what other standards can be mapped here. I'm waiting for more security folks to chime in on these to understand if this does, indeed, match expectations for how to communicate vulnerabilities via Events, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsuereth - agreed. If using numbers this should be clearly defined (ie. CVSS).

Also, what examples of "score.version" exist? The other examples in this file assume CVSS (like score), but the classification attribute technically allows for others which may not 1:1 align.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree that there could be some challenges in having the score.version allow for other systems. Some of the fields we've got here are specific to CVSS scoring (e.g. environmental and temporal scores). If the intent is to allow for other scoring regimes (and I do think that makes sense), I'd be inclined to have a vulnerability.score value and not have the other CVSS specific items.

It's also worth noting that generally scanners can't/shouldn't provide values for environmental and temporal metrics as those require information about the working environment of the impacted system that scanners often don't have.

One example of the type of score someone might want to add is EPSS (https://www.first.org/epss/model) which expresses a score as a percentage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsuereth and folks I agree with the idea that we must allow other standards to be taken into account though CVSS is probably the most widely used. We do have also notation about CVSS in comments.

I see here 2 ways of solving it

A

  • introducing score.standard field, which could be used to define the actual standard.
  • score.version and score.base will remain as is as common fields for all standards
  • if any standard requires more fields, those could be added with sub-namespacing in the beginning such score.cvss.temporal etc
  • as of now we will add only CVSS but the schema could be extended if needed

B

  • use every standard under it's own sub-namespace. i.e. current score fields all go under score.cvss.*
  • that way we will not have to define standard additionally.

I prefer the first option as it gives more flexibility in using any standard without updating schema, if we care only about score.base. But this will make base.score definition a bit vague, because different standards can define it using different measurements: 0.0 to 1.0, 0.0 to 10.0, 0.0 to 100.0 etc. But we can add it to the notes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that sounds like a good idea to me, Option A you've outlined there should help clarify this area.

Copy link

@leykin-valeriy leykin-valeriy Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are two things that need to be considered:

  1. The severity/score reported by the vendor that found this vulnerability in a specific artifact.
  2. The various scores this vulnerability may get from various standards

A decision to make: Are we defining a vulnerability object that can later be referenced from a finding, or are we defining a vulnerability finding reported by a vendor on a vulnerable artifact?

Depending on the purpose, it could go in one or another direction.
For 1. - we would need to have a separate severity/score for the finding, and potentially an array of scores defined by various standards.
For 2. - we would still need a score used by the vendor as the base score, and potentially an array of additional scores that were considered by that vendor.

See this example of Snyk:

  • They talk about a vulnerability, define their own score, and reference multiple standards.
  • When they will run the assessment of an artifact against this vulnerability, they will generate a finding event, with the finding score adjusted to additional contextual information.

image

Here is an example of a possible OCSF representation generated from a vulnerability finding:

{
"activity_id": 1,
"category_uid": 2,
"class_uid": 2001,
"finding": {
"created_time": "2024-06-26T07:15:06.139000Z",
"first_seen_time": "2024-06-26T07:15:06.160000Z",
"last_seen_time": "2024-06-26T07:15:06.160000Z",
"modified_time": "2024-06-26T07:15:06.139000Z",
"product_uid": "snyk",
"src_url": "https://app.snyk.io/org/capability_xyz/project/fffffffff-aaaa-dddd-aaaa-dddddddd885e#issue-SNYK-JAVA-COMMICROSOFTAZURE-7246766",
"title": "pack/manager:2.155.0:/app/libs: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition')",
"types": [
"PACKAGE_VULNERABILITY"
],
"uid": "ffffffffffff-e65a-fffff-8804-cacacacacaca:12345678901:us-east-1"
},
"metadata": {
"product": {
"name": "Snyk",
"url_string": "https://snyk.io",
"vendor_name": "Snyk"
},
"version": "1.0.0"
},
"resources": [
{
"data": {
"snyk_org_id": "bbbbbbbbb-dddd-4870-aaaa-abababababa",
"url": "https://app.snyk.io/org/capability_xyz/project/fffffffff-aaaa-dddd-aaaa-dddddddd885e"
},
"name": "pack/manager:2.155.0:/app/libs",
"type": "snyk_project",
"uid": "fffffffff-aaaa-dddd-aaaa-dddddddd885e"
}
],
"severity_id": 3,
"state": "New",
"state_id": 1,
"time": "2024-06-26T07:15:06.139000Z",
"type_uid": 200101,
"vulnerabilities": [
{
"cve": {
"created_time": "0001-01-01T00:00:00Z",
"cvss": {
"base_score": 7,
"version": ""
},
"modified_time": "0001-01-01T00:00:00Z",
"product": {
"name": "",
"vendor_name": ""
},
"uid": ""
}
},
{
"cve": {
"created_time": "0001-01-01T00:00:00Z",
"cvss": {
"base_score": 0,
"version": ""
},
"modified_time": "0001-01-01T00:00:00Z",
"product": {
"name": "",
"vendor_name": ""
},
"uid": "CVE-2024-35255"
},
"fix_available": true,
"packages": [
{
"epoch": "0001-01-01T00:00:00Z",
"name": "com.microsoft.azure",
"version": "msal4j"
}
],
"severity": "medium"
}
]
}

brief: >
A resource that provides additional information, context, and mitigations for the identified vulnerability.
examples: ['https://www.cve.org/CVERecord?id=CVE-2021-44228']
- id: report_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a future case where report would be moved to a namespace where things like url.full could be added to link to the report, or other attributes?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the id of the scan, it would make sense to name scan.id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrielp as we have agreed during one of semconv on having id always in it's own namespace, I will update this field

brief: >
The base score of the vulnerability.
note: >
Scores can range from 0.0 to 10.0, with 10.0 being the most severe. Base scores cover an assessment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an "alternative" would be EPSS which is 0 - 1 so this description wouldn't fit that If the classification changed.

members:
- id: low
value: 'Low'
brief: 'Low severity (0.1 - 3.9)'
Copy link
Contributor

@adrielp adrielp Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
brief: 'Low severity (0.1 - 3.9)'
brief: 'Low severity (0.0 - 3.9)'

https://nvd.nist.gov/vuln-metrics/cvss

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the latest cvss versions (3.x, 4.x) severity is scored from 0.1, because 0.0 means no vulnerability

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 12, 2024
@github-actions github-actions bot removed the Stale label Sep 19, 2024
@trisch-me trisch-me added the never stale PRs marked with this label will be never staled and automatically closed label Sep 23, 2024
For example [Common Vulnerabilities and Exposure CVE description](https://www.cve.org/ResourcesSupport/FAQs).
examples: ['A vulnerability in the firewall allows an attacker to bypass security controls.']
- id: enumeration
type: string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A vulnerability reported by a vendor might relate to multiple CVEs. Would it make sense to have the CVE enumeration as an array?
For example, https://security.snyk.io/vuln/SNYK-JAVA-COMFASTERXMLJACKSONCORE-559094

For example, [Common Vulnerabilities and Exposure CVE ID](https://www.cve.org/ResourcesSupport/FAQs).
examples: ['CVE-2021-44228']
- id: reference
type: string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field might include multiple values, similar to the possible multiple CVEs related to a vulnerability.

Scores can range from 0.0 to 10.0, with 10.0 being the most severe. Temporal scores cover an assessment for
code maturity, remediation level, and confidence. For example, [CVSS](https://www.first.org/cvss/specification-document).
examples: [7.5]
- id: score.version

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we always expect to have only one scoring system represented in the vulnerability?
For example, what if a vendor has its own scoring system and also adds the cvssv2 and cvssv3 info?
Should we allow to store all this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @leykin-valeriy I have provided 2 options in the comment above on how to include multiple standards into namespace, you can give you opinion there, which option do you like more (or maybe have another idea on how accomplish multiple standards)

brief: >
The report or scan identification number.
examples: ['20191018.0001']
- id: scanner.vendor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could come up with a namespace scan to include both the id, and the related vendor/product information.

brief: >
The name of the scanner vendor that identified the vulnerability.
examples: ['Tenable']
- id: score.base

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the terminology of base, temporal, environmental, are all from the CVSS-domain, would it make sense to put this under the corresponding namespace, for example: cvssv2 or cvssv3?

.chloggen/vulnerability.yaml Outdated Show resolved Hide resolved
@trisch-me trisch-me requested review from a team as code owners October 28, 2024 14:59
@trisch-me trisch-me changed the title Introduce vulnerability Introduce vulnerability component Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:security never stale PRs marked with this label will be never staled and automatically closed
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

8 participants