From 906b621fbf3c9d2f67259ce32d7b1161e15ef2b6 Mon Sep 17 00:00:00 2001 From: Tatiana Bradley Date: Fri, 14 Jun 2024 12:09:55 -0400 Subject: [PATCH] cmd/vulnreport/priority: take unexcluded reports into account When using the existing corpus to prioritize new vulnerabilities, treat unreviewed unexcluded reports the same as likely-binary excluded reports. Change-Id: Id803d05ecd33b4486086acac8ff124977b3725ef Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/592777 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI --- cmd/vulnreport/priority/priority.go | 67 ++++++++++++-------- cmd/vulnreport/priority/priority_test.go | 28 +++++++- cmd/vulnreport/testdata/TestTriage/all.txtar | 2 +- 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/cmd/vulnreport/priority/priority.go b/cmd/vulnreport/priority/priority.go index a5b4d1c1..52dcf0f6 100644 --- a/cmd/vulnreport/priority/priority.go +++ b/cmd/vulnreport/priority/priority.go @@ -69,14 +69,16 @@ func priority(mp string, importers int, sc map[reportState]int) (priority Priori } if importers >= highPriority { + rev := sc[reviewed] + binary := sc[excludedBinary] + sc[unreviewedUnexcluded] getReason := func(conj1, conj2 string) string { - return fmt.Sprintf("%s %s reviewed (%d) %s likely-binary excluded reports (%d)", - importersStr(">="), conj1, sc[reviewed], conj2, sc[excludedBinary]) + return fmt.Sprintf("%s %s reviewed (%d) %s likely-binary reports (%d)", + importersStr(">="), conj1, rev, conj2, binary) } - if sc[reviewed] > sc[excludedBinary] { + if rev > binary { return High, getReason("and more", "than") - } else if sc[reviewed] == sc[excludedBinary] { + } else if rev == binary { return High, getReason("and as many", "as") } @@ -98,39 +100,48 @@ type reportState int const ( unknownReportState reportState = iota reviewed - unreviewed + unreviewedStandard + unreviewedUnexcluded excludedBinary excludedNotGo excludedOther ) +func state(r *report.Report) reportState { + if r.IsExcluded() { + switch e := r.Excluded; e { + case "NOT_GO_CODE": + return excludedNotGo + case "EFFECTIVELY_PRIVATE", "NOT_IMPORTABLE", "LEGACY_FALSE_POSITIVE": + return excludedBinary + case "NOT_A_VULNERABILITY", "DEPENDENT_VULNERABILITY": + return excludedOther + default: + return unknownReportState + } + } + + switch rs := r.ReviewStatus; rs { + case report.Reviewed: + return reviewed + case report.Unreviewed: + if r.Unexcluded != "" { + return unreviewedUnexcluded + } + return unreviewedStandard + } + + return unknownReportState +} + func stateCounts(rs []*report.Report) map[reportState]int { counts := make(map[reportState]int) for _, r := range rs { - var state reportState - switch { - case r.IsExcluded(): - switch e := r.Excluded; e { - case "NOT_GO_CODE": - state = excludedNotGo - case "EFFECTIVELY_PRIVATE", "NOT_IMPORTABLE", "LEGACY_FALSE_POSITIVE": - state = excludedBinary - case "NOT_A_VULNERABILITY", "DEPENDENT_VULNERABILITY": - state = excludedOther - default: - panic("unknown excluded reason " + e) - } - default: - switch rs := r.ReviewStatus; rs { - case report.Reviewed: - state = reviewed - case report.Unreviewed: - state = unreviewed - default: - panic("unknown review status " + rs.String()) - } + st := state(r) + if st == unknownReportState { + panic("could not determine report state for " + r.ID) } - counts[state]++ + counts[st]++ } return counts } diff --git a/cmd/vulnreport/priority/priority_test.go b/cmd/vulnreport/priority/priority_test.go index daa966e5..05357ae9 100644 --- a/cmd/vulnreport/priority/priority_test.go +++ b/cmd/vulnreport/priority/priority_test.go @@ -21,6 +21,10 @@ var ( reviewed2 = &report.Report{ ReviewStatus: report.Reviewed, } + reviewedBinary = &report.Report{ + ReviewStatus: report.Reviewed, + Unexcluded: "NOT_IMPORTABLE", + } unreviewed1 = &report.Report{ ReviewStatus: report.Unreviewed, } @@ -33,6 +37,10 @@ var ( binary3 = &report.Report{ Excluded: "LEGACY_FALSE_POSITIVE", } + unreviewedBinary = &report.Report{ + ReviewStatus: report.Unreviewed, + Unexcluded: "NOT_IMPORTABLE", + } notAVuln1 = &report.Report{ Excluded: "NOT_A_VULNERABILITY", } @@ -75,7 +83,7 @@ func TestAnalyze(t *testing.T) { modulesToImports: map[string]int{"example.com/module": 100}, want: &Result{ Priority: High, - Reason: "example.com/module has 100 importers (>= 100) and as many reviewed (0) as likely-binary excluded reports (0)", + Reason: "example.com/module has 100 importers (>= 100) and as many reviewed (0) as likely-binary reports (0)", }, }, { @@ -85,7 +93,7 @@ func TestAnalyze(t *testing.T) { modulesToImports: map[string]int{"example.com/module": 101}, want: &Result{ Priority: High, - Reason: "example.com/module has 101 importers (>= 100) and more reviewed (2) than likely-binary excluded reports (1)", + Reason: "example.com/module has 101 importers (>= 100) and more reviewed (2) than likely-binary reports (1)", }, }, { @@ -99,7 +107,21 @@ func TestAnalyze(t *testing.T) { modulesToImports: map[string]int{"example.com/module": 101}, want: &Result{ Priority: Low, - Reason: "example.com/module has 101 importers (>= 100) but fewer reviewed (1) than likely-binary excluded reports (3)", + Reason: "example.com/module has 101 importers (>= 100) but fewer reviewed (1) than likely-binary reports (3)", + }, + }, + { + name: "unexcluded unreviewed considered binaries", + module: "example.com/module", + reportsForModule: []*report.Report{ + reviewed1, reviewedBinary, // reviewed + binary1, binary2, unreviewedBinary, // binary + unreviewed1, notAVuln1, dependent1, // ignored + }, + modulesToImports: map[string]int{"example.com/module": 101}, + want: &Result{ + Priority: Low, + Reason: "example.com/module has 101 importers (>= 100) but fewer reviewed (2) than likely-binary reports (3)", }, }, { diff --git a/cmd/vulnreport/testdata/TestTriage/all.txtar b/cmd/vulnreport/testdata/TestTriage/all.txtar index 57fd04bc..6c94c064 100644 --- a/cmd/vulnreport/testdata/TestTriage/all.txtar +++ b/cmd/vulnreport/testdata/TestTriage/all.txtar @@ -9,7 +9,7 @@ command: "vulnreport triage " issue test-issue-tracker/7 is likely duplicate - #7 shares alias(es) CVE-9999-0005 with data/reports/GO-9999-0005.yaml issue test-issue-tracker/10 is high priority - - golang.org/x/vuln has 101 importers (>= 100) and as many reviewed (0) as likely-binary excluded reports (0) + - golang.org/x/vuln has 101 importers (>= 100) and as many reviewed (0) as likely-binary reports (0) issue test-issue-tracker/11 is possibly not Go - more than 20 percent of reports (1 of 1) with this module are NOT_GO_CODE triaged 3 issues: