Fixed Safari cve false positives - #38903
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38903 +/- ##
=======================================
Coverage 66.10% 66.10%
=======================================
Files 2420 2420
Lines 193439 193457 +18
Branches 8434 8434
=======================================
+ Hits 127865 127878 +13
- Misses 53970 53977 +7
+ Partials 11604 11602 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis PR reduces false positives for Safari CVE-2023-28205 by adding a new CPEMatchingRule that filters matches to Apple Safari 16.0–16.4.0 on macOS Ventura 13.3.1+, paired with comprehensive test coverage across multiple macOS and Safari versions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/vulnerabilities/nvd/cpe_matching_rules.go`:
- Around line 349-372: The current macOS Ventura version check only compares
patch when parts length >= 3, causing versions like "13.4" to be treated as
vulnerable; update the logic in the block that inspects cpeMeta (after calling
wfn.StripSlashes and splitting into parts) to parse the minor version when
len(parts) >= 2 and treat a missing patch as 0 (i.e., set patchVer = 0 if parts
has only 2 elements), handle Atoi errors consistently, then perform the
comparison (if minorVer > 3 || (minorVer == 3 && patchVer >= 1) return true) so
versions like 13.4 correctly count as >= 13.3.1.
- Around line 332-346: The Safari CPE check in cpe_matching_rules.go currently
only checks major version and misses 16.4.1+; update the block that uses
cpeMeta.Vendor/Product and wfn.StripSlashes to parse version parts (major,
minor, patch) with strconv.Atoi (treat major parse errors as non-match/ignore)
and then: if major != 16 return true (ignore), if minor > 4 return true, if
minor == 4 and patch > 0 return true; otherwise allow the match — ensure
minor/patch parse errors default to 0 so versions like "16.4" are handled
correctly.
🧹 Nitpick comments (2)
server/vulnerabilities/nvd/cve_test.go (2)
371-394: Add a boundary test for Safari 16.4.1 (fixed).
Current cases verify 16.2 (vulnerable) and <16 (not), but don’t assert the fixed build. This would prevent regressions in the ignore logic.🧪 Suggested test addition
"cpe:2.3:a:apple:safari:16.2:*:*:*:*:macos:*:*": { includedCVEs: []cve{ {ID: "CVE-2023-28205", resolvedInVersion: "16.4.1"}, }, continuesToUpdate: true, }, + "cpe:2.3:a:apple:safari:16.4.1:*:*:*:*:macos:*:*": { + excludedCVEs: []string{ + "CVE-2023-28205", + }, + continuesToUpdate: true, + }, "cpe:2.3:a:apple:safari:15.6.1:*:*:*:*:macos:*:*": { excludedCVEs: []string{ "CVE-2023-28205", },
718-766: Add a Ventura 13.4 boundary test (no patch segment).
This validates that versions >= 13.3.1 are excluded even when only major/minor are present.🧪 Suggested test addition
{ platform: "darwin", version: "13.3.1", osID: 5, // macOS Ventura 13.3.1 includes system-level WebKit patch for CVE-2023-28205 excludedCVEs: []string{"CVE-2023-28205"}, }, + { + platform: "darwin", + version: "13.4", + osID: 11, + // macOS Ventura 13.4 includes system-level WebKit patch for CVE-2023-28205 + excludedCVEs: []string{"CVE-2023-28205"}, + },
Comment on lines
+332
to
+346
| // For Safari CPE matches, only match versions 16.0-16.4.0 | ||
| if cpeMeta.Vendor == "apple" && cpeMeta.Product == "safari" { | ||
| version := wfn.StripSlashes(cpeMeta.Version) | ||
| parts := strings.Split(version, ".") | ||
|
|
||
| if len(parts) > 0 { | ||
| if majorVer, err := strconv.Atoi(parts[0]); err == nil { | ||
| if majorVer < 16 { | ||
| return true | ||
| } | ||
| if majorVer > 16 { | ||
| return true | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Safari ignore logic doesn’t exclude fixed versions (16.4.1+).
Only the major version is checked, so 16.4.1 and 16.5+ will still match and be reported vulnerable, which undermines the false‑positive fix.
🛠️ Suggested fix
- if len(parts) > 0 {
- if majorVer, err := strconv.Atoi(parts[0]); err == nil {
- if majorVer < 16 {
- return true
- }
- if majorVer > 16 {
- return true
- }
- }
- }
+ if len(parts) > 0 {
+ majorVer, err := strconv.Atoi(parts[0])
+ if err != nil {
+ return false
+ }
+ minorVer, patchVer := 0, 0
+ if len(parts) > 1 {
+ if v, err := strconv.Atoi(parts[1]); err == nil {
+ minorVer = v
+ } else {
+ return false
+ }
+ }
+ if len(parts) > 2 {
+ if v, err := strconv.Atoi(parts[2]); err == nil {
+ patchVer = v
+ } else {
+ return false
+ }
+ }
+ if majorVer < 16 || majorVer > 16 ||
+ minorVer > 4 || (minorVer == 4 && patchVer >= 1) {
+ return true
+ }
+ }🤖 Prompt for AI Agents
In `@server/vulnerabilities/nvd/cpe_matching_rules.go` around lines 332 - 346, The
Safari CPE check in cpe_matching_rules.go currently only checks major version
and misses 16.4.1+; update the block that uses cpeMeta.Vendor/Product and
wfn.StripSlashes to parse version parts (major, minor, patch) with strconv.Atoi
(treat major parse errors as non-match/ignore) and then: if major != 16 return
true (ignore), if minor > 4 return true, if minor == 4 and patch > 0 return
true; otherwise allow the match — ensure minor/patch parse errors default to 0
so versions like "16.4" are handled correctly.
Contributor
Author
There was a problem hiding this comment.
this sounds accurate. I'll make the changes for this.
Comment on lines
+349
to
+372
| // For macOS CPE matches, only match Ventura < 13.3.1 | ||
| if cpeMeta.Vendor == "apple" && cpeMeta.Product == "macos" { | ||
| version := wfn.StripSlashes(cpeMeta.Version) | ||
| parts := strings.Split(version, ".") | ||
|
|
||
| if len(parts) > 0 { | ||
| majorVer, err := strconv.Atoi(parts[0]) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Ignore non-Ventura | ||
| if majorVer != 13 { | ||
| return true | ||
| } | ||
|
|
||
| // For Ventura, check if >= 13.3.1 | ||
| if len(parts) >= 3 { | ||
| minorVer, _ := strconv.Atoi(parts[1]) | ||
| patchVer, _ := strconv.Atoi(parts[2]) | ||
| if minorVer > 3 || (minorVer == 3 && patchVer >= 1) { | ||
| return true | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Ventura >=13.3.1 isn’t ignored when the patch segment is missing (e.g., 13.4).
The check only runs when len(parts) >= 3, so 13.4/13.5 are treated as vulnerable even though they’re >= 13.3.1.
🛠️ Suggested fix
- // For Ventura, check if >= 13.3.1
- if len(parts) >= 3 {
- minorVer, _ := strconv.Atoi(parts[1])
- patchVer, _ := strconv.Atoi(parts[2])
- if minorVer > 3 || (minorVer == 3 && patchVer >= 1) {
- return true
- }
- }
+ // For Ventura, check if >= 13.3.1 (treat missing patch as 0)
+ if len(parts) >= 2 {
+ minorVer, err := strconv.Atoi(parts[1])
+ if err != nil {
+ return false
+ }
+ patchVer := 0
+ if len(parts) >= 3 {
+ if v, err := strconv.Atoi(parts[2]); err == nil {
+ patchVer = v
+ } else {
+ return false
+ }
+ }
+ if minorVer > 3 || (minorVer == 3 && patchVer >= 1) {
+ return true
+ }
+ }🤖 Prompt for AI Agents
In `@server/vulnerabilities/nvd/cpe_matching_rules.go` around lines 349 - 372, The
current macOS Ventura version check only compares patch when parts length >= 3,
causing versions like "13.4" to be treated as vulnerable; update the logic in
the block that inspects cpeMeta (after calling wfn.StripSlashes and splitting
into parts) to parse the minor version when len(parts) >= 2 and treat a missing
patch as 0 (i.e., set patchVer = 0 if parts has only 2 elements), handle Atoi
errors consistently, then perform the comparison (if minorVer > 3 || (minorVer
== 3 && patchVer >= 1) return true) so versions like 13.4 correctly count as >=
13.3.1.
Contributor
Author
There was a problem hiding this comment.
That's assuming the version formats don't have a trailing 0. Such as 13.4.0 or 13.5.0. Let me try to find one that I can spin up on a VM and see what the version looks like
Contributor
Author
There was a problem hiding this comment.
Just double checked.
getvictor
previously approved these changes
Jan 29, 2026
getvictor
left a comment
getvictor
left a comment
Member
There was a problem hiding this comment.
Looks good overall. See minor comments.
| "CVE-2023-28205": {}, | ||
| }, | ||
| IgnoreIf: func(cpeMeta *wfn.Attributes) bool { | ||
| // For Safari CPE matches, only match versions 16.0-16.4.0 |
Member
There was a problem hiding this comment.
Is this supposed to say: only match versions 16.X?
Comment on lines
+349
to
+372
| // For macOS CPE matches, only match Ventura < 13.3.1 | ||
| if cpeMeta.Vendor == "apple" && cpeMeta.Product == "macos" { | ||
| version := wfn.StripSlashes(cpeMeta.Version) | ||
| parts := strings.Split(version, ".") | ||
|
|
||
| if len(parts) > 0 { | ||
| majorVer, err := strconv.Atoi(parts[0]) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Ignore non-Ventura | ||
| if majorVer != 13 { | ||
| return true | ||
| } | ||
|
|
||
| // For Ventura, check if >= 13.3.1 | ||
| if len(parts) >= 3 { | ||
| minorVer, _ := strconv.Atoi(parts[1]) | ||
| patchVer, _ := strconv.Atoi(parts[2]) | ||
| if minorVer > 3 || (minorVer == 3 && patchVer >= 1) { | ||
| return true | ||
| } | ||
| } |
getvictor
approved these changes
Jan 29, 2026
georgekarrv
pushed a commit
that referenced
this pull request
Feb 6, 2026
**Related issue:** Resolves #35194 The NVD database for CVE-2023-28205 contains two broad CPE match rules: Safari: Any version < 16.4.1 is vulnerable macOS: Any version < 13.3.1 is vulnerable The problem is Safari versions 13.x, 14.x, and 15.x were never actually vulnerable to this CVE and macOS versions 10.x, 11.x, and 12.x never received a system-level fix for this CVE. Apple fixed the cve in two ways 1. Safari 16.4.1 standalone update 2. macOS Ventura 13.3.1 system update fix at the OS level This is why there is such a complicated `IgnoreIf` for the `CPEMatchingRule`. If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [ ] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved accuracy of Safari CVE-2023-28205 vulnerability detection with version-specific filtering for Safari 16.0-16.4.0 and macOS Ventura. * **Tests** * Added comprehensive test coverage for CVE-2023-28205 across multiple Safari versions and macOS releases. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #35194
The NVD database for CVE-2023-28205 contains two broad CPE match rules:
Safari: Any version < 16.4.1 is vulnerable
macOS: Any version < 13.3.1 is vulnerable
The problem is Safari versions 13.x, 14.x, and 15.x were never actually vulnerable to this CVE and macOS versions 10.x, 11.x, and 12.x never received a system-level fix for this CVE.
Apple fixed the cve in two ways
This is why there is such a complicated
IgnoreIffor theCPEMatchingRule.If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.