I will let the vulncheck experts weigh in here, but from the peanut gallery this seems like a natural consequence of the way Go interfaces work.

This is a static analysis tool; if io.Copy invokes a method on an io.Reader interface object, the checker in most cases won't know the concrete type of the object underlying the interface. This means it will have to conservatively assume that the underlying object could be any type that satisfies the io.Reader interface. The type ssh.channel.Read is live (not deadcoded) in your binary, so the assumption has to be that it might be invoked here.

@golang/vulndb per owners

@thanm makes life harder for maintainers :(

@thanm In this particular case, I'd really expect the static analyzer to work out the concrete types, since it is already looking at particular traces. Namely, it ought to determine the io.Reader is an io.LimitedReader wrapping an os.File, since they are all local variables declared right beforehand. I'm sure that in general there are cases where determining the concrete type is prohibitively difficult, but vulncheck needs to minimize false positives in order to be useful.

In addition, if vulncheck is unable to determine the concrete type, it should emit something clearer in the log message. For example: "calls io.Copy, which eventually calls io.Reader.Read, which might resolve to ssh.channel.Read"

In addition, if vulncheck is unable to determine the concrete type, it should emit something clearer in the log message. For example: "calls io.Copy, which eventually calls io.Reader.Read, which might resolve to ssh.channel.Read"

for common stdlib interfaces like io.Reader that would still be an incredibly wild guess by the tool.

This is unfortunately a current limitation in the tool. Knowing the exact concrete types for an interface in general is an undecidable problem. Hence govulncheck has to approximate and it chooses to over-approximate.

It computes an over-approximate call graph and then it finds a call stack that leads to a vulnerability. There are typically many such call stacks and some of them might not exist in practice due to the over-approximate nature of the call graph. It is impossible to validate each call stack so we employ some heuristics with the goal of picking one that is least likely to be a false positive. We could probably do a better job: ssh.channel is passed to io.Copy somewhere else, but I don't think it is even mentioned in the call stack here. I will look into this.

how to deal with such false positives? will there be a block list, preferably based on the call path and not just CVE?

@thediveo go get golang.org/x/crypto@v0.17.0 or @latest ✨

You could have a situation where the only security fixes happen in versions where there are incompatible breaking changes.
But in my experience this is exceedingly rare in the Go community.
Except in cases where the original design was wrong like in GO-2022-0646.

If we really want a block list, I think it should be explicit like govulncheck --ignore=path/to/block/list.txt.

It is fairly clear that the reported path are not real. That should definitely be improved, but it does not appear to be the root cause.

From what I can tell, it appears that the core of the issue is that the analysis cannot conclude that io.Copy is not called with a Reader and a Writer from ssh like golang.org/x/crypto/ssh.channel. For example, ssh.Session.stdout() calls io.Copy(s.Stdout, s.ch). So to conclude the program is not vulnerable requires deducing that ssh.Session.stdout() (etc.) is unreachable within the program. These are themselves reachable from functions in the package k8s.io/kubernetes/test/e2e/framework/ssh. So deducing that the vulnerability is unreachable also requires finding functions in this package are unreachable from any main package.

Using go list -f '{{.Name}}: {{ .ImportPath }}: {{ .Deps }}' ./... we can conclude both the main packages k8s.io/kubernetes/test/e2e/network/scale/localrun and k8s.io/kubernetes/test/soak/serve_hostnames depend on the package k8s.io/kubernetes/test/e2e/framework/ssh. This package is in turn used in quite a few places (grep -R 'k8s.io/kubernetes/test/e2e/framework/ssh') . I have not yet found an obvious smoking gun for either of these. So my hunch is that functions like k8s.io/kubernetes/test/e2e/framework/ssh.SSH are more likely to be unreachable. Getting to the bottom of why govulncheck cannot conclude they are unreachable in these two packages will require a deep dive into its internals though.

As a practical point, partitioning large modules can a good way to improve precision. This can remove false positives due to aliasing. (If golang.org/x/crypto/ssh is not depended on, ssh.Session.stdout() will not be imported, will not create the alias, and will not that bogus trace.) So +1 to partitioning like kubernetes/kubernetes#122424 (comment) .

how to deal with such false positives?

At the moment this is up to each maintainer to triage and appropriately decide to update, ignore, or build a custom suppression mechanism.

will there be a block list, preferably based on the call path and not just CVE?

Keep in mind that the number of paths of this form are potentially exponential in the number of functions in the program. Even if govulncheck supported this, it should probably quickly give up trying to find a path and would report instead that 'Vuln #XYZ may be reachable, but we could not find a feasible path.' It is not clear this would put us in an appreciably different situation.

Edges are only quadratic in the # of functions in the program. For example, asserting that the Read in io.copyBuffer does not call golang.org/x/crypto/ssh.extChannel.Read. We could also have assertion that some functions are unreachable. (Pro: linear. Con: Requires the user to think about aliasing.) Also user assertions that are correct today can become wrong over time (and a file encourages them to be checked in) so if supported this should be done with care. There is no plan implement this at the moment, but I would encourage you to file a new issue with a feature request that includes what types of assertions you are hoping for if you are interested.

I'd really expect the static analyzer to work out the concrete types, since it is already looking at particular traces. Namely, it ought to determine the io.Reader is an io.LimitedReader wrapping an os.File, since they are all local variables declared right beforehand.

If you are familiar with trace based static analysis (like symbolic execution), that may mislead you to how govulncheck works under the hood. It is doing callgraph based reachability at the moment. However, the intuition that it should be able to statically determine from the callsite that certain variables within the callee have a fixed type is a good one and leads to what I suspect will be the long term fix for this case. In essence, if one was to inline both io.Copy and io.copyBuffer that is enough precision to devirtualize these calls, which would remove these spurious traces. Unfortunately doing depth 2 calls like this naively is like making the program O(n^3) in size in order to analyze it more precisely. This kind of 'inlining' is doable IMO, but will require tuning some heuristics in order to not blow up. This will take some investigation (and probably a Go proposal).