x/vuln: govulncheck does not match when using vulnerable symbol

[iainduncani]

What version of Go are you using (go version)?

$ go version
go version go1.19.1 darwin/amd64

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

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/iainduncan/Library/Caches/go-build"
GOENV="/Users/iainduncan/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/iainduncan/go/pkg/mod"
GONOPROXY="github.ibm.com"
GONOSUMDB="github.ibm.com"
GOOS="darwin"
GOPATH="/Users/iainduncan/go"
GOPRIVATE="github.ibm.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/iainduncan/go/src/github.ibm.com/iain-duncan/play_go_vuln_checker/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/pd/fg5ywdgs4897sgwtb0bp6f580000gn/T/go-build2077432827=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I was trying to test the new govulncheck tool. As I'm lazy I looked at the first vulnerability and saw that it applies to uses of github.com/gin-gonic/gin.defaultLogFormatter at v1.5.0. Looking at the logger instance at the vulnerable version a call to gin.Logger should hit that symbol:

https://github.com/gin-gonic/gin/blob/v1.5.0/logger.go#L183
Calls: https://github.com/gin-gonic/gin/blob/v1.5.0/logger.go#L204
Uses defaultLogFormatter: https://github.com/gin-gonic/gin/blob/v1.5.0/logger.go#L207

Therefore I created this main func:

package main

import (
	"github.com/gin-gonic/gin"
)

func main() {
	gin.Logger()(nil)
}

And this go.mod:

module github.ibm.com/iain-duncan/play_go_vuln_checker

go 1.19

require github.com/gin-gonic/gin v1.5.0

require (
	github.com/gin-contrib/sse v0.1.0 // indirect
	github.com/go-playground/locales v0.12.1 // indirect
	github.com/go-playground/universal-translator v0.16.0 // indirect
	github.com/golang/protobuf v1.3.2 // indirect
	github.com/json-iterator/go v1.1.7 // indirect
	github.com/leodido/go-urn v1.1.0 // indirect
	github.com/mattn/go-isatty v0.0.9 // indirect
	github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421 // indirect
	github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742 // indirect
	github.com/ugorji/go/codec v1.1.7 // indirect
	golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10 // indirect
	gopkg.in/go-playground/validator.v9 v9.29.1 // indirect
	gopkg.in/yaml.v2 v2.2.2 // indirect
)

Then I ran:

$ govulncheck ./cmd/vuln   
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Scanning for dependencies with known vulnerabilities...
Found 1 known vulnerability.

Vulnerability #1: GO-2021-0052
  Due to improper HTTP header santization, a malicious user can
  spoof their source IP address by setting the X-Forwarded-For
  header. This may allow a user to bypass IP based restrictions,
  or obfuscate their true source.

  Call stacks in your code:
      cmd/vuln/main.go:8:14: github.ibm.com/iain-duncan/play_go_vuln_checker/cmd/vuln.main calls github.com/gin-gonic/gin.LoggerWithConfig$1

  Found in: github.com/gin-gonic/[email protected]
  Fixed in: github.com/gin-gonic/[email protected]
  More info: https://pkg.go.dev/vuln/GO-2021-0052

=== Informational ===
The vulnerabilities below are in packages that you import, but your code
doesn't appear to call any vulnerable functions. You may not need to take any
action. See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck
for details.
...
Vulnerability #4: GO-2020-0001
  The default Formatter for the Logger middleware (LoggerConfig.Formatter),
  which is included in the Default engine, allows attackers to inject arbitrary
  log entries by manipulating the request path.

  Found in: github.com/gin-gonic/[email protected]
  Fixed in: github.com/gin-gonic/[email protected]
  More info: https://pkg.go.dev/vuln/GO-2020-0001

What did you expect to see?

I expected GO-2020-0001 to be found in my application.

What did you see instead?

GO-2020-0001 was listed as not applying to my application. I see in the vuln page for it that it says it is for All symbols even though the GitHub source says it is for defaultLogFormatter. Either way I would expect it to find it as my application uses defaultLogFormatter.

[timothy-king]

@zpavlinovic

[zpavlinovic]

Thanks for reporting this. This is indeed an issue.

My current quick analysis suggests that the issue appears because defaultLogFormatter is not a name of a vulnerable function, yet it is a name of a (mutable) variable holding a vulnerable function. So when govulncheck encounters the actual value of a function under defaultLogFormatter, then name of the function will not be defaultLogFormatter.

This is different from the case func defaultLogFormatter in which the underlying function has name defaultLogFormatter beacuse identifier defaultLogFormatter is immutable.

We will look into remedies for this.

@neild @julieqiu @jba @tatianab

[iainduncani]

@zpavlinovic thanks for getting back to me so quickly, I'm never sure when using a new tool if it was just user error!

This sounds like a tricky case. I wondered if a quick improvement would be to add Logger and LoggerWithWriter to the list of symbols as you will always end up with the defaultLogFormatter in those cases. It doesn't solve the problem of uses of LoggerWithConfig with a Formatter set, nor does it make sure that the log formatter is ever actually used (though given these are methods to setup a logger one would assume it will be!) so not sure if you think those slight improvements are worth it...

[zpavlinovic]

Yes, the quickest fix would be to add functions/methods that immediately use defaultLogFormatter. I believe the precision would be fine: the users would not complain about it, IMO. I am currently looking into a long term solution that could automate some of this reasoning. Is this issue urgent for you?

[iainduncani]

Is this issue urgent for you?

No, I was just playing around with the new tool and happened to notice it. Thanks for looking into it!

[zpavlinovic]

The fix was to add LoggerWithConfig as the vulnerable symbol instead of defaultLogFormatter.