internal/report: add quick fix for module merging issue
Previously, FixModules would attempt to merge versions even if it
resulted in an invalid version list (e.g., with overlapping ranges).
As a quick fix, bail out from the merge attempt if any of the
results wouldn't make sense, and add a note to the report.
This can be improved in the future by continuing to attempt to merge
modules that can be merged instead of bailing out early.
Change-Id: I5074cd3d04251423da9afa1a52933f0fbeab4b5f
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/580435
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/genericosv/testdata/yaml/GHSA-pg5p-wwp8-97g8.yaml b/internal/genericosv/testdata/yaml/GHSA-pg5p-wwp8-97g8.yaml
index 3e0017a..7f2007e 100644
--- a/internal/genericosv/testdata/yaml/GHSA-pg5p-wwp8-97g8.yaml
+++ b/internal/genericosv/testdata/yaml/GHSA-pg5p-wwp8-97g8.yaml
@@ -3,15 +3,23 @@
- module: github.com/cilium/cilium
versions:
- introduced: 1.7.0
- - introduced: 1.11.0
- fixed: 1.11.16
- - introduced: 1.12.0
- fixed: 1.12.9
- - introduced: 1.13.0
- fixed: 1.13.2
unsupported_versions:
- version: 1.10.0
type: last_affected
+ - module: github.com/cilium/cilium
+ versions:
+ - introduced: 1.11.0
+ fixed: 1.11.16
+ vulnerable_at: 1.11.15
+ - module: github.com/cilium/cilium
+ versions:
+ - introduced: 1.12.0
+ fixed: 1.12.9
+ vulnerable_at: 1.12.8
+ - module: github.com/cilium/cilium
+ versions:
+ - introduced: 1.13.0
+ fixed: 1.13.2
vulnerable_at: 1.13.1
summary: Debug mode leaks confidential data in Cilium in github.com/cilium/cilium
description: |-
@@ -60,9 +68,9 @@
references:
- advisory: https://github.com/cilium/cilium/security/advisories/GHSA-pg5p-wwp8-97g8
notes:
+ - fix: 'module merge error: could not merge versions of module github.com/cilium/cilium: introduced and fixed versions must alternate'
- lint: 'description: possible markdown formatting (found ### )'
- lint: 'description: possible markdown formatting (found [Slack](https://docs.cilium.io/en/latest/community/community/#slack))'
- lint: 'modules[0] "github.com/cilium/cilium": unsupported_versions: found 1 (want none)'
- - lint: 'modules[0] "github.com/cilium/cilium": versions: introduced and fixed versions must alternate'
source:
id: GHSA-pg5p-wwp8-97g8
diff --git a/internal/report/fix.go b/internal/report/fix.go
index fb36aa0..a2dec4e 100644
--- a/internal/report/fix.go
+++ b/internal/report/fix.go
@@ -17,6 +17,7 @@
"golang.org/x/vulndb/internal/cveschema5"
"golang.org/x/vulndb/internal/ghsa"
"golang.org/x/vulndb/internal/osv"
+ "golang.org/x/vulndb/internal/osvutils"
"golang.org/x/vulndb/internal/proxy"
"golang.org/x/vulndb/internal/version"
)
@@ -86,9 +87,15 @@
}
m.VulnerableAt = fixVersion(m.VulnerableAt)
- sort.SliceStable(m.Versions, func(i, j int) bool {
- intro, fixed := m.Versions[i].Introduced, m.Versions[i].Fixed
- intro2, fixed2 := m.Versions[j].Introduced, m.Versions[j].Fixed
+ m.Versions = fixVersionRanges(m.Versions)
+
+ m.fixVulnerableAt(pc)
+}
+
+func fixVersionRanges(vrs []VersionRange) []VersionRange {
+ sort.SliceStable(vrs, func(i, j int) bool {
+ intro, fixed := vrs[i].Introduced, vrs[i].Fixed
+ intro2, fixed2 := vrs[j].Introduced, vrs[j].Fixed
switch {
case intro != "" && intro2 != "":
return version.Before(intro, intro2)
@@ -104,25 +111,25 @@
})
// Remove duplicate version ranges.
- m.Versions = slices.Compact(m.Versions)
+ vrs = slices.Compact(vrs)
// Collect together version ranges that don't need to be separate,
// e.g:
// [ {Introduced: 1.1.0}, {Fixed: 1.2.0} ] becomes
// [ {Introduced: 1.1.0, Fixed: 1.2.0} ].
- for i := 0; i < len(m.Versions); i++ {
+ for i := 0; i < len(vrs); i++ {
if i != 0 {
- current, prev := m.Versions[i], m.Versions[i-1]
+ current, prev := vrs[i], vrs[i-1]
if (prev.Introduced != "" && prev.Fixed == "") &&
(current.Introduced == "" && current.Fixed != "") {
- m.Versions[i-1].Fixed = current.Fixed
- m.Versions = append(m.Versions[:i], m.Versions[i+1:]...)
+ vrs[i-1].Fixed = current.Fixed
+ vrs = append(vrs[:i], vrs[i+1:]...)
i--
}
}
}
- m.fixVulnerableAt(pc)
+ return vrs
}
func (m *Module) fixVulnerableAt(pc *proxy.Client) {
@@ -265,7 +272,12 @@
canonicalize(m, pc)
}
- r.Modules = merge(r.Modules)
+ merged, err := merge(r.Modules)
+ if err != nil {
+ r.AddNote(NoteTypeFix, "module merge error: %s", err)
+ } else {
+ r.Modules = merged
+ }
// Fix the versions *after* the modules have been merged.
for _, m := range r.Modules {
@@ -487,7 +499,7 @@
// merge merges all modules with the same module & package info
// (but possibly different versions) into one.
-func merge(ms []*Module) []*Module {
+func merge(ms []*Module) ([]*Module, error) {
type compMod struct {
path string
packages string // sorted, comma separated list of package names
@@ -504,15 +516,19 @@
}
}
- merge := func(m1, m2 *Module) *Module {
- // only run if m1 and m2 are same except versions
- // deletes vulnerable_at if set
+ // only run if m1 and m2 are same except versions
+ // deletes vulnerable_at if set
+ merge := func(m1, m2 *Module) (*Module, error) {
+ versions, err := mergeVersionRanges(m1.Versions, m2.Versions)
+ if err != nil {
+ return nil, fmt.Errorf("could not merge versions of module %s: %w", m1.Module, err)
+ }
return &Module{
Module: m1.Module,
- Versions: append(m1.Versions, m2.Versions...),
+ Versions: versions,
UnsupportedVersions: append(m1.UnsupportedVersions, m2.UnsupportedVersions...),
Packages: m1.Packages,
- }
+ }, nil
}
modules := make(map[compMod]*Module)
@@ -522,11 +538,27 @@
if !ok {
modules[c] = m
} else {
- modules[c] = merge(mod, m)
+ merged, err := merge(mod, m)
+ if err != nil {
+ // For now, bail out if any module can't be merged.
+ // This could be improved by continuing to try even if
+ // some merges fail.
+ return nil, err
+ }
+ modules[c] = merged
}
}
- return maps.Values(modules)
+ return maps.Values(modules), nil
+}
+
+func mergeVersionRanges(v1 []VersionRange, v2 []VersionRange) ([]VersionRange, error) {
+ v := append(v1, v2...)
+ v = fixVersionRanges(v)
+ if err := osvutils.ValidateRanges(AffectedRanges(v)); err != nil {
+ return nil, err
+ }
+ return v, nil
}
func first(vrs []VersionRange) string {
diff --git a/internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45283.txtar b/internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45283.txtar
index b87d895..1b3fa7a 100644
--- a/internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45283.txtar
+++ b/internal/report/testdata/cve/TestCVE5ToReport/CVE-2023-45283.txtar
@@ -22,12 +22,8 @@
- module: std
versions:
- fixed: 1.20.11
- - introduced: 1.20.11
- fixed: 1.20.12
- introduced: 1.21.0-0
fixed: 1.21.4
- - introduced: 1.21.4
- fixed: 1.21.5
packages:
- package: path/filepath
goos:
@@ -48,6 +44,31 @@
- VolumeName
- Walk
- WalkDir
+ - 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
@@ -75,5 +96,7 @@
cve_metadata:
id: CVE-2023-45283
cwe: 'CWE-41: Improper Resolution of Path Equivalence'
+notes:
+ - fix: 'module merge error: could not merge versions of module std: range events must be in strictly ascending order (found 1.20.11>=1.20.11)'
source:
id: CVE-2023-45283
