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

Jump to bottom

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

Closed
iainduncani opened this issue Sep 29, 2022 · 6 comments
Closed

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

iainduncani opened this issue Sep 29, 2022 · 6 comments
Assignees
@zpavlinovic
Labels
FrozenDueToAge vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Milestone
vuln/2022

Comments

@iainduncani
Copy link

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/gin@v1.5.0
  Fixed in: github.com/gin-gonic/gin@v1.7.7
  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/gin@v1.5.0
  Fixed in: github.com/gin-gonic/gin@v1.6.0
  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.
@iainduncani iainduncani added the vulncheck or vulndb Issues for the x/vuln or x/vulndb repo label Sep 29, 2022
@gopherbot gopherbot modified the milestones: Unreleased, vuln/unplanned Sep 29, 2022
@timothy-king
Copy link
Contributor

@zpavlinovic

@zpavlinovic zpavlinovic self-assigned this Sep 29, 2022
@zpavlinovic
Copy link
Contributor

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

@zpavlinovic zpavlinovic modified the milestones: vuln/unplanned, vuln/2022 Sep 29, 2022
@iainduncani
Copy link
Author

@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
Copy link
Contributor

zpavlinovic commented Sep 30, 2022 •
edited

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
Copy link
Author

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
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@zpavlinovic zpavlinovic

Labels
FrozenDueToAge vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
None yet
Milestone
vuln/2022
Development

No branches or pull requests
4 participants
@timothy-king @zpavlinovic @iainduncani @gopherbot