fix: add nvd fix version when versionEndExcluding provided

[jneate]

This could be a quick win to partially resolve https://github.com/anchore/grype/issues/1329

The thinking is, if the versionEndExcluding value is provided via NVD, then that version stated has to be the fixed version.

Not 100% sure how to use versionEndIncluding as there is no way to know what the next version of the package is so for now it seemed safer to use Excluding field.

Whilst it doesn't fix the exact okhttp issue in the original Grype issue, it helps for some of them. e.g. netty

[westonsteimel]

Hey @jneate thanks for the great suggestion!

We have actually tried putting this in previously and almost immediately got negative feedback from the community because of many non-existent "fixes" that it ends up surfacing. It also has the additional negative effect of surfacing false positives in grype output (due to overbroad CPE matching) when filtering to see only vulns with fixes. Currently that view eliminates all CPE based matches since the NVD records never have fixes. For these reasons we ended up having to revert that change.

I do agree with your logic that it would seem that versionEndExcluding should be able to be used to infer a fix, but it seems in actuality there are many high visibility matches where it just doesn't seem to be the case.

We are considering with a future grype-db schema including some sort of a way to mark a fix as inferred so that it could be configurable in grype whether or not to use those records as an indicator of a fix.

Also, we would really like to just disable all of the CPE matching behaviour by default in grype so that nvd-based matches never get surfaced directly but only as relationships to more precise matches. We had started by disabling for NPM but haven't gotten to others just yet.

@wagoodman - do you have anything to add here or any new thoughts on how we might be able to still include this info in the current grypedb schema without negativity affecting results in the same way as previously?

[dwertent]

Hey @westonsteimel ,

Firstly, I want to thank you all for your efforts in maintaining grype and for discussing this issue openly. Our team is also experiencing the problem mentioned in anchore/grype#1329. The discrepancies in vulnerability detection, particularly in cases of CVEs, have caused some challenges for us.

I understand from the discussion that the suggested fix using versionEndExcluding from NVD has had issues in the past with false positives and non-existent fixes. I also noted the proposed future improvements like marking fixes as inferred in the grype-db schema and disabling CPE matching by default, which all seem like sensible strategies for improving the tool's accuracy.

Given that this issue directly affects our operations, I wanted to ask: are there definite plans to resolve this discrepancy in vulnerability detection, especially for Java images? And, if so, is there a rough timeline for when we can expect this issue to be addressed?

[jneate]

I think the challenge here is that when Grype finds a real match, having the fix version would be helpful (if originating from NVD) but like you say above, the fix here would cause more problems due to the false-positive cpe matching - if/when the cpe false positives become less frequent then would there be an issue having this ability added back in with the PR?

Can this field not be relied on as much as the fixedIn field from GHSA?

[willmurphyscode]

Hi @jneate thanks for the PR!

My concern here is that NVD doesn't consistently use versionEndExcluding to mean, "fixed in the next version. For example, at https://services.nvd.nist.gov/rest/json/cves/2.0?cveId=CVE-2023-21930, we can see:

                "cpeMatch": [
                  {
                    "vulnerable": true,
                    "criteria": "cpe:2.3:a:oracle:openjdk:*:*:*:*:*:*:*:*",
                    "versionEndExcluding": "8",
                    "matchCriteriaId": "111E81BB-7D96-44EB-ACFA-415C3F3EA62A"
                  },
... snip ...
                  {
                    "vulnerable": true,
                    "criteria": "cpe:2.3:a:oracle:openjdk:8:-:*:*:*:*:*:*",
                    "matchCriteriaId": "70892D06-6E75-4425-BBF0-4B684EC62A1C"
                  },
                  {
                    "vulnerable": true,
                    "criteria": "cpe:2.3:a:oracle:openjdk:8:milestone1:*:*:*:*:*:*",
                    "matchCriteriaId": "7A165D71-71CC-4E6A-AA4F-FF8DB5B9A5AB"
                  },
                  {
                    "vulnerable": true,
                    "criteria": "cpe:2.3:a:oracle:openjdk:8:milestone2:*:*:*:*:*:*",
                    "matchCriteriaId": "7417B2BB-9AC2-4AF4-A828-C89A0735AD92"
                  },

This has a CPE match with versionEndExcluding of 8, but then later on version 8 itself, and version 8 milestone 1 (and a ton more version 8 variants) are marked as vulnerable. So I think this PR as written would cause false negatives in this case.

That said, Grype no longer uses CPE matches for Java JARs by default, instead using GHSA. GHSA does explicitly report a fixed version, obtained by asking the GraphQL API for details about https://github.com/advisories/GHSA-83q5-whqp-r8jr:

          {
            "package": {
              "name": "org.apache.pulsar:pulsar-websocket"
            },
            "severity": "HIGH",
            "firstPatchedVersion": {
              "identifier": "2.10.5"
            }
          }

As you can see, GHSA explicitly reports a version as patched, while NVD is just reporting version ranges, and will sometimes use surprising representations, like "versions before 8, and 8 itself, and 8-milestone 1" which the approach in this PR would misinterpret.

I think it probably makes sense to close this PR, since I think the approach here will make incorrect inferences from NVD data, but if anyone has any better ideas to address this class of issue, I'd be happy to hear about it.

[wagoodman]

We appreciate the PR on this, but I'm going to close this as there is evidence that this may make some cases better but other cases worse (I think weston's point here https://github.com/anchore/grype-db/pull/112#issuecomment-1573143580 is correct).

I'm going to close this for now since this isn't actionable as-is.

[westonsteimel]

Hi @jneate - thanks so much for this contribution and apologies that we were unable to merge this previously. We'd like to reconsider that now that we have more points of control over the data quality. We now have the https://github.com/anchore/cve-data-enrichment repo which provides a way for us to more quickly and easily enrich the raw NVD data when it is lacking, and we have also disabled CPE matching by default in grype for ecosystems which are already covered by GitHub Security Advisories, so combined we feel that this eliminates many of the cases where we received negative feedback when we tried to make this change previously. I'm going to re-open this and rebase + push a few minor adjustments and tests. Thanks again!

[jneate]

Oh nice that's great news - I'm travelling for work for the next few days so it'll be a few days before I'm in any state to commit coherently but feel free to enhance/merge without me!

[willmurphyscode]

Before merging, we want to do two things: (edit - I'm doing these things)

1. Run some experiments or code review to be sure that this won't cause wrong fixed-in information for Alpine/Chainguard/Wolfi, since those distros get some information from their own feed and some from NVD. If the change would cause bad information, fix that issue in Grype and release the fix before merging this. Edit: new grype unit tests and bug fix merged
2. Communicate on the community discourse that we're about to make a change that affects the fixed-in info (detailing the changes) and let it sit for a few days before actually merging. EDIT: discourse post is up.

Note that we need to do the time between communication and merging, not between communication and releasing, since the OSS grype db nightly job runs off main in this repo.

We plan to merge this PR later in the week.