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

x/vuln/cmd/govulncheck: verbose mode shows redundant call stacks #56176

Closed
zpavlinovic opened this issue Oct 12, 2022 · 3 comments
Closed

x/vuln/cmd/govulncheck: verbose mode shows redundant call stacks #56176

zpavlinovic opened this issue Oct 12, 2022 · 3 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Milestone

Comments

@zpavlinovic
Copy link
Contributor

govulncheck shows redundant call stacks in verbose mode. The issue happens when the actual vulnerable symbol is a private function/method.

During vuln db report creation, we always also compute derived symbols, i.e., exported symbols of the offending package that lead to the actual vulnerable symbols. Generated OSVs will have both of these groups of symbols as vulnerable, making no distinction. govulncheck will then search and report them independently as it does not know any better. For instance, in the verbose output

Vulnerability #8: GO-2022-0288
  An attacker can cause unbounded memory growth in servers
  accepting HTTP/2 requests.

  Call stacks in your code:
      #1: for function serverConn.canonicalHeader
        github.com/hashicorp/vault/vault.requestForwardingHandler.Handoff
            vault/request_forwarding.go:167:2
        github.com/hashicorp/vault/vault.Handoff$2
            vault/request_forwarding.go:168:19
        golang.org/x/net/http2.Server.ServeConn
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:472:10
        golang.org/x/net/http2.serverConn.serve
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:869:33
        golang.org/x/net/http2.serverConn.processFrameFromReader
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:1368:24
        golang.org/x/net/http2.serverConn.processFrame
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:1410:27
        golang.org/x/net/http2.serverConn.processHeaders
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:1803:34
        golang.org/x/net/http2.stream.processTrailerHeaders
            /[redacted]/golang.org/x/net@v0.0.0-20201110031124-69a78807bb2b/http2/server.go:1902:29
        golang.org/x/net/http2.serverConn.canonicalHeader
      #2: for function Server.ServeConn
        github.com/hashicorp/vault/vault.requestForwardingHandler.Handoff
            vault/request_forwarding.go:167:2
        github.com/hashicorp/vault/vault.Handoff$2
            vault/request_forwarding.go:168:19
        golang.org/x/net/http2.Server.ServeConn

govulncheck shows call stacks for vulnerable symbols Server.ServeConn and serverConn.canonicalHeader. The latter one is redundant as it goes through the derived symbol Server.ServeConn.

@hyangah

@zpavlinovic zpavlinovic added UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo labels Oct 12, 2022
@zpavlinovic zpavlinovic added this to the Backlog milestone Oct 12, 2022
@zpavlinovic
Copy link
Contributor Author

The ideal fix is to not show call stacks that are prefix of one another, where the prefix ends in a vulnerable symbol.

@zpavlinovic zpavlinovic self-assigned this Oct 12, 2022
@zpavlinovic zpavlinovic modified the milestones: Backlog, vuln/2022 Oct 12, 2022
@zpavlinovic zpavlinovic added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 12, 2022
@zpavlinovic
Copy link
Contributor Author

This should in principle be solved at the level of vulndb. If a private symbol has derived symbols, then we should exclude the private symbol from the OSV entry. This will at least not produce redundant call stacks between exported and non-exported vulnerable symbols. See #56185

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/445078 mentions this issue: internal/govulncheck: choose unique call stacks

softdev050 added a commit to softdev050/Golangvuln that referenced this issue Apr 5, 2023
A call stack is unique if it does not go through other detected
vulnerable symbols.

Fixes golang/go#56176

Change-Id: Iea214f9a879610131910dbede7fa87012bb91fa3
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/445078
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
sayjun0505 added a commit to sayjun0505/Golangvuln that referenced this issue Apr 8, 2023
A call stack is unique if it does not go through other detected
vulnerable symbols.

Fixes golang/go#56176

Change-Id: Iea214f9a879610131910dbede7fa87012bb91fa3
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/445078
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
stanislavkononiuk added a commit to stanislavkononiuk/Golangvuln that referenced this issue Jun 26, 2023
A call stack is unique if it does not go through other detected
vulnerable symbols.

Fixes golang/go#56176

Change-Id: Iea214f9a879610131910dbede7fa87012bb91fa3
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/445078
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
@golang golang locked and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
None yet
Development

No branches or pull requests

2 participants