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>

cmd/vulnreport/main.go

@@ -826,12 +826,11 @@ func newCVE(filename string) (err error) {
 
 // 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
 	}

cmd/worker/main.go

@@ -213,8 +213,7 @@ func updateCommand(ctx context.Context, commitHash string) error {
 		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

internal/ghsa/ghsa.go

@@ -149,9 +149,7 @@ func newGitHubClient(ctx context.Context, accessToken string) *githubv4.Client {
 
 // 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 @@ func List(ctx context.Context, accessToken string, since time.Time, withCVE bool
 			return nil, err
 		}
 		for _, sa := range query.SAs.Nodes {
-			if withCVE != isCVE(sa.Identifiers) {
-				continue
-			}
 			if len(sa.Vulnerabilities.Nodes) == 0 {
 				continue
 			}

internal/ghsa/ghsa_test.go

@@ -31,8 +31,7 @@ func TestList(t *testing.T) {
 	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)
 	}

internal/worker/server.go

@@ -109,9 +109,10 @@ func NewServer(ctx context.Context, cfg Config) (_ *Server, err error) {
 		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 @@ func (s *Server) doUpdate(r *http.Request) (err error) {
 		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

internal/worker/store/store.go

@@ -89,19 +89,22 @@ func (r *CVERecord) Validate() error {
 	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 @@ const (
 // 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)

internal/worker/update.go

@@ -18,6 +18,7 @@ import (
 	"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 @@ func (u *cveUpdater) handleCVE(f cvelistrepo.File, old *store.CVERecord, tx stor
 	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 @@ func (u *cveUpdater) handleCVE(f cvelistrepo.File, old *store.CVERecord, tx stor
 			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 @@ type UpdateGHSAStats struct {
 	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 @@ func updateGHSAs(ctx context.Context, listSAs GHSAListFunc, since time.Time, st
 	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

internal/worker/worker.go

@@ -66,7 +66,7 @@ func UpdateCVEsAtCommit(ctx context.Context, repoPath, commitHashString string,
 			return err
 		}
 	}
-	knownVulnIDs, err := readVulnDB(ctx)
+	knownVulnIDs, err := getAllCVEsAndGHSAsInVulnDB(ctx)
 	if err != nil {
 		return err
 	}
@@ -130,8 +130,9 @@ const (
 	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 @@ func readVulnDB(ctx context.Context) ([]string, error) {
 		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,17 +159,16 @@ func readVulnDB(ctx context.Context) ([]string, error) {
 			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
 		})
 	}
 	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.

internal/worker/worker_test.go

@@ -160,6 +160,13 @@ func TestCreateIssues(t *testing.T) {
 			},
 			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 day(year, month, day int) time.Time {
 }
 
 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 @@ func TestUpdateGHSAs(t *testing.T) {
 			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 @@ func TestUpdateGHSAs(t *testing.T) {
 		}
 	}
 
-	// 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 @@ func TestUpdateGHSAs(t *testing.T) {
 	}
 	want[0].GHSA = sas[0]
 	sas = append(sas, &ghsa.SecurityAdvisory{
-		ID:        "g4",
+		ID:        "g6",
 		UpdatedAt: day(2021, 12, 2),
 	})
 	listSAs = fakeListFunc(sas)