Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

Draft vulnerability schema extension #19

Merged

Conversation

@kakumara
Copy link
Contributor

kakumara commented Sep 2, 2019

Adding a draft software vulnerability schema for discussion and getting feedback.

Note: the descriptions are not final and to be updated later on, upon structure finalization.

References: https://github.com/jeremylong/DependencyCheck/blob/master/core/src/main/resources/schema/dependency-check.2.2.xsd

Copy link
Member

stevespringett left a comment

The vulnerability schema used by Dependency-Check is inadequate to represent vulnerabilities in a BOM.

schema/ext/vulnerability-1.0.xsd Outdated Show resolved Hide resolved
schema/ext/vulnerability-1.0.xsd Outdated Show resolved Hide resolved
schema/ext/vulnerability-1.0.xsd Outdated Show resolved Hide resolved
schema/ext/vulnerability-1.0.xsd Outdated Show resolved Hide resolved
</xs:element>
<xs:element name="detection" type="xs:string" minOccurs="0" maxOccurs="1">
<xs:annotation>
<xs:documentation>The detection of the vulnerability</xs:documentation>

This comment has been minimized.

Copy link
@stevespringett

stevespringett Sep 2, 2019

Member

This needs more clarification. What is detection?

This comment has been minimized.

Copy link
@kakumara

kakumara Sep 5, 2019

Author Contributor

This needs more clarification. What is detection?

This is used as an attribute to describe vulnerabilities in sonatype products. Removed it considering the generic nature of this schema. efa5e75

- Removed causes, detection sections
</xs:sequence>
</xs:complexType>

<xs:element name="vulnerabilities">

This comment has been minimized.

Copy link
@stevespringett

stevespringett Sep 5, 2019

Member

Is the intended use of the vulnerabilities node to reside inside each component?

<bom>
  <components>
    <component>
      <dg:vulnerabilities>
        <dg:vulnerability>
        </dg:vulnerability>
      </dg:vulnerabilities>
    </component>
  </components>
</bom>

Whatever the intended location, guidance in the form of documentation would be necessary

This comment has been minimized.

Copy link
@ja98200wilcox

ja98200wilcox Sep 5, 2019

Is the intended use of the vulnerabilities node to reside inside each component?

That is the intended location to be inside a component element. Seems like a natural fit. Would you rather have it somewhere else?

This comment has been minimized.

Copy link
@stevespringett

stevespringett Sep 5, 2019

Member

Makes perfect sense to me as well, but the element wasn't annotated with documentation that provided any guidance. So although I assumed this to be the case, the specification did not explicitly state that.

This comment has been minimized.

Copy link
@ja98200wilcox

ja98200wilcox Sep 5, 2019

guidance in the form of documentation would be necessary

Will add appropriate documentation elements that provide guidance

This is used by CVSS
@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 9, 2019

@stevespringett How does the vulnerability schema get tied to the SBOM schema? The vulnerability schema documentation is the tie?

@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 9, 2019

@ja98200wilcox
bom-ref might be an option as it mandates a unique identifier for each component. Then for each vulnerability, you could potentially refer to each component via its bom-ref.

I would prefer this schema to be flexible enough to be used inside and outside a component. Look at externalResources as an example. It supports both a bom-level representation and inside the component as well. If the vulnerability schema is used outside of a component, it will need to use the bom-ref.

The two use-cases that I see this schema being used for are:

  • Specifying zero or more vulnerabilities for each component
  • Specifying zero or more vulnerabilities for the asset/application that a bom represents
@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 9, 2019

bom-ref might be an option as it mandates a unique identifier for each component.

I see the dependency-graph-1.0.xsd does that. Thank you. Didn't see that tie in before.

@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 11, 2019

bom-ref might be an option as it mandates a unique identifier for each component.

Added bom-ref to vulnerability
<vulnerability ref="string">

@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 13, 2019

Hi @stevespringett, anything else you like changed? What are the next steps you like us to do?

@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 14, 2019

I will review this weekend and provide feedback. Once released, the extension will have a page similar to https://cyclonedx.org/ext/dependency-graph/

- Updated to use anyURI type for vuln URL
@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 18, 2019

I think we're really close. I see a few minor issues when using the schema:

<v:vulnerabilityScore>
    <v:score>
        <v:score>
            <v:base>9.8</v:base>
            <v:impact>5.9</v:impact>
            <v:exploitability>3.0</v:exploitability>
        </v:score>
        <v:severity>Critical</v:severity>
        <v:source>CVSSv3</v:source>
        <v:vector>AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H</v:vector>
    </v:score>
</v:vulnerabilityScore>

The <vulnerabilityScore> should be renamed to a plural. 'ratings' maybe?

Inside <ratings> should be a <rating> and inside that should be the <score>. Having two <score> nodes with different meanings is not acceptable.

Proposed changes:

<v:ratings>
    <v:rating>
        <v:score>
            <v:base>9.8</v:base>
            <v:impact>5.9</v:impact>
            <v:exploitability>3.0</v:exploitability>
        </v:score>
        <v:severity>Critical</v:severity>
        <v:source>CVSSv3</v:source>
        <v:vector>AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H</v:vector>
    </v:rating>
</v:ratings>

If you have a proposal other than 'ratings' that also works or is more descriptive of the nodes, please let me know.

The final thing that needs correction is the score decimal restrictions. Currently, any decimal is allowed. So -999, 15.8, and 5.99999 are all allowed. This should be restricted to 0.0 - 10.0 and be limited to a single decimal point.

@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 18, 2019

One final point on the above. The <v:source>CVSSv3</v:source> should likely be changed to <v:method>CVSSv3</v:method>. So we will be describing the method used in risk rating the vulnerability. To me, that makes more sense.

 - vulnerabilityScore --> rating
 - source --> method
@kakumara

This comment has been minimized.

Copy link
Contributor Author

kakumara commented Sep 18, 2019

The <vulnerabilityScore> should be renamed to a plural. 'ratings' maybe?

Inside <ratings> should be a <rating> and inside that should be the <score>. Having two <score> nodes with different meanings is not acceptable.

Agree and makes sense. 44a9bfa

@kakumara

This comment has been minimized.

Copy link
Contributor Author

kakumara commented Sep 18, 2019

One final point on the above. The <v:source>CVSSv3</v:source> should likely be changed to <v:method>CVSSv3</v:method>. So we will be describing the method used in risk rating the vulnerability. To me, that makes more sense.

Agree and makes sense. 44a9bfa

@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 18, 2019

Once released, the extension will have a page similar to https://cyclonedx.org/ext/dependency-graph/

Does Sonatype create this or @stevespringett?

Also, I would like to thank you for reviewing our PR.

@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 19, 2019

Does Sonatype create this or @

This is something I’ll do and if you’d like the page to state something specific or if Sonatype would like to be ack on the page, let me know.

Also, I would like to thank you for reviewing our PR.

Of course. I think the only thing remaining is the decimal restrictions. Other than that, does the PR satisfy your original requirements? Any foreseeable changes or is this ready for release?

@kakumara

This comment has been minimized.

Copy link
Contributor Author

kakumara commented Sep 19, 2019

The final thing that needs correction is the score decimal restrictions. Currently, any decimal is allowed. So -999, 15.8, and 5.99999 are all allowed. This should be restricted to 0.0 - 10.0 and be limited to a single decimal point.

👍 restriction added 57d27d8

@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 19, 2019

How will the schema be licensed? CycloneDX itself is Apache 2.0.

</xs:documentation>
</xs:annotation>
</xs:enumeration>
<xs:enumeration value="PCI DSS">

This comment has been minimized.

Copy link
@stevespringett

stevespringett Sep 19, 2019

Member

I looked at the PCI DSS doc and it doesn't specifically state a method for evaluating risk. Rather, it provides an example of how to do so. The only requirement appears to be that it's done. But I'm unclear if PCI DSS should actually be a valid risk rating method or not. Comments?

This comment has been minimized.

Copy link
@kakumara

kakumara Sep 20, 2019

Author Contributor

I looked at the PCI DSS doc and it doesn't specifically state a method for evaluating risk. Rather, it provides an example of how to do so. The only requirement appears to be that it's done. But I'm unclear if PCI DSS should actually be a valid risk rating method or not. Comments?

Think you are right. 👍 This came as an option in our initial research. But based on above and internal discussion this doesn't look like it can be placed as a risk rating method. b2e2b00

@ja98200wilcox

This comment has been minimized.

Copy link

ja98200wilcox commented Sep 20, 2019

How will the schema be licensed? CycloneDX itself is Apache 2.0.

Let's use the same license you use for CycloneDX.

@stevespringett stevespringett merged commit 44c3eca into CycloneDX:master Sep 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stevespringett

This comment has been minimized.

Copy link
Member

stevespringett commented Sep 20, 2019

I'll add the license and publish this weekend.

@kakumara kakumara deleted the sonatype:CLM-13327_vulnerability_extension branch Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.