internal/report: improve cve5ToReport

- Consider all "affected" blocks instead of just the first one.
- More cleverly account for vendor/product/package data. For example,
ignore it if it is "n/a", or if it is merely a suffix of the module path
we already have.
- Attempt to populate version data.

Skip the test that checks if v4 and v5 are handled equivalently, as
we are now taking into account data that is only available in v5.

Change-Id: Ibf46c2ad77bad6d72b50ed21b136e5ee014a99f8
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/548057
Reviewed-by: Sarawut Wansee <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: tatianab

internal/cveschema5/cveschema5.go

@@ -95,6 +95,9 @@ type VersionRange struct {
 	Fixed       Version       `json:"lessThan"`
 	Status      VersionStatus `json:"status"`
 	VersionType string        `json:"versionType"`
+
+	// Not used in Go CVEs, but supported in the schema.
+	LessThanOrEqual Version `json:"lessThanOrEqual,omitempty"`
 }
 
 type VersionStatus string

internal/report/cve.go

@@ -5,13 +5,15 @@
 package report
 
 import (
+	"fmt"
 	"regexp"
 	"strings"
 
 	"golang.org/x/vulndb/internal/cveschema"
 	"golang.org/x/vulndb/internal/cveschema5"
 	"golang.org/x/vulndb/internal/proxy"
 	"golang.org/x/vulndb/internal/stdlib"
+	"golang.org/x/vulndb/internal/version"
 )
 
 func vendor(modulePath string) string {
@@ -133,38 +135,9 @@ func cve5ToReport(c *cveschema5.CVERecord, id, modulePath string) *Report {
 		refs = append(refs, referenceFromUrl(ref.URL))
 	}
 
-	// For now, use the first product name as the package path.
-	// TODO(tatianabradley): Make this more sophisticated, to consider
-	// all the blocks in cna.Affected, versions, etc.
-	var pkgPath string
-	if affected := cna.Affected; len(affected) > 0 {
-		pkgPath = affected[0].Product
-	}
-	if stdlib.Contains(modulePath) {
-		pkgPath = modulePath
-		modulePath = stdlib.ModulePath
-	}
-	if modulePath == "" {
-		modulePath = "TODO"
-	}
-	if pkgPath == "" {
-		pkgPath = modulePath
-	}
-	modules := []*Module{
-		{
-			Module:   modulePath,
-			Versions: nil,
-			Packages: []*Package{
-				{
-					Package: pkgPath,
-				},
-			},
-		},
-	}
-
 	r := &Report{
 		ID:          id,
-		Modules:     modules,
+		Modules:     affectedToModules(cna.Affected, modulePath),
 		Summary:     Summary(cna.Title),
 		Description: description,
 		Credits:     credits,
@@ -185,3 +158,129 @@ func getCWE5(c *cveschema5.CNAPublishedContainer) string {
 func isGoCNA5(c *cveschema5.CNAPublishedContainer) bool {
 	return c.ProviderMetadata.OrgID == GoOrgUUID
 }
+
+func affectedToModules(as []cveschema5.Affected, modulePath string) []*Module {
+	// Use a placeholder module if there is no information on
+	// modules/packages in the CVE.
+	if len(as) == 0 {
+		return []*Module{{
+			Module: modulePath,
+		}}
+	}
+
+	var modules []*Module
+	for _, a := range as {
+		modules = append(modules, affectedToModule(&a, modulePath))
+	}
+
+	return modules
+}
+
+func affectedToModule(a *cveschema5.Affected, modulePath string) *Module {
+	var pkgPath string
+	isSet := func(s string) bool {
+		const na = "n/a"
+		return s != "" && s != na
+	}
+	switch {
+	case isSet(a.PackageName):
+		pkgPath = a.PackageName
+	case isSet(a.Product):
+		pkgPath = a.Product
+	case isSet(a.Vendor):
+		pkgPath = a.Vendor
+	default:
+		pkgPath = modulePath
+	}
+
+	// If the package path is just a suffix of the modulePath,
+	// it is probably not useful.
+	if strings.HasSuffix(modulePath, pkgPath) {
+		pkgPath = modulePath
+	}
+
+	if stdlib.Contains(pkgPath) {
+		if strings.HasPrefix(pkgPath, stdlib.ToolchainModulePath) {
+			modulePath = stdlib.ToolchainModulePath
+		} else {
+			modulePath = stdlib.ModulePath
+		}
+	}
+
+	var symbols []string
+	for _, s := range a.ProgramRoutines {
+		symbols = append(symbols, s.Name)
+	}
+
+	vs, uvs := convertVersions(a.Versions, a.DefaultStatus)
+
+	return &Module{
+		Module:              modulePath,
+		Versions:            vs,
+		UnsupportedVersions: uvs,
+		Packages: []*Package{
+			{
+				Package: pkgPath,
+				Symbols: symbols,
+				GOOS:    a.Platforms,
+			},
+		},
+	}
+}
+
+func convertVersions(vrs []cveschema5.VersionRange, defaultStatus cveschema5.VersionStatus) (vs []VersionRange, uvs []UnsupportedVersion) {
+	for _, vr := range vrs {
+		// Version ranges starting with "n/a" don't have any meaningful data.
+		if vr.Introduced == "n/a" {
+			continue
+		}
+		v, ok := toVersionRange(&vr, defaultStatus)
+		if ok {
+			vs = append(vs, *v)
+			continue
+		}
+		uvs = append(uvs, toUnsupported(&vr, defaultStatus))
+	}
+	return vs, uvs
+}
+
+func toVersionRange(cvr *cveschema5.VersionRange, defaultStatus cveschema5.VersionStatus) (*VersionRange, bool) {
+	// For now, we only support version ranges that are easy to convert to our format.
+	if cvr.VersionType != typeSemver ||
+		cvr.LessThanOrEqual != "" ||
+		!version.IsValid(string(cvr.Introduced)) ||
+		!version.IsValid(string(cvr.Fixed)) ||
+		cvr.Status != cveschema5.StatusAffected ||
+		defaultStatus != cveschema5.StatusUnaffected {
+		return nil, false
+	}
+
+	introduced := string(cvr.Introduced)
+	if introduced == "0" {
+		introduced = ""
+	}
+
+	return &VersionRange{
+		Introduced: introduced,
+		Fixed:      string(cvr.Fixed),
+	}, true
+}
+
+func toUnsupported(cvr *cveschema5.VersionRange, defaultStatus cveschema5.VersionStatus) UnsupportedVersion {
+	var version string
+	switch {
+	case cvr.Fixed != "":
+		version = fmt.Sprintf("%s from %s before %s", cvr.Status, cvr.Introduced, cvr.Fixed)
+	case cvr.LessThanOrEqual != "":
+		version = fmt.Sprintf("%s from %s to %s", cvr.Status, cvr.Introduced, cvr.Fixed)
+	default:
+		version = fmt.Sprintf("%s at %s", cvr.Status, cvr.Introduced)
+	}
+	if defaultStatus != "" {
+		version = fmt.Sprintf("%s (default: %s)", version, defaultStatus)
+	}
+	return UnsupportedVersion{
+		Version: version,
+		Type:    "cve_version_range",
+	}
+}

internal/report/cve_test.go

@@ -104,10 +104,12 @@ func TestCVE5ToReport(t *testing.T) {
 	}
 }
 
-// Check that the created report is the same for v4 and v5.
-// This is a transitional test that will be removed once we are OK
-// with divergence between v4 and v5, or if we remove support for v4 entirely.
 func TestV4V5Equivalence(t *testing.T) {
+	// Skip, but leave the test in case it is needed in the course of
+	// the transition.
+	// TODO(tatianabradley): Delete this test once we have completed the
+	// transition to V5.
+	t.Skip("V4 and V5 are no longer required to be equivalent.")
 	if err := filepath.WalkDir(filepath.Join(testdata, "TestCVE5ToReport"), func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err

internal/report/testdata/cve/TestCVE5ToReport/CVE-2020-9283.txtar

@@ -9,7 +9,7 @@ id: PLACEHOLDER-ID
 modules:
     - module: golang.org/x/crypto
       packages:
-        - package: n/a
+        - package: golang.org/x/crypto
 description: |
     golang.org/x/crypto before v0.0.0-20200220183623-bac4c82f6975 for Go allows a panic during signature verification in the golang.org/x/crypto/ssh package. A client can attack an SSH server that accepts public keys. Also, a server can attack any SSH client.
 cves:

internal/report/testdata/cve/TestCVE5ToReport/CVE-2021-3115.txtar

@@ -7,7 +7,7 @@ Expected output of TestCVE5ToReport/CVE-2021-3115.
 -- CVE-2021-3115 --
 id: PLACEHOLDER-ID
 modules:
-    - module: std
+    - module: cmd
       packages:
         - package: cmd/go
 description: |

internal/report/testdata/cve/TestCVE5ToReport/CVE-2022-39213.txtar

@@ -8,8 +8,11 @@ Expected output of TestCVE5ToReport/CVE-2022-39213.
 id: PLACEHOLDER-ID
 modules:
     - module: github.com/pandatix/go-cvss
+      unsupported_versions:
+        - version: affected at >= 0.2.0, < 0.4.0
+          type: cve_version_range
       packages:
-        - package: go-cvss
+        - package: github.com/pandatix/go-cvss
 summary: Out-of-bounds Read in go-cvss
 description: |
     go-cvss is a Go module to manipulate Common Vulnerability Scoring System (CVSS). In affected versions when a full CVSS v2.0 vector string is parsed using `ParseVector`, an Out-of-Bounds Read is possible due to a lack of tests. The Go module will then panic. The problem is patched in tag `v0.4.0`, by the commit `d9d478ff0c13b8b09ace030db9262f3c2fe031f4`. Users are advised to upgrade. Users unable to upgrade may avoid this issue by parsing only CVSS v2.0 vector strings that do not have all attributes defined (e.g. `AV:N/AC:L/Au:N/C:P/I:P/A:C/E:U/RL:OF/RC:C/CDP:MH/TD:H/CR:M/IR:M/AR:M`). As stated in [SECURITY.md](https://github.com/pandatix/go-cvss/blob/master/SECURITY.md), the CPE v2.3 to refer to this Go module is `cpe:2.3:a:pandatix:go_cvss:*:*:*:*:*:*:*:*`. The entry has already been requested to the NVD CPE dictionary.

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-29407.txtar

@@ -8,8 +8,14 @@ Expected output of TestCVE5ToReport/CVE-2023-29407.
 id: PLACEHOLDER-ID
 modules:
     - module: golang.org/x/image
+      versions:
+        - fixed: 0.10.0
       packages:
         - package: golang.org/x/image/tiff
+          symbols:
+            - newDecoder
+            - Decode
+            - DecodeConfig
 summary: Excessive CPU consumption when decoding 0-height images in golang.org/x/image/tiff
 description: |
     A maliciously-crafted image can cause excessive CPU consumption in decoding. A tiled image with a height of 0 and a very large width can cause excessive CPU consumption, despite the image size (width * height) appearing to be zero.

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-44378.txtar

@@ -8,8 +8,11 @@ Expected output of TestCVE5ToReport/CVE-2023-44378.
 id: PLACEHOLDER-ID
 modules:
     - module: github.com/Consensys/gnark
+      unsupported_versions:
+        - version: affected at < 0.9.0
+          type: cve_version_range
       packages:
-        - package: gnark
+        - package: github.com/Consensys/gnark
 summary: gnark vulnerable to unsoundness in variable comparison/non-unique binary decomposition
 description: |
     gnark is a zk-SNARK library that offers a high-level API to design circuits. Prior to version 0.9.0, for some in-circuit values, it is possible to construct two valid decomposition to bits. In addition to the canonical decomposition of `a`, for small values there exists a second decomposition for `a+r` (where `r` is the modulus the values are being reduced by). The second decomposition was possible due to overflowing the field where the values are defined. Upgrading to version 0.9.0 should fix the issue without needing to change the calls to value comparison methods.

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45141.txtar

@@ -8,8 +8,11 @@ Expected output of TestCVE5ToReport/CVE-2023-45141.
 id: PLACEHOLDER-ID
 modules:
     - module: github.com/gofiber/fiber
+      unsupported_versions:
+        - version: affected at < 2.50.0
+          type: cve_version_range
       packages:
-        - package: fiber
+        - package: github.com/gofiber/fiber
 summary: CSRF Token Validation Vulnerability in fiber
 description: |
     Fiber is an express inspired web framework written in Go. A Cross-Site Request Forgery (CSRF) vulnerability has been identified in the application, which allows an attacker to obtain tokens and forge malicious requests on behalf of a user. This can lead to unauthorized actions being taken on the user's behalf, potentially compromising the security and integrity of the application. The vulnerability is caused by improper validation and enforcement of CSRF tokens within the application. This vulnerability has been addressed in version 2.50.0 and users are advised to upgrade. Users should take additional security measures like captchas or Two-Factor Authentication (2FA) and set Session cookies with SameSite=Lax or SameSite=Secure, and the Secure and HttpOnly attributes.

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45283.txtar

@@ -8,8 +8,67 @@ Expected output of TestCVE5ToReport/CVE-2023-45283.
 id: PLACEHOLDER-ID
 modules:
     - module: std
+      versions:
+        - fixed: 1.20.11
+        - introduced: 1.21.0-0
+          fixed: 1.21.4
       packages:
         - package: path/filepath
+          goos:
+            - windows
+          symbols:
+            - Clean
+            - volumeNameLen
+            - join
+            - Abs
+            - Base
+            - Dir
+            - EvalSymlinks
+            - Glob
+            - IsLocal
+            - Join
+            - Rel
+            - Split
+            - VolumeName
+            - Walk
+            - WalkDir
+    - module: std
+      versions:
+        - fixed: 1.20.11
+        - introduced: 1.21.0-0
+          fixed: 1.21.4
+      packages:
+        - package: internal/safefilepath
+          goos:
+            - windows
+          symbols:
+            - fromFS
+            - FromFS
+    - module: std
+      versions:
+        - introduced: 1.20.11
+          fixed: 1.20.12
+        - introduced: 1.21.4
+          fixed: 1.21.5
+      packages:
+        - package: path/filepath
+          goos:
+            - windows
+          symbols:
+            - volumeNameLen
+            - Abs
+            - Base
+            - Clean
+            - Dir
+            - EvalSymlinks
+            - Glob
+            - IsLocal
+            - Join
+            - Rel
+            - Split
+            - VolumeName
+            - Walk
+            - WalkDir
 summary: Insecure parsing of Windows paths with a \??\ prefix in path/filepath
 description: |
     The filepath package does not recognize paths with a \??\ prefix as special. On Windows, a path beginning with \??\ is a Root Local Device path equivalent to a path beginning with \\?\. Paths with a \??\ prefix may be used to access arbitrary locations on the system. For example, the path \??\c:\x is equivalent to the more common path c:\x. Before fix, Clean could convert a rooted path such as \a\..\??\b into the root local device path \??\b. Clean will now convert this to .\??\b. Similarly, Join(\, ??, b) could convert a seemingly innocent sequence of path elements into the root local device path \??\b. Join will now convert this to \.\??\b. In addition, with fix, IsAbs now correctly reports paths beginning with \??\ as absolute, and VolumeName correctly reports the \??\ prefix as a volume name. UPDATE: Go 1.20.11 and Go 1.21.4 inadvertently changed the definition of the volume name in Windows paths starting with \?, resulting in filepath.Clean(\?\c:) returning \?\c: rather than \?\c:\ (among other effects). The previous behavior has been restored.

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45285.txtar

@@ -7,7 +7,11 @@ Expected output of TestCVE5ToReport/CVE-2023-45285.
 -- CVE-2023-45285 --
 id: PLACEHOLDER-ID
 modules:
-    - module: std
+    - module: cmd
+      versions:
+        - fixed: 1.20.12
+        - introduced: 1.21.0-0
+          fixed: 1.21.5
       packages:
         - package: cmd/go
 summary: Command 'go get' may unexpectedly fallback to insecure git in cmd/go

internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45286.txtar

@@ -8,8 +8,23 @@ Expected output of TestCVE5ToReport/CVE-2023-45286.
 id: PLACEHOLDER-ID
 modules:
     - module: github.com/go-resty/resty/v2
+      unsupported_versions:
+        - version: 'unaffected from 0 before 2.10.0 (default: affected)'
+          type: cve_version_range
       packages:
         - package: github.com/go-resty/resty/v2
+          symbols:
+            - handleRequestBody
+            - Backoff
+            - Request.Delete
+            - Request.Execute
+            - Request.Get
+            - Request.Head
+            - Request.Options
+            - Request.Patch
+            - Request.Post
+            - Request.Put
+            - Request.Send
 summary: HTTP request body disclosure in github.com/go-resty/resty/v2
 description: |
     A race condition in go-resty can result in HTTP request body disclosure across requests. This condition can be triggered by calling sync.Pool.Put with the same *bytes.Buffer more than once, when request retries are enabled and a retry occurs. The call to sync.Pool.Get will then return a bytes.Buffer that hasn't had bytes.Buffer.Reset called on it. This dirty buffer will contain the HTTP request body from an unrelated request, and go-resty will append the current HTTP request body to it, sending two bodies in one request. The sync.Pool in question is defined at package level scope, so a completely unrelated server could receive the request body.