Skip to content

Commit bca6ae2

Browse files
tatianabtatianab
committed
cmd/vulnreport: double-check priority after create and in presubmit
The algorithm that determines priority for a report relies on the affected modules. Sometimes not all affected modules are known at the outset (e.g., because they are fixed during report creation). Ensure that we don't accidentally create UNREVIEWED reports which are high priority by re-checking the priority of a report after creating it. As an extra safeguard, also do this check in the TestLintReports function which acts as a presubmit check. This involves some refactoring of the priority algorithm. The only change to the fundamental behavior is that an override list now exists, where we can add modules that should always have a certain priority regardless of what the priority algorithm would say. Also, the xref command now addionally prints out the priority decision for a report. Change-Id: Ia3301022678d7392fb3deb059f9a248dcb153ecc Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/598415 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Damien Neil <[email protected]>
1 parent bb794fc commit bca6ae2

File tree

13 files changed

+190
-80
lines changed

13 files changed

+190
-80
lines changed

‎all_test.go

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package main
99

1010
import (
11+
"context"
1112
"errors"
1213
"os"
1314
"os/exec"
@@ -20,7 +21,9 @@ import (
2021

2122
"github.com/google/go-cmp/cmp"
2223
"github.com/google/go-cmp/cmp/cmpopts"
24+
"golang.org/x/vulndb/cmd/vulnreport/priority"
2325
"golang.org/x/vulndb/internal/cve5"
26+
"golang.org/x/vulndb/internal/gitrepo"
2427
"golang.org/x/vulndb/internal/osvutils"
2528
"golang.org/x/vulndb/internal/proxy"
2629
"golang.org/x/vulndb/internal/report"
@@ -87,8 +90,19 @@ func TestLintReports(t *testing.T) {
8790
}
8891
}
8992

90-
// Map from aliases (CVEs/GHSAS) to report paths, used to check for duplicate aliases.
91-
aliases := make(map[string]string)
93+
vulndb, err := gitrepo.Open(context.Background(), ".")
94+
if err != nil {
95+
t.Fatal(err)
96+
}
97+
rc, err := report.NewClient(vulndb)
98+
if err != nil {
99+
t.Fatal(err)
100+
}
101+
modulesToImports, err := priority.LoadModuleMap()
102+
if err != nil {
103+
t.Fatal(err)
104+
}
105+
92106
// Map from summaries to report paths, used to check for duplicate summaries.
93107
summaries := make(map[string]string)
94108
sort.Strings(reports)
@@ -107,14 +121,14 @@ func TestLintReports(t *testing.T) {
107121
}
108122
duplicates := make(map[string][]string)
109123
for _, alias := range r.Aliases() {
110-
if report, ok := aliases[alias]; ok {
111-
duplicates[report] = append(duplicates[report], alias)
112-
} else {
113-
aliases[alias] = filename
124+
for _, r2 := range rc.ReportsByAlias(alias) {
125+
if r2.ID != r.ID {
126+
duplicates[r2.ID] = append(duplicates[r2.ID], alias)
127+
}
114128
}
115129
}
116-
for report, aliases := range duplicates {
117-
t.Errorf("report %s shares duplicate alias(es) %s with report %s", filename, aliases, report)
130+
for r2, aliases := range duplicates {
131+
t.Errorf("report %s shares duplicate alias(es) %s with report %s", filename, aliases, r2)
118132
}
119133
// Ensure that each reviewed report has a unique summary.
120134
if r.IsReviewed() {
@@ -126,6 +140,16 @@ func TestLintReports(t *testing.T) {
126140
}
127141
}
128142
}
143+
// Ensure that no unreviewed reports are high priority.
144+
// This can happen because the initial quick triage algorithm
145+
// doesn't know about all affected modules - just the one
146+
// listed in the Github issue.
147+
if r.IsUnreviewed() {
148+
pr, _ := priority.AnalyzeReport(r, rc, modulesToImports)
149+
if pr.Priority == priority.High {
150+
t.Errorf("UNREVIEWED report %s is high priority (should be REVIEWED) - reason: %s", filename, pr.Reason)
151+
}
152+
}
129153
// Check that a correct OSV file was generated for each YAML report.
130154
if r.Excluded == "" {
131155
generated, err := r.ToOSV(time.Time{})

‎cmd/vulnreport/creator.go

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313

1414
"golang.org/x/exp/slices"
1515
"golang.org/x/vulndb/cmd/vulnreport/log"
16+
"golang.org/x/vulndb/cmd/vulnreport/priority"
1617
"golang.org/x/vulndb/internal/idstr"
1718
"golang.org/x/vulndb/internal/issues"
1819
"golang.org/x/vulndb/internal/osv"
@@ -159,20 +160,23 @@ func (c *creator) metaToSource(ctx context.Context, meta *reportMeta) report.Sou
159160
return report.Original()
160161
}
161162

162-
func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlReport, error) {
163-
// Find the underlying module if the "module" provided is actually a package path.
164-
if module, err := c.pc.FindModule(meta.modulePath); err == nil { // no error
165-
meta.modulePath = module
166-
}
167-
meta.aliases = c.allAliases(ctx, meta.aliases)
168-
169-
raw := report.New(c.metaToSource(ctx, meta), c.pc,
163+
func (c *creator) rawReport(ctx context.Context, meta *reportMeta) *report.Report {
164+
return report.New(c.metaToSource(ctx, meta), c.pc,
170165
report.WithGoID(meta.id),
171166
report.WithModulePath(meta.modulePath),
172167
report.WithAliases(meta.aliases),
173168
report.WithReviewStatus(meta.reviewStatus),
174169
report.WithUnexcluded(meta.unexcluded),
175170
)
171+
}
172+
173+
func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlReport, error) {
174+
// Find the underlying module if the "module" provided is actually a package path.
175+
if module, err := c.pc.FindModule(meta.modulePath); err == nil { // no error
176+
meta.modulePath = module
177+
}
178+
meta.aliases = c.allAliases(ctx, meta.aliases)
179+
raw := c.rawReport(ctx, meta)
176180

177181
if meta.excluded != "" {
178182
raw = &report.Report{
@@ -188,6 +192,18 @@ func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlRe
188192
}
189193
}
190194

195+
// The initial quick triage algorithm doesn't know about all
196+
// affected modules, so double check the priority after the
197+
// report is created.
198+
if raw.IsUnreviewed() {
199+
pr, _ := c.reportPriority(raw)
200+
if pr.Priority == priority.High {
201+
log.Warnf("%s: re-generating; vuln is high priority and should be REVIEWED; reason: %s", raw.ID, pr.Reason)
202+
meta.reviewStatus = report.Reviewed
203+
raw = c.rawReport(ctx, meta)
204+
}
205+
}
206+
191207
fname, err := raw.YAMLFilename()
192208
if err != nil {
193209
return nil, err
@@ -221,9 +237,9 @@ func (c *creator) reportFromMeta(ctx context.Context, meta *reportMeta) (*yamlRe
221237
}
222238

223239
switch {
224-
case meta.excluded != "":
240+
case raw.IsExcluded():
225241
// nothing
226-
case meta.reviewStatus == report.Unreviewed:
242+
case raw.IsUnreviewed():
227243
r.removeUnreachableRefs()
228244
default:
229245
// Regular, full-length reports.

‎cmd/vulnreport/priority/priority.go

Lines changed: 67 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,18 @@ package priority
77

88
import (
99
"fmt"
10+
"strings"
1011

1112
"golang.org/x/vulndb/internal/report"
1213
)
1314

1415
type Result struct {
15-
Priority Priority
16-
Reason string
17-
NotGo bool
18-
NotGoReason string
16+
Priority Priority
17+
Reason string
18+
}
19+
20+
type NotGoResult struct {
21+
Reason string
1922
}
2023

2124
type Priority int
@@ -39,30 +42,66 @@ const (
3942
High
4043
)
4144

42-
func Analyze(mp string, reportsForModule []*report.Report, modulesToImports map[string]int) *Result {
45+
// AnalyzeReport returns the results for a report as a whole:
46+
// - priority is the priority of its highest-priority module
47+
// - not Go if all modules are not Go
48+
func AnalyzeReport(r *report.Report, rc *report.Client, modulesToImports map[string]int) (*Result, *NotGoResult) {
49+
var overall Priority
50+
var reasons []string
51+
var notGoReasons []string
52+
for _, m := range r.Modules {
53+
mp := m.Module
54+
result, notGo := Analyze(mp, rc.ReportsByModule(mp), modulesToImports)
55+
if result.Priority > overall {
56+
overall = result.Priority
57+
}
58+
reasons = append(reasons, result.Reason)
59+
if notGo != nil {
60+
notGoReasons = append(notGoReasons, notGo.Reason)
61+
}
62+
}
63+
64+
result := &Result{
65+
Priority: overall, Reason: strings.Join(reasons, "; "),
66+
}
67+
68+
// If all modules are not Go, the report is not Go.
69+
if len(notGoReasons) == len(r.Modules) {
70+
return result, &NotGoResult{Reason: strings.Join(notGoReasons, "; ")}
71+
}
72+
73+
return result, nil
74+
}
75+
76+
func Analyze(mp string, reportsForModule []*report.Report, modulesToImports map[string]int) (*Result, *NotGoResult) {
4377
sc := stateCounts(reportsForModule)
4478

45-
notGo, notGoReason := isPossiblyNotGo(len(reportsForModule), sc)
79+
notGo := isPossiblyNotGo(len(reportsForModule), sc)
4680
importers, ok := modulesToImports[mp]
4781
if !ok {
4882
return &Result{
49-
Priority: Unknown,
50-
Reason: fmt.Sprintf("module %s not found", mp),
51-
NotGo: notGo,
52-
NotGoReason: notGoReason,
53-
}
83+
Priority: Unknown,
84+
Reason: fmt.Sprintf("module %s not found", mp),
85+
}, notGo
5486
}
5587

56-
priority, reason := priority(mp, importers, sc)
57-
return &Result{
58-
Priority: priority,
59-
Reason: reason,
60-
NotGo: notGo,
61-
NotGoReason: notGoReason,
62-
}
88+
return priority(mp, importers, sc), notGo
6389
}
6490

65-
func priority(mp string, importers int, sc map[reportState]int) (priority Priority, reason string) {
91+
// override takes precedence over all other metrics in determining
92+
// a module's priority.
93+
var override map[string]Priority = map[string]Priority{
94+
// argo-cd is primarily a binary and usually has correct version
95+
// information without intervention.
96+
"github.com/argoproj/argo-cd": Low,
97+
"github.com/argoproj/argo-cd/v2": Low,
98+
}
99+
100+
func priority(mp string, importers int, sc map[reportState]int) *Result {
101+
if pr, ok := override[mp]; ok {
102+
return &Result{pr, fmt.Sprintf("%s is in the override list (priority=%s)", mp, pr)}
103+
}
104+
66105
const highPriority = 100
67106
importersStr := func(comp string) string {
68107
return fmt.Sprintf("%s has %d importers (%s %d)", mp, importers, comp, highPriority)
@@ -77,22 +116,24 @@ func priority(mp string, importers int, sc map[reportState]int) (priority Priori
77116
}
78117

79118
if rev > binary {
80-
return High, getReason("and more", "than")
119+
return &Result{High, getReason("and more", "than")}
81120
} else if rev == binary {
82-
return High, getReason("and as many", "as")
121+
return &Result{High, getReason("and as many", "as")}
83122
}
84123

85-
return Low, getReason("but fewer", "than")
124+
return &Result{Low, getReason("but fewer", "than")}
86125
}
87126

88-
return Low, importersStr("<")
127+
return &Result{Low, importersStr("<")}
89128
}
90129

91-
func isPossiblyNotGo(numReports int, sc map[reportState]int) (bool, string) {
130+
func isPossiblyNotGo(numReports int, sc map[reportState]int) *NotGoResult {
92131
if (float32(sc[excludedNotGo])/float32(numReports))*100 > 20 {
93-
return true, fmt.Sprintf("more than 20 percent of reports (%d of %d) with this module are NOT_GO_CODE", sc[excludedNotGo], numReports)
132+
return &NotGoResult{
133+
Reason: fmt.Sprintf("more than 20 percent of reports (%d of %d) with this module are NOT_GO_CODE", sc[excludedNotGo], numReports),
134+
}
94135
}
95-
return false, ""
136+
return nil
96137
}
97138

98139
type reportState int

‎cmd/vulnreport/priority/priority/main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,10 @@ func main() {
3838
}
3939

4040
for _, arg := range args {
41-
pr := priority.Analyze(arg, rc.ReportsByModule(arg), ms)
41+
pr, notGo := priority.Analyze(arg, rc.ReportsByModule(arg), ms)
4242
vlog.Outf("%s:\npriority = %s\n%s", arg, pr.Priority, pr.Reason)
43-
if pr.NotGo {
44-
vlog.Outf("%s is likely not Go because %s", arg, pr.NotGoReason)
43+
if notGo != nil {
44+
vlog.Outf("%s is likely not Go because %s", arg, notGo.Reason)
4545
}
4646
}
4747
}

‎cmd/vulnreport/priority/priority_test.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ func TestAnalyze(t *testing.T) {
5656
reportsForModule []*report.Report
5757
modulesToImports map[string]int
5858
want *Result
59+
wantNotGo *NotGoResult
5960
}{
6061
{
6162
name: "unknown priority",
@@ -130,18 +131,22 @@ func TestAnalyze(t *testing.T) {
130131
reportsForModule: []*report.Report{notGo1, reviewed1, binary1, unreviewed1},
131132
modulesToImports: map[string]int{"example.com/module": 99},
132133
want: &Result{
133-
Priority: Low,
134-
Reason: "example.com/module has 99 importers (< 100)",
135-
NotGo: true,
136-
NotGoReason: "more than 20 percent of reports (1 of 4) with this module are NOT_GO_CODE",
134+
Priority: Low,
135+
Reason: "example.com/module has 99 importers (< 100)",
136+
},
137+
wantNotGo: &NotGoResult{
138+
Reason: "more than 20 percent of reports (1 of 4) with this module are NOT_GO_CODE",
137139
},
138140
},
139141
} {
140142
t.Run(tc.name, func(t *testing.T) {
141-
got := Analyze(tc.module, tc.reportsForModule, tc.modulesToImports)
143+
got, gotNotGo := Analyze(tc.module, tc.reportsForModule, tc.modulesToImports)
142144
want := tc.want
143145
if diff := cmp.Diff(want, got); diff != "" {
144-
t.Errorf("mismatch (-want, +got):\n%s", diff)
146+
t.Errorf("result mismatch (-want, +got):\n%s", diff)
147+
}
148+
if diff := cmp.Diff(tc.wantNotGo, gotNotGo); diff != "" {
149+
t.Errorf("not go mismatch (-want, +got):\n%s", diff)
145150
}
146151
})
147152
}

‎cmd/vulnreport/testdata/TestXref/found_xrefs.txtar

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@ Expected output of test TestXref/found_xrefs
66
command: "vulnreport xref 4"
77

88
-- out --
9-
xrefs for data/reports/GO-9999-0004.yaml:
9+
data/reports/GO-9999-0004.yaml: found xrefs:
1010
Module golang.org/x/tools appears in data/reports/GO-9999-0005.yaml
11+
GO-9999-0004 priority is low
12+
- golang.org/x/tools has 50 importers (< 100)
1113
-- logs --
1214
info: xref: operating on 1 report(s)
1315
info: xref data/reports/GO-9999-0004.yaml

‎cmd/vulnreport/testdata/TestXref/no_xrefs.txtar

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@ Expected output of test TestXref/no_xrefs
66
command: "vulnreport xref 1"
77

88
-- out --
9-
xrefs for data/reports/GO-9999-0001.yaml:
10-
Module golang.org/x/vulndb appears in data/excluded/GO-9999-0002.yaml EFFECTIVELY_PRIVATE
9+
GO-9999-0001 priority is unknown
10+
- module golang.org/x/vulndb not found
1111
-- logs --
1212
info: xref: operating on 1 report(s)
1313
info: xref data/reports/GO-9999-0001.yaml
14+
info: data/reports/GO-9999-0001.yaml: no xrefs found
1415
info: xref: processed 1 report(s) (success=1; skip=0; error=0)

‎cmd/vulnreport/testdata/repo.txtar

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ review_status: REVIEWED
3030
-- data/excluded/GO-9999-0002.yaml --
3131
id: GO-9999-0002
3232
modules:
33-
- module: golang.org/x/vulndb
33+
- module: golang.org/x/exp
3434
cve_metadata:
3535
id: CVE-9999-0002
3636
excluded: EFFECTIVELY_PRIVATE

0 commit comments

Comments
 (0)