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

Adding ignore file support #7

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxbeutel
Copy link

Hello!

Wanted to check if it would there would be a possibility to add support for a file where users can specify vulnerability IDs that are supposed to be ignored.

Use case:

  • For example recently we got GO-2023-1621 highlighted "The ScalarMult and ScalarBaseMult methods of the P256 Curve may return an incorrect result if called with some specific unreduced scalars"
  • However, since this only affects very specific use cases, we were quite certain that we won't be affected by this.
  • So instead of upgrading right away, we could've added the vulnerability GO-2023-1621 to some file in the repo like .govulnignore, and then govulncheck would ignore the vulnerability, but still scan for other vulnerabilities.

Aquasec for container scanning supports a .trivyignore file that offers a similar feature (ignoring vulnerabilities), see: https://aquasecurity.github.io/trivy/v0.35/docs/vulnerability/examples/filter/

Implementation

The MR is designed to be a non-breaking change:

  • Add a new command line option -ignore-file which defaults to empty string
  • If the file exists, read it in line by line: Each line is supposed to contain a vulnerability id like GO-2023-1621 which is supposed to be ignored.
  • A lookup set is created from this file
  • The vulnerabilities found by govulncheck are then filtered against this lookup set before getting reported to the user.

Example command line invocation:

$ ../govulncheck/govulncheck -ignore-file=.govulnignore ./...         <<<<< This is the NEW option
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using go1.20.1 and govulncheck@v0.0.0-5c89caadefca-20230327072231 with
vulnerability data from https://vuln.go.dev (last modified 2023-03-21 23:29:51 +0000 UTC).

Scanning your code and 107 packages across 1 dependent module for known vulnerabilities...

Ignoring vulnerability GO-2023-1621       <<<<< Shows the vuln as ignored
No vulnerabilities found.

$ cat .govulnignore
GO-2023-1621         <<<<< This is how the file looks like, one vuln ID per line

Open tasks

  • Do we want to make the file format more complicated (add comments etc.?)
  • Using a pointer here so that 'ignored' field gets skipped in the output. Is that good?
  • Limitation: Need to resize scanner's capacity for lines over 64K in the ignore file
  • Improve output formatting of ignored vulns: Probably the user should still see more details than just the ID of the vuln?
  • Add more tests

.... And of course in general, if such a feature would be even accepted (which I don't take for granted but I thought it's worth a try.)

Thank you in advance for considering this proposal.

@google-cla
Copy link

google-cla bot commented Mar 27, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Chengxuan
Copy link

Hi @maxbeutel @julieqiu @jba @zpavlinovic , I was thinking of doing the same thing and found this PR. is this something you are considering?

I'm using a workaround script at the moment: https://github.com/kaleido-io/kaleido-sdk-go/blob/7ba2d147a282bd98b707879484667d9a14398cca/govulnchecktool.sh which is prone to changes of json structure of the output data.

@maxbeutel
Copy link
Author

Nice idea @Chengxuan . I was wondering if this PR doesn't get through how to implement ignoring reported vulns. Filtering the output on "client side" is indeed an option, as you demonstrated.

I'm wondering is this the wrong repo to raise the MR? Because I also saw MRs for this project in the main golang repo, under https://github.com/golang/go/labels/vulncheck%20or%20vulndb

@bsiegert
Copy link

@maxbeutel The main Go bug tracker is at https://github.com/golang/go, as you have noticed.

The Go project does not usually use Pull Requests (it uses Gerrit code review), but there is some support for dealing with PRs and converting them to code reviews.

There are a few options for you:

  1. You can continue here, which means signing the CLA and marking the PR as "ready to review".
  2. You can follow https://go.dev/doc/contribute and use Gerrit tooling for creating a code review request.

@AkihiroSuda
Copy link

What's the current status of this?

@zpavlinovic
Copy link
Contributor

Note that this is a mirror repo and we don't accept pull requests. This is also something that would need a proposal.

We are not looking to add ignore files at the moment. Note that govulncheck provides json (as well as sarif and openvex) output, so users can write their own simple wrappers that can do the filtering.

@Chengxuan
Copy link

@AkihiroSuda I share @zpavlinovic 's opinion that the feature requested here feels like a level above, it is worth looking at https://github.com/aquasecurity/trivy if you are not aware of it. It covers the scan tool wrap-around logic that is not golang specific.

@AkihiroSuda
Copy link

Note that govulncheck provides json (as well as sarif and openvex) output, so users can write their own simple wrappers that can do the filtering.

The lack of the standardization will incur reinventing a bunch of similar but incompatible wrappers.

it is worth looking at https://github.com/aquasecurity/trivy if you are not aware of it

I'm aware of it, and I don't think that having an extra dependency on Trivy should be (practically) necessary for scanning Go programs.

Also, until the upstream govulncheck officially supports ignorelist, program maintainers will continue to receive false-positive reports from people who just run govulncheck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants