From 9234b2e92055178758fed5f298f5077d01f55a60 Mon Sep 17 00:00:00 2001 From: "Adam D. Cornett" Date: Thu, 22 Jun 2023 15:13:26 -0700 Subject: [PATCH] adding rpm architecture of to our maps, so 32 bit and 64 bit rpms do not fail has modified files check Signed-off-by: Adam D. Cornett --- .../policy/container/has_modified_files.go | 51 ++++++++++++++--- .../container/has_modified_files_test.go | 57 +++++++++++++++++++ ...ner-fails-modifiedfiles-package.Dockerfile | 2 +- ...iner-fails-modifiedfiles-setuid.Dockerfile | 17 ++++++ ...tainer-passes-multiple-arch-rpm.Dockerfile | 17 ++++++ 5 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 test/containerfiles/container-fails-modifiedfiles-setuid.Dockerfile create mode 100644 test/containerfiles/container-passes-multiple-arch-rpm.Dockerfile diff --git a/internal/policy/container/has_modified_files.go b/internal/policy/container/has_modified_files.go index ce8c4b15..497157d9 100644 --- a/internal/policy/container/has_modified_files.go +++ b/internal/policy/container/has_modified_files.go @@ -231,17 +231,27 @@ func (p *HasModifiedFilesCheck) validate(ctx context.Context, layerIDs []string, } // Nope, nope, nope. File was modified without using RPM - logger.V(log.DBG).Info("found disallowed modification in layer", "file", modifiedFile) + logger.Info("found disallowed modification in layer", "file", modifiedFile) disallowedModifications = true continue } - // Check that release contains the same arch and OS release + + // Check that release contains the same arch, this is to ensure that a package did not get rebuilt with + // a different architecture previousOsRelease := strings.Contains(previousPackage.Release, packageDist) currentOsRelease := strings.Contains(currentPackage.Release, packageDist) - if (previousOsRelease && !currentOsRelease) || (previousPackage.Arch != currentPackage.Arch) { - // If either of these differ, that's a fail - return false, nil + if previousOsRelease && !currentOsRelease { + logger.Info("mismatch in OS release", "file", modifiedFile) + disallowedModifications = true + continue + } + + // Check that the architectures for previous version and current version of a given package match + if previousPackage.Arch != currentPackage.Arch { + logger.Info("mismatch in package architecture", "file", modifiedFile) + disallowedModifications = true + continue } // This appears like an update. This is allowed. @@ -274,7 +284,7 @@ func (p HasModifiedFilesCheck) Metadata() check.Metadata { func extractPackageNameVersionRelease(pkgList []*rpmdb.PackageInfo) map[string]packageMeta { pkgNameList := make(map[string]packageMeta, len(pkgList)) for _, pkg := range pkgList { - pkgNameList[fmt.Sprintf("%s-%s-%s", pkg.Name, pkg.Version, pkg.Release)] = packageMeta{ + pkgNameList[strings.Join([]string{pkg.Name, pkg.Version, pkg.Release, pkg.Arch}, "-")] = packageMeta{ Name: pkg.Name, Version: pkg.Version, Release: pkg.Release, @@ -393,19 +403,46 @@ func installedFileMapWithExclusions(ctx context.Context, pkglist []*rpmdb.Packag return m, err } + // converting directories to a map so we can filter them out quicker + pkgDirNamesMap := make(map[string]struct{}) + for _, dir := range pkg.DirNames { + pkgDirNamesMap[dir] = struct{}{} + } + for _, file := range files { + if _, found := pkgDirNamesMap[file.Path]; found { + // The file is a directory. Skip it. + continue + } + if int32(file.Flags)&okFlags > 0 { // It is one of the ok flags. Skip it. continue } + normalized := normalize(file.Path) if pathIsExcluded(ctx, normalized) || directoryIsExcluded(ctx, normalized) || prefixAndSuffixIsExcluded(ctx, normalized) { // It is either an explicitly excluded path or directory. Skip it. continue } - m[normalized] = fmt.Sprintf("%s-%s-%s", pkg.Name, pkg.Version, pkg.Release) + + // checking to see if the file is already in the map. + // check to see if all attributes of the rpm match except architecture. + // this is to support cross architecture file ownership, + // the 2nd architecture we encounter, we can skip it. + if val, found := m[normalized]; found { + s := strings.Split(val, "-") + name, version, release, arch := s[0], s[1], s[2], s[3] + + if name == pkg.Name && version == pkg.Version && release == pkg.Release && arch != pkg.Arch { + continue + } + } + + m[normalized] = strings.Join([]string{pkg.Name, pkg.Version, pkg.Release, pkg.Arch}, "-") } } + return m, nil } diff --git a/internal/policy/container/has_modified_files_test.go b/internal/policy/container/has_modified_files_test.go index c8209a51..03190d86 100644 --- a/internal/policy/container/has_modified_files_test.go +++ b/internal/policy/container/has_modified_files_test.go @@ -243,6 +243,37 @@ var _ = Describe("HasModifiedFiles", func() { Expect(ok).To(BeFalse()) }) }) + When("the package architecture changes", func() { + var pkgs map[string]packageFilesRef + BeforeEach(func() { + pkgs = pkgRef + + pkgSecondLayerPackageFiles := pkgs["secondlayer"].LayerPackageFiles + delete(pkgSecondLayerPackageFiles, "this") + pkgSecondLayerPackageFiles["this"] = "foo-1.0-1.d10" + + pkgSecondLayerPackages := pkgs["secondlayer"].LayerPackages + delete(pkgSecondLayerPackages, "foo-1.0-1.d9") + pkgSecondLayerPackages["foo-1.0-1.d10"] = packageMeta{ + Name: "foo", + Version: "1.0", + Release: "1.d9", + Arch: "differentarch", + } + + pkgs["secondlayer"] = packageFilesRef{ + LayerPackages: pkgSecondLayerPackages, + LayerPackageFiles: pkgSecondLayerPackageFiles, + LayerFiles: append(pkgs["secondlayer"].LayerFiles, "this"), + HasRPMDB: true, + } + }) + It("should fail because of different architectures dist", func() { + ok, err := hasModifiedFiles.validate(context.Background(), layers, pkgs, dist) + Expect(err).ToNot(HaveOccurred()) + Expect(ok).To(BeFalse()) + }) + }) When("release dist does not match installed OS", func() { When("package is a net-new", func() { When("a file is modified", func() { @@ -321,11 +352,37 @@ var _ = Describe("HasModifiedFiles", func() { var goodPkgList []*rpmdb.PackageInfo BeforeEach(func() { + // goodPkgList represents three mock RPM's, + // the first is a basic one to cover the happy path. + // the second is need to test our filtering logic for files from another architecture. + // the third is to test our filtering logic for directories. goodPkgList = []*rpmdb.PackageInfo{ { BaseNames: []string{basename}, DirIndexes: []int32{dirindex}, DirNames: []string{dirname}, + Name: "foo", + Version: "1.0.0", + Release: "100", + Arch: "x86_64", + }, + { + BaseNames: []string{basename}, + DirIndexes: []int32{dirindex}, + DirNames: []string{dirname}, + Name: "foo", + Version: "1.0.0", + Release: "100", + Arch: "i686", + }, + { + BaseNames: []string{""}, + DirIndexes: []int32{dirindex}, + DirNames: []string{"/"}, + Name: "just-dirs", + Version: "1.0.0", + Release: "100", + Arch: "x86_64", }, } }) diff --git a/test/containerfiles/container-fails-modifiedfiles-package.Dockerfile b/test/containerfiles/container-fails-modifiedfiles-package.Dockerfile index d6c49c04..1d07086c 100644 --- a/test/containerfiles/container-fails-modifiedfiles-package.Dockerfile +++ b/test/containerfiles/container-fails-modifiedfiles-package.Dockerfile @@ -11,7 +11,7 @@ LABEL name="preflight test image" \ summary="testing the preflight tool" \ description="test the preflight tool" -RUN dnf remove -y tar +RUN rm -f /bin/true USER preflightuser diff --git a/test/containerfiles/container-fails-modifiedfiles-setuid.Dockerfile b/test/containerfiles/container-fails-modifiedfiles-setuid.Dockerfile new file mode 100644 index 00000000..bedabc13 --- /dev/null +++ b/test/containerfiles/container-fails-modifiedfiles-setuid.Dockerfile @@ -0,0 +1,17 @@ +FROM registry.access.redhat.com/ubi8/ubi:latest + +RUN useradd preflightuser + +COPY --chown=preflightuser:preflightuser example-license.txt /licenses/ + +LABEL name="preflight test image" \ + vendor="preflight test vendor" \ + version="1" \ + release="1" \ + summary="testing the preflight tool" \ + description="test the preflight tool" + +RUN find / -xdev -perm -4000 -exec chmod ug-s {} + + +USER preflightuser + diff --git a/test/containerfiles/container-passes-multiple-arch-rpm.Dockerfile b/test/containerfiles/container-passes-multiple-arch-rpm.Dockerfile new file mode 100644 index 00000000..5e90411d --- /dev/null +++ b/test/containerfiles/container-passes-multiple-arch-rpm.Dockerfile @@ -0,0 +1,17 @@ +FROM registry.access.redhat.com/ubi8/ubi:8.7-1112 + +RUN useradd preflightuser + +COPY --chown=preflightuser:preflightuser example-license.txt /licenses/ + +LABEL name="preflight test image container-policy" \ + vendor="preflight test vendor" \ + version="1" \ + release="1" \ + summary="testing the preflight tool" \ + description="test the preflight tool" + +RUN dnf update glibc -y && dnf install glibc.i686 -y + +USER preflightuser +