From bbf3bb585691f3d784176f898c64dc56f70cd649 Mon Sep 17 00:00:00 2001 From: Dan Luhring Date: Fri, 23 May 2025 14:33:19 -0400 Subject: [PATCH] fix(relationship): favor real paths over symlinks for ownership by file (#3923) Signed-off-by: Dan Luhring Signed-off-by: Keith Zantow Co-authored-by: Keith Zantow --- internal/relationship/by_file_ownership.go | 69 +++++++++++++++++-- .../relationship/by_file_ownership_test.go | 58 ++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/internal/relationship/by_file_ownership.go b/internal/relationship/by_file_ownership.go index 7d26e89fc..92b2ddc49 100644 --- a/internal/relationship/by_file_ownership.go +++ b/internal/relationship/by_file_ownership.go @@ -92,6 +92,12 @@ func findOwnershipByFilesRelationships(resolver file.Resolver, catalog *pkg.Coll return relationships } + // Build a map of real paths to packages that directly own them. We'll use this + // to check if a file is already owned by the same type of package when we're + // determining ownership via symlink. + directOwnership := directOwnersByPath(catalog) + + // Now establish relationships, considering symlinks for _, candidateOwnerPkg := range catalog.Sorted() { id := candidateOwnerPkg.ID() if candidateOwnerPkg.Metadata == nil { @@ -105,16 +111,34 @@ func findOwnershipByFilesRelationships(resolver file.Resolver, catalog *pkg.Coll } for _, ownedFilePath := range pkgFileOwner.OwnedFiles() { - // find the first path that results in a hit - for _, ownedPath := range allPaths(ownedFilePath, resolver) { - if matchesAny(ownedPath, globsForbiddenFromBeingOwned) { + if ownedFilePath == "" { + continue + } + + // find paths that result in a hit (includes resolving symlinks) + resolvedPaths := resolvePaths(ownedFilePath, resolver) + + for _, resolvedPath := range resolvedPaths { + if matchesAny(resolvedPath, globsForbiddenFromBeingOwned) { // we skip over known exceptions to file ownership, such as the RPM package owning // the RPM DB path, otherwise the RPM package would "own" all RPMs, which is not intended continue } + // Skip claiming ownership via symlink if another package of the same type + // directly owns this real path. This is the specific fix for the issue where a + // symlink shouldn't allow a package to claim ownership when another package of + // the same type directly owns the real file. + if resolvedPath != ownedFilePath { // This is a resolved symlink path + // Check if another package of the same type directly owns this path + if paths := directOwnership[candidateOwnerPkg.Type]; paths != nil && paths.Has(resolvedPath) { + // Skip this path - a package of the same type directly owns it + continue + } + } + // look for package(s) in the catalog that may be owned by this package and mark the relationship - for _, subPackage := range catalog.PackagesByPath(ownedPath) { + for _, subPackage := range catalog.PackagesByPath(resolvedPath) { subID := subPackage.ID() if subID == id { continue @@ -126,7 +150,7 @@ func findOwnershipByFilesRelationships(resolver file.Resolver, catalog *pkg.Coll if _, exists := relationships[id][subID]; !exists { relationships[id][subID] = strset.New() } - relationships[id][subID].Add(ownedPath) + relationships[id][subID].Add(resolvedPath) } } } @@ -135,7 +159,40 @@ func findOwnershipByFilesRelationships(resolver file.Resolver, catalog *pkg.Coll return relationships } -func allPaths(ownedFilePath string, resolver file.Resolver) []string { +func directOwnersByPath(catalog *pkg.Collection) map[pkg.Type]*strset.Set { + directOwnership := map[pkg.Type]*strset.Set{} + + // First, identify direct ownership of all files + for _, p := range catalog.Sorted() { + if p.Metadata == nil { + continue + } + + // check to see if this is a file owner + pkgFileOwner, ok := p.Metadata.(pkg.FileOwner) + if !ok { + continue + } + + for _, ownedFilePath := range pkgFileOwner.OwnedFiles() { + if ownedFilePath == "" { + continue + } + + // Register direct ownership + paths := directOwnership[p.Type] + if paths == nil { + paths = strset.New() + directOwnership[p.Type] = paths + } + paths.Add(ownedFilePath) + } + } + + return directOwnership +} + +func resolvePaths(ownedFilePath string, resolver file.Resolver) []string { // though we have a string path, we need to resolve symlinks and other filesystem oddities since we cannot assume this is a real path var locs []file.Location var err error diff --git a/internal/relationship/by_file_ownership_test.go b/internal/relationship/by_file_ownership_test.go index 2afb89ff5..33aab7146 100644 --- a/internal/relationship/by_file_ownership_test.go +++ b/internal/relationship/by_file_ownership_test.go @@ -78,6 +78,64 @@ func TestOwnershipByFilesRelationship(t *testing.T) { return []pkg.Package{parent, child}, []artifact.Relationship{relationship} }, }, + { + name: "only-real-file-owner-gets-ownership-relationship-not-symlink-owner", + resolver: mockFR{ + translate: map[string]string{ + "/usr/bin/jenkins.war-symlink": "/usr/share/jenkins/jenkins.war", // symlink to the real file + }, + }, + setup: func(t testing.TB) ([]pkg.Package, []artifact.Relationship) { + // Package that owns the symlink + symlinkOwner := pkg.Package{ + Type: pkg.ApkPkg, + Metadata: pkg.ApkDBEntry{ + Package: "jenkins-symlink-pkg", + Files: []pkg.ApkFileRecord{ + {Path: "/usr/bin/jenkins.war-symlink"}, // this symlinks to the real file + }, + }, + } + symlinkOwner.SetID() + + // Package that owns the real file + realFileOwner := pkg.Package{ + Type: pkg.ApkPkg, + Metadata: pkg.ApkDBEntry{ + Package: "jenkins-real-pkg", + Files: []pkg.ApkFileRecord{ + {Path: "/usr/share/jenkins/jenkins.war"}, // the real file + }, + }, + } + realFileOwner.SetID() + + // A third package that is "located at" the real file path + // This simulates a package being discovered at the jenkins.war location + consumerPackage := pkg.Package{ + Type: pkg.JavaPkg, + Locations: file.NewLocationSet( + file.NewVirtualLocation("/usr/share/jenkins/jenkins.war", "/usr/share/jenkins/jenkins.war"), + ), + } + consumerPackage.SetID() + + // Only the real file owner should have an ownership relationship with the consumer package + // The symlink owner should NOT have a relationship with the consumer + relationship := artifact.Relationship{ + From: realFileOwner, + To: consumerPackage, + Type: artifact.OwnershipByFileOverlapRelationship, + Data: ownershipByFilesMetadata{ + Files: []string{ + "/usr/share/jenkins/jenkins.war", + }, + }, + } + + return []pkg.Package{symlinkOwner, realFileOwner, consumerPackage}, []artifact.Relationship{relationship} + }, + }, { name: "misses-by-dead-symlink", resolver: mockFR{