x/vuln: invalid finding: if Frame.Function is set, Frame.Package must also be

[zachgersh]

govulncheck version

This was on version 1.0.4 (I've since downgraded back to 1.0.1 which works fine).

Does this issue reproduce at the latest version of golang.org/x/vuln?

Haven't tried latest unforunately.

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/zachary/Library/Caches/go-build'
GOENV='/Users/zachary/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/zachary/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/zachary/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.1'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/zachary/workspace/my-repo/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-argume

What did you do?

A simple govulncheck ./...

What did you see happen?

returned the above error in the title. The fun part is, switching to package instead of symbol actually showed me that there was a protobuf vuln I need to upgrade dependencies for. I will try to get a minimal reproduction together.

What did you expect to see?

A successful scan with the protobuf dependencies called out as needing an upgrade due to an active vuln.

[mknyszek]

CC @golang/vulndb

[zpavlinovic]

Thanks for reporting this. Yes, reproduction steps would be really helpful here.

[zachgersh]

@zpavlinovic - yeah, I am trying to figure it out. I am wondering if it has something to do with generated code. The vuln it was trying to point out was protojson.Unmarshal. I tried to make a sample project with just that imported (but no generated proto code) and it worked just fine on 1.0.4.

I'll try it again with actually generated protobuf types to see if something weird is happening there.

[oliverpool]

The same error message started to appear on Forgejo on outdated branches: https://codeberg.org/forgejo/forgejo/actions/runs/7853/jobs/0

go run golang.org/x/vuln/cmd/govulncheck@latest ./...
go: downloading golang.org/x/vuln v1.0.4
Scanning your code and 932 packages across 232 dependent modules for known vulnerabilities...

govulncheck: invalid finding: if Frame.Function is set, Frame.Package must also be
exit status 1

It is likely related to the protobuf vulnerability, since merging the default branch (which has updated the dependency) yields:

Scanning your code and 935 packages across 232 dependent modules for known vulnerabilities...

=== Informational ===

There is 1 vulnerability in modules that you require that is neither
imported nor called. You may not need to take any action.
See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck for details.

Vulnerability #1: GO-2022-0470
    No access control in github.com/blevesearch/bleve and bleve/v2
  More info: https://pkg.go.dev/vuln/GO-2022-0470
  Module: github.com/blevesearch/bleve/v2
    Found in: github.com/blevesearch/bleve/v2@v2.3.10
    Fixed in: N/A

No vulnerabilities found.

Share feedback at https://go.dev/s/govulncheck-feedback.

So the cryptic initial error message does likely points to a vulnerability (however it does not help into solving it).

https://github.com/golang/vuln/blob/563994f0852c21fbdb2424841ebd8de389ff47ad/internal/scan/util.go#L34

[oliverpool]

Debugging vuln, I got the following offending frame, from OSV: "GO-2024-2611"

	*govulncheck.Frame{
                Module: "stdlib",
		Version:  "v1.22.1",
		Package:  "",
		Function: "NewUnaryHandler[code.gitea.io/actions-proto-go/ping/v1.PingRequest code.gitea.io/actions-proto-go/ping/v1.PingResponse]$2",
		Receiver: "",
		Position: *golang.org/x/vuln/internal/govulncheck.Position{Filename: ".cache/go/pkg/mod/connectrpc.com/connect@v1.15.0/handler.go",
			Offset: 2631,
			Line:   70,
			Column: 25,
		},
	}

Maybe the validateFindings function could return more information in case of error (OSV, Version, Package, Function), so that running a debugger would not be needed.

[xxgreg]

Looks like this function returning an empty string for the package which leads to the error:

// pkgPath returns the path of the f's enclosing package, if any.
// Otherwise, returns "".
func pkgPath(f *ssa.Function) string {
	if f.Package() != nil && f.Package().Pkg != nil {
		return f.Package().Pkg.Path()
	}
	return ""
}

The docs on ssa.Function.Pkg are:

// enclosing package; nil for shared funcs (wrappers and error.Error)

It looks like the package name isn't available for "synthetic functions". I'm assuming thats what these odd function names with $ in them are.

The function which appears to be causing this in my case is in the connect rpc package.

func NewClient[Req, Res any](httpClient HTTPClient, url string, options ...ClientOption) *Client[Req, Res]

https://github.com/connectrpc/connect-go/blob/75d634f2c4ebf49b4e604565d20bd42578c71f1c/client.go#L42

Also - If I remove the failing empty package name check, I actually get reasonable output.

[zpavlinovic]

Thanks for looking into this and providing the details. I think the bug is in how we are handling instantiations of generic functions. I will try to make a fix soon. I believe this should do the trick:

// pkgPath returns the path of the f's enclosing package, if any.
// Otherwise, returns "".
func pkgPath(f *ssa.Function) string {
        g := f
        if f.Origin() != nil {
           g = f.Origin()
        }

	if g.Package() != nil && g.Package().Pkg != nil {
		return g.Package().Pkg.Path()
	}
	return ""
}

[xxgreg]

@zpavlinovic No problem. Thanks for your hard work on this tool.

Edit: In case it's helpful, output included below after updating pkgPath with the code above, and running it on my codebase. Some Xs added to avoid sharing private paths/domains.

Vulnerability #1: GO-2024-2611
    Infinite loop in JSON unmarshaling in google.golang.org/protobuf
  More info: https://pkg.go.dev/vuln/GO-2024-2611
  Module: google.golang.org/protobuf
    Found in: google.golang.org/protobuf@v1.32.0
    Fixed in: google.golang.org/protobuf@v1.33.0
    Example traces found:
      #1: pkg/tables/meta.go:25:28: tables.DecodeMeta calls protojson.Unmarshal, which eventually calls json.Decoder.Peek
      #2: pkg/tables/meta.go:25:28: tables.DecodeMeta calls protojson.Unmarshal, which eventually calls json.Decoder.Read
      #3: pkg/tables/meta.go:25:28: tables.DecodeMeta calls protojson.Unmarshal
      #4: XXXXX/v1/workflowv1connect/workflow_service.connect.go:99:27: workflowv1connect.workflowServiceClient.Status calls connect.NewClient[XXXXX/workflow/v1.StatusRequest XXXXX/workflow/v1.StatusResponse], which eventually calls protojson.UnmarshalOptions.Unmarshal

This now includes the package name. i.e. connect.NewClient

I also think it may be worth considering changing the empty package name check to be a warning rather than a hard error. This means the vuln codebase will be more robust to future changes in x/tools/go/ssa.

[gopherbot]

Change https://go.dev/cl/570575 mentions this issue: internal/scan: add more info to validation errors

[gopherbot]

Change https://go.dev/cl/570616 mentions this issue: internal/vulncheck: get correctly package for instantiated functions

[zachgersh]

@xxgreg - thanks for tracking this down since I had not had time to come back and repro. Also big thanks to @zpavlinovic for being responsive as well with a changeset already up. I am going to bump forward again once this is fixed in 1.0.5

[AlekSi]

Do you plan to release 1.0.5 soon?

[zpavlinovic]

Yes, but we don't have an exact timeline yet. There are a few more features we'd like to add before we make the release.

[joeljuca]

Hi – I'm having a similar error msg from a project I've recently jumped on:

$ govulncheck ./...
==> Running govulncheck
Scanning your code and 395 packages across 53 dependent modules for known vulnerabilities...

govulncheck: invalid finding: if Frame.Function is set, Frame.Package must also be
make: *** [lint] Error 1

I've updated google.golang.org/protobuf to v1.33.0 but got no luck so far:

$ go get -u google.golang.org/protobuf
go: upgraded google.golang.org/protobuf v1.30.0 => v1.33.0

I'm definitely not a Golang specialist, and it's not clear how to fix this issue. Should I update some other package? Or, is there something obvious I'm missing? Should I downgrade govulncheck to v1.0.4 till it's fixed?

[zpavlinovic]

The latest tagged version of govulncheck is v1.0.4 and you should see the failure with this version. You should not see it with govulncheck@master.

[AlekSi]

There are a few more features we'd like to add before we make the release.

But why bundle features with a vital bug fix into a patch release and make everyone use an untagged version in the meantime?

[silverwind]

Is it really recommended to run govulncheck@master? I have my doubts about its future stability.

[awly]

@zpavlinovic any chance you could tag v1.0.5 with this fix, and then v1.1.0 with the new features in progress?
I'm not sure how much work it is to release a new version, but this would fix failures for a number of users without incurring tech debt of using govulncheck@master in CI pipelines.

[zpavlinovic]

We tagged the new version of govulncheck with v1.1.0.

Is it really recommended to run govulncheck@master? I have my doubts about its future stability.

We never recommend that. The point was that the fix is in and should be available at a more stable tag soon.

But why bundle features with a vital bug fix into a patch release and make everyone use an untagged version in the meantime?

We were in the process of adding a larger set of features that required a slightly breaking change in JSON output. This issue got discovered and fixed in between. Ideally, we would bundle all this together to keep releases cleaner, but given that the issue seems to affect more people than initially projected, we are providing a new tag right now. See release notes for more info.

[joeljuca]

@zpavlinovic sorry, what do you mean with "See release notes for more info"? I'm looking for these notes but I can't find them. golang/go doesn't seem to use GitHub Releases.

[silverwind]

Here: https://github.com/golang/vuln/releases/tag/v1.1.0