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

0.8.0 changes #1

Merged
merged 22 commits into from
Aug 26, 2021
Merged

0.8.0 changes #1

merged 22 commits into from
Aug 26, 2021

Conversation

oliverchang
Copy link
Contributor

@oliverchang oliverchang commented Aug 5, 2021

  • Rename "affects" to "affected", and make it a list of objects.
  • Represent version ranges using "events" rather than ranges of [introduced, fixed)
  • Move "package", "ecosystem_specific", "database_specific" to "affected".

- Rename "affects" to "affected", and make it a list of objects.
- Move "package", "ecosystem_specific", "database_specific" to "affected".
- Add "routines" and "platforms" to "affected" as well.
@oliverchang oliverchang requested a review from rsc August 6, 2021 02:07
@oliverchang oliverchang requested a review from rsc August 6, 2021 04:11
@oliverchang
Copy link
Contributor Author

Also moved repo as discussed: 67693a7

@oliverchang oliverchang changed the title Support multiple packages in one entry. 0.8.0 changes Aug 17, 2021
@joshbressers
Copy link

@oliverchang To followup from our conversation yesterday (or probably your today)

I mentioned I didn't like the new "affected" and I realize why now

	"affected": [ {
		"package": {
			"ecosystem": string,
			"name": string,
			"purl": string,
		},
		"ranges": [ {
			"type": string,
			"repo": string,
			"events": [ {
				"introduced": string,
				"fixed": string
			} ]
		} ],
		"versions": [ string ],
		"ecosystem_specific": { see description },
		"database_specific": { see description },
	} ],

I think the way CVE overloads the package field in an ID is wrong in the modern world. It made sense 20 years ago

Here is the example that made me realize this. It's the same problem for copy and pasting code, or embedding source files, so I ask whoever reads this not to nitpick it too much. It's a hard problem.

Let's say I have a npm project, npm will embed the dependencies, so for example it's VERY common for a project to ship multiple copies of the lodash library as different packages can depend on different versions.

If there is one vulnerability in lodash, today it would get one ID and you would see your application is affected by the same ID multiple times. This means you actually have to key off package:ID, because just the ID isn't enough useful information.

Now, package:ID as the key may not be the worst thing in the world, but it probably creates some new challenges we don't understand. Today I think just using an ID is the most common use case. I want to think about this more, but I wanted to suggest that allowing an array of packages should not be taken lightly.

@kaniini
Copy link

kaniini commented Aug 18, 2021

I don't follow this argument, any scanner worth anything should automatically dedup any reported vulnerabilities, I think.

@joshbressers
Copy link

I don't follow this argument, any scanner worth anything should automatically dedup any reported vulnerabilities, I think.

It's not about the dupes so much as it is about having a way to understand what is vulnerable to a given problem. I could be off my rocker here as well, this makes sense in my head.

Let me just make something up to illustrate my thinking. This is a little unrealistic I know.

Let's say I have a project, and there are two different libraries that run as webservers to service REST requests. The two libraries each share one common source file that is vulnerable to a RCE. In today's model, that would be one ID both libraries share, even though they are unrelated.

Is that acceptable, or would it be better to give each library their own ID?

From a purely automated perspective, it doesn't really matter. But if I have to discuss this problem with a fellow human, I now have to clarify which library we are talking about as part of the conversation, so the identifier is really package:ID, not just the ID

@kaniini
Copy link

kaniini commented Aug 18, 2021

I get what you're saying there. In that case, I think a scanner should present it as two vulnerabilities, but prefixing it as subcomponent:ID. But the scanner can do that itself, can't it?

@oliverchang
Copy link
Contributor Author

oliverchang commented Aug 18, 2021

Thanks for the feedback!

I completely agree that in most cases, there should only ever be one package specified in a single entry (and this is what we strongly recommend), and we really wanted to keep this.

But for ease of adoption, I think we need to make a tradeoff to support interoperability with other schemas (such as CVE, GHSA) which do support (and rarely) have multiple packages listed (example: https://github.com/advisories/GHSA-wh77-3x4m-4q9g). Otherwise, this is adding burden on them to generate new IDs/entries in a way that may not fit easily into their existing workflows.

We also got feedback from the Go vulnerability DB team, who wish to express that same package may have different import paths/names (vanity URLs, versioned imports) and they don't want to create separate entries for each, because it adds complexity for them.

I get what you're saying there. In that case, I think a scanner should present it as two vulnerabilities, but prefixing it as subcomponent:ID. But the scanner can do that itself, can't it?

I agree! The scanner would already know about what package it's scanning for, and then it just has to correlate that with what's listed in the vulnerability entry.

@joshbressers
Copy link

I think this is all fair, I appreciate your explanation

@oliverchang oliverchang force-pushed the packages-move branch 2 times, most recently from 88f212b to f5ebcd3 Compare August 24, 2021 08:15
software to answer the question "is this specific version affected?" without
having to contain code specific to every different ecosystem. The one exception
is if the affected versions are valid SemVer 2.0 versions which can be
accurately summarized by one or more non-overlapping SemVer ranges. In that

Choose a reason for hiding this comment

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

@oliverchang Singling out semver as being a perfect version scheme assumes that humans handling semver versions will never break semver. I think that this cannot hold at scale.
IMHO there is no reason to treat SEMVER specially and I explained some rationale in the related CVEProject/cve-schema#87 (comment)

Instead a version range may best treated as what it is: a hint for tools and humans to help form a proper versions enumeration.

Copy link
Contributor Author

@oliverchang oliverchang Aug 25, 2021

Choose a reason for hiding this comment

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

Thanks for the feedback @pombredanne. I agree an explicit list of versions makes this unambiguous in all cases.

I think tooling/validators can help with making sure the SEMVER values are valid. One other reason why we need this is that in ecosystems like Go (which use/enforce Semver), it's infeasible to enumerate every possible version. Every commit can be a "pseudoversion" (valid semver) in Go.

Choose a reason for hiding this comment

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

Thank you for the clarification. This makes sense now. I had not internalized this aspect of Go modules versioning. That's actually pretty sleek and clean. It creates some extra work when trying to enumerate these when compared to other package types... but that's an issue for tools/db to deal with..!

schema.md Outdated
- `WEB`: A web page of some unspecified kind.
The values of "introduced", "fixed" and "limit" are version strings as defined
by the `affected[].ranges[].type` field. Additionally,
- `"introduced"` allows a version of the value `"\*"` to represent a version that
Copy link

Choose a reason for hiding this comment

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

For consistency with CVE and easier conversion I think we should use 0 here.
* meaning zero in one field and * meaning infinity in another is odd anyway.

Copy link

@rsc rsc left a comment

Choose a reason for hiding this comment

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

Other than the 0 I'm happy.

@rsc
Copy link

rsc commented Aug 26, 2021

LGTM

@oliverchang oliverchang merged commit f1e13a7 into main Aug 26, 2021
gopherbot pushed a commit to golang/vulndb that referenced this pull request Sep 3, 2021
An implementation of the specification change proposed by
ossf/osv-schema#1. The significant change here
is that instead of generating multiple entries for reports with
multiple packages (in the additional_packages section), we instead
generate a single entry that covers all of the packages, and write the
same entry for each module path.

Change-Id: Ia9d8e0a82081ab7f5becd20c6adf976f4d6966db
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/340210
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Vulndb-Deploy: Roland Shoemaker <[email protected]>
@oliverchang oliverchang deleted the packages-move branch September 13, 2021 23:12
softdev050 added a commit to softdev050/Golangvuln that referenced this pull request Apr 5, 2023
An implementation of the specification change proposed by
ossf/osv-schema#1. The significant change here
is that instead of generating multiple entries for reports with
multiple packages (in the additional_packages section), we instead
generate a single entry that covers all of the packages, and write the
same entry for each module path.

Change-Id: Ia9d8e0a82081ab7f5becd20c6adf976f4d6966db
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/340210
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Vulndb-Deploy: Roland Shoemaker <[email protected]>
sayjun0505 added a commit to sayjun0505/Golangvuln that referenced this pull request Apr 8, 2023
An implementation of the specification change proposed by
ossf/osv-schema#1. The significant change here
is that instead of generating multiple entries for reports with
multiple packages (in the additional_packages section), we instead
generate a single entry that covers all of the packages, and write the
same entry for each module path.

Change-Id: Ia9d8e0a82081ab7f5becd20c6adf976f4d6966db
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/340210
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Vulndb-Deploy: Roland Shoemaker <[email protected]>
stanislavkononiuk added a commit to stanislavkononiuk/Golangvuln that referenced this pull request Jun 26, 2023
An implementation of the specification change proposed by
ossf/osv-schema#1. The significant change here
is that instead of generating multiple entries for reports with
multiple packages (in the additional_packages section), we instead
generate a single entry that covers all of the packages, and write the
same entry for each module path.

Change-Id: Ia9d8e0a82081ab7f5becd20c6adf976f4d6966db
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/340210
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Vulndb-Deploy: Roland Shoemaker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants