From 72705ef93a505c7fa52467b2500cab5367addf5c Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Tue, 30 Jan 2024 10:43:28 -0600 Subject: [PATCH] Switching from FixedBuild to FixedBuilds (part 1). (#16454) Switching from FixedBuild to FixedBuilds (part 1). #16412 Converting msrc files from using FixedBuild to FixedBuilds. In part 2, FixedBuild will be completely removed. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/` or `orbit/changes/`. See [Changes files](https://fleetdm.com/docs/contributing/committing-changes#changes-files) for more information. - [x] Added/updated tests --- ...windows-os-remediation-for-multiple-builds | 1 + server/vulnerabilities/msrc/analyzer.go | 22 ++++++------ .../msrc/parsed/security_bulletin.go | 34 ++++++++++++++----- server/vulnerabilities/msrc/parser.go | 2 ++ server/vulnerabilities/msrc/parser_test.go | 2 +- 5 files changed, 42 insertions(+), 19 deletions(-) create mode 100644 changes/16412-windows-os-remediation-for-multiple-builds diff --git a/changes/16412-windows-os-remediation-for-multiple-builds b/changes/16412-windows-os-remediation-for-multiple-builds new file mode 100644 index 0000000000..02512db202 --- /dev/null +++ b/changes/16412-windows-os-remediation-for-multiple-builds @@ -0,0 +1 @@ +Fixed Windows OS false negative when a remediation applies to multiple OS builds. diff --git a/server/vulnerabilities/msrc/analyzer.go b/server/vulnerabilities/msrc/analyzer.go index ed5130e41d..fbbb4087f1 100644 --- a/server/vulnerabilities/msrc/analyzer.go +++ b/server/vulnerabilities/msrc/analyzer.go @@ -130,17 +130,19 @@ func patched( continue } - // An empty FixBuild is a bug in the MSRC feed, last - // seen around apr-2021. Ignoring it to avoid false - // positive vulnerabilities. - if fix.FixedBuild == "" { - continue - } + for _, build := range fix.FixedBuilds { + // An empty FixBuild is a bug in the MSRC feed, last + // seen around apr-2021. Ignoring it to avoid false + // positive vulnerabilities. + if build == "" { + continue + } - isGreater, err := winBuildVersionGreaterOrEqual(fix.FixedBuild, os.KernelVersion) - // Return true on errors to prevent false positives - if err != nil || isGreater { - return true + isGreater, err := winBuildVersionGreaterOrEqual(build, os.KernelVersion) + // Return true on errors to prevent false positives + if err != nil || isGreater { + return true + } } } diff --git a/server/vulnerabilities/msrc/parsed/security_bulletin.go b/server/vulnerabilities/msrc/parsed/security_bulletin.go index 526adfec35..9a92b20c51 100644 --- a/server/vulnerabilities/msrc/parsed/security_bulletin.go +++ b/server/vulnerabilities/msrc/parsed/security_bulletin.go @@ -3,9 +3,9 @@ package parsed import ( "encoding/json" "errors" - "os" - "github.com/fleetdm/fleet/v4/server/ptr" + "golang.org/x/exp/slices" + "os" ) type SecurityBulletin struct { @@ -78,7 +78,7 @@ func (b *SecurityBulletin) Merge(other *SecurityBulletin) error { // Vendor fixes for kbID, r := range other.VendorFixes { if _, ok := b.VendorFixes[kbID]; !ok { - newVF := NewVendorFix(r.FixedBuild) + newVF := NewVendorFix(r.FixedBuilds...) for pID, v := range r.ProductIDs { newVF.ProductIDs[pID] = v } @@ -86,6 +86,14 @@ func (b *SecurityBulletin) Merge(other *SecurityBulletin) error { newVF.Supersedes = ptr.Uint(*r.Supersedes) } b.VendorFixes[kbID] = newVF + } else { + // TODO: Remove this code once we are done transitioning from FixedBuild to FixedBuilds + vf := b.VendorFixes[kbID] + if vf.FixedBuild != "" { + vf.AddFixedBuild(b.VendorFixes[kbID].FixedBuild) + vf.FixedBuild = "" + b.VendorFixes[kbID] = vf + } } } @@ -202,16 +210,26 @@ func NewVulnerability(publishedDateEpoch *int64) Vulnerability { // ---------------------- type VendorFix struct { - FixedBuild string + // TODO: FixedBuild is being replaced with FixedBuilds, remove this field once we are done transitioning. + FixedBuild string + FixedBuilds []string // Set of products ids that target this vendor fix ProductIDs map[string]bool // A Reference to what vendor fix this particular vendor fix 'replaces'. Supersedes *uint `json:",omitempty"` } -func NewVendorFix(fixedBuild string) VendorFix { - return VendorFix{ - FixedBuild: fixedBuild, - ProductIDs: make(map[string]bool), +func (vf *VendorFix) AddFixedBuild(fixedBuild string) { + if !slices.Contains(vf.FixedBuilds, fixedBuild) { + vf.FixedBuilds = append(vf.FixedBuilds, fixedBuild) + } +} + +func NewVendorFix(fixedBuilds ...string) VendorFix { + fixedBuildsCopy := make([]string, len(fixedBuilds)) + copy(fixedBuildsCopy, fixedBuilds) + return VendorFix{ + FixedBuilds: fixedBuildsCopy, + ProductIDs: make(map[string]bool), } } diff --git a/server/vulnerabilities/msrc/parser.go b/server/vulnerabilities/msrc/parser.go index ca020e8954..734a96e962 100644 --- a/server/vulnerabilities/msrc/parser.go +++ b/server/vulnerabilities/msrc/parser.go @@ -106,6 +106,8 @@ func mapToSecurityBulletins(rXML *msrcxml.FeedResult) (map[string]*parsed.Securi var vFix parsed.VendorFix if vFix, ok = b.VendorFixes[remediatedKBID]; !ok { vFix = parsed.NewVendorFix(rem.FixedBuild) + } else { + vFix.AddFixedBuild(rem.FixedBuild) } vFix.Supersedes = supersedes vFix.ProductIDs[pID] = true diff --git a/server/vulnerabilities/msrc/parser_test.go b/server/vulnerabilities/msrc/parser_test.go index 59b14127da..9bd7377102 100644 --- a/server/vulnerabilities/msrc/parser_test.go +++ b/server/vulnerabilities/msrc/parser_test.go @@ -1154,7 +1154,7 @@ func TestParser(t *testing.T) { for KBID, fix := range vF { sut := bulletin.VendorFixes[KBID] - require.Equal(t, fix.FixedBuild, sut.FixedBuild, pName, KBID) + require.Equal(t, fix.FixedBuild, sut.FixedBuilds[0], pName, KBID) require.Equal(t, fix.ProductIDs, sut.ProductIDs, pName, KBID) // We want to check that either both are nil or that both are not nil require.False(t, (fix.Supersedes == nil || sut.Supersedes == nil) && !(fix.Supersedes == nil || sut.Supersedes == nil), pName, KBID)