feat: suppress GHSA matches on language packages in fixed APKs

[willmurphyscode]

Note: this is an APK-specific fix for the following situation. The RPM and dpkg matchers will require follow-up changes.

Currently, in the following situation:

• a distro package exists that pulls in a language package (yum install python3-requests; some APK that brings a binary that's in Go)
• The distro data feed says that a CVE is fixed
• There is a GHSA that says that the language package is vulnerable at the version pulled in by the distro package

Is handled in basically two ways:

1. For "comprehensive" distros (distros where we believe that even unfixed vulns as disclosed) the language package is dropped at SBOM creation time.
2. For other distros, both packages exist, the fixed CVE from the distro feed doesn't make a match, but the GHSA does make a match.

This is incorrect in 2 ways: in the first situation, we could have a false negative if there's a GHSA that the distro feed is unaware of. In the second situation, we could have false positive.

Therefore, do the following:

1. If a distro feed says a CVE exists but is fixed for a given distro package
2. And that distro package has metadata that let us determine which paths it owns
3. Emit ignore rules that suppress that CVE and its aliases from being emitted by other matchers at those paths.

[kzantow]

I think we can reduce this to a single query by using search.ByFixedVersion, no? I think what we want to remove from other matches are these "distro packages' vulnerabilities that are in the feed and considered to be fixed"

[willmurphyscode]

I think the two searches are not equivalent. OnlyVulnerableVersions on an Alpine package eventually calls func (c *simpleRangeExpression) satisfiedWithConfig(, which considers OR and AND operators. Whereas search.ByFixedVersion returns true if the package version is <= any fixed version. In other words, one asks the question, "Is the version constraint satisfied?" and the other asks the question, "is there any fixed version <= to the version provided", and these answers can be different. The specific difference is if there's more than one fix in Alpine secDB, then a version that's above one fix but below the other will be considered vulnerable in the OnlyVulnerableVersions search (because the version is below the higher fix) but fixed in the search.ByFixedVersion search, because the version is above the low version.

Does that make sense?

[kzantow]

Ok, it took me a while to remember why I used the fix version in such a way rather than using the constraints. My memory is failing me and the best I came up with is let's say we have a vulnerable range of < 0.9 || > 1.0,<1.1. And let's say we have a fix version of 1.1 but not 0.9. Should this be considered fixed if I have version 0.9? Is this a case that would never happen because we should have constraints and fix versions kept in sync?

I asked because it seems to me this should be doing what you want, and if it doesn't then there's probably a bug we should fix in the logic of the ByFixedVersion constraint. I think it's probably the latter... if we have a vulnerability that's marked "fixed" and we have constraints that don't match, we should be considered "not vulnerable" even if there's nothing explicitly saying that this version is fixed. I'm probably making up some case that doesn't exist. This is all to say, we should probably fix the constraint. It seems a change to use constraints passes our current validations.

If the change in the PR ☝️ was made, would this do the trick?

I apologize for the long-windedness about to follow 😬 but please bear with it. I think there's an opportunity to simplify and centralize this behavior a little more. We're adding a select for all vulnerabilities matching the package without filtering by versions, and another for the vulns filtered by version -- all in all that's 3 queries with one of them fetching a superset of the others. There is a utility (the result.Provider )that Alex and I have talked about being a pattern to replace the current vulnerability provider to: it's basically the same thing except it returns a Set with utilities to make the rest of what you need to do easier; it simplified the EUS stuff a lot).

And, since we want this to happen for all distros, I'm pretty sure: there's already a spot in the MatchPackageByDistro that we could just modify to return both vulnerabilities and ignores. Combining these things the function would look something like:

func MatchPackageByDistro(provider vulnerability.Provider, searchPkg pkg.Package, catalogPkg *pkg.Package, upstreamMatcher match.MatcherType, cfg *version.ComparisonConfig) ([]match.Match, []match.IgnoreFilter, error) {

	... // first part is the same through the versionCriteria:

	versionCriteria := OnlyVulnerableVersions(pkgVersion)

	provider := result.NewProvider(provider, searchPkg, upstreamMatcher)

	// we have to select the superset, anyway, we can use it to get the subsets later:
	allVulns, err := provider.FindVulnerabilities(
		search.ByPackageName(searchPkg.Name),
		search.ByDistro(*searchPkg.Distro),
		OnlyQualifiedPackages(searchPkg),
	)
	if err != nil {
		return nil, nil, fmt.Errorf("matcher failed to fetch distro=%q pkg=%q: %w", searchPkg.Distro, searchPkg.Name, err)
	}

	// get the original set of vulnerable entries:
	vulns := allVulns.Filter(versionCriteria)

	// I think this whole loop could be replaced with `matches = vulns.ToMatches()`
	for _, vuln := range vulns {
		matches = append(matches, match.Match{
			Vulnerability: vuln,
			Package:       matchPackage(searchPkg, catalogPkg),
			Details:       distroMatchDetails(upstreamMatcher, searchPkg, catalogPkg, vuln),
		})
	}

	// get the set without vulnerable entries, this is what it looks like the PR is currently doing: removing vulns
	fixed := allVulns.Remove(vulns)

	var ignores []match.IgnoreFilter
	for _id := range fixed.ToVulns() { // this function is needed
		for _, path := range ownedPaths(searchPkg) {
			ignores = append(ignores, match.IgnoreRule{
				Vulnerability:  id,
				IncludeAliases: true,
				Reason:         "DistroPackageFixed",
				Package: match.IgnoreRulePackage{
					Location: path,
				},
			})
		}
	}

	return matches, ignores, err
}

There's a caveat that it only has a ToMatches() function and we would need to add the ToVulns() function, which would be a simpler version of ToMatches.

But the most important bit is If we did this, we would reduce the 3 queries to just 1 and keep it centralized for all calls to distro matchers, since one of the queries fetches the full list of vulns anyway.

Or, since I think you would prefer to just add this to APK first, instead of modifying the MatchPackageByDistro duplicate it with this new functionality, MatchPackageByDistroWithFixes or something similar, and call that function instead from the APK matcher everywhere MatchPackageByDistro is being used. I think the reduction in queries seems imporant.

Oh and one other detail I forgot about, it looks like Grype doesn't have the FileOwner interface that Syft has. We would need to add that and implement it for the APK metadata, too. But we would need that to apply this across other package types, anyway. In fact this function could just check if there are ownedFiles, and if so, run the full query or if not add versionCritier to the first query to avoid overfetching... sorry again for the length of this comment 😬

[aborrero]

I have tested this patch against 24 different images with different CVE/GHSA affecting them, and this works as expected 🎉 thanks!

[philroche]

@willmurphyscode Is there reasoning for only suppressing GHSA? And not NVD, enriched NVD?

[willmurphyscode]

Hi @philroche the title is maybe narrower than it could be. The APK matcher already suppresses NVD findings based on fixes from Alpine secDB (on Alpine; other secDBs on Wolfi for example). The real change here is that the suppression will now affect things more broadly: If an APK owns file paths, and it there's a secDB entry claiming it fixes a given CVE, then vulnerabilities that alias to that CVE (which are often GHSAs) that affect the same file paths will be suppressed. Does that answer your question?

It sounds like maybe there are particular false positives you're concerned about? Can you share any examples?

[kzantow]

LGTM 👍

[kzantow]

I think this was the last outstanding question -- do we keep the indirect package ignores? I could see either side of the argument. I think we have cases where the version is wrong between the found package and the upstream -- where the upstream is different and we have incorrect matches both FP and FN due to this version mismatch. I guess what we're doing here is aligning the owned packages with the related ones, so it seems reasonable to surface these ignores, probably. I just wanted to note this here, because I don't think we came to a conclusion on Zoom.

[willmurphyscode]

I think if we consider upstream fixed for a given vuln, we consider it fixed. Saying we consider it fixed, but then hedging by letting GHSA findings stand about the files the relevant package owns seems like an inconsistent position and I don't see any evidence that it's more correct than this.

[kzantow]

I agree; if there is some evidence it is causing FNs, we can always address it later

[philroche]

It sounds like maybe there are particular false positives you're concerned about? Can you share any examples?

@willmurphyscode Nothing specific. Just trying to understand the full impact on the Chainguard scans