From cafbf3dad82d86b1b3b9593ebad2350a92838dd9 Mon Sep 17 00:00:00 2001 From: crozzy Date: Thu, 17 Oct 2024 15:53:29 -0700 Subject: [PATCH] osv: account for event objects that have multiple streams It was discovered that some OSV documents can order minor releases in the same affected.ranges object. This meant that only ever counted the last range in a vulnerability. This change gathers range information for the affected product and creates a vulnerability per range. Signed-off-by: crozzy --- updater/osv/osv.go | 155 ++++++++++------ updater/osv/osv_test.go | 382 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 473 insertions(+), 64 deletions(-) diff --git a/updater/osv/osv.go b/updater/osv/osv.go index ca11c1959..be6497d21 100644 --- a/updater/osv/osv.go +++ b/updater/osv/osv.go @@ -484,6 +484,12 @@ func newECS(u string) ecs { } } +type rangeVer struct { + fixedInVersion string + semverRange *claircore.Range + ecosystemRanges url.Values +} + func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *advisory) (err error) { if a.GitOnly() { return nil @@ -533,15 +539,14 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso } b.WriteString(ref.URL) } + proto.Links = b.String() for i := range a.Affected { af := &a.Affected[i] - v := e.NewVulnerability() - (*v) = proto for _, r := range af.Ranges { + vers := []*rangeVer{} switch r.Type { case `SEMVER`: - v.Range = &claircore.Range{} case `ECOSYSTEM`: b.Reset() case `GIT`: @@ -553,25 +558,40 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso Msg("odd range type") } // This does some heavy assumptions about valid inputs. - ranges := make(url.Values) - for _, ev := range r.Events { + vs := &rangeVer{semverRange: &claircore.Range{}, ecosystemRanges: url.Values{}} + var seenIntroduced bool + for ie := range r.Events { + ev := r.Events[ie] var err error switch r.Type { case `SEMVER`: var ver *semver.Version switch { - case ev.Introduced == "0": // -Inf - v.Range.Lower.Kind = `semver` + case ev.Introduced != "" && seenIntroduced: + vs = &rangeVer{semverRange: &claircore.Range{}} + fallthrough case ev.Introduced != "": - ver, err = semver.NewVersion(ev.Introduced) - if err == nil { - v.Range.Lower = claircore.FromSemver(ver) + seenIntroduced = true + switch { + case ev.Introduced == "0": // -Inf + vs.semverRange.Lower.Kind = `semver` + default: + ver, err = semver.NewVersion(ev.Introduced) + if err == nil { + vs.semverRange.Lower = claircore.FromSemver(ver) + } } + // Is this the last event? If so append the range and + // implicitly terminate. + if ie == len(r.Events)-1 { + vers = append(vers, vs) + } + continue case ev.Fixed != "": // less than ver, err = semver.NewVersion(ev.Fixed) if err == nil { - v.Range.Upper = claircore.FromSemver(ver) - v.FixedInVersion = ver.Original() + vs.semverRange.Upper = claircore.FromSemver(ver) + vs.fixedInVersion = ver.Original() } case ev.LastAffected != "" && len(af.Versions) != 0: // less than equal to // TODO(hank) Should be able to convert this to a "less than." @@ -586,11 +606,11 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso ver, err = semver.NewVersion(ev.LastAffected) if err == nil { nv := ver.IncPatch() - v.Range.Upper = claircore.FromSemver(&nv) + vs.semverRange.Upper = claircore.FromSemver(&nv) } case ev.Limit == "*": // +Inf - v.Range.Upper.Kind = `semver` - v.Range.Upper.V[0] = 65535 + vs.semverRange.Upper.Kind = `semver` + vs.semverRange.Upper.V[0] = 65535 case ev.Limit != "": // Something arbitrary zlog.Info(ctx). Str("which", "limit"). @@ -600,14 +620,26 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso case `ECOSYSTEM`: switch af.Package.Ecosystem { case ecosystemMaven, ecosystemPyPI, ecosystemRubyGems: + vs.semverRange = nil switch { - case ev.Introduced == "0": + case ev.Introduced != "" && seenIntroduced: + vs = &rangeVer{ecosystemRanges: url.Values{}} + fallthrough case ev.Introduced != "": - ranges.Add("introduced", ev.Introduced) + seenIntroduced = true + switch { + case ev.Introduced == "0": + case ev.Introduced != "": + vs.ecosystemRanges.Add("introduced", ev.Introduced) + } + if ie == len(r.Events)-1 { + vers = append(vers, vs) + } + continue case ev.Fixed != "": - ranges.Add("fixed", ev.Fixed) + vs.ecosystemRanges.Add("fixed", ev.Fixed) case ev.LastAffected != "": - ranges.Add("lastAffected", ev.LastAffected) + vs.ecosystemRanges.Add("lastAffected", ev.LastAffected) } case ecosystemGo, ecosystemNPM: return fmt.Errorf(`unexpected "ECOSYSTEM" entry for %q ecosystem: %s`, af.Package.Ecosystem, a.ID) @@ -616,7 +648,7 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso case ev.Introduced == "0": // -Inf case ev.Introduced != "": case ev.Fixed != "": - v.FixedInVersion = ev.Fixed + vs.fixedInVersion = ev.Fixed case ev.LastAffected != "": case ev.Limit == "*": // +Inf case ev.Limit != "": @@ -626,49 +658,58 @@ func (e *ecs) Insert(ctx context.Context, skipped *stats, name string, a *adviso if err != nil { zlog.Warn(ctx).Err(err).Msg("event version error") } + vers = append(vers, vs) } - if len(ranges) > 0 { - switch af.Package.Ecosystem { - case ecosystemMaven, ecosystemPyPI, ecosystemRubyGems: - v.FixedInVersion = ranges.Encode() + // Now we need to cycle through the ranges and create the vulnerabilities. + for _, ve := range vers { + v := e.NewVulnerability() + (*v) = proto + v.Range = ve.semverRange + v.FixedInVersion = ve.fixedInVersion + if len(ve.ecosystemRanges) > 0 { + switch af.Package.Ecosystem { + case ecosystemMaven, ecosystemPyPI, ecosystemRubyGems: + v.FixedInVersion = ve.ecosystemRanges.Encode() + } } - } - if r := v.Range; r != nil { - // We have an implicit +Inf range if there's a single event, - // this should catch it? - if r.Upper.Kind == "" { - r.Upper.Kind = r.Lower.Kind - r.Upper.V[0] = 65535 + if r := v.Range; r != nil { + // We have an implicit +Inf range if there's a single event, + // this should catch it? + if r.Upper.Kind == "" { + r.Upper.Kind = r.Lower.Kind + r.Upper.V[0] = 65535 + } + if r.Lower.Compare(&r.Upper) == 1 { + e.RemoveVulnerability(v) + skipped.Ignored = append(skipped.Ignored, fmt.Sprintf("%s(%s,%s)", a.ID, r.Lower.String(), r.Upper.String())) + continue + } + } + var vs string + switch r.Type { + case `ECOSYSTEM`: + vs = b.String() } - if r.Lower.Compare(&r.Upper) == 1 { - e.RemoveVulnerability(v) - skipped.Ignored = append(skipped.Ignored, fmt.Sprintf("%s(%s,%s)", a.ID, r.Lower.String(), r.Upper.String())) - continue + pkgName := af.Package.PURL + switch af.Package.Ecosystem { + case ecosystemGo, ecosystemMaven, ecosystemNPM, ecosystemPyPI, ecosystemRubyGems: + pkgName = af.Package.Name + } + pkg, novel := e.LookupPackage(pkgName, vs) + v.Package = pkg + switch af.Package.Ecosystem { + case ecosystemGo, ecosystemMaven, ecosystemNPM, ecosystemPyPI, ecosystemRubyGems: + v.Package.Kind = claircore.BINARY + } + if novel { + pkg.RepositoryHint = af.Package.Ecosystem + } + if repo := e.LookupRepository(name); repo != nil { + v.Repo = repo } } - var vs string - switch r.Type { - case `ECOSYSTEM`: - vs = b.String() - } - pkgName := af.Package.PURL - switch af.Package.Ecosystem { - case ecosystemGo, ecosystemMaven, ecosystemNPM, ecosystemPyPI, ecosystemRubyGems: - pkgName = af.Package.Name - } - pkg, novel := e.LookupPackage(pkgName, vs) - v.Package = pkg - switch af.Package.Ecosystem { - case ecosystemGo, ecosystemMaven, ecosystemNPM, ecosystemPyPI, ecosystemRubyGems: - v.Package.Kind = claircore.BINARY - } - if novel { - pkg.RepositoryHint = af.Package.Ecosystem - } - if repo := e.LookupRepository(name); repo != nil { - v.Repo = repo - } + } } return nil diff --git a/updater/osv/osv_test.go b/updater/osv/osv_test.go index 3a4186f55..554449718 100644 --- a/updater/osv/osv_test.go +++ b/updater/osv/osv_test.go @@ -18,6 +18,9 @@ import ( "strings" "testing" + "github.com/Masterminds/semver" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/quay/zlog" "github.com/quay/claircore" @@ -138,18 +141,383 @@ func TestParse(t *testing.T) { } t.Logf("parsed %d vulnerabilities", len(vs)) if len(vs) != 0 { - t.Log("first one:") - var buf bytes.Buffer - enc := json.NewEncoder(&buf) - enc.SetIndent("", "\t") - if err := enc.Encode(vs[0]); err != nil { - t.Error(err) + for _, v := range vs { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + enc.SetIndent("", "\t") + if err := enc.Encode(v); err != nil { + t.Error(err) + } + t.Log(buf.String()) } - t.Log(buf.String()) } } } +var insertTestCases = []struct { + name string + ad *advisory + expectedVulns []claircore.Vulnerability +}{ + { + name: "normal", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "0", + }, + { + Fixed: "0.4.0", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("0.0.0")), + Upper: claircore.FromSemver(semver.MustParse("0.4.0")), + }, + FixedInVersion: "0.4.0", + }, + }, + }, + { + name: "unfixed", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "0", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("0.0.0")), + Upper: claircore.Version{ + Kind: "semver", + V: [10]int32{65535, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + }, + }, + FixedInVersion: "", + }, + }, + }, + { + name: "two_affected", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "0", + }, + { + Fixed: "0.4.10", + }, + }, + }, + }, + }, + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "0.5.0", + }, + { + Fixed: "0.5.3", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("0.0.0")), + Upper: claircore.FromSemver(semver.MustParse("0.4.10")), + }, + FixedInVersion: "0.4.10", + }, + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("0.5.0")), + Upper: claircore.FromSemver(semver.MustParse("0.5.3")), + }, + FixedInVersion: "0.5.3", + }, + }, + }, + { + name: "three_fixes", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "0", + }, + { + Fixed: "2.1.16", + }, + { + Introduced: "2.2.0", + }, + { + Fixed: "2.2.10", + }, + { + Introduced: "2.3.0", + }, + { + Fixed: "2.3.5", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("0.0.0")), + Upper: claircore.FromSemver(semver.MustParse("2.1.16")), + }, + FixedInVersion: "2.1.16", + }, + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("2.2.0")), + Upper: claircore.FromSemver(semver.MustParse("2.2.10")), + }, + FixedInVersion: "2.2.10", + }, + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("2.3.0")), + Upper: claircore.FromSemver(semver.MustParse("2.3.5")), + }, + FixedInVersion: "2.3.5", + }, + }, + }, + { + // In this situation we're just expecting the last one. + name: "two_consecutive_introduced_invalid", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "go", + Name: "something", + }, + Ranges: []_range{ + { + Type: "SEMVER", + Events: []rangeEvent{ + { + Introduced: "2.2.0", + }, + { + Introduced: "2.3.0", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: &claircore.Range{ + Lower: claircore.FromSemver(semver.MustParse("2.3.0")), + Upper: claircore.Version{ + Kind: "semver", + V: [10]int32{65535, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + }, + }, + FixedInVersion: "", + }, + }, + }, + { + name: "ecosystem_multi", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "PyPI", + Name: "cherrypy", + }, + Ranges: []_range{ + { + Type: "ECOSYSTEM", + Events: []rangeEvent{ + { + Introduced: "0", + }, + { + Fixed: "2.1.1", + }, + { + Introduced: "3.0", + }, + { + Fixed: "3.0.2", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: nil, + FixedInVersion: "fixed=2.1.1", + }, + { + Name: "test1", + Updater: "test", + Range: nil, + FixedInVersion: "fixed=3.0.2&introduced=3.0", + }, + }, + }, + { + name: "ecosystem_unfixed", + ad: &advisory{ + ID: "test1", + Affected: []affected{ + { + Package: _package{ + Ecosystem: "PyPI", + Name: "cherrypy", + }, + Ranges: []_range{ + { + Type: "ECOSYSTEM", + Events: []rangeEvent{ + { + Introduced: "3.0", + }, + }, + }, + }, + }, + }, + }, + expectedVulns: []claircore.Vulnerability{ + { + Name: "test1", + Updater: "test", + Range: nil, + FixedInVersion: "introduced=3.0", + }, + }, + }, +} + +// cmpIgnore will ignore everything expect the Name, Updater and Range +var cmpIgnore = cmpopts.IgnoreFields( + claircore.Vulnerability{}, "ID", "Updater", "Description", "Severity", "NormalizedSeverity", "Package", "Repo") + +func TestInsert(t *testing.T) { + ctx := zlog.Test(context.Background(), t) + + for _, tt := range insertTestCases { + t.Run(tt.name, func(t *testing.T) { + ecs := newECS("test") + + err := ecs.Insert(ctx, nil, "", tt.ad) + if err != nil { + t.Error("got error Inserting advisory", err) + } + if len(ecs.Vulnerability) != len(tt.expectedVulns) { + t.Fatalf("should have %d vulnerability but got %d", len(tt.expectedVulns), len(ecs.Vulnerability)) + } + + if !cmp.Equal(ecs.Vulnerability, tt.expectedVulns, cmpIgnore) { + t.Error(cmp.Diff(ecs.Vulnerability, tt.expectedVulns, cmpIgnore)) + } + }) + } +} + var severityTestCases = []struct { name string a *advisory