Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rhel: add csaf/vex updater #1165

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- The rhel-vex updater will now be responsible for RHEL advisories so we have
-- to delete the existing rhel vulnerabilities.
DELETE FROM update_operation WHERE updater ~ 'RHEL[5-9]-*';
DELETE FROM vuln where updater ~ 'RHEL[5-9]-*';
4 changes: 4 additions & 0 deletions datastore/postgres/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,8 @@ var MatcherMigrations = []migrate.Migration{
ID: 12,
Up: runFile("matcher/12-add-latest_update_operation-index.sql"),
},
{
ID: 13,
Up: runFile("matcher/13-delete-rhel-oval.sql"),
},
}
3 changes: 3 additions & 0 deletions datastore/postgres/querybuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/doug-martin/goqu/v8"
_ "github.com/doug-martin/goqu/v8/dialect/postgres"
"github.com/doug-martin/goqu/v8/exp"

"github.com/quay/claircore"
"github.com/quay/claircore/datastore"
Expand Down Expand Up @@ -70,6 +71,8 @@ func buildGetQuery(record *claircore.IndexRecord, opts *datastore.GetOpts) (stri
ex = goqu.Ex{"dist_arch": record.Distribution.Arch}
case driver.RepositoryName:
ex = goqu.Ex{"repo_name": record.Repository.Name}
case driver.HasFixedInVersion:
ex = goqu.Ex{"fixed_in_version": goqu.Op{exp.NeqOp.String(): ""}}
default:
return "", fmt.Errorf("was provided unknown matcher: %v", m)
}
Expand Down
14 changes: 14 additions & 0 deletions datastore/postgres/querybuilder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,20 @@ func TestGetQueryBuilderDeterministicArgs(t *testing.T) {
}
},
},
{
name: "FixedInVersion",
expectedQuery: preamble + both +
`("fixed_in_version" != '')` + epilogue,
matchExps: []driver.MatchConstraint{driver.HasFixedInVersion},
indexRecord: func() *claircore.IndexRecord {
pkgs := test.GenUniquePackages(1)
dists := test.GenUniqueDistributions(1)
return &claircore.IndexRecord{
Package: pkgs[0],
Distribution: dists[0],
}
},
},
}

// This is safe to do because SQL doesn't care about what whitespace is
Expand Down
3 changes: 2 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ require (
github.com/knqyf263/go-apk-version v0.0.0-20200609155635-041fdbb8563f
github.com/knqyf263/go-deb-version v0.0.0-20190517075300-09fca494f03d
github.com/knqyf263/go-rpm-version v0.0.0-20170716094938-74609b86c936
github.com/package-url/packageurl-go v0.1.2
github.com/prometheus/client_golang v1.19.0
github.com/quay/claircore/toolkit v1.2.0
github.com/quay/claircore/toolkit v1.2.1
github.com/quay/claircore/updater/driver v1.0.0
github.com/quay/goval-parser v0.8.8
github.com/quay/zlog v1.1.8
Expand Down
6 changes: 4 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ github.com/mattn/go-sqlite3 v1.10.0 h1:jbhqpg7tQe4SupckyijYiy0mJJ/pRyHvXf7JdWK86
github.com/mattn/go-sqlite3 v1.10.0/go.mod h1:FPy6KqzDD04eiIsT53CuJW3U88zkxoIYsOqkbpncsNc=
github.com/ncruces/go-strftime v0.1.9 h1:bY0MQC28UADQmHmaF5dgpLmImcShSi2kHU9XLdhx/f4=
github.com/ncruces/go-strftime v0.1.9/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls=
github.com/package-url/packageurl-go v0.1.2 h1:0H2DQt6DHd/NeRlVwW4EZ4oEI6Bn40XlNPRqegcxuo4=
github.com/package-url/packageurl-go v0.1.2/go.mod h1:uQd4a7Rh3ZsVg5j0lNyAfyxIeGde9yrlhjF78GzeW0c=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
Expand All @@ -146,8 +148,8 @@ github.com/prometheus/common v0.48.0 h1:QO8U2CdOzSn1BBsmXJXduaaW+dY/5QLjfB8svtSz
github.com/prometheus/common v0.48.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc=
github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
github.com/quay/claircore/toolkit v1.2.0 h1:cgMCIJH8Gzl/7p6y4hVAhSEvPvT9iGWHcmpIeO8PbJQ=
github.com/quay/claircore/toolkit v1.2.0/go.mod h1:m6ZRpxJClVAraNpIYyCsW/ULF/33ye7KkGTyNTMwvDY=
github.com/quay/claircore/toolkit v1.2.1 h1:6cpa0xGWtFIn7QAsc8zbJohQ7Tm8yOGZMCq/iQkrsqQ=
github.com/quay/claircore/toolkit v1.2.1/go.mod h1:m6ZRpxJClVAraNpIYyCsW/ULF/33ye7KkGTyNTMwvDY=
github.com/quay/goval-parser v0.8.8 h1:Uf+f9iF2GIR5GPUY2pGoa9il2+4cdES44ZlM0mWm4cA=
github.com/quay/goval-parser v0.8.8/go.mod h1:Y0NTNfPYOC7yxsYKzJOrscTWUPq1+QbtHw4XpPXWPMc=
github.com/quay/zlog v1.1.8 h1:/cKgHpqKu3g7mB9OlvnfbqrbPIssyxeameDBytGPrNs=
Expand Down
2 changes: 1 addition & 1 deletion internal/matcher/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (mc *Controller) filter(ctx context.Context, interested []*claircore.IndexR
if err != nil {
return nil, err
}
filtered[record.Package.ID] = match
filtered[record.Package.ID] = append(filtered[record.Package.ID], match...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hard to tell without context: what was this overwriting before? How bad was this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context:

  • We get the relevant indexing records by leveraging the matcher's Filter() method
  • We pass those records to the query function to get back a map of potential vulns (map[record.Package.ID][]vuln).
  • When checking if Vulnerable() we iterate through the records and check if it (the record) is vulnerable to any of it's potential vulns.
  • The matched vulns are save in the filtered map we're referring to
  • Index records are create as one per package-repository combination

So we have a situation where previously the record.Package.ID was specific enough because there was only one record with that record.Package.ID because we were Querying on the RepoName. Now there are mutiple records with the same record.Package.ID but different record.Repos. Hence, the last record with a random Repo would overwrite any potential legitimate vulnerabilities if the matches weren't appended

}
return filtered, nil
}
Expand Down
2 changes: 2 additions & 0 deletions libvuln/driver/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ const (
DistributionPrettyName
// should match claircore.Package.Repository.Name => claircore.Vulnerability.Package.Repository.Name
RepositoryName
// should match claircore.Vulnerability.FixedInVersion != ""
HasFixedInVersion
)

// Matcher is an interface which a Controller uses to query the vulnstore for vulnerabilities.
Expand Down
2 changes: 1 addition & 1 deletion matchers/defaults/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ var defaultMatchers = []driver.Matcher{
&photon.Matcher{},
&python.Matcher{},
rhcc.Matcher,
&rhel.Matcher{},
&ruby.Matcher{},
&suse.Matcher{},
&ubuntu.Matcher{},
}

func inner(ctx context.Context) error {
registry.Register("rhel", &rhel.MatcherFactory{})
for _, m := range defaultMatchers {
mf := driver.MatcherStatic(m)
registry.Register(m.Name(), mf)
Expand Down
55 changes: 49 additions & 6 deletions rhel/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,21 @@

import (
"context"
"strings"

version "github.com/knqyf263/go-rpm-version"

"github.com/quay/zlog"

"github.com/quay/claircore"
"github.com/quay/claircore/libvuln/driver"
"github.com/quay/claircore/toolkit/types/cpe"
)

// Matcher implements driver.Matcher.
type Matcher struct{}
type Matcher struct {
ignoreUnpatched bool
}

var _ driver.Matcher = (*Matcher)(nil)

Expand All @@ -25,15 +31,52 @@
}

// Query implements driver.Matcher.
func (*Matcher) Query() []driver.MatchConstraint {
return []driver.MatchConstraint{
driver.PackageModule,
driver.RepositoryName,
crozzy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove the repo check here and place it in Vulnerable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a result of SECDATA-496 (not linking as it's internal) it was decided that unpatched advisories would not be related to all potential repos but they would be related to a subset (using CPE's URI Binding matching notation). This means that comparing the repo is more involved than == is it was previously. Hence, we need to move the comparison logic out of the query layer and expand it in the application code.

func (m *Matcher) Query() []driver.MatchConstraint {
mcs := []driver.MatchConstraint{driver.PackageModule}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why do we check the package module? does every package have a related module? why not check for distribution name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not deriving any distribution info from the VEX data. The OVAL updater didn't derive distribution from the data either, it parses the name of the file, and this info isn't used for matching.

if m.ignoreUnpatched {
mcs = append(mcs, driver.HasFixedInVersion)

Check warning on line 37 in rhel/matcher.go

View check run for this annotation

Codecov / codecov/patch

rhel/matcher.go#L34-L37

Added lines #L34 - L37 were not covered by tests
}
return mcs

Check warning on line 39 in rhel/matcher.go

View check run for this annotation

Codecov / codecov/patch

rhel/matcher.go#L39

Added line #L39 was not covered by tests
}

// isCPESubstringMatch is a hack that accounts for CPEs in the VEX
// data that are expected to be treated as subset matching CPEs but
// don't use the correct syntax defined in the spec.
// E.g. cpe:/a:redhat:openshift:4.13::el8 is expected to match to
// cpe:/a:redhat:openshift:4.
// TODO: Remove once RH VEX data updates CPEs with the correct matching
// syntax.
func isCPESubstringMatch(recordCPE cpe.WFN, vulnCPE cpe.WFN) bool {
return strings.HasPrefix(strings.TrimRight(recordCPE.String(), ":*"), strings.TrimRight(vulnCPE.String(), ":*"))
}

// Vulnerable implements driver.Matcher.
func (m *Matcher) Vulnerable(_ context.Context, record *claircore.IndexRecord, vuln *claircore.Vulnerability) (bool, error) {
//
// Vulnerable will interpret the claircore.Vulnerability.Repo.CPE
// as a CPE match expression, and to be considered vulnerable,
// the relationship between claircore.IndexRecord.Repository.CPE and
// the claircore.Vulnerability.Repo.CPE needs to be a CPE Name Comparison
// Relation of SUBSET(⊂)(source is a subset of, or equal to the target).
// https://nvlpubs.nist.gov/nistpubs/Legacy/IR/nistir7696.pdf Section 6.2.
func (m *Matcher) Vulnerable(ctx context.Context, record *claircore.IndexRecord, vuln *claircore.Vulnerability) (bool, error) {
if vuln.Repo == nil || record.Repository == nil || vuln.Repo.Key != repositoryKey {
return false, nil

Check warning on line 63 in rhel/matcher.go

View check run for this annotation

Codecov / codecov/patch

rhel/matcher.go#L63

Added line #L63 was not covered by tests
}
var err error
// This conversion has to be done because our current data structure doesn't
// support the claircore.Vulnerability.Repo.CPE field.
vuln.Repo.CPE, err = cpe.Unbind(vuln.Repo.Name)
if err != nil {
zlog.Warn(ctx).
Str("vulnerability name", vuln.Name).
Err(err).
Msg("unable to unbind repo CPE")
return false, nil
}
if !cpe.Compare(record.Repository.CPE, vuln.Repo.CPE).IsSubset() && !isCPESubstringMatch(record.Repository.CPE, vuln.Repo.CPE) {
return false, nil
}

pkgVer := version.NewVersion(record.Package.Version)
var vulnVer version.Version
// Assume the vulnerability record we have is for the last known vulnerable
Expand Down
114 changes: 112 additions & 2 deletions rhel/matcher_test.go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test OpenShift CPEs? Has there been an agreement on whether Prod Sec or Claircore will have to handle OpenShift CPEs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the provided pattern doesn't match what it's "meant" to (and there's not a bug in the match implementation), it's a bug in the data.

Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/quay/claircore/pkg/ctxlock"
"github.com/quay/claircore/test/integration"
pgtest "github.com/quay/claircore/test/postgres"
"github.com/quay/claircore/toolkit/types/cpe"
)

func TestMain(m *testing.M) {
Expand Down Expand Up @@ -118,43 +119,119 @@ func TestVulnerable(t *testing.T) {
Package: &claircore.Package{
Version: "0.33.0-6.el8",
},
Repository: &claircore.Repository{
CPE: cpe.MustUnbind("cpe:/o:redhat:enterprise_linux:8::baseos"),
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
openshiftRecord := &claircore.IndexRecord{
Package: &claircore.Package{
Version: "0.33.0-6.el8",
},
Repository: &claircore.Repository{
CPE: cpe.MustUnbind("cpe:/a:redhat:openshift:4.13::el8"),
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
fixedVulnPast := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "0.33.0-5.el8",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
fixedVulnCurrent := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "0.33.0-6.el8",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
fixedVulnFuture := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "0.33.0-7.el8",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
unfixedVuln := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
unfixedVulnBadCPE := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "",
Repo: &claircore.Repository{
Name: "cep:o:redhat:enterprise_linux:8::baseos",
Key: "rhel-cpe-repository",
},
}
unfixedVulnRepoIsSubset := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8",
Key: "rhel-cpe-repository",
},
}
unfixedVulnRepoNotSubset := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "",
Repo: &claircore.Repository{
Name: "cpe:/o:redhat:enterprise_linux:8::appstream",
Key: "rhel-cpe-repository",
},
}
unfixedVulnRepoSubstring := &claircore.Vulnerability{
Package: &claircore.Package{
Version: "",
},
FixedInVersion: "",
Repo: &claircore.Repository{
Name: "cpe:/a:redhat:openshift:4",
Key: "rhel-cpe-repository",
},
}

testCases := []vulnerableTestCase{
{ir: record, v: fixedVulnPast, want: false, name: "vuln fixed in past version"},
{ir: record, v: fixedVulnCurrent, want: false, name: "vuln fixed in current version"},
{ir: record, v: fixedVulnFuture, want: true, name: "outdated package"},
{ir: record, v: unfixedVuln, want: true, name: "unfixed vuln"},
{ir: record, v: unfixedVulnBadCPE, want: false, name: "unfixed vuln, invalid CPE"},
{ir: record, v: unfixedVulnRepoIsSubset, want: true, name: "unfixed vuln, Repo is a subset"},
{ir: record, v: unfixedVulnRepoNotSubset, want: false, name: "unfixed vuln, Repo not a subset"},
{ir: openshiftRecord, v: unfixedVulnRepoSubstring, want: true, name: "unfixed vuln, Repo is a substring match"},
}

m := &Matcher{}

ctx := context.Background()
ctx = zlog.Test(ctx, t)
for _, tc := range testCases {
got, err := m.Vulnerable(nil, tc.ir, tc.v)
got, err := m.Vulnerable(ctx, tc.ir, tc.v)
if err != nil {
t.Error(err)
}
Expand All @@ -163,3 +240,36 @@ func TestVulnerable(t *testing.T) {
}
}
}

func TestIsCPEStringSubsetMatch(t *testing.T) {
t.Parallel()

testcases := []struct {
name string
recordCPE, vulnCPE cpe.WFN
match bool
}{
{
name: "simple_case",
recordCPE: cpe.MustUnbind("cpe:/a:redhat:openshift:4.13::el8"),
vulnCPE: cpe.MustUnbind("cpe:/a:redhat:openshift:4"),
match: true,
},
{
name: "wrong_minor",
recordCPE: cpe.MustUnbind("cpe:/a:redhat:openshift:4.13::el8"),
vulnCPE: cpe.MustUnbind("cpe:/a:redhat:openshift:4.1::el8"),
match: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
tt := tc
matched := isCPESubstringMatch(tt.recordCPE, tt.vulnCPE)
if matched != tt.match {
t.Errorf("unexpected matching %s and %s", tt.recordCPE, tt.vulnCPE)
}
})
}
}