From 19744097578f46c61d8be56d70cb21ab55913021 Mon Sep 17 00:00:00 2001 From: anemeth Date: Wed, 26 Aug 2026 11:56:37 -0700 Subject: [PATCH 1/2] fix: Make ACLProcessor guid cache singleton and thread-safe --- src/CommonLib/Processors/ACLProcessor.cs | 38 ++++++++++++++------ test/unit/ACLProcessorTest.cs | 45 +++++++++++++++++++++++- 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 119a6679..f30601e2 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -12,15 +12,30 @@ using SharpHoundCommonLib.Enums; using SharpHoundCommonLib.OutputTypes; using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading; namespace SharpHoundCommonLib.Processors { public class ACLProcessor { private static readonly Dictionary BaseGuids; - private readonly ConcurrentDictionary _guidMap = new(); + /// This is a shared cache of GUID mappings for each ILdapUtils instance. It allows multiple ACLProcessor instances to share the same GUID cache for a given ILdapUtils instance. + /// This resolves an issue from back when the guid cache was static and shared across all ILdapUtils instances, which was causing issues when domains were being processed in parallel for tests: https://github.com/SpecterOps/SharpHoundCommon/pull/169 + /// Learn about Conditional Weak Tables https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2?view=netframework-4.7.2#examples + /// But the short version is that this allows us to have a shared cache for each ILdapUtils instance, but when the ILdapUtils instance is garbage collected, the cache will be garbage collected as well. + private static readonly ConditionalWeakTable SharedGuidCaches = new(); private readonly ILogger _log; private readonly ILdapUtils _utils; - private readonly ConcurrentHashSet _builtDomainCaches = new(StringComparer.OrdinalIgnoreCase); - private readonly object _lock = new(); + private readonly GuidCacheState _guidCache; + + private sealed class GuidCacheState { + // This is a mapping of GUIDs to their corresponding names for LDAP rights. + // The collection represents the response from the LDAP query Task kept in BuildTasks. + public readonly ConcurrentDictionary GuidMap = new(); + // This is a mapping of domains to their corresponding build tasks for the GUID cache. + // The Lazy ensures that the build task is only executed once per domain, even if multiple threads attempt to build the cache for the same domain simultaneously. + public readonly ConcurrentDictionary> BuildTasks = + new(StringComparer.OrdinalIgnoreCase); + } static ACLProcessor() { //Create a dictionary with the base GUIDs of each object type @@ -45,6 +60,7 @@ static ACLProcessor() { public ACLProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; + _guidCache = SharedGuidCaches.GetValue(utils, _ => new GuidCacheState()); _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } @@ -73,14 +89,14 @@ public override string ToString() { /// LAPS /// private async Task BuildGuidCache(string domain) { - lock (_lock) { - if (_builtDomainCaches.Contains(domain)) { - return; - } + var buildTask = _guidCache.BuildTasks.GetOrAdd(domain, + // The ExecutionAndPublication mode ensures that only one thread can execute the factory method at a time, and all other threads will wait for the result of that execution. This prevents multiple threads from building the cache simultaneously for the same domain. + _ => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); - _builtDomainCaches.Add(domain); - } + await buildTask.Value; + } + private async Task BuildGuidCacheCore(string domain) { _log.LogInformation("Building GUID Cache for {Domain}", domain); await foreach (var result in _utils.PagedQuery(new LdapQueryParameters { DomainName = domain, @@ -108,7 +124,7 @@ private async Task BuildGuidCache(string domain) { if (name is LDAPProperties.LAPSPlaintextPassword or LDAPProperties.LAPSEncryptedPassword or LDAPProperties.LegacyLAPSPassword) { _log.LogInformation("Found GUID for ACL Right {Name}: {Guid} in domain {Domain}", name, guid, domain); - _guidMap.TryAdd(guid, name); + _guidCache.GuidMap.TryAdd(guid, name); } } else { _log.LogDebug("Error while building GUID cache for {Domain}: {Message}", domain, result.Error); @@ -676,7 +692,7 @@ public async IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, strin IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, }; - else if (_guidMap.TryGetValue(aceType, out var lapsAttribute)) { + else if (_guidCache.GuidMap.TryGetValue(aceType, out var lapsAttribute)) { // Compare the retrieved attribute name against LDAPProperties values if (lapsAttribute == LDAPProperties.LegacyLAPSPassword || lapsAttribute == LDAPProperties.LAPSPlaintextPassword || diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index a8e4d3b3..2e5513f3 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -55,6 +55,49 @@ public void SanityCheck() { Assert.True(true); } + [Fact] + public async Task ACLProcessor_BuildGuidCache_AcrossInstances_QueriesOncePerDomain() { + var mockLdapUtils = new Mock(); + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); + var domain = $"{Guid.NewGuid():N}.TEST"; + var processors = Enumerable.Range(0, 50) + .Select(_ => new ACLProcessor(mockLdapUtils.Object)) + .ToArray(); + + await Task.WhenAll(processors.Select(processor => + processor.ProcessACL(null, domain, Label.Computer, false).ToArrayAsync())); + + mockLdapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Once); + } + + [Fact] + public async Task ACLProcessor_BuildGuidCache_AcrossLdapUtils_QueriesOncePerUtility() { + var firstLdapUtils = new Mock(); + var secondLdapUtils = new Mock(); + foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { + ldapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); + } + + var domain = $"{Guid.NewGuid():N}.TEST"; + await Task.WhenAll( + new ACLProcessor(firstLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), + new ACLProcessor(secondLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { + ldapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Once); + } + } + [Fact] public void ACLProcessor_IsACLProtected_NullNTSD_ReturnsFalse() { var processor = new ACLProcessor(new MockLdapUtils()); @@ -2289,4 +2332,4 @@ public async Task ACLProcessor_ProcessACL_GenericWrite_Computer_WritePublicInfor Assert.Equal(actual.RightName, expectedRightName); } } -} \ No newline at end of file +} From be826ecae7a23ce2c39044f04a5d60c71fe1e180 Mon Sep 17 00:00:00 2001 From: anemeth Date: Thu, 27 Aug 2026 12:46:42 -0700 Subject: [PATCH 2/2] feat: Apply new ProcessorContext pattern to ACLProcessor for shared state and state lifetime management --- src/CommonLib/Processors/ACLProcessor.cs | 99 ++++++++++++++++++------ test/unit/ACLProcessorTest.cs | 58 +++++++++----- 2 files changed, 115 insertions(+), 42 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index f30601e2..9c8e69c6 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -12,29 +12,82 @@ using SharpHoundCommonLib.Enums; using SharpHoundCommonLib.OutputTypes; using System.Linq; -using System.Runtime.CompilerServices; using System.Threading; namespace SharpHoundCommonLib.Processors { + /// + /// Owns state shared by processor instances and gives that state an explicit lifetime. + /// + public sealed class ACLProcessorContext : IDisposable { + private readonly ACLProcessor.GuidCache _aclGuidCache = new(); + private int _disposed; + + /// + /// Creates an that shares its GUID cache with other + /// ACL processors created by this context. + /// + public ACLProcessor CreateACLProcessor(ILdapUtils utils, ILogger log = null) { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(ACLProcessorContext)); + } + + return new ACLProcessor(utils, _aclGuidCache, log); + } + + /// + /// Clears the shared processor state. Processors created by this context must not + /// be used after the context is disposed. + /// + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _aclGuidCache.Dispose(); + } + } + public class ACLProcessor { private static readonly Dictionary BaseGuids; - /// This is a shared cache of GUID mappings for each ILdapUtils instance. It allows multiple ACLProcessor instances to share the same GUID cache for a given ILdapUtils instance. - /// This resolves an issue from back when the guid cache was static and shared across all ILdapUtils instances, which was causing issues when domains were being processed in parallel for tests: https://github.com/SpecterOps/SharpHoundCommon/pull/169 - /// Learn about Conditional Weak Tables https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2?view=netframework-4.7.2#examples - /// But the short version is that this allows us to have a shared cache for each ILdapUtils instance, but when the ILdapUtils instance is garbage collected, the cache will be garbage collected as well. - private static readonly ConditionalWeakTable SharedGuidCaches = new(); private readonly ILogger _log; private readonly ILdapUtils _utils; - private readonly GuidCacheState _guidCache; - - private sealed class GuidCacheState { - // This is a mapping of GUIDs to their corresponding names for LDAP rights. - // The collection represents the response from the LDAP query Task kept in BuildTasks. - public readonly ConcurrentDictionary GuidMap = new(); - // This is a mapping of domains to their corresponding build tasks for the GUID cache. - // The Lazy ensures that the build task is only executed once per domain, even if multiple threads attempt to build the cache for the same domain simultaneously. - public readonly ConcurrentDictionary> BuildTasks = + private readonly GuidCache _guidCache; + + internal sealed class GuidCache : IDisposable { + private readonly ConcurrentDictionary _guidMap = new(); + private readonly ConcurrentDictionary> _buildTasks = new(StringComparer.OrdinalIgnoreCase); + private int _disposed; + + public Lazy GetOrAddBuildTask(string domain, Func> buildTaskFactory) { + ThrowIfDisposed(); + return _buildTasks.GetOrAdd(domain, _ => buildTaskFactory()); + } + + public void AddGuid(string guid, string name) { + ThrowIfDisposed(); + _guidMap.TryAdd(guid, name); + } + + public bool TryGetGuid(string guid, out string name) { + ThrowIfDisposed(); + return _guidMap.TryGetValue(guid, out name); + } + + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _buildTasks.Clear(); + _guidMap.Clear(); + } + + private void ThrowIfDisposed() { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(ACLProcessorContext)); + } + } } static ACLProcessor() { @@ -57,10 +110,12 @@ static ACLProcessor() { }; } - public ACLProcessor(ILdapUtils utils, ILogger log = null) - { + public ACLProcessor(ILdapUtils utils, ILogger log = null) : this(utils, new GuidCache(), log) { + } + + internal ACLProcessor(ILdapUtils utils, GuidCache guidCache, ILogger log = null) { _utils = utils; - _guidCache = SharedGuidCaches.GetValue(utils, _ => new GuidCacheState()); + _guidCache = guidCache; _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } @@ -89,9 +144,9 @@ public override string ToString() { /// LAPS /// private async Task BuildGuidCache(string domain) { - var buildTask = _guidCache.BuildTasks.GetOrAdd(domain, + var buildTask = _guidCache.GetOrAddBuildTask(domain, // The ExecutionAndPublication mode ensures that only one thread can execute the factory method at a time, and all other threads will wait for the result of that execution. This prevents multiple threads from building the cache simultaneously for the same domain. - _ => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); + () => new Lazy(() => BuildGuidCacheCore(domain), LazyThreadSafetyMode.ExecutionAndPublication)); await buildTask.Value; } @@ -124,7 +179,7 @@ private async Task BuildGuidCacheCore(string domain) { if (name is LDAPProperties.LAPSPlaintextPassword or LDAPProperties.LAPSEncryptedPassword or LDAPProperties.LegacyLAPSPassword) { _log.LogInformation("Found GUID for ACL Right {Name}: {Guid} in domain {Domain}", name, guid, domain); - _guidCache.GuidMap.TryAdd(guid, name); + _guidCache.AddGuid(guid, name); } } else { _log.LogDebug("Error while building GUID cache for {Domain}: {Message}", domain, result.Error); @@ -692,7 +747,7 @@ public async IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, strin IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, }; - else if (_guidCache.GuidMap.TryGetValue(aceType, out var lapsAttribute)) { + else if (_guidCache.TryGetGuid(aceType, out var lapsAttribute)) { // Compare the retrieved attribute name against LDAPProperties values if (lapsAttribute == LDAPProperties.LegacyLAPSPassword || lapsAttribute == LDAPProperties.LAPSPlaintextPassword || diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index 2e5513f3..a01f752b 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -56,14 +56,15 @@ public void SanityCheck() { } [Fact] - public async Task ACLProcessor_BuildGuidCache_AcrossInstances_QueriesOncePerDomain() { + public async Task ProcessorContext_ACLProcessors_QueryOncePerDomain() { var mockLdapUtils = new Mock(); mockLdapUtils .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) .Returns(Array.Empty>().ToAsyncEnumerable); var domain = $"{Guid.NewGuid():N}.TEST"; + using var context = new ACLProcessorContext(); var processors = Enumerable.Range(0, 50) - .Select(_ => new ACLProcessor(mockLdapUtils.Object)) + .Select(_ => context.CreateACLProcessor(mockLdapUtils.Object)) .ToArray(); await Task.WhenAll(processors.Select(processor => @@ -76,26 +77,43 @@ await Task.WhenAll(processors.Select(processor => } [Fact] - public async Task ACLProcessor_BuildGuidCache_AcrossLdapUtils_QueriesOncePerUtility() { - var firstLdapUtils = new Mock(); - var secondLdapUtils = new Mock(); - foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { - ldapUtils - .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) - .Returns(Array.Empty>().ToAsyncEnumerable); - } - + public async Task ProcessorContext_ACLProcessors_DoNotShareCacheAcrossContexts() { + var mockLdapUtils = new Mock(); + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(Array.Empty>().ToAsyncEnumerable); var domain = $"{Guid.NewGuid():N}.TEST"; + using var firstContext = new ACLProcessorContext(); + using var secondContext = new ACLProcessorContext(); + await Task.WhenAll( - new ACLProcessor(firstLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), - new ACLProcessor(secondLdapUtils.Object).ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); - - foreach (var ldapUtils in new[] { firstLdapUtils, secondLdapUtils }) { - ldapUtils.Verify( - x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), - It.IsAny()), - Times.Once); - } + firstContext.CreateACLProcessor(mockLdapUtils.Object) + .ProcessACL(null, domain, Label.Computer, false).ToArrayAsync(), + secondContext.CreateACLProcessor(mockLdapUtils.Object) + .ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + mockLdapUtils.Verify( + x => x.PagedQuery(It.Is(parameters => parameters.DomainName == domain), + It.IsAny()), + Times.Exactly(2)); + } + + [Fact] + public void ProcessorContext_CreateACLProcessor_AfterDispose_Throws() { + var context = new ACLProcessorContext(); + context.Dispose(); + + Assert.Throws(() => context.CreateACLProcessor(new MockLdapUtils())); + } + + [Fact] + public async Task ProcessorContext_ACLProcessor_AfterDispose_Throws() { + var context = new ACLProcessorContext(); + var processor = context.CreateACLProcessor(new MockLdapUtils()); + context.Dispose(); + + await Assert.ThrowsAsync(() => + processor.ProcessACL(null, "TEST.LOCAL", Label.Computer, false).ToArrayAsync()); } [Fact]