Go detector overreports dependencies, sometimes causing false vuln claims that are unusually difficult to "upgrade anyway"

[dagood]

I'm helping work on a project that needs to build https://github.com/konveyor/kantra/tree/215adcbbdfc337b5dee439404dc280d78e3e62f6 in Microsoft systems. This means component-detection is used to detect its dependencies, and the dependencies are associated with known vulnerabilities.

Behavior

CG claimed recently that that commit depends on golang.org/x/crypto v0.23.0, and the alert says:

Upgrade golang.org/x/crypto from v0.23.0 to 0.31.0 to fix the vulnerability.

We think this is a false positive: the project doesn't depend on x/crypto. But, it wouldn't hurt to upgrade anyway, and we might as well, to be sure. So, we run go get -v -u golang.org/x/crypto@v0.31.0. That adds the dependency to go.mod and go.sum.

Then we run go mod tidy to clean up and prepare to submit the PR. It removes the dependency from go.mod and go.sum!

This puts us in a bad state, where there are a few workarounds:

• Never run go mod tidy again (and adjust CI to not require tidiness). This is a bad continuous state to be in.
• Add fake usage of x/crypto to a Go file just so that the dependency sticks around in go.mod.
  • Fortunately, by using a tools build tag, this has no side effects other than clutter. But you need to be familiar with Go and with the specific project's infrastructure to apply this workaround consistently.

I think component-detection needs to fix this type of false positive. It's particularly disruptive, compared with other types of false positive where we can simply upgrade the dependency whether or not it's strictly necessary.

Repro example and go list

I put together a simplified example of this false positive. https://github.com/dagood/repro/tree/repro/go-cg-unupgradeable-component/myapp is an example app myapp that depends on a module somelibrary that uses x/crypto v0.23.0, but only one of its two library packages uses a package from x/crypto.

The detector currently uses go list -mod=readonly -m -json all to discover dependencies. The -m means it's in "module mode", which doesn't match what go list, go mod tidy, and the rest of the Go ecosystem normally cares about, which are packages. go mod graph would overreport in a similar way if it were used for initial component detection.

...
{
	"Path": "golang.org/x/crypto",
	"Version": "v0.23.0",
	"Time": "2024-05-06T13:42:02Z",
	"Indirect": true
}

If you use e.g. go list -mod=readonly -deps -f '{{.ImportPath}} ---- {{.Module}}' ./..., it doesn't include x/crypto because at a package level, x/crypto is totally unused.

...
fmt ---- <nil>
strings ---- <nil>
example.org/somelibrary/numbers ---- example.org/somelibrary v1.0.0 => ../somelibrary
example.org/myapp ---- example.org/myapp

(You can also add logic to the command itself to filter down to packages that are associated with a module: go list -mod=readonly -deps -f '{{if .Module}}{{.ImportPath}} ---- {{.Module}}{{end}}' ./....)

I also wrote a test app that does use the part of somelibrary that uses x/crypto, and you can see x/crypto 0.23.0 show up in the right places to be detected: https://github.com/dagood/repro/tree/repro/go-cg-unupgradeable-component/myappcrypto.

govulncheck

I'm not proposing to use govulncheck for Component Governance. It would be good, but not necessary to fix the most disruptive false positives. It looks even deeper at the dependencies. For example:

$ govulncheck ./... # in myapp:
No vulnerabilities found.

$ govulncheck ./... # in myappcrypto
=== Symbol Results ===

No vulnerabilities found.

Your code is affected by 0 vulnerabilities.
This scan also found 0 vulnerabilities in packages you import and 1
vulnerability in modules you require, but your code doesn't appear to call these
vulnerabilities.
Use '-show verbose' for more details.

(The vulnerability is specifically in golang.org/x/crypto/ssh, which isn't used here.)

[grvillic]

@dagood - We have seen multiple approaches Go experts have suggested over the years, but none seems to have checked all boxes. Mind introducing a new Go detector that we can experiment with using this new strategy?

We can leverage the experiments framework to capture deltas between new / current detector and analyze the deltas.

Questions:

1. Does this one respect the "replace" directive?
2. Can we build full dependency graph or just flat list of dependencies?

[dagood]

I wouldn't say I have a strategy here, so far this is just a bug report with a few tools and args that I suggest avoiding, with other commands to contrast against the bad info being reported by those tools/args. Getting this repro case into the test suite and figuring out what to do from there was how I was thinking this could be approached.

1. I'm not sure how replace interacts with a non--m go list, or what might need to be added to make that work.
2. I don't see why a non--m go list wouldn't be enough info for a graph--it can provide extensive info: https://pkg.go.dev/cmd/go#hdr-List_packages_or_modules. If necessary, I don't think using go mod graph for this is a problem, the same way it's used now to fill in the graph.

[grvillic]

@dagood - I am having an offline thread with another dev reporting these false positives and it seems like go.mod file in go 1.17+ already includes all transitive dependencies and applies both pruning and Minimal Version Selection (MVS).

See go-mod-file-require

At go 1.17 and above, the go command adds an indirect requirement for each module that provides any package imported (even indirectly) by a package or test in the main module or passed as an argument to go get. These more comprehensive requirements enable module graph pruning and lazy module loading.

It appears that it is possible to retrieve the complete list of dependencies in a Go module after pruning. Running go mod graph may help capture the relationships between roots and their dependencies, but using as a superset the list of dependencies from go.mod file as opposed to go.mod.

[dagood]

If I'm understanding this right, you're saying that it looks like parsing a valid (buildable) >=1.17 go.mod file is enough to gather a complete list of dependencies because of the guarantees about how Go keeps it up to date--I agree from my experience, and it would definitely cover this particular case!

I suppose there's a chance that someone has an unused dependency in the go.mod file and they haven't tidyed yet, and that would be picked up as a dependency even though it isn't used. I don't think this is necessarily a problem, though--they should run go mod tidy, and it seems bad for a vulnerable version to be in a go.mod file even if it isn't used. (It makes it easier for a dev to later unintentionally use the vulnerable version, which can be bad, even if it only lasts until the next time CG scans the code.)

Actually, reading https://github.com/microsoft/component-detection/blob/main/docs/detectors/go.md again, it seems like the fallback strategy already covers a lot of this... it's not clear to me why it's the fallback rather than the main strategy. (The benefit of having the full graph isn't clear to me, if that's the reason.)

The doc does mention "The fallback detections trategy is known to overreport", but it seems that this might just be an old sentence from when there was only the <1.17 fallback strategy and not the >=1.17 strategy.

using as a superset the list of dependencies from go.mod file as opposed to go.mod.

Was that meant to be "as opposed to go.sum", referring to the old fallback strategy?

---

I do wonder if there's a go command with output that corresponds directly to the presence/absence of modules in the go.mod file. A handful of fields in list-m-all.jsonl are conspicuously missing vs. the json when x/crypto is used. So maybe, after all, just a bit more filtering is needed? Maybe something to check out with the test framework and a variety of module dependencies.

[grvillic]

Your understanding is correct.

Originally the Go documentation wasn't very clear on what was included in Go.mod file vs what wasn't and it that included MVS was part of go.mod or not (it is).

The benefit of having the full graph isn't clear to me

If a vulnerability comes from a transitive dependency, you can easily pinpoint which was the root dependency to blame. If you don't have the graph you need to go hunting for the roots (which isn't a dealbreaker but slightly more work for devs).

The doc does mention "The fallback detection strategy is known to overreport", but it seems that this might just be an old sentence from when there was only the <1.17 fallback strategy and not the >=1.17 strategy

You are correct, we will update docs to match the latest behavior. overreporting only happens on Go <1.17 since we parse both go.mod and go.sum files.

I do wonder if there's a go command with output that corresponds directly to the presence/absence of modules in the go.mod file

The only one that we know of as of today is go mod why <dependency>, but this doesn't scale when capturing all of them. I think with latest versions of go you can't even build it if go.mod is outdated, but that is out of scope for the detector. If you are aware of other alternatives on how to improve the quality of the detector let us know. As for now we will just change the detector to do the fallback strategy by default and generate the graph (edge mappings) for the dependencies present in go.mod file.