Skip to content

Commit

Permalink
internal/scan: suggest earliest valid fixed version as the fix
Browse files Browse the repository at this point in the history 
Instead of the latest fix, suggest the earliest fixed version following
the current version in use. One caveat is that, in theory, the earliest
fix can also be vulnerable, e.g., a different symbol for the same
vulnerability can be present at a different range that contains the
earliest fix. We hence return the earliest fix for a module that is not
subsumed by other vulnerable ranges for the same module.

Fixes golang/go#62276

Change-Id: Idaa01c6d6e18cbc26f2b95ac745917f6a575cb9a
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/530679
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
@zpavlinovic
zpavlinovic committed Oct 2, 2023
1 parent a67dfbc commit 6987ccb
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 60 deletions.
4 changes: 2 additions & 2 deletions cmd/govulncheck/testdata/source_stdlib_text.ct
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Vulnerability #1: GO-2022-0969
More info: https://pkg.go.dev/vuln/GO-2022-0969
Standard library
Found in: net/http@go1.18
Fixed in: net/http@go1.19.1
Fixed in: net/http@go1.18.6
Example traces found:
#1: .../stdlib.go:17:31: stdlib.main calls http.ListenAndServe

Expand All @@ -30,7 +30,7 @@ Vulnerability #1: GO-2022-0969
More info: https://pkg.go.dev/vuln/GO-2022-0969
Standard library
Found in: net/http@go1.18
Fixed in: net/http@go1.19.1
Fixed in: net/http@go1.18.6
Example traces found:
#1: for function net/http.ListenAndServe
.../stdlib.go:17:31: golang.org/stdlib.main
Expand Down
4 changes: 2 additions & 2 deletions internal/scan/source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func emitResult(handler govulncheck.Handler, vr *vulncheck.Result, callstacks ma
seen := map[string]bool{}
for _, vv := range vr.Vulns {
osvs[vv.OSV.ID] = vv.OSV
fixed := fixedVersion(vv.ImportSink.Module.Path, vv.OSV.Affected)
fixed := fixedVersion(modPath(vv.ImportSink.Module), modVersion(vv.ImportSink.Module), vv.OSV.Affected)
stack := callstacks[vv]
if stack == nil {
continue
Expand All @@ -85,7 +85,7 @@ func emitResult(handler govulncheck.Handler, vr *vulncheck.Result, callstacks ma
emitted[vv.OSV.ID] = true
emitFinding(handler, osvs, seen, &govulncheck.Finding{
OSV: vv.OSV.ID,
FixedVersion: fixedVersion(vv.ImportSink.Module.Path, vv.OSV.Affected),
FixedVersion: fixedVersion(modPath(vv.ImportSink.Module), modPath(vv.ImportSink.Module), vv.OSV.Affected),
Trace: []*govulncheck.Frame{frameFromPackage(vv.ImportSink)},
})
}
Expand Down
103 changes: 82 additions & 21 deletions internal/scan/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ package scan

import (
"fmt"
"sort"
"strings"

"golang.org/x/tools/go/packages"
"golang.org/x/vuln/internal"
"golang.org/x/vuln/internal/govulncheck"
"golang.org/x/vuln/internal/osv"
isem "golang.org/x/vuln/internal/semver"
"golang.org/x/vuln/internal/semver"
)

// validateFindings checks that the supplied findings all obey the protocol
Expand Down Expand Up @@ -38,33 +41,77 @@ func validateFindings(findings ...*govulncheck.Finding) error {
return nil
}

// latestFixed returns the latest fixed version in the list of affected ranges,
// or the empty string if there are no fixed versions.
func latestFixed(modulePath string, as []osv.Affected) string {
v := ""
for _, a := range as {
if modulePath != a.Module.Path {
continue
func fixedVersion(modulePath, version string, affected []osv.Affected) string {
fixed := earliestValidFix(modulePath, version, affected)
// Add "v" prefix if one does not exist. moduleVersionString
// will later on replace it with "go" if needed.
if fixed != "" && !strings.HasPrefix(fixed, "v") {
fixed = "v" + fixed
}
return fixed
}

// earliestValidFix returns the earliest fix for version of modulePath that
// itself is not vulnerable in affected.
//
// Suppose we have a version "v1.0.0" and we use {...} to denote different
// affected regions. Assume for simplicity that all affected apply to the
// same input modulePath.
//
// {[v0.1.0, v0.1.9), [v1.0.0, v2.0.0)} -> v2.0.0
// {[v1.0.0, v1.5.0), [v2.0.0, v2.1.0}, {[v1.4.0, v1.6.0)} -> v2.1.0
func earliestValidFix(modulePath, version string, affected []osv.Affected) string {
var moduleAffected []osv.Affected
for _, a := range affected {
if a.Module.Path == modulePath {
moduleAffected = append(moduleAffected, a)
}
fixed := isem.NonSupersededFix(a.Ranges)
// Special case: if there is any affected block for this module
// with no fix, the module is considered unfixed.
if fixed == "" {
return ""
}

vFixes := validFixes(version, moduleAffected)
for _, fix := range vFixes {
if !fixNegated(fix, moduleAffected) {
return fix
}
if isem.Less(v, fixed) {
v = fixed
}
return ""

}

// validFixes computes all fixes for version in affected and
// returns them sorted increasingly. Assumes that all affected
// apply to the same module.
func validFixes(version string, affected []osv.Affected) []string {
var fixes []string
for _, a := range affected {
for _, r := range a.Ranges {
if r.Type != osv.RangeTypeSemver {
continue
}
for _, e := range r.Events {
fix := e.Fixed
if fix != "" && semver.Less(version, fix) {
fixes = append(fixes, fix)
}
}
}
}
return v
sort.SliceStable(fixes, func(i, j int) bool { return semver.Less(fixes[i], fixes[j]) })
return fixes
}

func fixedVersion(modulePath string, affected []osv.Affected) string {
fixed := latestFixed(modulePath, affected)
if fixed != "" {
fixed = "v" + fixed
// fixNegated checks if fix is negated to by a re-introduction
// of a vulnerability in affected. Assumes that all affected apply
// to the same module.
func fixNegated(fix string, affected []osv.Affected) bool {
for _, a := range affected {
for _, r := range a.Ranges {
if semver.ContainsSemver(r, fix) {
return true
}
}
}
return fixed
return false
}

func moduleVersionString(modulePath, version string) string {
Expand All @@ -76,3 +123,17 @@ func moduleVersionString(modulePath, version string) string {
}
return version
}

func modPath(mod *packages.Module) string {
if mod.Replace != nil {
return mod.Replace.Path
}
return mod.Path
}

func modVersion(mod *packages.Module) string {
if mod.Replace != nil {
return mod.Replace.Version
}
return mod.Version
}
66 changes: 35 additions & 31 deletions internal/scan/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,22 @@ import (
"golang.org/x/vuln/internal/osv"
)

func TestLatestFixed(t *testing.T) {
func TestFixedVersion(t *testing.T) {
for _, test := range []struct {
name string
module string
in []osv.Affected
want string
name string
module string
version string
in []osv.Affected
want string
}{
{
name: "empty",
want: "",
},
{
name: "no semver",
module: "example.com/module",
name: "no semver",
module: "example.com/module",
version: "v1.2.0",
in: []osv.Affected{
{
Module: osv.Module{
Expand All @@ -33,16 +35,17 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeType("unspecified"),
Events: []osv.RangeEvent{
{Introduced: "v1.0.0", Fixed: "v1.2.3"},
{Introduced: "v1.0.0"}, {Fixed: "v1.2.3"},
},
}},
},
},
want: "",
},
{
name: "one",
module: "example.com/module",
name: "one",
module: "example.com/module",
version: "v1.0.1",
in: []osv.Affected{
{
Module: osv.Module{
Expand All @@ -52,16 +55,17 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "v1.0.0", Fixed: "v1.2.3"},
{Introduced: "v1.0.0"}, {Fixed: "v1.2.3"},
},
}},
},
},
want: "v1.2.3",
},
{
name: "several",
module: "example.com/module",
name: "several",
module: "example.com/module",
version: "v1.2.0",
in: []osv.Affected{
{
Module: osv.Module{
Expand All @@ -71,8 +75,8 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "v1.0.0", Fixed: "v1.2.3"},
{Introduced: "v1.5.0", Fixed: "v1.5.6"},
{Introduced: "v1.0.0"}, {Fixed: "v1.2.3"},
{Introduced: "v1.5.0"}, {Fixed: "v1.5.6"},
},
}},
},
Expand All @@ -84,7 +88,7 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "v1.3.0", Fixed: "v1.4.1"},
{Introduced: "v1.3.0"}, {Fixed: "v1.4.1"},
},
}},
},
Expand All @@ -97,16 +101,17 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "0", Fixed: "v1.6.0"},
{Introduced: "0"}, {Fixed: "v1.6.0"},
},
}},
},
},
want: "v1.5.6",
want: "v1.2.3",
},
{
name: "no v prefix",
module: "example.com/module",
name: "no v prefix",
version: "1.18.1",
module: "example.com/module",
in: []osv.Affected{
{
Module: osv.Module{
Expand All @@ -128,15 +133,15 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "1.18.0", Fixed: "1.18.4"},
{Introduced: "1.18.0"}, {Fixed: "1.18.4"},
},
}},
},
},
want: "1.18.4",
want: "v1.18.4",
},
{
name: "unfixed",
name: "overlapping",
module: "example.com/module",
in: []osv.Affected{
{
Expand All @@ -147,8 +152,10 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
{Introduced: "v1.0.0", Fixed: "v1.2.3"},
// Reintroduced and never fixed.
// v1.2.3 is nominally the earliest fix,
// but it is contained in vulnerable range
// for the next affected value.
{Introduced: "v1.0.0"}, {Fixed: "v1.2.3"},
{Introduced: "v1.5.0"},
},
}},
Expand All @@ -161,19 +168,16 @@ func TestLatestFixed(t *testing.T) {
{
Type: osv.RangeTypeSemver,
Events: []osv.RangeEvent{
// Even though this block has a fix,
// it will be overridden by the block
// with no fix.
{Introduced: "v1.3.0", Fixed: "v1.4.1"},
{Introduced: "v1.2.0"}, {Fixed: "v1.4.1"},
},
}},
},
},
want: "",
want: "v1.4.1",
},
} {
t.Run(test.name, func(t *testing.T) {
got := latestFixed(test.module, test.in)
got := fixedVersion(test.module, test.version, test.in)
if got != test.want {
t.Errorf("got %q, want %q", got, test.want)
}
Expand Down
9 changes: 5 additions & 4 deletions internal/semver/affects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func Affects(a []osv.Range, v string) bool {
continue
}
semverRangePresent = true
if containsSemver(r, v) {
if ContainsSemver(r, v) {
return true
}
}
Expand All @@ -32,17 +32,18 @@ func Affects(a []osv.Range, v string) bool {
return !semverRangePresent
}

// containsSemver checks if semver version v is in the
// ContainsSemver checks if semver version v is in the
// range encoded by ar. If ar is not a semver range,
// returns false.
// returns false. A range is interpreted as a left-closed
// and right-open interval.
//
// Assumes that
// - exactly one of Introduced or Fixed fields is set
// - ranges in ar are not overlapping
// - beginning of time is encoded with .Introduced="0"
// - no-fix is not an event, as opposed to being an
// event where Introduced="" and Fixed=""
func containsSemver(ar osv.Range, v string) bool {
func ContainsSemver(ar osv.Range, v string) bool {
if ar.Type != osv.RangeTypeSemver {
return false
}
Expand Down

0 comments on commit 6987ccb

Please sign in to comment.