x/vuln: OpenVEX report lacks affected product

macedogm commented 3 months ago

govulncheck version

govulncheck -version
Go: go1.22.4
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='/home/gmacedo/.cache/go-build'
GOENV='/home/gmacedo/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/gmacedo/go/pkg/mod'
GOOS='linux'
GOPATH='/home/gmacedo/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib64/go/1.22'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/lib64/go/1.22/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.4'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/gmacedo/code/github.com/macedogm/vuln/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2885216502=/tmp/go-build -gno-record-gcc-switches'



### What did you do?

Executing `govulncheck` with `-format openvex` produces a valid OpenVEX report, but the list of the affect products lacks valid information, instead it always contains `Unknown Product`.

### What did you see happen?

Please see above.

### What did you expect to see?

I would expect to see the correct list of affected products, i.e., the Go package path and version, where the vulnerabilities was identified, in the right Purl format for Go packages - `pkg:golang/...`.

gabyhelp commented 3 months ago

Similar Issues

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

macedogm commented 3 months ago

A tentative fix/improvement for this is done in https://github.com/golang/vuln/pull/11.

zpavlinovic commented 3 months ago

@maceonthompson

To summarize, the proposal here is to use (purls of) packages of detected vulnerabilities as product IDs. This would allow govulncheck openvex output to work with other tools, such as trivy and grype.

This proposal is in contrast with what we were thinking initially and that is to have the artifact under analysis as the product. That approach, however, is difficult when dealing with, say, different package patterns of a module. The proposal here does not have these issues. It also seems openvex spec allows what is proposed here.

macedogm commented 3 months ago

Hey @maceonthompson 👋🏻

@zpavlinovic thanks for explanation above.

FYI we were having an initial discussion about a possible implementation for this in https://github.com/golang/vuln/pull/11#issuecomment-2186987079. The code is still available in my fork https://github.com/macedogm/vuln/commits/openvex-improvement/.

It's simple, but works. The biggest challenge that I saw is that govulncheck's OSV struct doesn't contain a way to track the exact affected version of the vulnerable package/path that was identified in the scanned code (see in https://github.com/golang/vuln/blob/d44b651a2e0f43966413e20b65cb619bdb621e7e/internal/vulncheck/emit.go#L21). Due to that, I added an Internal struct inside OSV to hold the needed info. Otherwise, the biggest benefit of the OpenVEX spec cannot be achieved, which is to identify the exact product (package and version) not affected, which can be removed by the downstream vulnerability scanners (Trivy, Grype etc.), although govulncheck knows about the exact version.

The product, in the OpenVEX spec, can have subcomponents. My initial implementation doesn't add this.

Where I work we are planning to use this as a way to remove hundreds of false positives flagged by vulnerability scanners. Initially we are still using my forked version.

Happy to collaborate further if needed.

zpavlinovic commented 3 months ago

Pushing things inside OSV struct is not the way to go IMO. There should be a different way of doing this, but that is an implementation detail. Your code is useful nonetheless. We'll use the issue here to decide whether to proceed with using vulnerable packages as product IDs.

maceonthompson commented 3 months ago

Hi @macedogm,

Thank you for the work/time you've already spent on this issue!

If the idea is to use the source of the vulnerability (i.e. the identifier of the offending dependency) in the product field, I think govulncheck’s output already contains all the information we need. OSVs contain module and package paths for a vulnerability, and findings always contain the module path and imported version of the offending vulnerable package/path. If govulncheck emits an OSV and no findings, that means that the vulnerable package was not imported at a vulnerable version.

I really like this proposal, but I think it goes against the VEX specification (see Section 2.5.1 and 2.5.2), which state that the Product ID should be the artifact that you’re scanning, and subcomponents are where vulnerable product IDs can be filled in. If we kept the initial idea but populated the subcomponent field instead, would this work/help your use case?

macedogm commented 3 months ago

Hey @maceonthompson.

Thanks for the update.

I really like this proposal, but I think it goes against the VEX specification (see Section 2.5.1 and 2.5.2), which state that the Product ID should be the artifact that you’re scanning, and subcomponents are where vulnerable product IDs can be filled in.

You are totally right, in the implementation that I did I was listing directly the affected path/package and version. Example from my forked version:

    {    
      "vulnerability": {
        "@id": "https://pkg.go.dev/vuln/GO-2021-0061",
        "name": "GO-2021-0061",
        "description": "Denial of service in gopkg.in/yaml.v2",
        "aliases": [
          "CVE-2021-4235",
          "GHSA-r88r-gmrh-7j83"
        ]
      },   
      "products": [
        {
          "@id": "pkg:golang/gopkg.in/yaml.v2@2.4.0"
        }
      ],   
      "status": "not_affected",
      "justification": "vulnerable_code_not_present",
      "impact_statement": "Govulncheck determined that the vulnerable code isn't called"
    },

If we kept the initial idea but populated the subcomponent field instead, would this work/help your use case?

Yes, it would definitely work and be in full compliance with the spec 👍🏻 Out of curiosity, how do you plan to derive the artifact, for example, when the scan is done in source versus binary mode?

Thanks for working on this, it's much appreciated.

puerco commented 2 months ago

Hello from the @openvex project :wave:

Thanks for opening this issue, I have been meaning to contribute a patch to fix this but I'm glad this discussion is already underway!

Let me add some context on the VEX product and subproduct.

@maceonthompson's reading of the spec is correct here:

the VEX specification (see Section 2.5.1 and 2.5.2), which state that the Product ID should be the artifact that you’re scanning

Let's imagine two scenarios, source and binary.

For example, when running in source mode, to craft a statement for the source of vexctl (at a random commit) the main vexctl module would act as the product and the subcomponent would point to the dependency containing the vulnerability. Here is an example fixed from the output of govulncheck:

    {
      "vulnerability": {
        "@id": "https://pkg.go.dev/vuln/GO-2024-2947",
        "name": "GO-2024-2947",
        "description": "Leak of sensitive information to log files in github.com/hashicorp/go-retryablehttp",
        "aliases": [
          "GHSA-3633-5h82-39pq"
        ]
      },
      "products": [
        {
          "@id": "pkg:golang/github.com/openvex/vexctl@c7362b73a06e2f8e4285b115d98e84f0ebfb3db8",
          "subcomponents":  [
              {
                  "@id": "pkg:golang/github.com/hashicorp/go-retryablehttp@v0.7.5"
              }
          ]
        }
      ],
      "status": "affected",
    }

Now when it comes to binaries, you don't need to specify a package URL (a purl). If you have a purl for one, its fine but the product identifier field in OpenVEX takes a URI so, in the case of binaries, the best way to specify them is to use a URI with a file schema but note that the real value comes from anchoring the statement to the file with its hashes. Our tooling will operate with hashes when available.

This is the same output but with a linux binary built from the same build point as above. Note that the path really becomes irrelevant once we can content-address the binary with a hash:

    {
      "vulnerability": {
        "@id": "https://pkg.go.dev/vuln/GO-2024-2947",
        "name": "GO-2024-2947",
        "description": "Leak of sensitive information to log files in github.com/hashicorp/go-retryablehttp",
        "aliases": [
          "GHSA-3633-5h82-39pq"
        ]
      },
      "products": [
        {
          "@id": "file:///vexctl-linux-amd64",
          "hashes": {
              "sha-256": "f8d37ec5f6ee5fb1352b0162443a9c8fcd50dab5125fde288daca0533a9286b7",
              "sha-512": "875079d0b61c6a03dcba46eef3a877d5be8789a068ad5f182a876ee08f754e1f2d7cbb5f22be73a77ff49d5565e13579c85e3f9f32884ec107e5e7624484f093"
              },
              "subcomponents":  [
                  {
                      "@id": "pkg:golang/github.com/hashicorp/go-retryablehttp@v0.7.5"
                  }
              ]
        }
      ],
      "status": "affected",
    }

The important part is that the vex statement operates on the triad of a vulnerability in a dependency evaluated in the context of the product (source or an artifact including the dependency).

I'm happy to contribute a patch to handle these :)

zpavlinovic commented 2 months ago

Thanks for all the details. A few questions/remarks.

Users can specify a package pattern different from ./..., effectively analyzing just part of a module. Should the product ID stay the same in that case?
What if there is no information on the commit or module version? Should the product ID then just be the module name?
What if there does exist a commit but the user has changed the code locally? Currently, go command uses devel as the module version. Perhaps that should be used here too?

The above scenarios are less likely to happen when users need openvex format, but we should have the story for them nonetheless.

puerco commented 2 months ago

effectively analyzing just part of a module. Should the product ID stay the same in that case?

For those cases, the golang package URL spec defines a way to specify a subpath (the example doesn't have the @+version but illustrates the part under analysis):

 The `subpath` is used to point to a subpath inside a package.
 Examples:
 pkg:golang/google.golang.org/genproto#googleapis/api/annotations

What if there is no information on the commit or module version? Should the product ID then just be the module name?

Is it possible to know the hash of the package (the one that'd go into go.sum)? If so I'd add it to the hashes to make it explicit. If not just, I'd use the unversioned purl (eg pkg:golang/google.golang.org/genproto ). I think it is preferable to a blank product or an invalid value. Or adding a qualifier to not match as we take an unversioned purl in OpenVEX as "match any version of this module". See the next point about the qualifier :point_down:

What if there does exist a commit but the user has changed the code locally? Currently, go command uses devel as the module version. Perhaps that should be used here too?

This is an excellent question. Yes, it sounds to me like a qualifier that should be appended to the package URL (eg pkg:golang/google.golang.org/genproto@commitSHA?devel ) it would serve to effectively get a different identifier from the untouched source. We could propose it as an addition to the purl spec (although it could take a long time).

I'd prefer adding the ?devel over the unversioned purl to avoid matching an unversioned purl and the resulting identifier still carries enough context to identify the package for tools that know what they are doing, same case when the version is unknown.

zpavlinovic commented 2 months ago

effectively analyzing just part of a module. Should the product ID stay the same in that case?

For those cases, the golang package URL spec defines a way to specify a subpath (the example doesn't have the @+version but illustrates the part under analysis):

I think this is an easier case. A bigger issue is when someone analyzes something like ./pkg/... (e.g., happens in kubernetes). This pattern can resolve to multiple packages of the module, but not the whole module. We might not have a better option here then using just the module path.

 The `subpath` is used to point to a subpath inside a package.
 Examples:
 pkg:golang/google.golang.org/genproto#googleapis/api/annotations
What if there is no information on the commit or module version? Should the product ID then just be the module name?

Is it possible to know the hash of the package (the one that'd go into go.sum)? If so I'd add it to the hashes to make it explicit. If not just, I'd use the unversioned purl (eg pkg:golang/google.golang.org/genproto ). I think it is preferable to a blank product or an invalid value. Or adding a qualifier to not match as we take an unversioned purl in OpenVEX as "match any version of this module". See the next point about the qualifier 👇

Since we are talking about the module being analyzed, then there won't be anything for it in the go.sum. I also think just having the unversioned purl should be fine here.

What if there does exist a commit but the user has changed the code locally? Currently, go command uses devel as the module version. Perhaps that should be used here too?

This is an excellent question. Yes, it sounds to me like a qualifier that should be appended to the package URL (eg pkg:golang/google.golang.org/genproto@commitSHA?devel ) it would serve to effectively get a different identifier from the untouched source. We could propose it as an addition to the purl spec (although it could take a long time).

I'd prefer adding the ?devel over the unversioned purl to avoid matching an unversioned purl and the resulting identifier still carries enough context to identify the package for tools that know what they are doing, same case when the version is unknown.

In general, I think we would likely need to get an agreement on the purl encoding of Go artifacts at the level of the language.

maceonthompson commented 2 months ago

I feel that the best step forward (for now/the immediate next release) is to implement the dependencies as subcomponents, which won't be affected by the edge cases detailed above. This matches the originally requested functionality and allows us to keep working on identifying the module being analyzed in the meantime.

puerco commented 2 months ago

Agree @maceonthompson that one is critical to match components and some initial uses cases can unblocked when tools reading the VEX documents have the full context of the artifact being scanned.

zpavlinovic commented 2 months ago

If we kept the initial idea but populated the subcomponent field instead, would this work/help your use case?

Yes, it would definitely work and be in full compliance with the spec 👍🏻 ...

By looking at trivy code, it seems that not having a package identifier for the product will result in nothing being matched regardless of subproducts. I might be wrong as I am not familiar with the code base.

macedogm commented 2 months ago

Yes, the package/module is needed inside the product, with the affected/fixed dependency/path as the subcomponent (please excuse if I'm mixing terminologies by package per module per dependency path).

For example, if govulncheck generates the following Vex file (just an example that I partially generated with my forked govulncheck then passed it to vexctl):

{
  "@context": "https://openvex.dev/ns/v0.2.0",
  "@id": "https://openvex.dev/docs/public/vex-9c71e5482d226764f5e1da8f8fd88201aa79d143f9a37e651f6c4306e584dbde",
  "author": "Unknown Author",
  "timestamp": "2024-07-10T17:19:23.734221546-03:00",
  "version": 1,
  "statements": [
    {
      "vulnerability": {
        "name": "CVE-2023-32193"
      },
      "timestamp": "2024-07-10T17:19:23.7342222-03:00",
      "products": [
        {
          "@id": "pkg:golang/github.com/rancher/webhook",
          "subcomponents": [
            {
              "@id": "pkg:golang/github.com/rancher/norman@0.0.0-20240206180703-6eda4bc94b4c"
            }
          ]
        }
      ],
      "status": "not_affected",
      "impact_statement": "vulnerable_code_not_present"
    }
  ]
}

Where the products[0].@id comes from the scanned module by govulncheck -> https://github.com/rancher/webhook/blob/release/v0.4/go.mod#L1 . This could be further extended if a version is passed, for example, pkg:golang/github.com/rancher/webhook@v0.4.1 in order to make the matching of vulnerabilities more strict.

Then passing it to Trivy will result in the "not_affected" CVE being correctly suppressed.

> trivy repo --scanners vuln --branch release/v0.4 --vex vex.json --show-suppressed https://github.com/rancher/webhook
2024-07-10T17:20:00-03:00   INFO    Vulnerability scanning is enabled
Enumerating objects: 2765, done.
Counting objects: 100% (2765/2765), done.
Compressing objects: 100% (1260/1260), done.
^_Total 2765 (delta 1544), reused 2315 (delta 1201), pack-reused 0
2024-07-10T17:20:15-03:00   INFO    Number of language-specific files   num=1
2024-07-10T17:20:15-03:00   INFO    [gomod] Detecting vulnerabilities...

go.mod (gomod)

Total: 3 (UNKNOWN: 0, LOW: 1, MEDIUM: 0, HIGH: 2, CRITICAL: 0)

┌──────────────────────────────────────────────────────────────┬────────────────┬──────────┬────────┬───────────────────┬─────────────────────────┬─────────────────────────────────────────────────────────────┐
│                           Library                            │ Vulnerability  │ Severity │ Status │ Installed Version │      Fixed Version      │                            Title                            │
├──────────────────────────────────────────────────────────────┼────────────────┼──────────┼────────┼───────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────────┤
│ go.opentelemetry.io/contrib/instrumentation/google.golang.o- │ CVE-2023-47108 │ HIGH     │ fixed  │ 0.35.0            │ 0.46.0                  │ opentelemetry-go-contrib: DoS vulnerability in otelgrpc due │
│ rg/grpc/otelgrpc                                             │                │          │        │                   │                         │ to unbound cardinality metrics                              │
│                                                              │                │          │        │                   │                         │ https://avd.aquasec.com/nvd/cve-2023-47108                  │
├──────────────────────────────────────────────────────────────┼────────────────┤          │        ├───────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────────┤
│ go.opentelemetry.io/contrib/instrumentation/net/http/otelht- │ CVE-2023-45142 │          │        │ 0.35.1            │ 0.44.0                  │ opentelemetry: DoS vulnerability in otelhttp                │
│ tp                                                           │                │          │        │                   │                         │ https://avd.aquasec.com/nvd/cve-2023-45142                  │
├──────────────────────────────────────────────────────────────┼────────────────┼──────────┤        ├───────────────────┼─────────────────────────┼─────────────────────────────────────────────────────────────┤
│ k8s.io/kubernetes                                            │ CVE-2024-3177  │ LOW      │        │ 1.28.6            │ 1.27.13, 1.29.4, 1.28.9 │ kubernetes: kube-apiserver: bypassing mountable secrets     │
│                                                              │                │          │        │                   │                         │ policy imposed by the ServiceAccount admission plugin...    │
│                                                              │                │          │        │                   │                         │ https://avd.aquasec.com/nvd/cve-2024-3177                   │
└──────────────────────────────────────────────────────────────┴────────────────┴──────────┴────────┴───────────────────┴─────────────────────────┴─────────────────────────────────────────────────────────────┘

Suppressed Vulnerabilities (Total: 2)

┌───────────────────────────┬────────────────┬──────────┬──────────────┬───────────┬─────────┐
│          Library          │ Vulnerability  │ Severity │    Status    │ Statement │ Source  │
├───────────────────────────┼────────────────┼──────────┼──────────────┼───────────┼─────────┤
│ github.com/rancher/norman │ CVE-2023-32193 │ HIGH     │ not_affected │ N/A       │ OpenVEX │
│                           │                │          │              │           │         │
│                           │                │          │              │           │         │
└───────────────────────────┴────────────────┴──────────┴──────────────┴───────────┴─────────┘

puerco commented 2 months ago

Right, if there is no product nothing will get matched (same in grype). This is why we should populate the product when possible. External tooling may be able to fill in the missing product but it's not ideal.

knqyf263 commented 2 months ago

By looking at trivy code, it seems that not having a package identifier for the product will result in nothing being matched regardless of subproducts. I might be wrong as I am not familiar with the code base.

Yes, that's correct. If a product has subcomponents, Trivy applies VEX to the dependency graph (the same idea is described in the OSV blog). Incorrect product ID results in nothing filtered as it doesn't match the graph. https://aquasecurity.github.io/trivy/v0.53/docs/supply-chain/vex/#applying-vex-to-dependency-trees

If a product has no subcomponents, it works as a simple filter in Trivy, but in that case it may incorrectly filter vulnerabilities in another product.

{
  "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",
  },
  "products": [
    {
      "@id": "pkg:golang/k8s.io/client-go@0.20.0-alpha.2"
    }
  ],
  "status": "not_affected",
  "justification": "vulnerable_code_not_present",
  "impact_statement": "Govulncheck determined that the vulnerable code isn't called"
}

For example, my project is not affected by GO-2021-0064 in k8s.io/client-go, but other projects may be affected. Ideally, the correct product and subcomponents should be filled in.

External tooling may be able to fill in the missing product but it's not ideal.

That's exactly what we're doing now.

gopherbot commented 2 months ago

Change https://go.dev/cl/598956 mentions this issue: internal/openvex: populate product subcomponents

maceonthompson commented 2 months ago

A follow up question on subcomponent IDs: Right now we are planning on having the subcomponent ID be just the module path (since that seems to be most helpful based on the examples above), but govulncheck has more specific package and symbol information that we could theoretically include as well. Would it be helpful to encode that extra information at the end of the PURL?

puerco commented 2 months ago

I think the import path and version should be enough. What else could you encode in the purl? The more specific the purl is the harder it is to match but perhaps I'm missing another use case that may make more sense.

maceonthompson commented 2 months ago

I agree that the matching becomes more difficult, which is why we're currently basing it just off the module path. We could, however, include the package path as well - which is technically part of the import path. But if the consensus is that the extra info isn't useful for this use case, I'm happy to leave it as is.

zpavlinovic commented 2 months ago

FWIW, this is also what the https://github.com/golang/vuln/pull/11 proposed fix does. There is also a consistency issue. If someone runs govulncheck in -scan module mode, then package info will be missing.

macedogm commented 2 months ago

My first answer is to agree with:

I think the import path and version should be enough.

@maceonthompson could you please provide some examples of how the subcomponent ID would be regarding what you mentioned?

... include the package path as well - which is technically part of the import path.

Asking, so we have detailed info to make a call and how different the information would be for each one of the scan mode - 'module', 'package', or 'symbol'.

maceonthompson commented 2 months ago

@maceonthompson could you please provide some examples of how the subcomponent ID would be regarding what you mentioned?

@macedogm An example: Say there is a vulnerability in the module `golang.org/x/vuln` at version `v1.0.0`. More specifically, it is present in the package `golang.org/x/vuln/scan` and has the vulnerable symbol `foo`. Given each scan mode, this is what the ID could look like	module	package	symbol
pkg:golang/golang.org/x/vuln@v1.0.0	pkg:golang/golang.org/x/vuln@v1.0.0#scan	pkg:golang/golang.org/x/vuln@v1.0.0#scan

This matches how pkgsite presents packages vs modules. Otherwise, every scan mode would match "module" mode, not including the package path.

I was more interested in if that would be useful - it seems that the consensus is that easier matching is more important, so I'm happy to just include the module path.

macedogm commented 2 months ago

@maceonthompson thanks for providing the examples.

I was more interested in if that would be useful - it seems that the consensus is that easier matching is more important, so I'm happy to just include the module path.

It's useful, but I fully agree that easier matching is more important, specially because it allows removing false positives at scale. 👍🏻

golang / go