internal/worker: do not skip GHSAs that have CVEs
The worker was missing some GHSAs because it always filtered out GHSAs with CVEs (and sometimes CVEs are miscategorized as not Go vulns, aren't published yet, etc).
This change modifies the logic to look at all GHSAs and create an issue if there is not yet an issue for the associated CVE.
Note that this leaves a gap (which will be fixed in a subsequent CL) in which a CVE that is later found by the worker will have a duplicate issue created for it.
Change-Id: I54008c2b2772ee6de9ece2f129de8668e80bed27
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/432095
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/cmd/vulnreport/main.go b/cmd/vulnreport/main.go
index a15d726..4c51d81 100644
--- a/cmd/vulnreport/main.go
+++ b/cmd/vulnreport/main.go
@@ -826,12 +826,11 @@
// loadGHSAsByCVE returns a map from CVE ID to GHSA IDs.
// It does this by using the GitHub API to list all Go security
-// advisories with CVEs.
+// advisories.
func loadGHSAsByCVE(ctx context.Context, accessToken string) (_ map[string][]string, err error) {
defer derrors.Wrap(&err, "loadGHSAsByCVE")
- const withCVE = true
- sas, err := ghsa.List(ctx, accessToken, time.Time{}, withCVE)
+ sas, err := ghsa.List(ctx, accessToken, time.Time{})
if err != nil {
return nil, err
}
diff --git a/cmd/worker/main.go b/cmd/worker/main.go
index ec39ce2..7c94d5c 100644
--- a/cmd/worker/main.go
+++ b/cmd/worker/main.go
@@ -213,8 +213,7 @@
return nil
}
listSAs := func(ctx context.Context, since time.Time) ([]*ghsa.SecurityAdvisory, error) {
- const withoutCVES = false
- return ghsa.List(ctx, cfg.GitHubAccessToken, since, withoutCVES)
+ return ghsa.List(ctx, cfg.GitHubAccessToken, since)
}
_, err = worker.UpdateGHSAs(ctx, listSAs, cfg.Store)
return err
diff --git a/internal/ghsa/ghsa.go b/internal/ghsa/ghsa.go
index d47ed1a..0512b9d 100644
--- a/internal/ghsa/ghsa.go
+++ b/internal/ghsa/ghsa.go
@@ -149,9 +149,7 @@
// List returns all SecurityAdvisories that affect Go,
// published or updated since the given time.
-// If withCVE is true, selects only advisories that are
-// connected to CVEs, otherwise selects only advisories without CVEs.
-func List(ctx context.Context, accessToken string, since time.Time, withCVE bool) ([]*SecurityAdvisory, error) {
+func List(ctx context.Context, accessToken string, since time.Time) ([]*SecurityAdvisory, error) {
client := newGitHubClient(ctx, accessToken)
var query struct { // the GraphQL query
@@ -177,9 +175,6 @@
return nil, err
}
for _, sa := range query.SAs.Nodes {
- if withCVE != isCVE(sa.Identifiers) {
- continue
- }
if len(sa.Vulnerabilities.Nodes) == 0 {
continue
}
diff --git a/internal/ghsa/ghsa_test.go b/internal/ghsa/ghsa_test.go
index 9652f66..4739bcd 100644
--- a/internal/ghsa/ghsa_test.go
+++ b/internal/ghsa/ghsa_test.go
@@ -31,8 +31,7 @@
accessToken := mustGetAccessToken(t)
// There were at least three relevant SAs since this date.
since := time.Date(2022, 1, 1, 0, 0, 0, 0, time.UTC)
- const withoutCVEs = false
- got, err := List(context.Background(), accessToken, since, withoutCVEs)
+ got, err := List(context.Background(), accessToken, since)
if err != nil {
t.Fatal(err)
}
diff --git a/internal/worker/server.go b/internal/worker/server.go
index b5724e7..39304ad 100644
--- a/internal/worker/server.go
+++ b/internal/worker/server.go
@@ -109,9 +109,10 @@
return nil
})
- // update: Update the DB from the cvelist repo head and decide which CVEs need issues.
+ // update: Update the DB from the cvelist repo head and the Github Security
+ // Advisories API and decide which CVEs and GHSAs need issues.
s.handle(ctx, "/update", s.handleUpdate)
- // issues: File issues on GitHub for CVEs that need them.
+ // issues: File issues on GitHub for CVEs and GHSAs that need them.
s.handle(ctx, "/issues", s.handleIssues)
// update-and-issues: do update followed by issues.
s.handle(ctx, "/update-and-issues", s.handleUpdateAndIssues)
@@ -333,8 +334,7 @@
return err
}
listSAs := func(ctx context.Context, since time.Time) ([]*ghsa.SecurityAdvisory, error) {
- const withoutCVES = false
- return ghsa.List(ctx, s.cfg.GitHubAccessToken, since, withoutCVES)
+ return ghsa.List(ctx, s.cfg.GitHubAccessToken, since)
}
_, err = UpdateGHSAs(r.Context(), listSAs, s.cfg.Store)
return err
diff --git a/internal/worker/store/store.go b/internal/worker/store/store.go
index 624b929..710359f 100644
--- a/internal/worker/store/store.go
+++ b/internal/worker/store/store.go
@@ -89,19 +89,22 @@
return r.TriageState.Validate()
}
-// TriageState is the state of our work on the CVE.
+// TriageState is the state of our work on the CVE or GHSA.
// It is implemented as a string rather than an int so that stored values are
// immune to renumbering.
type TriageState string
const (
- // No action is needed on the CVE (perhaps because it is rejected, reserved or invalid).
+ // No action is needed on the CVE or GHSA (perhaps because it is rejected, reserved or invalid).
TriageStateNoActionNeeded TriageState = "NoActionNeeded"
// The CVE needs to have an issue created.
TriageStateNeedsIssue TriageState = "NeedsIssue"
// An issue has been created in the issue tracker.
// The IssueReference and IssueCreatedAt fields have more information.
TriageStateIssueCreated TriageState = "IssueCreated"
+ // This vulnerability has already been handled under an alias (i.e., a CVE
+ // or GHSA that refers to the same vulnerability).
+ TriageStateAlias TriageState = "Alias"
// The CVE state was changed after the CVE was created.
TriageStateUpdatedSinceIssueCreation TriageState = "UpdatedSinceIssueCreation"
// Although the triager might think this CVE is relevant to Go, it is not.
@@ -113,7 +116,7 @@
// Validate returns an error if the TriageState is not one of the above values.
func (s TriageState) Validate() error {
switch s {
- case TriageStateNoActionNeeded, TriageStateNeedsIssue, TriageStateIssueCreated, TriageStateUpdatedSinceIssueCreation, TriageStateFalsePositive, TriageStateHasVuln:
+ case TriageStateNoActionNeeded, TriageStateNeedsIssue, TriageStateIssueCreated, TriageStateAlias, TriageStateUpdatedSinceIssueCreation, TriageStateFalsePositive, TriageStateHasVuln:
return nil
default:
return fmt.Errorf("bad TriageState %q", s)
diff --git a/internal/worker/update.go b/internal/worker/update.go
index 4f3f3f3..1bfbdff 100644
--- a/internal/worker/update.go
+++ b/internal/worker/update.go
@@ -18,6 +18,7 @@
"golang.org/x/vulndb/internal/cvelistrepo"
"golang.org/x/vulndb/internal/cveschema"
"golang.org/x/vulndb/internal/derrors"
+ "golang.org/x/vulndb/internal/ghsa"
"golang.org/x/vulndb/internal/worker/log"
"golang.org/x/vulndb/internal/worker/store"
)
@@ -269,6 +270,9 @@
pathname := path.Join(f.DirPath, f.Filename)
// If the CVE is not in the database, add it.
if old == nil {
+ // TODO(https://go.dev/issues/54049): Check if there is already
+ // a record in the store for a GHSA that is an alias of this CVE ID,
+ // to avoid creating duplicate issues.
cr := store.NewCVERecord(cve, pathname, f.BlobHash.String(), u.commit)
switch {
case result != nil:
@@ -325,6 +329,8 @@
mp = result.modulePath
}
mod.TriageStateReason = fmt.Sprintf("CVE changed; affected module = %q", mp)
+ case store.TriageStateAlias:
+ // For now, do nothing.
case store.TriageStateHasVuln:
// There is already a Go vuln report for this CVE, so
// nothing to do.
@@ -406,6 +412,38 @@
NumModified int
}
+// triageNewGHSA determines the initial triage state for the GHSA.
+// It checks if we have already handled a CVE associated with this GHSA.
+func triageNewGHSA(sa *ghsa.SecurityAdvisory, tx store.Transaction) (store.TriageState, error) {
+ for _, alias := range sa.Identifiers {
+ if alias.Type == "CVE" {
+ cveID := alias.Value
+ cves, err := tx.GetCVERecords(cveID, cveID)
+ if err != nil {
+ return store.TriageStateNeedsIssue, err
+ }
+ if len(cves) == 0 {
+ continue
+ }
+ switch cves[0].TriageState {
+ case store.TriageStateIssueCreated,
+ store.TriageStateHasVuln, store.TriageStateNeedsIssue, store.TriageStateUpdatedSinceIssueCreation:
+ // The vuln was already covered by the CVE.
+ // TODO(https://go.dev/issues/55303): Add comment to
+ // existing issue with new alias.
+ return store.TriageStateAlias, nil
+ case store.TriageStateFalsePositive, store.TriageStateNoActionNeeded, store.TriageStateAlias:
+ // Create an issue for the GHSA since no issue
+ // was created for the CVE.
+ return store.TriageStateNeedsIssue, nil
+ }
+ }
+ }
+
+ // The GHSA has no associated CVEs.
+ return store.TriageStateNeedsIssue, nil
+}
+
func updateGHSAs(ctx context.Context, listSAs GHSAListFunc, since time.Time, st store.Store) (stats UpdateGHSAStats, err error) {
defer derrors.Wrap(&err, "updateGHSAs(%s)", since)
ctx = event.Start(ctx, "updateGHSAs")
@@ -435,26 +473,29 @@
err = st.RunTransaction(ctx, func(ctx context.Context, tx store.Transaction) error {
numAdded = 0
numModified = 0
- // Read the existing records from the store.
+ // Read the existing GHSA records from the store.
sars, err := tx.GetGHSARecords()
if err != nil {
return err
}
- idToRecord := map[string]*store.GHSARecord{}
+ ghsaIDToRecord := map[string]*store.GHSARecord{}
for _, r := range sars {
- idToRecord[r.GHSA.ID] = r
+ ghsaIDToRecord[r.GHSA.ID] = r
}
// Determine what needs to be added and modified.
for _, sa := range sas {
- old := idToRecord[sa.ID]
+ old := ghsaIDToRecord[sa.ID]
if old == nil {
- // Add record.
+ // ghsa.List already filters for vulns in the Go ecosystem,
+ // so add a record for all found GHSAs.
+ triageState, err := triageNewGHSA(sa, tx)
+ if err != nil {
+ return err
+ }
r := &store.GHSARecord{
- GHSA: sa,
- // All new records need an issue, since ghsa.List already
- // filters for vulns in the Go ecosystem.
- TriageState: store.TriageStateNeedsIssue,
+ GHSA: sa,
+ TriageState: triageState,
}
if err := tx.CreateGHSARecord(r); err != nil {
return err
diff --git a/internal/worker/worker.go b/internal/worker/worker.go
index 383204d..d4465e0 100644
--- a/internal/worker/worker.go
+++ b/internal/worker/worker.go
@@ -66,7 +66,7 @@
return err
}
}
- knownVulnIDs, err := readVulnDB(ctx)
+ knownVulnIDs, err := getAllCVEsAndGHSAsInVulnDB(ctx)
if err != nil {
return err
}
@@ -130,8 +130,9 @@
vulnDBURL = "https://storage.googleapis.com/" + vulnDBBucket
)
-// readVulnDB returns a list of all CVE IDs in the Go vuln DB.
-func readVulnDB(ctx context.Context) ([]string, error) {
+// getAllCVEsAndGHSAsInVulnDB returns a list of all CVE IDs and
+// GHSA IDs in the Go vuln DB.
+func getAllCVEsAndGHSAsInVulnDB(ctx context.Context) ([]string, error) {
const concurrency = 4
client, err := vulnc.NewClient([]string{vulnDBURL}, vulnc.Options{})
@@ -144,8 +145,8 @@
return nil, err
}
var (
- mu sync.Mutex
- cveIDs []string
+ mu sync.Mutex
+ vulnIDs []string
)
sem := make(chan struct{}, concurrency)
g, ctx := errgroup.WithContext(ctx)
@@ -158,9 +159,8 @@
if err != nil {
return err
}
- // Assume all the aliases are CVE IDs.
mu.Lock()
- cveIDs = append(cveIDs, e.Aliases...)
+ vulnIDs = append(vulnIDs, e.Aliases...)
mu.Unlock()
return nil
})
@@ -168,7 +168,7 @@
if err := g.Wait(); err != nil {
return nil, err
}
- return cveIDs, nil
+ return vulnIDs, nil
}
// GHSAListFunc is the type of a function that lists GitHub security advisories.
diff --git a/internal/worker/worker_test.go b/internal/worker/worker_test.go
index 9e0fc93..4beb2ed 100644
--- a/internal/worker/worker_test.go
+++ b/internal/worker/worker_test.go
@@ -160,6 +160,13 @@
},
TriageState: store.TriageStateIssueCreated,
},
+ {
+ GHSA: &ghsa.SecurityAdvisory{
+ ID: "g4",
+ Vulns: []*ghsa.Vuln{{Package: "p4"}},
+ },
+ TriageState: store.TriageStateAlias,
+ },
}
createGHSARecords(t, mstore, grs)
@@ -325,7 +332,8 @@
}
func TestUpdateGHSAs(t *testing.T) {
- ctx := context.Background()
+ ctx := event.WithExporter(context.Background(),
+ event.NewExporter(log.NewLineHandler(os.Stderr), nil))
sas := []*ghsa.SecurityAdvisory{
{
ID: "g1",
@@ -339,6 +347,16 @@
ID: "g3",
UpdatedAt: day(2021, 12, 1),
},
+ {
+ ID: "g4",
+ Identifiers: []ghsa.Identifier{{Type: "CVE", Value: "CVE-2000-1111"}},
+ UpdatedAt: day(2021, 12, 1),
+ },
+ {
+ ID: "g5",
+ Identifiers: []ghsa.Identifier{{Type: "CVE", Value: "CVE-2000-2222"}},
+ UpdatedAt: day(2021, 12, 1),
+ },
}
mstore := store.NewMemStore()
@@ -358,15 +376,43 @@
}
}
- // First run, from an empty store: all SAs entered with NeedsIssue.
+ // Add some existing CVE records.
+ ctime := time.Date(2020, 1, 2, 0, 0, 0, 0, time.UTC)
+ crs := []*store.CVERecord{
+ {
+ ID: "CVE-2000-1111",
+ BlobHash: "bh1",
+ CommitHash: "ch",
+ CommitTime: ctime,
+ Path: "path1",
+ TriageState: store.TriageStateNoActionNeeded,
+ },
+ {
+ ID: "CVE-2000-2222",
+ BlobHash: "bh2",
+ CommitHash: "ch",
+ CommitTime: ctime,
+ Path: "path2",
+ TriageState: store.TriageStateIssueCreated,
+ },
+ }
+ createCVERecords(t, mstore, crs)
+
+ // First four SAs entered with NeedsIssue.
var want []*store.GHSARecord
- for _, sa := range sas {
+ for _, sa := range sas[:4] {
want = append(want, &store.GHSARecord{
GHSA: sa,
TriageState: store.TriageStateNeedsIssue,
})
}
- updateAndCheck(UpdateGHSAStats{3, 3, 0}, want)
+ // SA "g5" entered with Alias state because it is an alias of
+ // "CVE-2000-2222" which already has an issue.
+ want = append(want, &store.GHSARecord{
+ GHSA: sas[4],
+ TriageState: store.TriageStateAlias,
+ })
+ updateAndCheck(UpdateGHSAStats{5, 5, 0}, want)
// New SA added, old one updated.
sas[0] = &ghsa.SecurityAdvisory{
@@ -375,7 +421,7 @@
}
want[0].GHSA = sas[0]
sas = append(sas, &ghsa.SecurityAdvisory{
- ID: "g4",
+ ID: "g6",
UpdatedAt: day(2021, 12, 2),
})
listSAs = fakeListFunc(sas)
