Non-thread-safe Dictionary in ScriptTrustRegistry under concurrent invalidation

[michaellwest]

M1: Non-Thread-Safe Dictionary in ScriptTrustRegistry Under Concurrent Invalidation

Severity: MEDIUM
Category: Concurrency / Privilege Escalation
File: src/Spe/Core/Settings/Authorization/ScriptTrustRegistry.cs
Lines: 22–26 (dictionary declarations), 120 (Invalidate method)

---

Risk Explanation

ScriptTrustRegistry uses plain Dictionary<string, ScriptTrustEntry> for _entriesById and _entriesByName (lines 22–26). These dictionaries are populated once under lock during EnsureInitialized() and then read concurrently by multiple request threads without locking.

The Invalidate() method (line 120) acquires the lock and calls .Clear() on both dictionaries, then sets _initialized = false. The critical problem: a concurrent read that already passed the EnsureInitialized() check can be iterating the dictionary's internal bucket array while Clear() is modifying it. Dictionary<K,V> is not thread-safe for concurrent read + write — the .NET documentation explicitly states this produces undefined behavior.

Failure modes:

• Corrupted read: TryGetValue on a partially-cleared dictionary may return a default entry, causing an untrusted script to appear trusted (false positive).
• NullReferenceException or IndexOutOfRangeException: Internal bucket corruption can throw during enumeration, which may either block script execution or fall through to a default-allow path.
• Stale positive after invalidation: Thread A reads _entriesById[scriptId] = trusted just before Thread B clears the dictionary. Thread A proceeds to execute the now-untrusted script.

Trigger: Any Sitecore item save that fires the trust invalidation event while a concurrent request is checking script trust.

---

Implementation Plan

Fix: Atomic dictionary reference swap (immutable snapshot pattern)

Replace in-place mutation with atomic reference replacement:

private volatile Dictionary<string, ScriptTrustEntry> _entriesById = new Dictionary<string, ScriptTrustEntry>();
private volatile Dictionary<string, ScriptTrustEntry> _entriesByName = new Dictionary<string, ScriptTrustEntry>();

private void LoadEntries()
{
    var newById = new Dictionary<string, ScriptTrustEntry>();
    var newByName = new Dictionary<string, ScriptTrustEntry>();
    
    // ... populate newById and newByName ...
    
    // Atomic swap — readers always see a complete, consistent snapshot
    _entriesById = newById;
    _entriesByName = newByName;
    _initialized = true;
}

public void Invalidate()
{
    // Next read will trigger a full reload
    _initialized = false;
}

Key changes:

1. Mark dictionary fields as volatile to ensure memory ordering.
2. LoadEntries() builds new dictionaries and assigns them atomically.
3. Invalidate() only flips _initialized — never calls .Clear() on a dictionary that readers may hold.
4. Readers get a consistent snapshot even during reload.

Also make _initialized volatile.

Files to modify

File	Change
src/Spe/Core/Settings/Authorization/ScriptTrustRegistry.cs	Atomic dictionary swap, volatile fields

---

Test Plan

Reproduce the race condition

1. Stress test — concurrent read + invalidation:

   [Test]
   public void ScriptTrustRegistry_ConcurrentReadAndInvalidation_NeverCorrupts()
   {
       var registry = new ScriptTrustRegistry();
       registry.Initialize();
       
       var errors = new ConcurrentBag<Exception>();
       var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
       
       var readers = Enumerable.Range(0, 10).Select(_ => Task.Run(() =>
       {
           while (!cts.IsCancellationRequested)
           {
               try { registry.IsTrusted("known-script-id"); }
               catch (Exception ex) { errors.Add(ex); }
           }
       }));
       
       var invalidator = Task.Run(() =>
       {
           while (!cts.IsCancellationRequested)
           {
               registry.Invalidate();
               Thread.Sleep(1);
           }
       });
       
       Task.WhenAll(readers.Append(invalidator)).Wait();
       Assert.IsEmpty(errors, $"Concurrent access threw: {errors.FirstOrDefault()}");
   }

2. Before fix: Occasional exceptions or incorrect results.

3. After fix: Zero exceptions, consistent results.

Verify the fix

4. Functional test: Trust lookup works after invalidation.
5. Existing CLM/trust tests pass.