Sanity check to prevent duplicate commits (#2127)

This adds a check for the same commit being added to more than once when
converting versions to commits.

#1984 surfaced that OpenSSL CVEs are being converted with the same
commit as both `introduced` and `fixed` (due to incorrect normalization
of OpenSSL versions). While the underlying defect is being addressed,
add some sanity checking so that if the same commit hash can't be added
to a GIT range being generated when converting versions to commits.

Author: andrewpollock

vulnfeeds/cmd/nvd-cve-osv/main.go

@@ -131,6 +131,7 @@ func InScopeGitRepo(repoURL string) bool {
 // Examines repos and tries to convert versions to commits by treating them as Git tags.
 // Takes a CVE ID string (for logging), cves.VersionInfo with AffectedVersions and
 // typically no AffectedCommits and attempts to add AffectedCommits (including Fixed commits) where there aren't any.
+// Refuses to add the same commit to AffectedCommits more than once.
 func GitVersionsToCommits(CVE cves.CVEID, versions cves.VersionInfo, repos []string, cache git.RepoTagsCache) (v cves.VersionInfo, e error) {
 	// versions is a VersionInfo with AffectedVersions and typically no AffectedCommits
 	// v is a VersionInfo with AffectedCommits (containing Fixed commits) included
@@ -148,7 +149,11 @@ func GitVersionsToCommits(CVE cves.CVEID, versions cves.VersionInfo, repos []str
 					Logger.Warnf("[%s]: Failed to get a Git commit for introduced version %q from %q: %v", CVE, av.Introduced, repo, err)
 				} else {
 					Logger.Infof("[%s]: Successfully derived %+v for introduced version %q", CVE, ac, av.Introduced)
-					v.AffectedCommits = append(v.AffectedCommits, ac)
+					if v.Duplicated(ac) {
+						Logger.Warnf("[%s]: Duplicate: %#v already present in %#v", CVE, ac, v)
+					} else {
+						v.AffectedCommits = append(v.AffectedCommits, ac)
+					}
 				}
 			}
 			// Only try and convert fixed versions to commits via tags if there aren't any Fixed commits already.
@@ -163,7 +168,11 @@ func GitVersionsToCommits(CVE cves.CVEID, versions cves.VersionInfo, repos []str
 					Logger.Warnf("[%s]: Failed to get a Git commit for fixed version %q from %q: %v", CVE, av.Fixed, repo, err)
 				} else {
 					Logger.Infof("[%s]: Successfully derived %+v for fixed version %q", CVE, ac, av.Fixed)
-					v.AffectedCommits = append(v.AffectedCommits, ac)
+					if v.Duplicated(ac) {
+						Logger.Warnf("[%s]: Duplicate: %#v already present in %#v", CVE, ac, v)
+					} else {
+						v.AffectedCommits = append(v.AffectedCommits, ac)
+					}
 				}
 			}
 			// Only try and convert last_affected versions to commits via tags if there aren't any Fixed commits already (to maintain schema compliance).
@@ -175,7 +184,11 @@ func GitVersionsToCommits(CVE cves.CVEID, versions cves.VersionInfo, repos []str
 					Logger.Warnf("[%s]: Failed to get a Git commit for last_affected version %q from %q: %v", CVE, av.LastAffected, repo, err)
 				} else {
 					Logger.Infof("[%s]: Successfully derived %+v for last_affected version %q", CVE, ac, av.LastAffected)
-					v.AffectedCommits = append(v.AffectedCommits, ac)
+					if v.Duplicated(ac) {
+						Logger.Warnf("[%s]: Duplicate: %#v already present in %#v", CVE, ac, v)
+					} else {
+						v.AffectedCommits = append(v.AffectedCommits, ac)
+					}
 				}
 			}
 		}

vulnfeeds/cves/versions.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"net/url"
 	"path"
+	"reflect"
 	"regexp"
 	"strings"
 	"time"
@@ -88,50 +89,82 @@ func (vi *VersionInfo) HasLastAffectedVersions() bool {
 }
 
 func (vi *VersionInfo) HasIntroducedCommits(repo string) bool {
-	for _, av := range vi.AffectedCommits {
-		if av.Repo == repo && av.Introduced != "" {
+	for _, ac := range vi.AffectedCommits {
+		if ac.Repo == repo && ac.Introduced != "" {
 			return true
 		}
 	}
 	return false
 }
 
 func (vi *VersionInfo) HasFixedCommits(repo string) bool {
-	for _, av := range vi.AffectedCommits {
-		if av.Repo == repo && av.Fixed != "" {
+	for _, ac := range vi.AffectedCommits {
+		if ac.Repo == repo && ac.Fixed != "" {
 			return true
 		}
 	}
 	return false
 }
 
 func (vi *VersionInfo) HasLastAffectedCommits(repo string) bool {
-	for _, av := range vi.AffectedCommits {
-		if av.Repo == repo && av.LastAffected != "" {
+	for _, ac := range vi.AffectedCommits {
+		if ac.Repo == repo && ac.LastAffected != "" {
 			return true
 		}
 	}
 	return false
 }
 
 func (vi *VersionInfo) FixedCommits(repo string) (FixedCommits []string) {
-	for _, av := range vi.AffectedCommits {
-		if av.Repo == repo && av.Fixed != "" {
-			FixedCommits = append(FixedCommits, av.Fixed)
+	for _, ac := range vi.AffectedCommits {
+		if ac.Repo == repo && ac.Fixed != "" {
+			FixedCommits = append(FixedCommits, ac.Fixed)
 		}
 	}
 	return FixedCommits
 }
 
 func (vi *VersionInfo) LastAffectedCommits(repo string) (LastAffectedCommits []string) {
-	for _, av := range vi.AffectedCommits {
-		if av.Repo == repo && av.LastAffected != "" {
-			LastAffectedCommits = append(LastAffectedCommits, av.Fixed)
+	for _, ac := range vi.AffectedCommits {
+		if ac.Repo == repo && ac.LastAffected != "" {
+			LastAffectedCommits = append(LastAffectedCommits, ac.Fixed)
 		}
 	}
 	return LastAffectedCommits
 }
 
+// Check if the same commit appears in multiple fields of the AffectedCommits array.
+// See https://github.com/google/osv.dev/issues/1984 for more context.
+func (vi *VersionInfo) Duplicated(candidate AffectedCommit) bool {
+	fieldsToCheck := []string{"Introduced", "LastAffected", "Limit", "Fixed"}
+
+	// Get the commit hash to look for.
+	v := reflect.ValueOf(&candidate).Elem()
+
+	commit := ""
+	for _, field := range fieldsToCheck {
+		commit = v.FieldByName(field).String()
+		if commit != "" {
+			break
+		}
+	}
+	if commit == "" {
+		return false
+	}
+
+	// Look through what is already present.
+	for _, ac := range vi.AffectedCommits {
+		v = reflect.ValueOf(&ac).Elem()
+		for _, field := range fieldsToCheck {
+			existingCommit := v.FieldByName(field).String()
+			if existingCommit == commit {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // Synthetic enum of supported commit types.
 type CommitType int
 
@@ -492,18 +525,18 @@ var (
 )
 
 func repoGitWeb(parsedURL *url.URL) (string, error) {
-		params := strings.Split(parsedURL.RawQuery, ";")
-		for _, param := range params {
-			if !strings.HasPrefix(param, "p=") {
-				continue
-			}
-			repo, err := url.JoinPath(strings.TrimSuffix(strings.TrimSuffix(parsedURL.Path, "/gitweb.cgi"), "cgi-bin"), strings.Split(param, "=")[1])
-			if err != nil {
-				return "", err
-			}
-			return fmt.Sprintf("git://%s%s", parsedURL.Hostname(), repo), nil
+	params := strings.Split(parsedURL.RawQuery, ";")
+	for _, param := range params {
+		if !strings.HasPrefix(param, "p=") {
+			continue
 		}
-		return "", fmt.Errorf("unsupported GitWeb URL: %s", parsedURL.String())
+		repo, err := url.JoinPath(strings.TrimSuffix(strings.TrimSuffix(parsedURL.Path, "/gitweb.cgi"), "cgi-bin"), strings.Split(param, "=")[1])
+		if err != nil {
+			return "", err
+		}
+		return fmt.Sprintf("git://%s%s", parsedURL.Hostname(), repo), nil
+	}
+	return "", fmt.Errorf("unsupported GitWeb URL: %s", parsedURL.String())
 }
 
 // Returns the base repository URL for supported repository hosts.