diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 53e346ab..ed980367 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -13,16 +13,46 @@ using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using System.Linq; +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; - private readonly ConcurrentDictionary _guidMap = new(); private readonly ILogger _log; private readonly ILdapUtils _utils; - private readonly ConcurrentHashSet _builtDomainCaches = new(StringComparer.OrdinalIgnoreCase); private readonly ConcurrentDictionary _exchangeTrusteeSidCache = new(StringComparer.OrdinalIgnoreCase); - private readonly object _lock = new(); // These Exchange principals commonly carry product-added deny ACEs that we intentionally suppress. private static readonly HashSet ExchangeTrusteeNames = new(StringComparer.OrdinalIgnoreCase) { "Exchange Windows Permissions", @@ -30,6 +60,52 @@ public class ACLProcessor { "Exchange Servers", "Organization Management" }; + 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 bool RemoveBuildTask(string domain, Lazy buildTask) { + // Remove only this instance so a delayed fault cannot remove a newer retry task. + // _buildTasks.TryRemove does not guarantee that the value is the same as the one being removed, so we need to cast to ICollection and use Remove instead. + // This lets us conditionally remove the task only if it is the same instance as the one we expect. + return ((ICollection>>)_buildTasks) + .Remove(new KeyValuePair>(domain, buildTask)); + } + + 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() { //Create a dictionary with the base GUIDs of each object type @@ -51,9 +127,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 = guidCache; _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } @@ -120,14 +199,20 @@ public override string ToString() { /// LAPS /// private async Task BuildGuidCache(string domain) { - lock (_lock) { - if (_builtDomainCaches.Contains(domain)) { - return; - } + 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)); - _builtDomainCaches.Add(domain); + try { + await buildTask.Value; } + catch { + _guidCache.RemoveBuildTask(domain, buildTask); + throw; + } + } + 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, @@ -155,7 +240,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.AddGuid(guid, name); } } else { _log.LogDebug("Error while building GUID cache for {Domain}: {Message}", domain, result.Error); @@ -770,7 +855,7 @@ await CountCustomDenyAce(ace, customDenyAceAccumulator, objectDomain, objectType IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, }; - else if (_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 8a3a2b5b..0cb9ddbb 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -58,6 +58,95 @@ public void SanityCheck() { Assert.True(true); } + [Fact] + 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(_ => context.CreateACLProcessor(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 ProcessorContext_ACLProcessors_RetriesGuidCacheBuildAfterFailure() { + var mockLdapUtils = new Mock(); + var queryAttempts = 0; + mockLdapUtils + .Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(() => { + if (Interlocked.Increment(ref queryAttempts) == 1) { + throw new InvalidOperationException("Expected test failure"); + } + + return Array.Empty>().ToAsyncEnumerable(); + }); + var domain = $"{Guid.NewGuid():N}.TEST"; + using var context = new ACLProcessorContext(); + var processor = context.CreateACLProcessor(mockLdapUtils.Object); + + await Assert.ThrowsAsync(() => + processor.ProcessACL(null, domain, Label.Computer, false).ToArrayAsync()); + + await processor.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 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( + 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] public void ACLProcessor_IsACLProtected_NullNTSD_ReturnsFalse() { var processor = new ACLProcessor(new MockLdapUtils());