diff --git a/pkg/security/resolvers/sbom/resolver.go b/pkg/security/resolvers/sbom/resolver.go index d73c8165cded..2047f05e0db6 100644 --- a/pkg/security/resolvers/sbom/resolver.go +++ b/pkg/security/resolvers/sbom/resolver.go @@ -79,8 +79,10 @@ const ( ) // Data use the keep the result of a scan of a same workload across multiple -// container +// container. A *Data is shared across SBOMs that run the same image (via the +// dataCache), so it needs its own lock rather than relying on each SBOM's lock. type Data struct { + mu sync.RWMutex files fileQuerier packages []sbomtypes.Package // per-package metadata (without the plain-text installed-file lists) kept for forwarding } @@ -435,8 +437,12 @@ func (r *Resolver) triggerForwarding(sbom *SBOM) { // Snapshot the package metadata: the forwarded report outlives the // lock and the backing slice keeps being mutated (LastAccess) at runtime. + // Take the Data lock because *Data is shared across SBOMs via the data + // cache, so the SBOM lock alone does not protect the packages slice. + sbom.data.mu.RLock() packages := make([]sbomtypes.Package, len(sbom.data.packages)) copy(packages, sbom.data.packages) + sbom.data.mu.RUnlock() // Create SBOM report and notify listeners packagesReport := NewPackagesReport(packages, sbom.ContainerID) @@ -776,6 +782,7 @@ func (r *Resolver) ResolvePackage(pc *model.ProcessContext, file *model.FileEven if pkg != nil { seclog.Tracef("file '%s' found in sbom for container '%s'", file.PathnameStr, sbom.ContainerID) + sbom.data.mu.Lock() oldLastAccess := pkg.LastAccess oldSuidBit := pkg.SuidBit oldAccessedByRoot := pkg.AccessedByRoot @@ -787,10 +794,17 @@ func (r *Resolver) ResolvePackage(pc *model.ProcessContext, file *model.FileEven pkg.LastAccess = time.Now() pkg.SuidBit = pkg.SuidBit || fs.FileMode(file.Mode)&04000 != 0 pkg.AccessedByRoot = pkg.AccessedByRoot || pc.UID == 0 + // Snapshot the updated values before unlocking so the invalidation + // comparison below doesn't race with concurrent writers on the shared + // *Data (another container resolving a file from the same package). + newLastAccess := pkg.LastAccess + newSuidBit := pkg.SuidBit + newAccessedByRoot := pkg.AccessedByRoot + sbom.data.mu.Unlock() // Trigger forwarding debouncer to send updated SBOM to remote collector - if pkg.LastAccess.Sub(oldLastAccess) > r.cfg.SBOMResolverEnrichmentInterval || - pkg.SuidBit != oldSuidBit || pkg.AccessedByRoot != oldAccessedByRoot { + if newLastAccess.Sub(oldLastAccess) > r.cfg.SBOMResolverEnrichmentInterval || + newSuidBit != oldSuidBit || newAccessedByRoot != oldAccessedByRoot { sbom.invalidated = true } } @@ -855,6 +869,8 @@ func (r *Resolver) processPendingFileEvents(sbom *SBOM) { seclog.Debugf("processing %d pending file events for container '%s'", len(events), sbom.ContainerID) now := time.Now() + sbom.data.mu.Lock() + defer sbom.data.mu.Unlock() for filePath, event := range events { pkg := sbom.data.files.queryFile(filePath) if pkg == nil { diff --git a/pkg/security/resolvers/sbom/resolver_test.go b/pkg/security/resolvers/sbom/resolver_test.go index ee3cc7efde66..95c44ff298bd 100644 --- a/pkg/security/resolvers/sbom/resolver_test.go +++ b/pkg/security/resolvers/sbom/resolver_test.go @@ -9,6 +9,7 @@ package sbom import ( "fmt" + "sync" "testing" "time" @@ -322,3 +323,56 @@ func TestProcessPendingFileEventsEnrichesPackages(t *testing.T) { t.Errorf("sbom was not marked for forwarding") } } + +// TestSharedDataConcurrentForwardingAndResolve checks that the forwarding snapshot +// (copy of packages) and the package enrichment (LastAccess/SuidBit/AccessedByRoot +// writes) are safe when they run on different SBOMs sharing the same *Data via the +// dataCache. The SBOM lock is per-container, so without the Data-level lock the +// copy and the writes race on the shared packages slice. +func TestSharedDataConcurrentForwardingAndResolve(t *testing.T) { + data := newData([]sbomtypes.PackageWithInstalledFiles{{ + Package: sbomtypes.Package{Name: "shadow-utils"}, + InstalledFiles: []string{"/usr/bin/su"}, + }}, false) + + sbomA := NewSBOM("container-a", nil, "image:tag") + sbomA.data = data + sbomA.state.Store(computedState) + + sbomB := NewSBOM("container-b", nil, "image:tag") + sbomB.data = data + sbomB.state.Store(computedState) + + r := newPendingFileEventsResolver(t) + + var wg sync.WaitGroup + + // Writer: simulate ResolvePackage enriching packages on sbomA (writes + // LastAccess/SuidBit/AccessedByRoot on the shared Data). + wg.Go(func() { + for range 2000 { + r.queuePendingFileEvent("container-a", "/usr/bin/su", 04755, 0) + sbomA.Lock() + r.processPendingFileEvents(sbomA) + sbomA.Unlock() + } + }) + + // Reader: simulate triggerForwarding.func1 snapshotting packages on sbomB + // (reads the shared Data via copy()). + wg.Go(func() { + for range 2000 { + sbomB.Lock() + if sbomB.data != nil && len(sbomB.data.packages) > 0 { + sbomB.data.mu.RLock() + packages := make([]sbomtypes.Package, len(sbomB.data.packages)) + copy(packages, sbomB.data.packages) + sbomB.data.mu.RUnlock() + _ = packages + } + sbomB.Unlock() + } + }) + + wg.Wait() +}