x/vuln: enhance deduping of similar traces

hyangah commented 1 week ago

govulncheck version

Go: devel go1.23-9d33956503 Thu Jun 20 17:46:05 2024 +0000 Scanner: govulncheck@v1.1.2 DB: https://vuln.go.dev DB updated: 2024-06-20 18:18:26 +0000 UTC

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

yes.

Output of `go env` in your module/workspace:

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/hakim/Library/Caches/go-build'
GOENV='/Users/hakim/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/hakim/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/hakim/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/hakim/sdk/gotip'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/hakim/sdk/gotip/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='devel go1.23-9d33956503 Thu Jun 20 17:46:05 2024 +0000'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/hakim/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/hakim/release/vscode-go/extension/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 x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/5p/zn7ykc111kn3lm09h_47mz2w001py5/T/go-build1839044179=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

git clone https://github.com/golang/vscode-go git checkout 540e146da cd extension govulncheck ./tools/...

What did you see happen?

several similar traces with the same entry points (tools/generate.go).

=== Symbol Results ===

Vulnerability #1: GO-2024-2687
    HTTP/2 CONTINUATION flood in net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2687
  Module: golang.org/x/net
    Found in: golang.org/x/net@v0.19.0
    Fixed in: golang.org/x/net@v0.23.0
    Example traces found:
      #1: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.ConnectionError.Error
      #2: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.ErrCode.String
      #3: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.FrameHeader.String
      #4: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.FrameType.String
      #5: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.GoAwayError.Error
      #6: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.Setting.String
      #7: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.SettingID.String
      #8: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.StreamError.Error
      #9: tools/generate.go:178:14: tools.main calls fmt.Fprintf, which eventually calls http2.chunkWriter.Write
      #10: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.connError.Error
      #11: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.duplicatePseudoHeaderError.Error
      #12: tools/relnotes/relnotes.go:328:2: relnotes.getURL calls http2.gzipReader.Close
      #13: tools/relnotes/relnotes.go:329:29: relnotes.getURL calls ioutil.ReadAll, which eventually calls http2.gzipReader.Read
      #14: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.headerFieldNameError.Error
      #15: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.headerFieldValueError.Error
      #16: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.pseudoHeaderError.Error
      #17: tools/generate.go:178:14: tools.main calls fmt.Fprintf, which eventually calls http2.stickyErrWriter.Write
      #18: tools/relnotes/relnotes.go:328:2: relnotes.getURL calls http2.transportResponseBody.Close
      #19: tools/relnotes/relnotes.go:329:29: relnotes.getURL calls ioutil.ReadAll, which eventually calls http2.transportResponseBody.Read
      #20: tools/generate.go:251:52: tools.main calls fmt.Sprintf, which eventually calls http2.writeData.String

govulncheck -show=traces shows vulnerable types implementing Stringer will appear.

Example traces found:
      #1: for function golang.org/x/net/http2.ConnectionError.Error
        tools/generate.go:251:52: github.com/golang/vscode-go/extension/tools.main
        src/fmt/print.go:239:12: fmt.Sprintf
        src/fmt/print.go:1074:16: fmt.pp.doPrintf
        src/fmt/print.go:749:22: fmt.pp.printArg
        src/fmt/print.go:667:24: fmt.pp.handleMethods
        http2/errors.go:67:26: golang.org/x/net/http2.ConnectionError.Error
      #2: for function golang.org/x/net/http2.ErrCode.String
        tools/generate.go:251:52: github.com/golang/vscode-go/extension/tools.main
        src/fmt/print.go:239:12: fmt.Sprintf
        src/fmt/print.go:1074:16: fmt.pp.doPrintf
        src/fmt/print.go:749:22: fmt.pp.printArg
        src/fmt/print.go:673:25: fmt.pp.handleMethods
        http2/errors.go:49:18: golang.org/x/net/http2.ErrCode.String
        ...

What did you expect to see?

Ideally no report unless the types are really used. However, I guess this is a type of false positives hard for the current govulncheck hard to handle.

We think we can still reduce the volume of text and make it less overwhelming, by enhancing the deduping mechanism.

From @ianthehat

We do try to limit the redundant traces both in the original call graph analysis and the text output, but we know the heuristics could be improved, we are just not sure it is worth doing. In this case we collapse the call chain to three elements, user code, entry point of non user code, vulnerable symbol, and then only output one entry for each of those triples. We could instead dedupe by user code, entry point of non user code, vulnerability which would clearly be better in this case.

gabyhelp commented 1 week ago

Similar Issues

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

zpavlinovic commented 1 week ago

Putting the false positive discussion aside, note that these are not duplicate traces. Although they do start from the same entry point, in this case main, they lead to a different vulnerable symbol.

hyangah commented 1 week ago

The true vulnerable symbol in this specific case is an internal symbol, but unfortunately it affects most symbols in the http2 package. If the internal symbol could be used as the final vulnerable symbol, the text summary of each trace would look identical. On the other hand, given the nature of this false positive, I am afraid the number of displayed traces won't shrink with the suggested heuristic. There will be many different entries that use fmt. :-(

zpavlinovic commented 1 week ago

I agree, this is just a misfortunate false positive. If the vulnerability was indeed reachable, govulncheck would report only exported symbols and then I think we'd see much less traces because the user would likely use just a few of the net/http APIs.

golang / go