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

Added vulnerabilities as part of core spec #91

Merged
merged 26 commits into from
Dec 20, 2021
Merged

Conversation

stevespringett
Copy link
Member

@stevespringett stevespringett commented Oct 3, 2021

This PR is based on work from #44 but without the limitations of the existing vulnerability extension. This implements #38.

Major changes from the vulnerability extension include:

  • Each vulnerability now has its own bom-ref and can be referenced elsewhere in the BOM.
  • Each vulnerability can now be applicable to 1 or more components or services. In the extension, there was a 1..1 mapping.
  • A vulnerability can now reference zero or more aliases for the same vulnerability across different sources of vuln intel. For example, private vuln intel services typically have their own naming or numbering scheme. It is now possible to use these proprietary identifiers and still reference a CVE in the NVD, if the two vulnerabilities are in fact the same.
  • Each rating now describes the source (e.g NVD, VulnDB, etc) as different sources may have different scores for the same vulnerability.
  • The base, impact, and exploitable subscores were removed in favor of a single 'score' field. These were specific to CVSSv2 and CVSSv3 and are not applicable to OWASP. It is not clear if they will be applicable to CVSSv4. If these scores are wanted, use of the vector can recalculate the scores via automation, so no capabilities are lost.
  • A justification field was added to provide additional context why something was rated the way it was.
  • Vulnerability now has a details field which provides additional information over the 'description' field
  • Recommendations was previously an array. This has changed to a single field and renamed 'remediation'
  • Advisories were previously URLs. This has changed to being objects containing a title and URL.
  • Created, published, and updated timestamp fields were added to the vulnerability
  • Vulnerability now has a 'credits' field
  • Vulnerability now has a 'tools' array which can describe the tools used to discover the vulnerability
  • Vulnerability now has an 'analysis' object containing impact analysis of the vulnerability
  • Vulnerability now includes the ability to utilize properties (introduced in v1.3) so that other data not supported by the schema can be added.

@stevespringett
Copy link
Member Author

Pinging @ddillard, @garethr, @wagoodman, @luhring, @planetlevel.

Your feedback on this (draft) PR is much appreciated. Once the details are flushed out, I'll update the PR to include XML and Protobuf schema support as well.

@stevespringett
Copy link
Member Author

Also pinging @brianf, @ahoog42, @llamahunter

Your feedback on this (draft) PR is much appreciated. Once the details are flushed out, I'll update the PR to include XML and Protobuf schema support as well.

@stevespringett
Copy link
Member Author

stevespringett commented Oct 3, 2021

Also it should be noted that when you review this PR, look at it from two perspectives:

  • Vulnerability information being in the same BOM as the components and other inventory
  • Vulnerability information being externalized into a separate BOM, independent from inventory (e.g. a VEX BOM)

Both use cases will be possible which is exciting.

For external use, since every component or service can have a reference to its own BOM, it would be possible to either:

  • Have a single VEX BOM for the component the BOM describes (the root metadata component)
  • Have multiple VEX BOMs for each and every component and service within the BOM.
  • A combination of the two

Also note that I mentioned components or services. So also look at this PR from the perspective of:

  • known vulnerabilities - identified by SCA and similar tools
  • unknown vulnerabilities - identified from DAST and similar tools

In the future, we may want to add support for vulnerabilities from SAST and IAST/RASP tools, but that will require support for the different types of analyzers (e.g. semantic, control flow, data flow, etc) as well as the ability to support call graphs (including source and sinks). Or we could simply integrate more closely with SARIF. So although this PR will not address SAST today, please keep in mind that it's not off the table for a future version of the spec.

@VinodAnandan
Copy link

@knqyf263 @masahiro331 @raesene @samj1912 @mrutkows @nadgowdas @oliverchang @JLLeitschuh @mlieberman85 @tjarrettveracode

@brianf
Copy link

brianf commented Oct 4, 2021

I looked through the spec but apologies if I missed it, is it possible to include mitigating information about the included vulns? eg "this thing has an old commons-collection cve but it only applies to unsafe deserialization and either this app doesn't have any serialization, or it's already whitelisted and so this can't be exploited here".

@stevespringett
Copy link
Member Author

is it possible to include mitigating information about the included vulns

Absolutely. The spec proposes to include the following analysis states:

  • exploitable
  • false_positive
  • not_applicable
  • in_triage

In addition, both impact and detail fields are provided to provide context about why an analysis decision was made the way it was.

In the event a library was 'fixed'. This is already supported by the 'pedigree' capabilities implemented v1.2 of the spec where users can specify the commit and diffs along with the issues (defects, enhancements, security) that they've resolved in their custom version of a library.

@ddillard
Copy link

ddillard commented Oct 4, 2021

What exactly does "exploitable" mean? Does it mean that it's directly exploitable or both directly and indirectly exploitable?

An example might help: suppose I have an XML parser vuln, but I authenticate the sender of the XML and it's trusted so that vulnerability should not be directly exploitable. However, suppose there's another vulnerability (perhaps one that I'm currently unaware of) and an attacker was able to bypass or otherwise subvert that authentication, and the attacker could then gain some additional privilege or knowledge by then exploiting the parser vuln. Would the parser vuln be considered exploitable?

@stevespringett
Copy link
Member Author

@ddillard I think what you'll find is that organizations will say they're not_affected by this particular vulnerability and in the impact and detail fields they will likely try to communicate why. In this case because access control prohibits exploitability to only authenticated users.

The organization receiving the BOM will have an opportunity to either accept that analysis or push back on it.

But to your point, it might be worth expanding on what exploitable means.

For example:

  • exploitable_confirmed - a PoC exists that demonstrates the exploitability of the vulnerability
  • exploitable_unconfirmed - the vulnerability should theoretically be exploitable, but a PoC does not exist
  • exploitable_directly - The vulnerable code is executed and is exploitable
  • exploitable_indirectly - The vulnerable code is executed but mitigating controls prohibit exploitability

Thoughts?

@nadgowdas
Copy link

Yes, not every user is equally affected by a common single vulnerability. I believe some alignment with this vulnerability disclosure guide might help: https://github.com/ossf/oss-vulnerability-guide/blob/main/guide.md

@oliverchang
Copy link

oliverchang commented Oct 5, 2021

Thanks for looping me in @VinodAnandan !

Would it be possible to align this more with the OSSF OSV vulnerability schema: https://ossf.github.io/osv-schema/ (which has already been adopted by several vulnerability DBs) to enable easier interop?

There seems to be a number of overlapping fields. In particular, OSV's "affected" field lets us be a bit more descriptive about describing version/commit ranges (and this aligns well with the corresponding field in the CVE 5.0 schema which adapted the OSV format).

@stevespringett
Copy link
Member Author

Hi @oliverchang

Is the affected.package field required? I'm not sure how a BOM could properly support affected.package when it needs to support things like containers, an installable operating system in its entirety (e.g. Windows XP Embedded), firmware, a hardware device, or a file. But in the event a CycloneDX component has a purl, then affected.package could be determined for interop.

I think the version and range options are likely easily supportable and I think they would make good additions. I'll add them to the proposal.

It looks like the rest of the OSV spec is already covered by this proposal, so should be easy interop.

@oliverchang
Copy link

Hi @oliverchang

Is the affected.package field required? I'm not sure how a BOM could properly support affected.package when it needs to support things like containers, an installable operating system in its entirety (e.g. Windows XP Embedded), firmware, a hardware device, or a file. But in the event a CycloneDX component has a purl, then affected.package could be determined for interop.

No, it is not a required field. In cases where there is no PURL/package it can be left out.

I think the version and range options are likely easily supportable and I think they would make good additions. I'll add them to the proposal.

Thank you!

It looks like the rest of the OSV spec is already covered by this proposal, so should be easy interop.

Agreed :)

@masahiro331
Copy link

masahiro331 commented Oct 5, 2021

I'm masahiro331. May I join this discussion?
I looked through the spec but apologies if I missed it, I would like to ask this question in the context of SCA.

The recommendation component describes how to fix or work around a vulnerability, but it does not mechanically determine whether a patch will be provided in the first place.
So, how about defining the existence of patches?

@stevespringett
Copy link
Member Author

@masahiro331 Software patches can refer to different things depending on if you're a developer or operator of software. The term 'patch' is already used in CycloneDX to describe code modifications made to a component. When you refer to 'patches' are you referring to the availability of unaffected (safe) versions? If so, would you recommend this be a boolean field, or an array or range of alternative versions?

@masahiro331
Copy link

masahiro331 commented Oct 5, 2021

@stevespringett
Thank you for your reply.

I referring to the availability of unaffected (safe) versions.

If so, would you recommend this be a boolean field or an array or range of alternative versions?

I think the modification state and the modified versions need to be defined separately.

I think enum is better for the modification state, not bool.
There are at least three states in the modification state.

  • Fixed
  • Will not fixed
  • Not fixed (to be modified in the future)

The other states are possible, but they are considered to be a combination of the vulnerability impact.

At the very least, it would be better if the modified version is listed in the recommendation and can be parsed mechanically if possible. (This is a very difficult problem, so I don't think it is essential.)

The unaffected(safe) version is described in the recommendation, so it is not a problem.
However, it would be better to be able to parse them mechanically if possible. (This is a very difficult issue, so I don't think it's necessary at this time.)

@stevespringett
Copy link
Member Author

@masahiro331

I referring to the availability of unaffected (safe) versions.

Good. I'll add support for that.

I think the modification state and the modified versions need to be defined separately.

I'm a little confused on the word modification and modified version. CycloneDX already support modified components though it support of pedigree. See https://cyclonedx.org/use-cases/#pedigree. With pedigree, uses can communicate specifically what they've fixed (enhancement, defect, security issues) and how (commits, diffs). CycloneDX does not support a 'fixed' state without specifying more info.

I think 'wont fix' and 'not fixed' would possibly be good additions to the vulnerability analysis section, likely in relation to impact.

"description": "Textual representation of the severity of the vulnerability adopted by the risk analysis method. If an other risk analysis method is used other than whats defined in scoreSourceType, the user is expected to translate appropriately to match with an element value below.",
"enum": [
"critical",
"high",

Choose a reason for hiding this comment

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

Is this a generic rating of this vulnerability, like the CVSS score? Or is the scoring intended to be specific to the particular application that this component is part of? If the latter, then the description could be updated to emphasize that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Multiple ratings can be specified, including those from the NVD and for the creators or operators of the application that component is a part of. OWASP Risk Rating is also supported here.

@planetlevel
Copy link

  • known vulnerabilities - identified by SCA and similar tools
  • unknown vulnerabilities - identified from DAST and similar tools

In the future, we may want to add support for vulnerabilities from SAST and IAST/RASP tools, but that will require support for the different types of analyzers (e.g. semantic, control flow, data flow, etc) as well as the ability to support call graphs (including source and sinks). Or we could simply integrate more closely with SARIF. So although this PR will not address SAST today, please keep in mind that it's not off the table for a future version of the spec.

I'm confused. Why couldn't we represent vulnerabilities from SAST and IAST tools in the BOM? I wasn't thinking that the BOM would have all the details from a DAST/SAST/IAST tool... certainly not a fulll SARIF file... just the high level details. We don't have any of that kind of detail for vulnerabilities in OSS components. Can you explain the thinking here a bit more?

@stevespringett
Copy link
Member Author

@planetlevel

Why couldn't we represent vulnerabilities from SAST and IAST tools in the BOM?

Well, you can. My thought process was more around WHY. So if a vulnerable function/method is not called, being able to communicate the technical details of why a vulnerable function/method is not reachable is information that SAST or IAST tools would know, but would be really difficult to communicate in an SBOM. That's what I meant by that statement.

Of course, IAST can communicate if something is or is not vulnerable. I think we need to figure out the right level of detail without over engineering.

@stevespringett
Copy link
Member Author

One approach may be to do something like:

"affects": [
  {
    "ref": "pkg:maven/com.fasterxml.jackson.core/[email protected]",
    "versions": [
      {
        "version": "2.9.0",
        "versionType": "semver",
        "lessThanOrEqual": "2.9.4",
        "status": "affected"
      }
    ],
    "constraints": [
      "appliesTo": [
        {
          "route": ["acme-x", "component-a", "component-b", "component-c"],
          "status": "affected"
        },
        {
          "route": ["acme-x", "component-d", "component-b", "component-c"],
          "status": "not_affected"
        },
        {
          "route": ["acme-x", "component-e", "component-c"],
          "status": "not_affected"
        },
        {
          "route": ["acme-x", "component-c"],
          "status": "affected"
        }
      ]
    ]
  }
]

If this is valuable information to represent, we could certainly do that. My only issue with this is that its too high level and focuses on the component, not the call graph. For example,

There may be multiple paths through acme-x -> component-e -> component-c, and only one leads to a vulnerability.
"route": ["acme-x", "component-e", "component-c"],

Representing the source and sink in a call stack would be highly valuable in that case, but would require us to integrate with SARIF.

@stevespringett
Copy link
Member Author

@coderpatros, @DarthHater, any feedback on #91 (comment)?

@stevespringett
Copy link
Member Author

Per CycloneDX Core meeting - make 'response' as shown here an array.

…array of organizations and individuals.

Signed-off-by: Steve Springett <[email protected]>
@stevespringett
Copy link
Member Author

stevespringett commented Dec 7, 2021

Migrate version range syntax to Package URL version range syntax (vers)
package-url/purl-spec#139

Currently, this PR is based on CVE 5.0 schema, which has a number of problems including an undefined way to specify version range type.

…ge URL version range syntax, which is compatible with CVE 5.0 and OSV syntax.

Signed-off-by: Steve Springett <[email protected]>
Signed-off-by: Steve Springett <[email protected]>
@stevespringett stevespringett marked this pull request as ready for review December 17, 2021 21:02
@stevespringett stevespringett added RFC vote accepted and removed RFC notice sent A public RFC notice was distributed to the CycloneDX mailing list for consideration labels Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.