Skip to content

Commit 59d29ba

Browse files
committed
fix(security): protect shared SBOM Data with per-Data lock
Data races found via a race-detector-enabled build in staging (see #54333). The *Data object is shared across SBOMs that run the same image via the dataCache, but each SBOM has its own sync.RWMutex. So sbomA.Lock() and sbomB.Lock() don't protect the shared Data.packages slice. The forwarding debouncer's copy(packages, sbom.data.packages) races with ResolvePackage's pkg.LastAccess = time.Now() when two containers share the same image. Fix: add a sync.RWMutex to Data. Take data.mu.RLock() in the forwarding snapshot (reader) and data.mu.Lock() in ResolvePackage and processPendingFileEvents (writers). This mirrors the existing pattern in fixedSizeQueue, which already has its own lock for the same reason.
1 parent 768df0b commit 59d29ba

2 files changed

Lines changed: 68 additions & 1 deletion

File tree

pkg/security/resolvers/sbom/resolver.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,10 @@ const (
7979
)
8080

8181
// Data use the keep the result of a scan of a same workload across multiple
82-
// container
82+
// container. A *Data is shared across SBOMs that run the same image (via the
83+
// dataCache), so it needs its own lock rather than relying on each SBOM's lock.
8384
type Data struct {
85+
mu sync.RWMutex
8486
files fileQuerier
8587
packages []sbomtypes.Package // per-package metadata (without the plain-text installed-file lists) kept for forwarding
8688
}
@@ -435,8 +437,12 @@ func (r *Resolver) triggerForwarding(sbom *SBOM) {
435437

436438
// Snapshot the package metadata: the forwarded report outlives the
437439
// lock and the backing slice keeps being mutated (LastAccess) at runtime.
440+
// Take the Data lock because *Data is shared across SBOMs via the data
441+
// cache, so the SBOM lock alone does not protect the packages slice.
442+
sbom.data.mu.RLock()
438443
packages := make([]sbomtypes.Package, len(sbom.data.packages))
439444
copy(packages, sbom.data.packages)
445+
sbom.data.mu.RUnlock()
440446

441447
// Create SBOM report and notify listeners
442448
packagesReport := NewPackagesReport(packages, sbom.ContainerID)
@@ -776,6 +782,7 @@ func (r *Resolver) ResolvePackage(pc *model.ProcessContext, file *model.FileEven
776782
if pkg != nil {
777783
seclog.Tracef("file '%s' found in sbom for container '%s'", file.PathnameStr, sbom.ContainerID)
778784

785+
sbom.data.mu.Lock()
779786
oldLastAccess := pkg.LastAccess
780787
oldSuidBit := pkg.SuidBit
781788
oldAccessedByRoot := pkg.AccessedByRoot
@@ -787,6 +794,7 @@ func (r *Resolver) ResolvePackage(pc *model.ProcessContext, file *model.FileEven
787794
pkg.LastAccess = time.Now()
788795
pkg.SuidBit = pkg.SuidBit || fs.FileMode(file.Mode)&04000 != 0
789796
pkg.AccessedByRoot = pkg.AccessedByRoot || pc.UID == 0
797+
sbom.data.mu.Unlock()
790798

791799
// Trigger forwarding debouncer to send updated SBOM to remote collector
792800
if pkg.LastAccess.Sub(oldLastAccess) > r.cfg.SBOMResolverEnrichmentInterval ||
@@ -855,6 +863,8 @@ func (r *Resolver) processPendingFileEvents(sbom *SBOM) {
855863
seclog.Debugf("processing %d pending file events for container '%s'", len(events), sbom.ContainerID)
856864

857865
now := time.Now()
866+
sbom.data.mu.Lock()
867+
defer sbom.data.mu.Unlock()
858868
for filePath, event := range events {
859869
pkg := sbom.data.files.queryFile(filePath)
860870
if pkg == nil {

pkg/security/resolvers/sbom/resolver_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ package sbom
99

1010
import (
1111
"fmt"
12+
"sync"
1213
"testing"
1314
"time"
1415

@@ -322,3 +323,59 @@ func TestProcessPendingFileEventsEnrichesPackages(t *testing.T) {
322323
t.Errorf("sbom was not marked for forwarding")
323324
}
324325
}
326+
327+
// TestSharedDataConcurrentForwardingAndResolve checks that the forwarding snapshot
328+
// (copy of packages) and the package enrichment (LastAccess/SuidBit/AccessedByRoot
329+
// writes) are safe when they run on different SBOMs sharing the same *Data via the
330+
// dataCache. The SBOM lock is per-container, so without the Data-level lock the
331+
// copy and the writes race on the shared packages slice.
332+
func TestSharedDataConcurrentForwardingAndResolve(t *testing.T) {
333+
data := newData([]sbomtypes.PackageWithInstalledFiles{{
334+
Package: sbomtypes.Package{Name: "shadow-utils"},
335+
InstalledFiles: []string{"/usr/bin/su"},
336+
}}, false)
337+
338+
sbomA := NewSBOM("container-a", nil, "image:tag")
339+
sbomA.data = data
340+
sbomA.state.Store(computedState)
341+
342+
sbomB := NewSBOM("container-b", nil, "image:tag")
343+
sbomB.data = data
344+
sbomB.state.Store(computedState)
345+
346+
r := newPendingFileEventsResolver(t)
347+
348+
var wg sync.WaitGroup
349+
wg.Add(2)
350+
351+
// Writer: simulate ResolvePackage enriching packages on sbomA (writes
352+
// LastAccess/SuidBit/AccessedByRoot on the shared Data).
353+
go func() {
354+
defer wg.Done()
355+
for range 2000 {
356+
r.queuePendingFileEvent("container-a", "/usr/bin/su", 04755, 0)
357+
sbomA.Lock()
358+
r.processPendingFileEvents(sbomA)
359+
sbomA.Unlock()
360+
}
361+
}()
362+
363+
// Reader: simulate triggerForwarding.func1 snapshotting packages on sbomB
364+
// (reads the shared Data via copy()).
365+
go func() {
366+
defer wg.Done()
367+
for range 2000 {
368+
sbomB.Lock()
369+
if sbomB.data != nil && len(sbomB.data.packages) > 0 {
370+
sbomB.data.mu.RLock()
371+
packages := make([]sbomtypes.Package, len(sbomB.data.packages))
372+
copy(packages, sbomB.data.packages)
373+
sbomB.data.mu.RUnlock()
374+
_ = packages
375+
}
376+
sbomB.Unlock()
377+
}
378+
}()
379+
380+
wg.Wait()
381+
}

0 commit comments

Comments
 (0)