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.

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

Add versions to product names in MSRC bulletins to aid Windows vulnerability matching #24172

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

sgress454
Copy link
Contributor

@sgress454 sgress454 commented Nov 26, 2024

for #24041

This PR addresses an issue that can cause Windows vulnerability checks to fail (possibly causing false negatives). We determine whether a vulnerability in an MSRC bulletin applies to any hosts in a Fleet instance by attempting to matching the data in each row of the operating_systems table with at least one "product" in a bulletin, including matching architecture and "display version". However a subset of products listed in these bulletins do not include the display version, so for example a host whose OS was listed as Microsoft Windows Server 2022 Datacenter 21H2 (21H2 being the "display version") would match nothing in the bulletins because no listed Server 2022 products include "21H2" in their names.

The fix made here is to add relevant version info to the products list when we do our ETL of the MSRC bulletins. The version info was gleaned from https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions.

We see logs related to this issue a lot, so cleaning this up will alleviate some noise and infra costs as well.

@sgress454 sgress454 requested a review from a team as a code owner November 26, 2024 17:55
@sgress454 sgress454 changed the title add versions to product names in bulletins Add versions to product names in MSRC bulletins to aid Windows vulnerability matching Nov 26, 2024
Comment on lines +70 to +73
p := Product(fullName)
if p.HasDisplayVersion() {
return p
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures this function is still idempotent and won't keep adding version strings.

Comment on lines +49 to +51
for pID, name := range bulletin.Products {
bulletin.Products[pID] = NewProductFromFullName(string(name))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Utilize the new logic in NewProductFromFullName to add version strings when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure this is needed here as NewProductFromFullName is already modifying the published feed here:

bulletins[name].Products[pID] = parsed.NewProductFromFullName(p.FullName)

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 think I needed it here for when we process existing .json files that were saved without the versions added to the product names.

versionString = "1809"

case strings.Contains(fullName, "Windows 8.1"):
versionString = "6.3 / NT 6.3"
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've seen it both ways (6.3 and NT 6.3) and it's hard to say what the devices report, so putting both to be safe.

@lukeheath
Copy link
Member

@mostlikelee Please give this a review when you have a moment. Thanks!

Copy link
Contributor

@mostlikelee mostlikelee left a comment

Choose a reason for hiding this comment

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

it seems like the general assumption we're making is that by adding a "display version" into our msrc feed, more operating system "products" will start matching against Fleet operating systems. We may need more data to see if osquery does indeed report display versions on these systems.

case strings.Contains(fullName, "Windows Server 2019"):
versionString = "1809"

case strings.Contains(fullName, "Windows 8.1"):
Copy link
Contributor

Choose a reason for hiding this comment

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

a solution here could be to drop the 8.1 feed as it is EOL by microsoft last year. Fleet generally follows the EOL lifecycle of the vendor. There may be a few more in this list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we have Windows 7 in there too, could definitely drop those two if we want. The bigger culprits are the different Windows Server versions, which tend to not have version info in the product name.

@sgress454
Copy link
Contributor Author

it seems like the general assumption we're making is that by adding a "display version" into our msrc feed, more operating system "products" will start matching against Fleet operating systems. We may need more data to see if osquery does indeed report display versions on these systems.

That's definitely the assumption I'm making here. I dumped the OS table from dogfood into this spreadsheet, and for the versions in there the display version was always reported. I can't guarantee it for other versions without checking against VMs for different systems, which I'm happy to do if we think it's worth the effort (but would still be far from exhaustive given all the different Windows flavors). As it stands, this update won't make things worse, and if it turns out that some systems don't report their version # then we'll have to put more serious thought into how to do this matching.

Another idea would be to update the "no product matches" error with the name of the OS we're trying to match, to gather datapoints that way. I can put that in this PR.

@mostlikelee mostlikelee self-assigned this Dec 11, 2024
mostlikelee
mostlikelee previously approved these changes Dec 12, 2024
Copy link
Contributor

@mostlikelee mostlikelee left a comment

Choose a reason for hiding this comment

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

agree if we're getting consistent display versions, this should help although we will have some maintenance to do as new os versions are released. We may also want to consider reverting some of this change as it addressed an issue on first release before hosts had a chance to report display versions. reverting it will match os versions against the "display name"-less product in the list.

@@ -309,7 +309,7 @@ func checkWinVulnerabilities(
"found new", len(r))
results = append(results, r...)
if err != nil {
errHandler(ctx, logger, "analyzing hosts for Windows vulnerabilities", err)
errHandler(ctx, kitlog.With(logger, "os name", o.Name, "display version", o.DisplayVersion), "analyzing hosts for Windows vulnerabilities", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should help identify any OSs that still aren't matching.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.55%. Comparing base (3d671f1) to head (1611db6).
Report is 48 commits behind head on main.

Files with missing lines Patch % Lines
cmd/fleet/cron.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24172      +/-   ##
==========================================
- Coverage   63.56%   63.55%   -0.02%     
==========================================
  Files        1601     1595       -6     
  Lines      151671   151243     -428     
  Branches     3898     3713     -185     
==========================================
- Hits        96416    96119     -297     
+ Misses      47585    47463     -122     
+ Partials     7670     7661       -9     
Flag Coverage Δ
backend 64.37% <97.22%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sgress454
Copy link
Contributor Author

@mostlikelee had to push an update as some tests were failing. I also updated the error log to show exactly what OS isn't matching, so if the logs don't drop completely after this change we can at least see where the gaps are.

We may also want to consider reverting some of this change as it addressed an issue on first release before hosts had a chance to report display versions

I'll defer to you on this. Could removing this lead to false positives for systems that match on the OS name aren't one of those builds? Or are you saying that the systems we were concerned with when that fix was put in will now report a display version?

@sgress454 sgress454 merged commit edc68d3 into main Dec 17, 2024
22 checks passed
@sgress454 sgress454 deleted the 24041-fix-win-vuln-job branch December 17, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants