fix(redhat): correct rewriting of recommendations for the same vulnerability

[DmitriyLewen]

Description

Rewriting advisories for the same vulnerability is currently tied to the advisory order.
See #8061 and new tests for more details

This PR sets the order for advisories and fixes overwriting of advisories for the same vulnerability.

Example:
before:

➜  trivy -q image registry.access.redhat.com/ubi8/ubi --cache-backend memory | grep expat
│ expat                       │ CVE-2022-23990   │ MEDIUM   │                     │ 2.2.5-16.el8_10       │                 │ expat: integer overflow in the doProlog function             │
│                             │ CVE-2024-45491   │          │                     │                       │                 │ libexpat: Integer Overflow or Wraparound                     │
│                             │ CVE-2024-45492   │          │                     │                       │                 │ libexpat: integer overflow                                   │
│                             │ CVE-2024-50602   │          │ affected            │                       │                 │ libexpat: expat: DoS via XML_ResumeParser                    │

after:

➜  ./trivy -q image registry.access.redhat.com/ubi8/ubi --cache-backend memory | grep expat
│ expat                       │ CVE-2022-23990   │ MEDIUM   │                     │ 2.2.5-16.el8_10       │                 │ expat: integer overflow in the doProlog function             │

Related issues

• Close bug(redhat): Trivy doesn't skip vulnerability from CVE-ID if package version is not affected for RHSA-ID of this vulnerability #8061

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).
• I've added usage information (if the PR introduces new options)
• I've included a "before" and "after" example to the description (if the PR is a user interface change).

[knqyf263]

What do you think about de-duplicating advisories first? This change will prevent the display of multiple RHSA-IDs that fix the same CVE-ID, but I think that it would be better to display only the most recent RHSA-ID since multiple RHSA-IDs confuses users in the first place.

diff --git a/pkg/detector/ospkg/redhat/redhat.go b/pkg/detector/ospkg/redhat/redhat.go
index 232c14ba9..a8d061395 100644
--- a/pkg/detector/ospkg/redhat/redhat.go
+++ b/pkg/detector/ospkg/redhat/redhat.go
@@ -9,11 +9,9 @@ import (
 	"time"
 
 	version "github.com/knqyf263/go-rpm-version"
-	"github.com/samber/lo"
 	"golang.org/x/xerrors"
 
 	dbTypes "github.com/aquasecurity/trivy-db/pkg/types"
-	ustrings "github.com/aquasecurity/trivy-db/pkg/utils/strings"
 	redhat "github.com/aquasecurity/trivy-db/pkg/vulnsrc/redhat-oval"
 	"github.com/aquasecurity/trivy-db/pkg/vulnsrc/vulnerability"
 	osver "github.com/aquasecurity/trivy/pkg/detector/ospkg/version"
@@ -116,16 +114,24 @@ func (s *Scanner) detect(osVer string, pkg ftypes.Package) ([]types.DetectedVuln
 		return nil, xerrors.Errorf("failed to get Red Hat advisories: %w", err)
 	}
 
-	// Sort advosiries by FixedVersion to avoid incorrectly overwriting vulnerabilities
-	sort.Slice(advisories, func(i, j int) bool {
-		return version.NewVersion(advisories[i].FixedVersion).LessThan(version.NewVersion(advisories[j].FixedVersion))
-	})
+	// Chose the latest fixed version for each CVE-ID
+	// Take the single RHSA-ID with the latest fixed version
+	uniqAdvisories := make(map[string]dbTypes.Advisory)
+	for _, adv := range advisories {
+		if a, ok := uniqAdvisories[adv.VulnerabilityID]; ok {
+			if version.NewVersion(a.FixedVersion).LessThan(version.NewVersion(adv.FixedVersion)) {
+				uniqAdvisories[adv.VulnerabilityID] = adv
+			}
+		} else {
+			uniqAdvisories[adv.VulnerabilityID] = adv
+		}
+	}
 
 	installed := utils.FormatVersion(pkg)
 	installedVersion := version.NewVersion(installed)
 
-	uniqVulns := make(map[string]types.DetectedVulnerability)
-	for _, adv := range advisories {
+	var vulns []types.DetectedVulnerability
+	for _, adv := range uniqAdvisories {
 		// if Arches for advisory is empty or pkg.Arch is "noarch", then any Arches are affected
 		if len(adv.Arches) != 0 && pkg.Arch != "noarch" {
 			if !slices.Contains(adv.Arches, pkg.Arch) {
@@ -136,6 +142,8 @@ func (s *Scanner) detect(osVer string, pkg ftypes.Package) ([]types.DetectedVuln
 		vulnID := adv.VulnerabilityID
 		vuln := types.DetectedVulnerability{
 			VulnerabilityID:  vulnID,
+			VendorIDs:        adv.VendorIDs,    // Will be empty for unpatched vulnerabilities
+			FixedVersion:     adv.FixedVersion, // Will be empty for unpatched vulnerabilities
 			PkgID:            pkg.ID,
 			PkgName:          pkg.Name,
 			InstalledVersion: utils.FormatVersion(pkg),
@@ -151,43 +159,18 @@ func (s *Scanner) detect(osVer string, pkg ftypes.Package) ([]types.DetectedVuln
 
 		// unpatched vulnerabilities
 		if adv.FixedVersion == "" {
-			// Advisories are sorted and unpatched advisories always go before patched ones.
-			// So we just save unpatched advisories and will overwrite
-			uniqVulns[vulnID] = vuln
+			vulns = append(vulns, vuln)
 			continue
 		}
 
 		// patched vulnerabilities
 		fixedVersion := version.NewVersion(adv.FixedVersion)
-		if !installedVersion.LessThan(fixedVersion) {
-			// The advisories are sorted by fixedVersion, so the following cases are possible:
-			// 1. uniqVulns doesn't contain this vulnID -> just move on to checking the next advisory.
-			// 2. uniqVulns contains this vuln without fixedVersion.
-			// 3. uniqVulns contains this vuln with fixedVersion.
-			// For cases 2 and 3, we should remove this vulnerability from uniqVulns, since in both cases this package is not vulnerable.
-			delete(uniqVulns, vulnID)
-			continue
+		if installedVersion.LessThan(fixedVersion) {
+			vulns = append(vulns, vuln)
 		}
 
-		vuln.VendorIDs = adv.VendorIDs
-		vuln.FixedVersion = fixedVersion.String()
-
-		if v, ok := uniqVulns[vulnID]; ok {
-			// In case two advisories resolve the same CVE-ID.
-			// e.g. The first fix might be incomplete.
-			v.VendorIDs = ustrings.Unique(append(v.VendorIDs, vuln.VendorIDs...))
-
-			// The newer fixed version should be taken.
-			if version.NewVersion(v.FixedVersion).LessThan(fixedVersion) {
-				v.FixedVersion = vuln.FixedVersion
-			}
-			uniqVulns[vulnID] = v
-		} else {
-			uniqVulns[vulnID] = vuln
-		}
 	}
 
-	vulns := lo.Values(uniqVulns)
 	sort.Slice(vulns, func(i, j int) bool {
 		return vulns[i].VulnerabilityID < vulns[j].VulnerabilityID
 	})

[DmitriyLewen]

but I think that it would be better to display only the most recent RHSA-ID since multiple RHSA-IDs confuses users in the first place.

I tried to maintain this logic on the contrary 😄

I refactor and updated (arches are checked before saving unique advisories) your logic - b8b6c16
Take a look

[knqyf263]

What do you think? Should we show all relevant RHSA-IDs?

[DmitriyLewen]

I tried to keep this logic, because I thought users were asking about it.

But my opinion is that it is better to show only the current (with the latest fixed version) RHSA-ID. Multiple RHSA-IDs add noise and confuse users.

[knqyf263]

I agree. If Trivy shows the non-latest RHSA-ID, users may consume the advisory and get confused as the vulnerability will not be fixed.

[DmitriyLewen]

@aqua-bot backport release/v0.58

[aqua-bot]

Backport PR created: #8135