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

fix: distro FP data not applied correctly #1603

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

luhring
Copy link
Contributor

@luhring luhring commented Nov 15, 2023

fixes #1236

This demonstrates an implementation that more correctly applies distro false positive data to package matches across all matcher types, for the specific case when the file(s) that comprise the package are claimed by the distro's package DB.

In talking with @wagoodman, we agreed that in the long term, this kind of second-pass filtering won't be needed: once Grype has fully implemented "negative matches", a more elegant approach becomes possible; and at the same time, that it still makes sense to pursue more accurate results for Grype in short term, via leveraging the distro's security data correctly to avoid including (language ecosystem) matches to packages owned by distro packages that have been analyzed by the distro's security team.

This implementation appears to be tailored for just a few distros... It would normally apply across all distros, but the matching for many distros (excluding Alpine, Wolfi, and Chainguard) was recently changed in #1387 such that distro-owned language ecosystem packages are exempt from matching altogether (IIUC). Per the discussion in #1236, this approach isn't suitable for the "non-comprehensive-feed" distros since Grype would have no means of finding unfixed vulnerabilities in distro packages.

So instead, this PR proposes a solution that enables Grype to continue finding unfixed vulnerabilities in these distros, while still correctly applying the security data from the distro's security team.

This PR still needs tests and what not, but wanted to start the discussion on the approach itself. Thanks for your time!

@luhring
Copy link
Contributor Author

luhring commented Nov 15, 2023

One secondary issue I noticed while working on this is that it looks like the GHSA entries from the Wolfi and Chainguard secdbs aren't making it into the Grype DB. This contributes to the distro data not getting applied correctly, since fixes/nacks in GHSAs are not available to Grype at time of matching.

Is this something we can look at adjusting?

Example using the "apko" package:

// ...
    {
      "pkg": {
        "name": "apko",
        "secfixes": {
          "0": [
            "CVE-2023-45283",
            "CVE-2023-45284",
            "GHSA-jq35-85cj-fj4p" //  <-- this vuln doesn't have a CVE ID assigned!
          ],
          "0.10.0-r6": [
            "CVE-2023-39325",
            "CVE-2023-3978"
          ],
          "0.7.3-r1": [
            "CVE-2023-28840",
            "CVE-2023-28841",
            "CVE-2023-28842"
          ],
          "0.8.0-r1": [
            "CVE-2023-30551"
          ]
        }
      }
    },
// ...
sqlite> SELECT * FROM vulnerability WHERE namespace LIKE '%wolfi%' AND package_name = 'apko';
pk       id              package_name  namespace                   package_qualifiers  version_constraint  version_format  cpes  related_vulnerabilities                          fixed_in_versions  fix_state  advisories
-------  --------------  ------------  --------------------------  ------------------  ------------------  --------------  ----  -----------------------------------------------  -----------------  ---------  ----------
1822413  CVE-2023-28840  apko          wolfi:distro:wolfi:rolling                      < 0.7.3-r1          apk                   [{"id":"CVE-2023-28840","namespace":"nvd:cpe"}]  ["0.7.3-r1"]       fixed
1822417  CVE-2023-28841  apko          wolfi:distro:wolfi:rolling                      < 0.7.3-r1          apk                   [{"id":"CVE-2023-28841","namespace":"nvd:cpe"}]  ["0.7.3-r1"]       fixed
1822421  CVE-2023-28842  apko          wolfi:distro:wolfi:rolling                      < 0.7.3-r1          apk                   [{"id":"CVE-2023-28842","namespace":"nvd:cpe"}]  ["0.7.3-r1"]       fixed
1822441  CVE-2023-30551  apko          wolfi:distro:wolfi:rolling                      < 0.8.0-r1          apk                   [{"id":"CVE-2023-30551","namespace":"nvd:cpe"}]  ["0.8.0-r1"]       fixed
1822558  CVE-2023-39325  apko          wolfi:distro:wolfi:rolling                      < 0.10.0-r6         apk                   [{"id":"CVE-2023-39325","namespace":"nvd:cpe"}]  ["0.10.0-r6"]      fixed
1822724  CVE-2023-3978   apko          wolfi:distro:wolfi:rolling                      < 0.10.0-r6         apk                   [{"id":"CVE-2023-3978","namespace":"nvd:cpe"}]   ["0.10.0-r6"]      fixed
1823032  CVE-2023-45283  apko          wolfi:distro:wolfi:rolling                      < 0                 apk                   [{"id":"CVE-2023-45283","namespace":"nvd:cpe"}]  ["0"]              fixed
1823380  CVE-2023-45284  apko          wolfi:distro:wolfi:rolling                      < 0                 apk                   [{"id":"CVE-2023-45284","namespace":"nvd:cpe"}]  ["0"]              fixed

@westonsteimel
Copy link
Contributor

Yeah, looks like just an adjustment to vunnel to either not do this filtering at all or adjust it to allow GHSA prefix https://github.com/anchore/vunnel/blob/main/src/vunnel/providers/wolfi/parser.py#L100-L104

@westonsteimel
Copy link
Contributor

anchore/vunnel#399 removes the id filtering from the wolfi provider entirely. I can make the same change for the chainguard provider if everyone agrees with the approach

@luhring
Copy link
Contributor Author

luhring commented Nov 15, 2023

Wow, thank you @westonsteimel!

grype/pkg/package.go Outdated Show resolved Hide resolved
@luhring luhring marked this pull request as ready for review December 20, 2023 17:57
@luhring
Copy link
Contributor Author

luhring commented Dec 20, 2023

@wagoodman This should be ready for a look now! I think it incorporates all of the changes we talked about offline.

Thanks again for the discussion on this ❤️

@luhring
Copy link
Contributor Author

luhring commented Dec 21, 2023

I've found one outstanding issue, related to how upstream vuln data gets discovered — which we had already started talking about with #1602.

I'm wondering if we should tackle it in this PR or in 1602 — but leaning toward this PR...

Here's what Grype shows in a particular scan (incorporating the work from this PR 1603):

$ grype -q cgr.dev/chainguard/argo-workflowcontroller:latest
NAME                      INSTALLED             FIXED-IN  TYPE       VULNERABILITY        SEVERITY
github.com/docker/docker  v24.0.0+incompatible  24.0.7    go-module  GHSA-jq35-85cj-fj4p  Medium

We can see that Syft was able to identify the origin package for the APK package that owns the Go module shown above (for brevity, I'm skipping the step of inspecting Grype's JSON for the above scan, but you can trust me!):

$ syft -q cgr.dev/chainguard/argo-workflowcontroller:latest -o json | jq '.artifacts | map(select(.name == "argo-workflow-controller")) | first'
{
  "id": "64b20153a4589427",
  "name": "argo-workflow-controller",
  "version": "3.5.2-r3",
  "type": "apk",
  "foundBy": "apk-db-cataloger",
  // ...
  "metadata": {
    "package": "argo-workflow-controller",
    "originPackage": "argo-workflows",
    // ...
  }
}

And, we can see that the Wolfi secdb shows the vulnerability reported from Grype (GHSA-jq35-85cj-fj4p) is a false positive:

$ curl -sS https://packages.wolfi.dev/os/security.json | jq '.packages | map(select(.pkg.name == "argo-workflows")) | first'
{
  "pkg": {
    "name": "argo-workflows",
    "secfixes": {
      "0": [
        "CVE-2023-45283",
        "GHSA-vvjp-q62m-2vph",
        "CVE-2023-45284",
        "GHSA-rq3x-83w4-p28c",
        "GHSA-jq35-85cj-fj4p"
      ],
  // ...
}

So that data should be consulted when generating the false positive index at the end of matching.

Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
@luhring
Copy link
Contributor Author

luhring commented Dec 21, 2023

Addressed the scenario from my comment above, and added coverage for that scenario in the unit and integration tests ✅

Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and behavior work as described in the PR with the new false positive information being flagged and removed based on the locations used by getDistroFalsePositivesByLocation

The only comment I had for this is the new addition of the grype error case - I agree that we should definitely error in the case that the new functionality fails, but am undecided on if it should be grype [exit] vs grype logs some error and moves on with other information

grype/pkg/syft_sbom_provider_test.go Show resolved Hide resolved
grype/vulnerability_matcher.go Show resolved Hide resolved
grype/vulnerability_matcher.go Show resolved Hide resolved
grype/vulnerability_matcher.go Show resolved Hide resolved
grype/vulnerability_matcher.go Show resolved Hide resolved
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix here 🙌

@wagoodman wagoodman merged commit 474030c into anchore:main Jan 4, 2024
9 checks passed
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.

Vulnerabilities marked as fixed in distro packages should be reported as fixed for all contained packages too
4 participants