golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.32k stars 17.7k forks source link

x/vuln: improve OpenVEX status output for fixed vulnerabilities #68338

Closed knqyf263 closed 4 months ago

knqyf263 commented 4 months ago

govulncheck version

Go: go1.22.4 Scanner: govulncheck@v1.1.2 DB: https://vuln.go.dev DB updated: 2024-07-03 16:27:09 +0000 UTC

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

Yes

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/teppei/Library/Caches/go-build'
GOENV='/Users/teppei/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/teppei/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/teppei'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.4/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.4/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.4'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/teppei/src/github.com/aquasecurity/trivy/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-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/9_/3y18vrrs5bz6gbkq0kc_4fv80000gn/T/go-build2940602674=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Run govulncheck -format openvex ./... on my project

What did you see happen?

When scanning projects with govulncheck, all non-impacting vulnerabilities are reported with the status "not_affected" in the generated OpenVEX statements, regardless of whether the project is using a version that has already fixed the vulnerability.

For example, when scanning a project using k8s.io/client-go v0.29.0, govulncheck outputs the following for vulnerability GO-2021-0064:

{
  "vulnerability": {
    "@id": "https://pkg.go.dev/vuln/GO-2021-0064",
    "name": "GO-2021-0064",
    "description": "Unauthorized credential disclosure via debug logs in k8s.io/kubernetes and k8s.io/client-go",
    "aliases": [
      "CVE-2020-8565",
      "GHSA-8cfg-vx93-jvxw"
    ]
  },
  "products": [
    {
      "@id": "Unknown Product"
    }
  ],
  "status": "not_affected",
  "justification": "vulnerable_code_not_present",
  "impact_statement": "Govulncheck determined that the vulnerable code isn't called"
}

What did you expect to see?

I expected govulncheck to differentiate between vulnerabilities that are not present and those that have been fixed in the version being used. In the case of GO-2021-0064, I believe the status should be reported as "fixed" for version 0.29.0 since this vulnerability was addressed in k8s.io/client-go v0.20.0-alpha.2.

Currently, govulncheck seems to only output 'affected' or 'not_affected' statuses when generating OpenVEX. From my understanding of the OpenVEX specification, it could be more precise by using three statuses: 'affected', 'not_affected', and 'fixed'. I think it should use the 'fixed' status when the project is using a version that has already addressed the vulnerability.

In fact, the "fixed" vulnerabilities are not shown with govulncheck -show verbose ./.... They appear to be internally distinct. Please correct me if I'm missing something.

Thanks for the great work!

mauri870 commented 4 months ago

Introducing a new status sounds reasonable. I reviewed the OpenVEX specification but couldn't find the appropriate field to inform about the fixed version. From my understanding, we should have a follow-up statement with the status set to "fixed" and products.@id containing the fixed version. Currently, this key is not used and always contains Unknown Product. I believe this requires further consideration to properly align the implementation with the specification.

I think we could start by differentiating the status of "fixed" from "not_affected."

cc @golang/vulndb

cagedmantis commented 4 months ago

@golang/vulndb

knqyf263 commented 4 months ago

but couldn't find the appropriate field to inform about the fixed version.

Interesting. I had never thought about including a fixed version.

@puerco Is it possible to include fixed versions?

puerco commented 4 months ago

Yes fixed is one of the defined vex status labels.

An algorithm would go like this:

govulncheck looks at a binary or source code (any of those go into the vex product, see https://github.com/golang/go/issues/68152#issuecomment-2216458381 ).

When looking at the dependencies, if one has a CVE associated with it:

Note that the critical part is getting the dependency versions correct in the subcomponent (see the examples in https://github.com/golang/go/issues/68152#issuecomment-2216458381 )

@mauri870 :

we should have a follow-up statement with the status set to "fixed" and products.@id containing the fixed version.

Two notes there:

  1. First the fixed version should go into the subcomponent @ id of the product.
  2. While VEX is designed to be a chronological series of statements, in this case I think you can't issue the sequenced statements, just a fixed statement and that's it. The VEX product (the source or a binary) is fixed at a point in time. In other words, the go.mod in the analyzed commit (when the product is the main module source) or the built binary either include the fixed dependency or don't. They would never change (as changing it would result in another commit or different binary). This means that a transition from affected to fixed never occurs when looking at a specific commit or built binary.
knqyf263 commented 4 months ago

First the fixed version should go into the subcomponent @ id of the product.

That is the fixed version currently used in the product (a.k.a installed version) and different from the version that initially fixed the vulnerability, right? In the above example, the subcomponent version is v0.29.0, which already fixed GO-2021-0064, but the first fixed version of GO-2021-0064 is v0.20.0-alpha.2. The subcomponent id should be pkg:golang/k8s.io/client-go@v0.29.0, not pkg:golang/k8s.io/client-go@v0.20.0-alpha.2.

I reviewed the OpenVEX specification but couldn't find the appropriate field to inform about the fixed version.

@mauri870 Which version are you talking about? I thought you meant a fixed version, not an installed version, which already fixed the vulnerability. I may be wrong.

mauri870 commented 4 months ago

@mauri870 Which version are you talking about? I thought you meant a fixed version, not an installed version, which already fixed the vulnerability. I may be wrong.

I was referring to the version that fixed the vuln, not the installed version. I thought we'll have to use a chronological series of statements but it seems that a single statement with a status "fixed" is the the way to do it.

Thanks @puerco for the detailed summary! Looks like handling https://github.com/golang/go/issues/68152#issuecomment-2216458381 would be part of the implementation needed to support the "fixed" status.

knqyf263 commented 4 months ago

I was referring to the version that fixed the vuln, not the installed version.

Thanks for confirming. If I understand correctly, there is no place to fill in the fixed version, but you can put the installed version into the subcomponent id with the "fixed" status as below.

{
  "vulnerability": {
    "@id": "https://pkg.go.dev/vuln/GO-2021-0064",
    "name": "GO-2021-0064",
    "description": "Unauthorized credential disclosure via debug logs in k8s.io/kubernetes and k8s.io/client-go",
    "aliases": [
      "CVE-2020-8565",
      "GHSA-8cfg-vx93-jvxw"
    ]
  },
  "products": [
    {
      "@id": "pkg:golang/github.com/org/repo@1.2.3",
      "subcomponents": [
      {
          "@id": "pkg:golang/k8s.io/client-go@0.29.0",
        }
      ]
    }
  ],
  "status": "fixed",
}
macedogm commented 4 months ago

When scanning projects with govulncheck, all non-impacting vulnerabilities are reported with the status "not_affected" in the generated OpenVEX statements, regardless of whether the project is using a version that has already fixed the vulnerability.

In case it helps, on my local fork of govulncheck that I did the initial work on https://github.com/golang/go/issues/68152, I'm using the following modification to filter only the applicable vulnerabilities based on the used versions:

diff --git a/internal/vulncheck/fetch.go b/internal/vulncheck/fetch.go
index 700a7f9..0dcff98 100644
--- a/internal/vulncheck/fetch.go
+++ b/internal/vulncheck/fetch.go
@@ -17,11 +17,14 @@ func FetchVulnerabilities(ctx context.Context, c *client.Client, modules []*pack
        mreqs := make([]*client.ModuleRequest, len(modules))
        for i, mod := range modules {
                modPath := mod.Path
+               modVersion := mod.Version
                if mod.Replace != nil {
                        modPath = mod.Replace.Path
+                       modVersion = mod.Replace.Version
                }
                mreqs[i] = &client.ModuleRequest{
-                       Path: modPath,
+                       Path:    modPath,
+                       Version: modVersion,
                }
        }
        resps, err := c.ByModules(ctx, mreqs)

That helps reduce the noise.

FYI: In SUSE Rancher we are doing an initial work with govulncheck to reduce the noise in CVEs in the images that we publish.

gabyhelp commented 4 months ago

Related Issues and Documentation

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

zpavlinovic commented 4 months ago

@maceonthompson

I think govulncheck should not report these vulnerabilities at all. That would be the simplest solution; it wouldn't complicate things internally for us. How satisfactory would that be?

maceonthompson commented 4 months ago

I'm happy to just drop those vulnerabilities. Right now, govulncheck emits those vulnerabilities in the JSON with no findings (it just emits the OSV), but we could definitely omit vulns with only an osv and no findings from the VEX output.

knqyf263 commented 4 months ago

I think govulncheck should not report these vulnerabilities at all.

In our use case, we filter out "fixed" vulnerabilities as we're more interested in suppressing false positives, so it's totally fine.

gopherbot commented 4 months ago

Change https://go.dev/cl/597396 mentions this issue: internal/openvex: omit vulns with no findings

zpavlinovic commented 4 months ago

I think that should be the first approach then. We can add the "fixed" vulns if that is something users would really benefit from.

maceonthompson commented 4 months ago

Closing this, as the vex output now matches the text and SARIF outputs (in that it omits vulns that are not imported at a vulnerable version). We can re-open the issue if we end up changing this behavior.