From af473fb3f7261230b15edc31decdf510029157f8 Mon Sep 17 00:00:00 2001 From: RTann Date: Fri, 9 Aug 2024 15:39:35 -0700 Subject: [PATCH 1/6] fix: validate digest prior to layer download --- benchmarks/util.go | 7 ++++++- pkg/scan/scan.go | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/benchmarks/util.go b/benchmarks/util.go index aa0e93673..968dd2f66 100644 --- a/benchmarks/util.go +++ b/benchmarks/util.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/opencontainers/go-digest" + "github.com/pkg/errors" "github.com/stackrox/scanner/pkg/clairify/types" server "github.com/stackrox/scanner/pkg/scan" "github.com/stretchr/testify/require" @@ -41,7 +42,11 @@ func MustGetLayerReadClosers(b *testing.B, imageName string) []*layerReadCloser func getLayerDownloadReadCloser(reg types.Registry, image *types.Image, layerName string) *server.LayerDownloadReadCloser { return &server.LayerDownloadReadCloser{ Downloader: func() (io.ReadCloser, error) { - return reg.DownloadLayer(image.Remote, digest.Digest(layerName)) + dig, err := digest.Parse(layerName) + if err != nil { + return nil, errors.Wrapf(err, "invalid layer digest %q", layerName) + } + return reg.DownloadLayer(image.Remote, dig) }, } } diff --git a/pkg/scan/scan.go b/pkg/scan/scan.go index 24adae4bc..a8d91c39b 100644 --- a/pkg/scan/scan.go +++ b/pkg/scan/scan.go @@ -44,7 +44,11 @@ func analyzeLayers(storage database.Datastore, registry types.Registry, image *t for _, layer := range layers { layerReadCloser := &LayerDownloadReadCloser{ Downloader: func() (io.ReadCloser, error) { - return registry.DownloadLayer(image.Remote, digest.Digest(layer)) + dig, err := digest.Parse(layer) + if err != nil { + return nil, errors.Wrapf(err, "invalid layer digest %q", layer) + } + return registry.DownloadLayer(image.Remote, dig) }, } From 80fcd2d50472ebfc78c815e1c83ab973bc50cb1a Mon Sep 17 00:00:00 2001 From: RTann Date: Mon, 12 Aug 2024 09:28:10 -0700 Subject: [PATCH 2/6] bump docker-registry-client dep --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index ad2098f99..74bcd9d8a 100644 --- a/go.mod +++ b/go.mod @@ -183,7 +183,7 @@ require ( replace ( github.com/facebookincubator/nvdtools => github.com/stackrox/nvdtools v0.0.0-20231111002313-57e262e4797e - github.com/heroku/docker-registry-client => github.com/stackrox/docker-registry-client v0.1.0 + github.com/heroku/docker-registry-client => github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989 // The current latest version of github.com/mholt/archiver/v3 (v3.5.1) suffers from CVE-2024-0406. // There is currently a PR in place to resolve it (https://github.com/mholt/archiver/pull/396), diff --git a/go.sum b/go.sum index e94e5c2cc..3603d78f3 100644 --- a/go.sum +++ b/go.sum @@ -612,8 +612,8 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace h1:9PNP1jnUjRhfmGMlkXHjYPishpcw4jpSt/V/xYY3FMA= github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spf13/viper v1.10.0/go.mod h1:SoyBPwAtKDzypXNDFKN5kzH7ppppbGZtls1UpIy5AsM= -github.com/stackrox/docker-registry-client v0.1.0 h1:TbNgSP3dhNbLAqlwyTIYNXbNvl9TDTAbh+cfzVyJ6II= -github.com/stackrox/docker-registry-client v0.1.0/go.mod h1:4TU+pA11iczIvhtL0own2OJcJXc1o26tBHDivaXhlZU= +github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989 h1:uyRyi22LD2G//T3+ZuOZydwdHKx9nWTangB7vvAvfKA= +github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989/go.mod h1:4TU+pA11iczIvhtL0own2OJcJXc1o26tBHDivaXhlZU= github.com/stackrox/dotnet-scraper v0.0.0-20201023051640-72ef543323dd h1:vEjp7Q66zd4W72//Uk3uyVN50Mh/nFLbN9pb7CVK7mE= github.com/stackrox/dotnet-scraper v0.0.0-20201023051640-72ef543323dd/go.mod h1:HILeV3i/EyJz844GcrC3+oU7oZONhjfujaIYBMJ/bZE= github.com/stackrox/istio-cves v0.0.0-20221007013142-0bde9b541ec8 h1:rUIvoAHokPcd92aJT2gJwVeyE8tMuaqS5l5s3cEgXFY= From 29d61833c8802d7037ea84f3977c4a525d8b9ead Mon Sep 17 00:00:00 2001 From: RTann Date: Mon, 12 Aug 2024 14:58:10 -0700 Subject: [PATCH 3/6] use release version --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 74bcd9d8a..0a1feb1f5 100644 --- a/go.mod +++ b/go.mod @@ -183,7 +183,7 @@ require ( replace ( github.com/facebookincubator/nvdtools => github.com/stackrox/nvdtools v0.0.0-20231111002313-57e262e4797e - github.com/heroku/docker-registry-client => github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989 + github.com/heroku/docker-registry-client => github.com/stackrox/docker-registry-client v0.2.1 // The current latest version of github.com/mholt/archiver/v3 (v3.5.1) suffers from CVE-2024-0406. // There is currently a PR in place to resolve it (https://github.com/mholt/archiver/pull/396), diff --git a/go.sum b/go.sum index 3603d78f3..d181df379 100644 --- a/go.sum +++ b/go.sum @@ -612,8 +612,8 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace h1:9PNP1jnUjRhfmGMlkXHjYPishpcw4jpSt/V/xYY3FMA= github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spf13/viper v1.10.0/go.mod h1:SoyBPwAtKDzypXNDFKN5kzH7ppppbGZtls1UpIy5AsM= -github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989 h1:uyRyi22LD2G//T3+ZuOZydwdHKx9nWTangB7vvAvfKA= -github.com/stackrox/docker-registry-client v0.2.1-0.20240809225852-a122013c5989/go.mod h1:4TU+pA11iczIvhtL0own2OJcJXc1o26tBHDivaXhlZU= +github.com/stackrox/docker-registry-client v0.2.1 h1:0SxojNYZXV2tXo1BxtxfQpiPZavAmw+B1aWT8CSSqYE= +github.com/stackrox/docker-registry-client v0.2.1/go.mod h1:4TU+pA11iczIvhtL0own2OJcJXc1o26tBHDivaXhlZU= github.com/stackrox/dotnet-scraper v0.0.0-20201023051640-72ef543323dd h1:vEjp7Q66zd4W72//Uk3uyVN50Mh/nFLbN9pb7CVK7mE= github.com/stackrox/dotnet-scraper v0.0.0-20201023051640-72ef543323dd/go.mod h1:HILeV3i/EyJz844GcrC3+oU7oZONhjfujaIYBMJ/bZE= github.com/stackrox/istio-cves v0.0.0-20221007013142-0bde9b541ec8 h1:rUIvoAHokPcd92aJT2gJwVeyE8tMuaqS5l5s3cEgXFY= From ca891ded41df019ea69f181c291fb149e7dee45c Mon Sep 17 00:00:00 2001 From: RTann Date: Mon, 12 Aug 2024 17:09:45 -0700 Subject: [PATCH 4/6] more validation --- pkg/scan/scan.go | 73 +++++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/pkg/scan/scan.go b/pkg/scan/scan.go index a8d91c39b..4eb9be2de 100644 --- a/pkg/scan/scan.go +++ b/pkg/scan/scan.go @@ -34,7 +34,9 @@ var manifestTypes = []string{ manifestV1.MediaTypeManifest, } -// analyzeLayers processes all of the layers and returns the lineage for the last layer so that we can uniquely identify it +// analyzeLayers processes all the layers and returns the lineage for the last layer so that we can uniquely identify it. +// +// It is the caller's responsibility to ensure the layers are valid digests. func analyzeLayers(storage database.Datastore, registry types.Registry, image *types.Image, layers []string, uncertifiedRHEL bool) (string, error) { var prevLayer string @@ -44,11 +46,8 @@ func analyzeLayers(storage database.Datastore, registry types.Registry, image *t for _, layer := range layers { layerReadCloser := &LayerDownloadReadCloser{ Downloader: func() (io.ReadCloser, error) { - dig, err := digest.Parse(layer) - if err != nil { - return nil, errors.Wrapf(err, "invalid layer digest %q", layer) - } - return registry.DownloadLayer(image.Remote, dig) + // It is assumed layer is a valid digest.Digest, so no need to parse and validate here. + return registry.DownloadLayer(image.Remote, digest.Digest(layer)) }, } @@ -82,14 +81,14 @@ func ProcessImage(storage database.Datastore, image *types.Image, registry, user if err != nil { return "", err } - digest, lineage, layer, err := process(storage, image, reg, uncertifiedRHEL) + dig, lineage, layer, err := process(storage, image, reg, uncertifiedRHEL) if err != nil { - return digest, err + return dig, err } if image.SHA == "" { - image.SHA = digest + image.SHA = dig } - return digest, storage.AddImage(layer, image.SHA, lineage, image.TaggedName(), &database.DatastoreOptions{ + return dig, storage.AddImage(layer, image.SHA, lineage, image.TaggedName(), &database.DatastoreOptions{ UncertifiedRHEL: uncertifiedRHEL, }) } @@ -97,21 +96,21 @@ func ProcessImage(storage database.Datastore, image *types.Image, registry, user // process fetches and analyzes the layers for the requested image returning the image digest, the lineage of the last layer and the last layer digest func process(storage database.Datastore, image *types.Image, reg types.Registry, uncertifiedRHEL bool) (string, string, string, error) { logrus.Debugf("Processing image %s", image) - digest, layers, err := FetchLayers(reg, image) + dig, layers, err := FetchLayers(reg, image) if err != nil { - return digest, "", "", err + return dig, "", "", errors.Errorf("fetching layers for %s: %v", image.String(), err) } if len(layers) == 0 { - return digest, "", "", fmt.Errorf("No layers to process for image %s", image.String()) + return dig, "", "", fmt.Errorf("no layers to process for image %s", image.String()) } logrus.Infof("Found %v layers for image %v", len(layers), image) lineage, err := analyzeLayers(storage, reg, image, layers, uncertifiedRHEL) if err != nil { logrus.Errorf("Failed to analyze image %q: %v", image.String(), err) - return digest, "", "", err + return dig, "", "", err } - return digest, lineage, layers[len(layers)-1], err + return dig, lineage, layers[len(layers)-1], err } func isEmptyLayer(layer string) bool { @@ -163,6 +162,9 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man return "", nil, errors.Errorf("no valid manifests found for %s:%s", remote, ref) } if len(manifests) == 1 { + if err := manifests[0].Digest.Validate(); err != nil { + return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", manifests[0].Digest, err) + } return handleManifest(reg, manifests[0].MediaType, remote, manifests[0].Digest.String()) } var amdManifest manifestlist.ManifestDescriptor @@ -173,6 +175,9 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man } // Matching platform for GOARCH takes priority so return immediately if m.Platform.Architecture == runtime.GOARCH { + if err := m.Digest.Validate(); err != nil { + return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", m.Digest, err) + } return handleManifest(reg, m.MediaType, remote, m.Digest.String()) } if m.Platform.Architecture == "amd64" { @@ -181,56 +186,60 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man } } if foundAMD { + if err := amdManifest.Digest.Validate(); err != nil { + return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", amdManifest.Digest, err) + } return handleManifest(reg, amdManifest.MediaType, remote, amdManifest.Digest.String()) } return "", nil, errors.Errorf("no manifest in list matched linux and amd64 or %s architectures: %s:%s", runtime.GOARCH, remote, ref) } -func handleManifest(reg types.Registry, manifestType, remote, ref string) (digest.Digest, []string, error) { +// handleManifest returns the image digest and layers from the given image and manifest type. +// The returned image digest and layers are valid unless the function returns a non-nil error. +// +// The caller is responsible for ensuring ref is a valid digest. +func handleManifest(reg types.Registry, manifestType, remote, ref string) (dig digest.Digest, layers []string, err error) { switch manifestType { case manifestV1.MediaTypeManifest: manifest, err := reg.Manifest(remote, ref) if err != nil { return "", nil, err } - dig, err := renderDigest(manifest) + dig, err = renderDigest(manifest) if err != nil { return "", nil, err } - layers := parseV1Layers(manifest) - return dig, layers, nil + layers = parseV1Layers(manifest) case manifestV1.MediaTypeSignedManifest: manifest, err := reg.SignedManifest(remote, ref) if err != nil { return "", nil, err } - dig, err := renderDigest(manifest) + dig, err = renderDigest(manifest) if err != nil { return "", nil, err } - layers := parseV1Layers(manifest) - return dig, layers, nil + layers = parseV1Layers(manifest) case manifestV2.MediaTypeManifest: manifest, err := reg.ManifestV2(remote, ref) if err != nil { return "", nil, err } - dig, err := renderDigest(manifest) + dig, err = renderDigest(manifest) if err != nil { return "", nil, err } - layers := parseLayers(manifest.Layers) - return dig, layers, nil + layers = parseLayers(manifest.Layers) case registry.MediaTypeImageManifest: manifest, err := reg.ManifestOCI(remote, ref) if err != nil { return "", nil, err } - dig, err := renderDigest(manifest) + dig, err = renderDigest(manifest) if err != nil { return "", nil, err } - return dig, parseLayers(manifest.Layers), nil + layers = parseLayers(manifest.Layers) case manifestlist.MediaTypeManifestList: manifestList, err := reg.ManifestList(remote, ref) if err != nil { @@ -244,8 +253,16 @@ func handleManifest(reg types.Registry, manifestType, remote, ref string) (diges } return handleManifestLists(reg, remote, ref, imageIndex.Manifests) default: - return "", nil, fmt.Errorf("Could not parse manifest type %q", manifestType) + return "", nil, fmt.Errorf("could not parse manifest type %q", manifestType) } + + for _, layer := range layers { + if _, err := digest.Parse(layer); err != nil { + return "", nil, errors.Errorf("invalid layer %s: %v", layer, err) + } + } + + return dig, layers, nil } // FetchLayers downloads the layers for the given image. From bf8ef9fea7e2a77ae780758066be9425248734da Mon Sep 17 00:00:00 2001 From: RTann Date: Wed, 14 Aug 2024 18:02:48 -0700 Subject: [PATCH 5/6] one more check and replace errorf with wrapf --- pkg/scan/scan.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/scan/scan.go b/pkg/scan/scan.go index 4eb9be2de..9aa37caa9 100644 --- a/pkg/scan/scan.go +++ b/pkg/scan/scan.go @@ -46,8 +46,11 @@ func analyzeLayers(storage database.Datastore, registry types.Registry, image *t for _, layer := range layers { layerReadCloser := &LayerDownloadReadCloser{ Downloader: func() (io.ReadCloser, error) { - // It is assumed layer is a valid digest.Digest, so no need to parse and validate here. - return registry.DownloadLayer(image.Remote, digest.Digest(layer)) + dig, err := digest.Parse(layer) + if err != nil { + return nil, errors.Wrapf(err, "invalid layer digest %q", layer) + } + return registry.DownloadLayer(image.Remote, dig) }, } @@ -98,7 +101,7 @@ func process(storage database.Datastore, image *types.Image, reg types.Registry, logrus.Debugf("Processing image %s", image) dig, layers, err := FetchLayers(reg, image) if err != nil { - return dig, "", "", errors.Errorf("fetching layers for %s: %v", image.String(), err) + return dig, "", "", errors.Wrapf(err, "fetching layers for image %s", image.String()) } if len(layers) == 0 { return dig, "", "", fmt.Errorf("no layers to process for image %s", image.String()) @@ -163,7 +166,7 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man } if len(manifests) == 1 { if err := manifests[0].Digest.Validate(); err != nil { - return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", manifests[0].Digest, err) + return "", nil, errors.Wrapf(err, "chosen manifest in list has invalid digest %s", manifests[0].Digest.String()) } return handleManifest(reg, manifests[0].MediaType, remote, manifests[0].Digest.String()) } @@ -176,7 +179,7 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man // Matching platform for GOARCH takes priority so return immediately if m.Platform.Architecture == runtime.GOARCH { if err := m.Digest.Validate(); err != nil { - return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", m.Digest, err) + return "", nil, errors.Wrapf(err, "chosen manifest in list has invalid digest %s", m.Digest.String()) } return handleManifest(reg, m.MediaType, remote, m.Digest.String()) } @@ -187,7 +190,7 @@ func handleManifestLists(reg types.Registry, remote, ref string, manifests []man } if foundAMD { if err := amdManifest.Digest.Validate(); err != nil { - return "", nil, errors.Errorf("chosen manifest in list has invalid digest %v: %v", amdManifest.Digest, err) + return "", nil, errors.Wrapf(err, "chosen manifest in list has invalid digest %s", amdManifest.Digest.String()) } return handleManifest(reg, amdManifest.MediaType, remote, amdManifest.Digest.String()) } @@ -258,7 +261,7 @@ func handleManifest(reg types.Registry, manifestType, remote, ref string) (dig d for _, layer := range layers { if _, err := digest.Parse(layer); err != nil { - return "", nil, errors.Errorf("invalid layer %s: %v", layer, err) + return "", nil, errors.Wrapf(err, "invalid layer %q", layer) } } From dd7e720bbea9485779bf7bf5b40caf5c27990635 Mon Sep 17 00:00:00 2001 From: RTann Date: Thu, 15 Aug 2024 12:35:51 -0700 Subject: [PATCH 6/6] update comment --- pkg/scan/scan.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/scan/scan.go b/pkg/scan/scan.go index 9aa37caa9..573fcc312 100644 --- a/pkg/scan/scan.go +++ b/pkg/scan/scan.go @@ -35,8 +35,6 @@ var manifestTypes = []string{ } // analyzeLayers processes all the layers and returns the lineage for the last layer so that we can uniquely identify it. -// -// It is the caller's responsibility to ensure the layers are valid digests. func analyzeLayers(storage database.Datastore, registry types.Registry, image *types.Image, layers []string, uncertifiedRHEL bool) (string, error) { var prevLayer string