oracle: omit ksplice-related vulnerabilities

[BradLugo]

Oracle's OVAL data includes ksplice userspace packages that aren't supported to run in containers.

This PR changes the parsing of this data to omit ksplice-related vulnerabilities by checking the affected CPEs list. If all CPEs are all related to ksplice, we'll omit it. If there is at least one CPE that is not related to ksplice, we'll add all the vulnerabilities anyway, which may cause us to record false positives for that vulnerability, but that case seems very unlikely.

Related to #547.

[hdonnay]

This looks right, although it should probably just omit advisories that are for ksplice instead of erroring.

[BradLugo]

It appears one way to omit it is by erroring in the protoVulns callback:

claircore/pkg/ovalutil/rpm.go

Lines 49 to 56 in 178b694

	protoVulns, err := protoVulns(def)
	if err != nil {
	zlog.Debug(ctx).
	Err(err).
	Str("def_id", def.ID).
	Msg("could not create prototype vulnerabilities")
	continue
	}

I don't know which is more idiomatic to claircore. The other way is to log and return an empty list + nil error, I think.

[crozzy]

Nit: Which condition should (almost) never happen?

[BradLugo]

This is referring to the comment above: https://github.com/quay/claircore/pull/1511/files#diff-306dfd83be9b72dac194cdc4cc9022d33bc8f67b73567be2a23fa53df4b4412fR84-R86
Do you have any suggestions on how to make this clearer? I could copy+paste the whole comment here.

[crozzy]

Do we want to break here? Doesn't look like anything will change in subsequent calls after these are set

[crozzy]

LGTM, just a few nits

[RTann]

I'm wondering if it may be cleaner to just count the number of userspace_ksplice CPEs and then compare this with the total number of CPEs after the loop

[BradLugo]

I'm open to suggestions.

[RTann]

how do we know for a fact this is the case?

[RTann]

The comment above says we are assuming this is not a ksplice package, but this is set to true. Is that intentional?

[RTann]

I think the logs typically do underscore instead of dot

Suggested change

	zlog.Warn(ctx).Str("def.Title", def.Title).Msg("potential false positives: vulnerability has at least one unskippable ksplice match")
	zlog.Warn(ctx).Str("def_title", def.Title).Msg("potential false positives: vulnerability has at least one unskippable ksplice match")

[RTann]

Suggested change

	return nil, fmt.Errorf("skippable ksplice vulnerabilities")
	return nil, errors.New("skippable ksplice vulnerabilities")

actually, how come we want to error here? Would it not be valid to return nil, nil, or do we prefer the signal an error message?

[BradLugo]

I thought it might be dangerous to return a nil pointer to the caller if there's no error, but I just tested that with a list of pointers in Go Playground, and it doesn't seem to be a problem. I'll add a debug log instead.

[BradLugo]

@RTann I updated the comments in the changes. Let me know if they answer some of your review questions.

[RTann]

how does something like this look? I was getting confused about some of the isOnlyKsplice and atLeastOneKsplice uses

Also, I realized we never really accounted for CPEs prior to this, but we do for RHEL. Should we be making an entry per CPE for this, too?

Suggested change

	// Check if the vulnerability only affects a userspace_ksplice package.
	// These errata should never be applied to a container since ksplice
	// userspace packages are not supported to be run within a container.
	// If we couldn't find a CPE list, make sure to include the
	// vulnerability. We'd rather have false positives for
	// userspace_ksplice packages than have false negatives for
	// *everything*.
	isOnlyKsplice := len(def.Advisory.AffectedCPEList) > 0
	// If there's at least one ksplice CPE and not all the affected CPEs
	// are ksplice related, this will cause false positives we can catch.
	// This should rarely happen. The most common case for this is if one
	// of the CPEs wasn't parseable.
	atLeastOneKsplice := false
	for _, affected := range def.Advisory.AffectedCPEList {
	wfn, err := cpe.Unbind(affected)
	if err != nil {
	// Found a CPE but could not parse it. Let's break out of these
	// checks and signal that these vulnerabilities should be
	// added to the list, but that there may be false positives.
	zlog.Warn(ctx).Str("cpe", affected).Msg("could not parse CPE")
	isOnlyKsplice = false
	atLeastOneKsplice = true
	break
	}
	if wfn.Attr[cpe.Edition].V == "userspace_ksplice" {
	atLeastOneKsplice = true
	} else {
	isOnlyKsplice = false
	}
	}
	if isOnlyKsplice {
	zlog.Debug(ctx).Msg("skipping ksplice vulnerabilities")
	return nil, nil
	}
	if atLeastOneKsplice {
	zlog.Warn(ctx).
	Str("def_title", def.Title).
	Msg("potential false positives: vulnerability has at least one unskippable ksplice match")
	}
	return vs, nil
	kspliceCPEs := 0
	cpes := len(def.Advisory.AffectedCPEList)
	for _, affected := range def.Advisory.AffectedCPEList {
	wfn, err := cpe.Unbind(affected)
	if err != nil {
	// Found a CPE but could not parse it. Just log a warning and return successfully.
	zlog.Warn(ctx).
	Str("def_title", def.Title).
	Str("cpe", affected).
	Msg("could not parse CPE: there may be a false positive match with a userspace_ksplice package")
	return vs, nil
	}
	if wfn.Attr[cpe.Edition].V == "userspace_ksplice" {
	kspliceCPEs++
	}
	}
	switch diff := len(def.Advisory.AffectedCPEList) - kspliceCPEs; {
	case cpes == 0:
	// Just continue if there are no CPEs.
	case diff == 0:
	zlog.Debug(ctx).Msg("skipping userspace_ksplice vulnerabilities")
	return nil, nil
	case diff > 0:
	zlog.Warn(ctx).
	Str("def_title", def.Title).
	Msg("potential false positives: OVAL may have a userspace_ksplice CPE which could not be skipped")
	default:
	panic("programmer error")
	}
	return vs, nil

[BradLugo]

Also, I realized we never really accounted for CPEs prior to this, but we do for RHEL. Should we be making an entry per CPE for this, too?

I'm not sure. There's probably an improvement we can make to make an entry per CPE, but I think the rest of the OVAL data (criteria + rpm object lookups) exposes all the entries we need. I might be missing the point of your question, though (I'm still getting comfortable with CPEs and such).