From c200896a9644f9b6bd4bc3785c848276c33bb53c Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Thu, 9 May 2024 15:35:22 -0400 Subject: [PATCH] fix pruning binary packages when considering ELF packages (#2862) Signed-off-by: Alex Goodman --- .../package_binary_elf_relationships_test.go | 6 +- .../binary/binary_dependencies.go | 63 ++++++++------ .../binary/binary_dependencies_test.go | 82 +++++++++++-------- .../binary/shared_library_index_test.go | 4 +- 4 files changed, 96 insertions(+), 59 deletions(-) diff --git a/cmd/syft/internal/test/integration/package_binary_elf_relationships_test.go b/cmd/syft/internal/test/integration/package_binary_elf_relationships_test.go index c60aa5e02..354c7fba8 100644 --- a/cmd/syft/internal/test/integration/package_binary_elf_relationships_test.go +++ b/cmd/syft/internal/test/integration/package_binary_elf_relationships_test.go @@ -1,10 +1,12 @@ package integration import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/source" - "github.com/stretchr/testify/require" - "testing" ) func TestBinaryElfRelationships(t *testing.T) { diff --git a/internal/relationship/binary/binary_dependencies.go b/internal/relationship/binary/binary_dependencies.go index da51154b2..0c43ed074 100644 --- a/internal/relationship/binary/binary_dependencies.go +++ b/internal/relationship/binary/binary_dependencies.go @@ -4,7 +4,6 @@ import ( "path" "github.com/anchore/syft/internal/log" - "github.com/anchore/syft/internal/sbomsync" "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/file" @@ -67,34 +66,24 @@ func generateRelationships(resolver file.Resolver, accessor sbomsync.Accessor, i func PackagesToRemove(resolver file.Resolver, accessor sbomsync.Accessor) []artifact.ID { pkgsToDelete := make([]artifact.ID, 0) accessor.ReadFromSBOM(func(s *sbom.SBOM) { - // OTHER > ELF > Binary + // OTHER package type > ELF package type > Binary package type pkgsToDelete = append(pkgsToDelete, getBinaryPackagesToDelete(resolver, s)...) - pkgsToDelete = append(pkgsToDelete, compareElfBinaryPackages(resolver, s)...) + pkgsToDelete = append(pkgsToDelete, compareElfBinaryPackages(s)...) }) return pkgsToDelete } -func compareElfBinaryPackages(resolver file.Resolver, s *sbom.SBOM) []artifact.ID { +func compareElfBinaryPackages(s *sbom.SBOM) []artifact.ID { pkgsToDelete := make([]artifact.ID, 0) - for _, p := range s.Artifacts.Packages.Sorted(pkg.BinaryPkg) { - for _, loc := range p.Locations.ToSlice() { - if loc.Annotations[pkg.EvidenceAnnotationKey] != pkg.PrimaryEvidenceAnnotation { - continue - } - locations, err := resolver.FilesByPath(loc.RealPath) - if err != nil { - log.WithFields("error", err).Trace("unable to find path for owned file") - continue - } - for _, ownedL := range locations { - for _, pathPkg := range s.Artifacts.Packages.PackagesByPath(ownedL.RealPath) { - // we only care about comparing binary packages to each other (not other types) - if pathPkg.Type != pkg.BinaryPkg { - continue - } - if _, ok := pathPkg.Metadata.(pkg.ELFBinaryPackageNoteJSONPayload); !ok { - pkgsToDelete = append(pkgsToDelete, pathPkg.ID()) - } + for _, elfPkg := range allElfPackages(s) { + for _, loc := range onlyPrimaryEvidenceLocations(elfPkg) { + for _, otherPkg := range s.Artifacts.Packages.PackagesByPath(loc.RealPath) { + // we only care about comparing binary packages to each other (not other types) + if otherPkg.Type != pkg.BinaryPkg { + continue + } + if !isElfPackage(otherPkg) { + pkgsToDelete = append(pkgsToDelete, otherPkg.ID()) } } } @@ -102,6 +91,34 @@ func compareElfBinaryPackages(resolver file.Resolver, s *sbom.SBOM) []artifact.I return pkgsToDelete } +func onlyPrimaryEvidenceLocations(p pkg.Package) []file.Location { + var locs []file.Location + for _, loc := range p.Locations.ToSlice() { + if loc.Annotations[pkg.EvidenceAnnotationKey] != pkg.PrimaryEvidenceAnnotation { + continue + } + locs = append(locs, loc) + } + + return locs +} + +func allElfPackages(s *sbom.SBOM) []pkg.Package { + var elfPkgs []pkg.Package + for _, p := range s.Artifacts.Packages.Sorted(pkg.BinaryPkg) { + if !isElfPackage(p) { + continue + } + elfPkgs = append(elfPkgs, p) + } + return elfPkgs +} + +func isElfPackage(p pkg.Package) bool { + _, ok := p.Metadata.(pkg.ELFBinaryPackageNoteJSONPayload) + return ok +} + func getBinaryPackagesToDelete(resolver file.Resolver, s *sbom.SBOM) []artifact.ID { pkgsToDelete := make([]artifact.ID, 0) for p := range s.Artifacts.Packages.Enumerate() { diff --git a/internal/relationship/binary/binary_dependencies_test.go b/internal/relationship/binary/binary_dependencies_test.go index e43e3bfc1..92de9da88 100644 --- a/internal/relationship/binary/binary_dependencies_test.go +++ b/internal/relationship/binary/binary_dependencies_test.go @@ -34,13 +34,11 @@ func TestPackagesToRemove(t *testing.T) { glibCPackage.SetID() glibCBinaryELFPackage := pkg.Package{ - Name: "glibc", - Version: "", + Name: "glibc", Locations: file.NewLocationSet( file.NewLocation(glibcCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation), ), - Language: "", - Type: pkg.BinaryPkg, + Type: pkg.BinaryPkg, Metadata: pkg.ELFBinaryPackageNoteJSONPayload{ Type: "testfixture", Vendor: "syft", @@ -52,17 +50,25 @@ func TestPackagesToRemove(t *testing.T) { glibCBinaryELFPackage.SetID() glibCBinaryClassifierPackage := pkg.Package{ - Name: "glibc", - Version: "", + Name: "glibc", Locations: file.NewLocationSet( file.NewLocation(glibcCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation), ), - Language: "", Type: pkg.BinaryPkg, Metadata: pkg.BinarySignature{}, } glibCBinaryClassifierPackage.SetID() + libCBinaryClassifierPackage := pkg.Package{ + Name: "libc", + Locations: file.NewLocationSet( + file.NewLocation(glibcCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation), + ), + Type: pkg.BinaryPkg, + Metadata: pkg.BinarySignature{}, + } + libCBinaryClassifierPackage.SetID() + tests := []struct { name string resolver file.Resolver @@ -72,21 +78,33 @@ func TestPackagesToRemove(t *testing.T) { { name: "remove packages that are overlapping rpm --> binary", resolver: file.NewMockResolverForPaths(glibcCoordinate.RealPath), - accessor: newAccesor([]pkg.Package{glibCPackage, glibCBinaryELFPackage}, map[file.Coordinates]file.Executable{}, nil), + accessor: newAccessor([]pkg.Package{glibCPackage, glibCBinaryELFPackage}, map[file.Coordinates]file.Executable{}, nil), want: []artifact.ID{glibCBinaryELFPackage.ID()}, }, { name: "remove no packages when there is a single binary package", resolver: file.NewMockResolverForPaths(glibcCoordinate.RealPath), - accessor: newAccesor([]pkg.Package{glibCBinaryELFPackage}, map[file.Coordinates]file.Executable{}, nil), + accessor: newAccessor([]pkg.Package{glibCBinaryELFPackage}, map[file.Coordinates]file.Executable{}, nil), want: []artifact.ID{}, }, { name: "remove packages when there is a single binary package and a classifier package", resolver: file.NewMockResolverForPaths(glibcCoordinate.RealPath), - accessor: newAccesor([]pkg.Package{glibCBinaryELFPackage, glibCBinaryClassifierPackage}, map[file.Coordinates]file.Executable{}, nil), + accessor: newAccessor([]pkg.Package{glibCBinaryELFPackage, glibCBinaryClassifierPackage}, map[file.Coordinates]file.Executable{}, nil), want: []artifact.ID{glibCBinaryClassifierPackage.ID()}, }, + { + name: "ensure we're considering ELF packages, not just binary packages (supporting evidence)", + resolver: file.NewMockResolverForPaths(glibcCoordinate.RealPath), + accessor: newAccessor([]pkg.Package{glibCBinaryClassifierPackage}, map[file.Coordinates]file.Executable{}, nil), + want: []artifact.ID{}, + }, + { + name: "ensure we're considering ELF packages, not just binary packages (primary evidence)", + resolver: file.NewMockResolverForPaths(glibcCoordinate.RealPath), + accessor: newAccessor([]pkg.Package{libCBinaryClassifierPackage}, map[file.Coordinates]file.Executable{}, nil), + want: []artifact.ID{}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -103,7 +121,7 @@ func TestNewDependencyRelationships(t *testing.T) { glibcCoordinate := file.NewCoordinates("/usr/lib64/libc.so.6", "") secondGlibcCoordinate := file.NewCoordinates("/usr/local/lib64/libc.so.6", "") nestedLibCoordinate := file.NewCoordinates("/usr/local/bin/elftests/elfbinwithnestedlib/bin/elfbinwithnestedlib", "") - parrallelLibCoordinate := file.NewCoordinates("/usr/local/bin/elftests/elfbinwithsisterlib/bin/elfwithparallellibbin1", "") + parallelLibCoordinate := file.NewCoordinates("/usr/local/bin/elftests/elfbinwithsisterlib/bin/elfwithparallellibbin1", "") // rpm package that was discovered in linked section of the ELF binary package glibCPackage := pkg.Package{ @@ -151,7 +169,7 @@ func TestNewDependencyRelationships(t *testing.T) { FoundBy: "", Locations: file.NewLocationSet( file.NewLocation(nestedLibCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation), - file.NewLocation(parrallelLibCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation), + file.NewLocation(parallelLibCoordinate.RealPath).WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation), ), Language: "", Type: pkg.BinaryPkg, @@ -214,13 +232,13 @@ func TestNewDependencyRelationships(t *testing.T) { resolver: file.NewMockResolverForPaths( glibcCoordinate.RealPath, nestedLibCoordinate.RealPath, - parrallelLibCoordinate.RealPath, + parallelLibCoordinate.RealPath, ), // path -> executable (above mock resolver needs to be able to resolve to paths in this map) coordinateIndex: map[file.Coordinates]file.Executable{ - glibcCoordinate: glibcExecutable, - nestedLibCoordinate: syftTestFixtureExecutable, - parrallelLibCoordinate: syftTestFixtureExecutable2, + glibcCoordinate: glibcExecutable, + nestedLibCoordinate: syftTestFixtureExecutable, + parallelLibCoordinate: syftTestFixtureExecutable2, }, packages: []pkg.Package{glibCPackage, syftTestFixturePackage}, want: []artifact.Relationship{ @@ -237,13 +255,13 @@ func TestNewDependencyRelationships(t *testing.T) { glibcCoordinate.RealPath, secondGlibcCoordinate.RealPath, nestedLibCoordinate.RealPath, - parrallelLibCoordinate.RealPath, + parallelLibCoordinate.RealPath, ), coordinateIndex: map[file.Coordinates]file.Executable{ - glibcCoordinate: glibcExecutable, - secondGlibcCoordinate: glibcExecutable, - nestedLibCoordinate: syftTestFixtureExecutable, - parrallelLibCoordinate: syftTestFixtureExecutable2, + glibcCoordinate: glibcExecutable, + secondGlibcCoordinate: glibcExecutable, + nestedLibCoordinate: syftTestFixtureExecutable, + parallelLibCoordinate: syftTestFixtureExecutable2, }, packages: []pkg.Package{glibCPackage, glibCustomPackage, syftTestFixturePackage}, want: []artifact.Relationship{ @@ -264,12 +282,12 @@ func TestNewDependencyRelationships(t *testing.T) { resolver: file.NewMockResolverForPaths( glibcCoordinate.RealPath, nestedLibCoordinate.RealPath, - parrallelLibCoordinate.RealPath, + parallelLibCoordinate.RealPath, ), coordinateIndex: map[file.Coordinates]file.Executable{ - glibcCoordinate: glibcExecutable, - nestedLibCoordinate: syftTestFixtureExecutable, - parrallelLibCoordinate: syftTestFixtureExecutable2, + glibcCoordinate: glibcExecutable, + nestedLibCoordinate: syftTestFixtureExecutable, + parallelLibCoordinate: syftTestFixtureExecutable2, }, packages: []pkg.Package{glibCPackage, glibCustomPackage, syftTestFixturePackage}, prexistingRelationships: []artifact.Relationship{ @@ -285,9 +303,9 @@ func TestNewDependencyRelationships(t *testing.T) { name: "given a package that imports a library that is not tracked by the resolver, expect no relationships to be created", resolver: file.NewMockResolverForPaths(), coordinateIndex: map[file.Coordinates]file.Executable{ - glibcCoordinate: glibcExecutable, - nestedLibCoordinate: syftTestFixtureExecutable, - parrallelLibCoordinate: syftTestFixtureExecutable2, + glibcCoordinate: glibcExecutable, + nestedLibCoordinate: syftTestFixtureExecutable, + parallelLibCoordinate: syftTestFixtureExecutable2, }, packages: []pkg.Package{glibCPackage, syftTestFixturePackage}, want: []artifact.Relationship{}, @@ -295,7 +313,7 @@ func TestNewDependencyRelationships(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - accessor := newAccesor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) + accessor := newAccessor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) // given a resolver that knows about the paths of the packages and executables, // and given an SBOM accessor that knows about the packages and executables, // we should be able to create a set of relationships between the packages and executables @@ -316,7 +334,7 @@ func relationshipComparer(x, y []artifact.Relationship) string { )) } -func newAccesor(pkgs []pkg.Package, coordinateIndex map[file.Coordinates]file.Executable, prexistingRelationships []artifact.Relationship) sbomsync.Accessor { +func newAccessor(pkgs []pkg.Package, coordinateIndex map[file.Coordinates]file.Executable, preexistingRelationships []artifact.Relationship) sbomsync.Accessor { sb := sbom.SBOM{ Artifacts: sbom.Artifacts{ Packages: pkg.NewCollection(), @@ -329,8 +347,8 @@ func newAccesor(pkgs []pkg.Package, coordinateIndex map[file.Coordinates]file.Ex accessor := builder.(sbomsync.Accessor) accessor.WriteToSBOM(func(s *sbom.SBOM) { s.Artifacts.Executables = coordinateIndex - if prexistingRelationships != nil { - s.Relationships = prexistingRelationships + if preexistingRelationships != nil { + s.Relationships = preexistingRelationships } }) return accessor diff --git a/internal/relationship/binary/shared_library_index_test.go b/internal/relationship/binary/shared_library_index_test.go index 0594067d6..fadaaa303 100644 --- a/internal/relationship/binary/shared_library_index_test.go +++ b/internal/relationship/binary/shared_library_index_test.go @@ -27,7 +27,7 @@ func Test_newShareLibIndex(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - accessor := newAccesor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) + accessor := newAccessor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) sharedLibraryIndex := newShareLibIndex(tt.resolver, accessor) if sharedLibraryIndex == nil { t.Errorf("newShareLibIndex() = %v, want non-nil", sharedLibraryIndex) @@ -93,7 +93,7 @@ func Test_sharedLibraryIndex_build(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - accessor := newAccesor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) + accessor := newAccessor(tt.packages, tt.coordinateIndex, tt.prexistingRelationships) sharedLibraryIndex := newShareLibIndex(tt.resolver, accessor) sharedLibraryIndex.build(tt.resolver, accessor) pkgs := sharedLibraryIndex.owningLibraryPackage(path.Base(glibcCoordinate.RealPath))