Skip to content

ROX-28353: use NVD 2.0 JSON feeds #2015

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ROX-28353: use NVD 2.0 JSON feeds #2015

wants to merge 3 commits into from

Conversation

RTann
Copy link
Collaborator

@RTann RTann commented Jul 31, 2025

NVD will remove the 1.1 JSON feeds August 20, 2025, so this PR switches our feed loader to use the new 2.0 feeds. The schema is the same as the API, so this was pretty straightforward.

The NVD 2.0 API and feeds includes vulnerabilities with more NVD statuses than the 1.1 JSON feeds did (https://nvd.nist.gov/vuln/vulnerability-status). For example, "Deferred" vulnerabilities are included. As long as the vulnerability is not rejected and has all the fields we need, we do not care if NVD opted to change its status.

We should backport this to currently supported releases.

@RTann RTann requested a review from a team as a code owner July 31, 2025 23:02
@RTann RTann added the generate-dumps-on-pr Generates the image based on dumps from the PR label Jul 31, 2025
@RTann RTann force-pushed the ROX-28353 branch 4 times, most recently from ae8b52c to ee8b69f Compare August 1, 2025 23:18
Copy link
Contributor

@dcaravel dcaravel left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 155 to 166
// Prefer CVSS 3.1.
baseMetric := toBaseMetricV31(metrics31)
if baseMetric != nil {
return baseMetric
}

baseMetric = toBaseMetricV30(metrics30)
if baseMetric != nil {
return baseMetric
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't like the len checks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it took O(n) to avoid a function call 🤷
Test: https://go.dev/play/p/0ZRLq9-4G-I

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't seem very necessary, and would have caused for more if/else, which is nice to avoid for readability :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also just want to clarify the semantic change. Before, if there was a 3.1 score, we only read that. However, this was a problem when the 3.1 score was from a "Secondary" (non-NVD) source. There was a case where NVD had both a 3.1 score (secondary) and a 3.0 score (primary). We should use the NVD score in that case, so we should use the 3.0 score instead of the 3.1 score. This change allows for that now

@@ -31,6 +31,11 @@ func toJSON(vulns []*apischema.CVEAPIJSON20DefCVEItem) ([]*jsonschema.NVDCVEFeed
continue
}

// Ignore rejected vulnerabilities.
if strings.EqualFold(vuln.CVE.VulnStatus, "Rejected") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dcaravel @BradLugo I added this conditional after noticing the 2.0 feeds include rejected vulns. They will lack a lot of fields, but just to be safe, I opted to explicitly ignore it

Copy link
Contributor

@BradLugo BradLugo Aug 6, 2025

Choose a reason for hiding this comment

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

I vaguely recall there being some contention with us supporting rejected vulns in the past, and ignoring them as the solution, so your change tracks 👍
Is this covered by a test? If not, should it be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add any because they didn't exist before, but that's a bad reason to not add tests. Just added one which covers this rejected case and some basic stuff

@RTann RTann requested a review from BradLugo August 6, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generate-dumps-on-pr Generates the image based on dumps from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants