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

fix: use Go main module version when possible #1797

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

luhring
Copy link
Contributor

@luhring luhring commented Apr 10, 2024

Previously, if Grype saw a Go binary's main module package version was set to a string prefixed by v0.0.0-, it would avoid any kind of vulnerability matching for that package.

Meanwhile, Syft makes several reasonable attempts to find a sane version to use as this main module's version. In the case of VCS information being embedded in the Go binary, Syft has enough information to produce a Go pseudoversion.

The "no vulnerability matching at all" approach may have been to avoid some false positive edge cases, or it may have predated some of the enhancements to Syft's version computation. But in any case, we were seeing this behavior result in false negatives, because many Go projects produce Go binaries (and are thus the main module for that binary), and when Syft can compute a pseudoversion from VCS data, and these projects have GHSAs filed for them using pseudoversions (e.g. GHSA-xx8w-mq23-29g4), Grype was missing these matches 100% of the time.

To be upfront, I do expect this change to yield an increase in false positives. But I expect this change to yield a much bigger decrease in false negatives. If I understand the current Grype philosophy, distros with "comprehensive feeds" aren't even subject to this kind of matching, so it's really just the distros that are left (Wolfi, Chainguard, and Alpine), and then non-distro Go binaries where the developer is likely in much more control of the version information embedded in the binary.

For context, this change was discussed in advance in Anchore's community slack with @kzantow. (thread link)

Let me know if I can answer any questions!

When its helpful, that is. This doesnt change the behavior of matching a main module with "(devel") as the version, but in cases where a more useful version is provided, such as when Syft was able to compute a reasonable pseudoversion, we use the version in for best effort matching.

Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
@spiffcs
Copy link
Contributor

spiffcs commented Apr 12, 2024

Good news! I cannot find an increase in FP that this PR introduces from a first glance. @luhring Is there a case you saw where a new FP was introduced by this change? I can confirm that the FN example you provided does work and is now showing up in results.

@luhring
Copy link
Contributor Author

luhring commented Apr 12, 2024

@luhring Is there a case you saw where a new FP was introduced by this change?

No actually, good question. My reason for expecting them is that I can dream up edge cases, like when the GHSA fixed version is v3.0.1, but the computed pseudoversion would be v0.0.0-<some commit that's later than v3.0.1>, and so Grype would show the main module as vulnerable when it's not. But I think this is mitigated by multiple factors, like I mentioned above. So this seems like a well worthwhile tradeoff in the meantime.

@spiffcs
Copy link
Contributor

spiffcs commented Apr 15, 2024

I did a bit more digging here to find an FP that would get surfaced by this change. Something like this:

syft -o json ollama/ollama:0.1.32 | ~/go/bin/grype

Now reports:

github.com/ollama/ollama    v0.0.0-20240414223325-7027f264fbb3  0.1.29             go-module  GHSA-5jx5-hqx5-2vrj  High

Here grype now surfaces github.com/ollama/ollama even though 7027f264fbb3 is a fixed version.

I still agree with this change in so much that grype should not be assuming the data coming from the SBOM for go versions are incorrect or incomplete and discounting pseudo versions from matching.

The cataloging of main module versions has to get better on the SBOM side. Current challenges exist for the Golang ecosystem summarized here:
golang/go#50603

@spiffcs
Copy link
Contributor

spiffcs commented Apr 15, 2024

I think @willmurphyscode had a comment about understanding the new FP introduced here more concretely so I'll let him chime in before merging:

FN Impact

Query I put together to examine where we improve for this change:

sqlite> select count(*) from vulnerability WHERE namespace = "github:language:go" AND fixed_in_versions LIKE '%0.0.0%';
57

Packages Impacted by potential False Negatives (Note: this change only affects results when they are the main module.)

code.cloudfoundry.org/archiver
code.cloudfoundry.org/gorouter
github.com/btcsuite/go-socks
github.com/btcsuitereleases/go-socks
github.com/cloudflare/cloudflared
github.com/cloudflare/golz4
github.com/cloudfoundry/archiver
github.com/cloudfoundry/gorouter
github.com/devfile/registry-support/registry-library
github.com/dhowden/tag
github.com/elazarl/goproxy
github.com/elgs/gosqljson
github.com/flipped-aurora/gin-vue-admin/server
github.com/go-macaron/csrf
github.com/gomarkdown/markdown
github.com/gopistolet/gopistolet
github.com/kudelskisecurity/crystals-go
github.com/lxc/lxd
github.com/matrix-org/gomatrixserverlib
github.com/minio/minio
github.com/nanobox-io/golang-nanoauth
github.com/openshift/apiserver-library-go
github.com/rancher/apiserver
github.com/rancher/norman
github.com/robbert229/jwt
github.com/square/squalor
github.com/whyrusleeping/tar-utils
golang.org/x/crypto
golang.org/x/net
golang.org/x/net/http2
golang.org/x/sys
k8s.io/apimachinery

So this has the potential to FIX 57 false negatives when those packages are the main module. I don't have a good impression with data that shows the false positive impact beyond my experimentation with the ollama image that showed how a potential new FP could be introduced.

FP Impact

@willmurphyscode had a good statement on the FP impact:

the potential FPs are basically, "every Golang GHSA against something that is main module, 
the version could be wrong depending on how it's compiled or listed in the SBOM" which is a much higher number

Final Thoughts (OPINION):

I’m in favor of not tying improvements in Syft's Golang main module version discovery to this change.

I think if a user brings an SBOM that was not generated with Syft, then grype should be considering all valid cases.

Versions prefixed with v0.0.0 are valid cases:
GHSA-xx8w-mq23-29g4

Any introduction of FP is regrettable here, but if there are two outcomes for grype, then I’d rather go down the path where Grype is correct after considering all given valid version input for ANY given SBOM, rather than a narrow case where Grype trys to shore up edge cases of the main module version discovery in Syft’s Golang cataloger by not considering all options.

@willmurphyscode
Copy link
Contributor

Thanks for the investigation @spiffcs! I think @luhring is right that the v0.0.0 prefix defensive check pre-dates some of the attempts in syft to get a main module version. Syft's main module detection was improved as recently as anchore/syft#2608 (merged Feb 9 2024).

I think I'm comfortable approving this because, without this change, Grype is effectively saying, "the sbom must be wrong; no one ships v0.0.0- main module versions," but sometimes there are v0.0.0- versions that are shipped, and have vulnerabilities.

@luhring
Copy link
Contributor Author

luhring commented Apr 16, 2024

Sounds good to me! Thanks for helping to vet this approach!

@spiffcs spiffcs merged commit 6dde5ce into anchore:main Apr 16, 2024
10 checks passed
spiffcs added a commit that referenced this pull request Apr 18, 2024
spiffcs added a commit that referenced this pull request Apr 18, 2024
This reverts commit 6dde5ce.

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@kzantow kzantow added the enhancement New feature or request label Apr 18, 2024
luhring added a commit to luhring/wolfictl that referenced this pull request Apr 23, 2024
After my Grype PR (anchore/grype#1797), Anchore added a follow-up PR that makes this behavior cnofigurable. It is on by default for the Grype CLI but not for the Grype library. This commit enables it for the Grype library so we can see this kind of match from wolfictl scans.

Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants