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.

Sign up for GitHub

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(redhat): correct rewriting of recommendations for the same vulnerability #8063

Merged
merged 3 commits into from
Dec 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f31611
fix(redhat): sort advisories and remove vulns from uniqVulns if needed
DmitriyLewen Dec 6, 2024
29ab009
test: add unit test
DmitriyLewen Dec 6, 2024
b8b6c16
refactor: de-duplicate advisories first
DmitriyLewen Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 32 additions & 21 deletions pkg/detector/ospkg/redhat/redhat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
 	})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@knqyf263 knqyf263 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

sort.Slice(advisories, func(i, j int) bool {
return version.NewVersion(advisories[i].FixedVersion).LessThan(version.NewVersion(advisories[j].FixedVersion))
})

installed := utils.FormatVersion(pkg)
installedVersion := version.NewVersion(installed)

Expand Down Expand Up @@ -146,33 +151,39 @@ func (s *Scanner) detect(osVer string, pkg ftypes.Package) ([]types.DetectedVuln

// unpatched vulnerabilities
if adv.FixedVersion == "" {
// Red Hat may contain several advisories for the same vulnerability (RHSA advisories).
// To avoid overwriting the fixed version by mistake, we should skip unpatched vulnerabilities if they were added earlier
if _, ok := uniqVulns[vulnID]; !ok {
uniqVulns[vulnID] = vuln
}
// Advisories are sorted and unpatched advisories always go before patched ones.
// So we just save unpatched advisories and will overwrite
uniqVulns[vulnID] = vuln
continue
}

// patched vulnerabilities
fixedVersion := version.NewVersion(adv.FixedVersion)
if installedVersion.LessThan(fixedVersion) {
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
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
}

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
}
}

Expand Down
48 changes: 48 additions & 0 deletions pkg/detector/ospkg/redhat/redhat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,54 @@ func TestScanner_Detect(t *testing.T) {
},
},
},
{
name: "happy path: CVE-ID and RHSA-ID for same vulnerability",
fixtures: []string{
"testdata/fixtures/redhat.yaml",
"testdata/fixtures/cpe.yaml",
},
args: args{
osVer: "8.3",
pkgs: []ftypes.Package{
{
Name: "expat",
Version: "2.2.5",
Release: "16.el8_10",
Epoch: 0,
Arch: "x86_64",
SrcName: "expat",
SrcVersion: "2.2.5",
SrcRelease: "16.el8_10",
SrcEpoch: 0,
Layer: ftypes.Layer{
DiffID: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
},
BuildInfo: &ftypes.BuildInfo{
ContentSets: []string{"rhel-8-for-x86_64-baseos-rpms"},
},
},
},
},
want: []types.DetectedVulnerability{
{
VulnerabilityID: "CVE-2024-45490",
VendorIDs: []string{
"RHSA-2024:6989-2",
"RHSA-2024:6989-3",
},
PkgName: "expat",
InstalledVersion: "2.2.5-16.el8_10",
FixedVersion: "2.2.5-18.el8_10",
SeveritySource: vulnerability.RedHat,
Vulnerability: dbTypes.Vulnerability{
Severity: dbTypes.SeverityMedium.String(),
},
Layer: ftypes.Layer{
DiffID: "sha256:932da51564135c98a49a34a193d6cd363d8fa4184d957fde16c9d8527b3f3b02",
},
},
},
},
{
name: "happy path: package without architecture",
fixtures: []string{
Expand Down
65 changes: 64 additions & 1 deletion pkg/detector/ospkg/redhat/testdata/fixtures/redhat.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,67 @@
- ID: CVE-2016-5195
Severity: 3
Arches:
- aarch64
- aarch64
- bucket: expat
pairs:
- key: RHSA-2024:6989-2 # created for test only
value:
Entries:
- FixedVersion: 0:2.2.5-17.el8_10
Affected:
- 4
Arches:
- x86_64
Cves:
- ID: CVE-2024-45490
Severity: 2
- key: RHSA-2024:6989-3 # created for test only
value:
Entries:
- FixedVersion: 0:2.2.5-18.el8_10
Affected:
- 4
Arches:
- x86_64
Cves:
- ID: CVE-2024-45490
Severity: 2
- key: CVE-2024-45490
value:
Entries:
- Affected:
- 4
Cves:
- Severity: 2
Status: 5
- key: CVE-2024-45491
value:
Entries:
- Affected:
- 4
Cves:
- Severity: 2
Status: 5
- key: RHSA-2024:6989
value:
Entries:
- FixedVersion: 0:2.2.5-15.el8_10
Affected:
- 4
Arches:
- x86_64
Cves:
- ID: CVE-2024-45490
Severity: 2
- ID: CVE-2024-45491
Severity: 2
- ID: CVE-2024-45492
Severity: 2
- key: CVE-2024-45492
value:
Entries:
- Affected:
- 4
Cves:
- Severity: 2
Status: 5
Loading