From 19744097578f46c61d8be56d70cb21ab55913021 Mon Sep 17 00:00:00 2001 From: anemeth Date: Wed, 26 Aug 2026 11:56:37 -0700 Subject: [PATCH 1/3] 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 119a66791..f30601e2a 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 a8e4d3b33..2e5513f32 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/3] 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 f30601e2a..9c8e69c68 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 2e5513f32..a01f752b3 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] From dcd2b3f468b2a7dbfae0c7da1597874fba0cba0f Mon Sep 17 00:00:00 2001 From: anemeth Date: Fri, 28 Aug 2026 13:00:49 -0700 Subject: [PATCH 3/3] chore: Apply context pattern to other processors using shared state --- .../Processors/GPOLocalGroupProcessor.cs | 145 ++++++++++++++---- src/CommonLib/Processors/PortScanner.cs | 91 +++++++++-- test/unit/GPOLocalGroupProcessorTest.cs | 105 ++++++++++++- test/unit/PortScannerTest.cs | 43 +++++- 4 files changed, 336 insertions(+), 48 deletions(-) diff --git a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs index 28a6996f6..b34e96cf3 100644 --- a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs +++ b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Text.RegularExpressions; +using System.Threading; using System.Threading.Tasks; using System.Xml.XPath; using Microsoft.Extensions.Logging; @@ -13,6 +14,38 @@ using SharpHoundCommonLib.OutputTypes; namespace SharpHoundCommonLib.Processors { + /// + /// Owns state shared by GPOLocalGroupProcessor instances and gives that state an explicit lifetime. + /// + public sealed class GPOLocalGroupProcessorContext : IDisposable { + private readonly GPOLocalGroupProcessor.ActionCache _actionCache = new(); + private int _disposed; + + /// + /// Creates a that shares its GPO action cache with other + /// processors created by this context. + /// + public GPOLocalGroupProcessor CreateGPOLocalGroupProcessor(ILdapUtils utils, ILogger log = null) { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(GPOLocalGroupProcessorContext)); + } + + return new GPOLocalGroupProcessor(utils, _actionCache, 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; + } + + _actionCache.Dispose(); + } + } + public class GPOLocalGroupProcessor { private static readonly Regex KeyRegex = new(@"(.+?)\s*=(.*)", RegexOptions.Compiled); @@ -30,8 +63,6 @@ public class GPOLocalGroupProcessor { private static readonly Regex ExtractRid = new(@"S-1-5-32-([0-9]{3})", RegexOptions.Compiled | RegexOptions.IgnoreCase); - private static readonly ConcurrentDictionary> GpoActionCache = new(); - private static readonly Dictionary ValidGroupNames = new(StringComparer.OrdinalIgnoreCase) { { "Administrators", LocalGroupRids.Administrators }, @@ -43,9 +74,46 @@ public class GPOLocalGroupProcessor { private readonly ILogger _log; private readonly ILdapUtils _utils; + private readonly ActionCache _actionCache; + + internal sealed class ActionCache : IDisposable { + private readonly ConcurrentDictionary>>> _buildTasks = + new(StringComparer.OrdinalIgnoreCase); + private int _disposed; + + public Lazy>> GetOrAddBuildTask(string distinguishedName, + Func>>> buildTaskFactory) { + ThrowIfDisposed(); + return _buildTasks.GetOrAdd(distinguishedName, _ => buildTaskFactory()); + } + + public void RemoveBuildTask(string distinguishedName, Lazy>> buildTask) { + ThrowIfDisposed(); + ((ICollection>>>>)_buildTasks).Remove( + new KeyValuePair>>>(distinguishedName, buildTask)); + } + + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _buildTasks.Clear(); + } - public GPOLocalGroupProcessor(ILdapUtils utils, ILogger log = null) { + private void ThrowIfDisposed() { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(GPOLocalGroupProcessorContext)); + } + } + } + + public GPOLocalGroupProcessor(ILdapUtils utils, ILogger log = null) : this(utils, new ActionCache(), log) { + } + + internal GPOLocalGroupProcessor(ILdapUtils utils, ActionCache actionCache, ILogger log = null) { _utils = utils; + _actionCache = actionCache; _log = log ?? Logging.LogProvider.CreateLogger("GPOLocalGroupProc"); } @@ -124,36 +192,22 @@ public async Task ReadGPOLocalGroups(string gpLink, string foreach (var rid in Enum.GetValues(typeof(LocalGroupRids))) data[(LocalGroupRids)rid] = new GroupResults(); foreach (var linkDn in orderedLinks) { - if (!GpoActionCache.TryGetValue(linkDn.ToLower(), out var actions)) { - actions = new List(); - - var gpoDomain = Helpers.DistinguishedNameToDomain(linkDn); - var result = await _utils.Query(new LdapQueryParameters() { - LDAPFilter = new LdapFilter().AddAllObjects().GetFilter(), - SearchScope = SearchScope.Base, - Attributes = [LDAPProperties.GPCFileSYSPath, LDAPProperties.Flags], - SearchBase = linkDn, - DomainName = gpoDomain - }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); - - if (!result.IsSuccess) { - continue; - } - - if (!result.Value.TryGetProperty(LDAPProperties.GPCFileSYSPath, out var filePath) || - // Filter out GPOs that are disabled or the computer configuration is disabled - (result.Value.TryGetProperty(LDAPProperties.Flags, out var flags) && flags is "2" or "3")) { - GpoActionCache.TryAdd(linkDn, actions); - continue; - } - - //Add the actions for each file. The GPO template file actions will override the XML file actions - await foreach (var item in ProcessGPOXmlFile(filePath, gpoDomain)) actions.Add(item); - await foreach (var item in ProcessGPOTemplateFile(filePath, gpoDomain)) actions.Add(item); + var buildTask = _actionCache.GetOrAddBuildTask(linkDn, + () => new Lazy>>(() => BuildGPOActionCache(linkDn), + LazyThreadSafetyMode.ExecutionAndPublication)); + List actions; + try { + actions = await buildTask.Value; + } catch { + _actionCache.RemoveBuildTask(linkDn, buildTask); + throw; } - //Cache the actions for this GPO for later - GpoActionCache.TryAdd(linkDn.ToLower(), actions); + // Query failures are not cached so a later attempt can retry the GPO. + if (actions == null) { + _actionCache.RemoveBuildTask(linkDn, buildTask); + continue; + } //If there are no actions, then we can move on from this GPO if (actions.Count == 0) @@ -248,6 +302,33 @@ public async Task ReadGPOLocalGroups(string gpLink, string return ret; } + private async Task> BuildGPOActionCache(string linkDn) { + var actions = new List(); + var gpoDomain = Helpers.DistinguishedNameToDomain(linkDn); + var result = await _utils.Query(new LdapQueryParameters() { + LDAPFilter = new LdapFilter().AddAllObjects().GetFilter(), + SearchScope = SearchScope.Base, + Attributes = [LDAPProperties.GPCFileSYSPath, LDAPProperties.Flags], + SearchBase = linkDn, + DomainName = gpoDomain + }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); + + if (!result.IsSuccess) { + return null; + } + + if (!result.Value.TryGetProperty(LDAPProperties.GPCFileSYSPath, out var filePath) || + // Filter out GPOs that are disabled or the computer configuration is disabled + (result.Value.TryGetProperty(LDAPProperties.Flags, out var flags) && flags is "2" or "3")) { + return actions; + } + + //Add the actions for each file. The GPO template file actions will override the XML file actions + await foreach (var item in ProcessGPOXmlFile(filePath, gpoDomain)) actions.Add(item); + await foreach (var item in ProcessGPOTemplateFile(filePath, gpoDomain)) actions.Add(item); + return actions; + } + /// /// Parses a GPO GptTmpl.inf file and pulls group membership changes out /// @@ -576,4 +657,4 @@ internal enum LocalGroupRids { PSRemote = 580 } } -} \ No newline at end of file +} diff --git a/src/CommonLib/Processors/PortScanner.cs b/src/CommonLib/Processors/PortScanner.cs index 6ff9cbd2f..6615d069d 100644 --- a/src/CommonLib/Processors/PortScanner.cs +++ b/src/CommonLib/Processors/PortScanner.cs @@ -1,23 +1,90 @@ using System; using System.Collections.Concurrent; using System.Net.Sockets; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using SharpHoundRPC.PortScanner; namespace SharpHoundCommonLib.Processors { + /// + /// Owns state shared by PortScanner instances and gives that state an explicit lifetime. + /// + public sealed class PortScannerContext : IDisposable { + private readonly PortScanner.ScanCache _scanCache = new(); + private int _disposed; + + /// + /// Creates a that shares its scan cache with other scanners + /// created by this context. + /// + public PortScanner CreatePortScanner(ILogger log = null, int maxTimeout = 10000) { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(PortScannerContext)); + } + + return new PortScanner(_scanCache, log, maxTimeout); + } + + /// + /// Clears the shared scanner state. Scanners created by this context must not + /// be used after the context is disposed. + /// + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _scanCache.Dispose(); + } + } + public class PortScanner : IPortScanner { - private static readonly ConcurrentDictionary PortScanCache = new(); private readonly ILogger _log; private readonly AdaptiveTimeout _adaptiveTimeout; + private readonly ScanCache _scanCache; - public PortScanner() : this(null) { + internal sealed class ScanCache : IDisposable { + private readonly ConcurrentDictionary _portScanCache = new(); + private int _disposed; + + public bool TryGet(PingCacheKey key, out bool status) { + ThrowIfDisposed(); + return _portScanCache.TryGetValue(key, out status); + } + + public void Add(PingCacheKey key, bool status) { + ThrowIfDisposed(); + _portScanCache.TryAdd(key, status); + } + + public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) { + return; + } + + _portScanCache.Clear(); + } + + private void ThrowIfDisposed() { + if (Volatile.Read(ref _disposed) != 0) { + throw new ObjectDisposedException(nameof(PortScannerContext)); + } + } + } + + public PortScanner() : this((ILogger)null) { } - public PortScanner(ILogger log = null, int maxTimeout = 10000) { + public PortScanner(ILogger log = null, int maxTimeout = 10000) : this( + new ScanCache(), log, maxTimeout) { + } + + internal PortScanner(ScanCache scanCache, ILogger log = null, int maxTimeout = 10000) { + _scanCache = scanCache; _log = log ?? Logging.LogProvider.CreateLogger("PortScanner"); - _adaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMilliseconds(maxTimeout), _log); + _adaptiveTimeout = new AdaptiveTimeout(TimeSpan.FromMilliseconds(maxTimeout), _log); } /// @@ -35,7 +102,7 @@ public virtual async Task CheckPort(string hostname, int port = 445, HostName = hostname }; - if (PortScanCache.TryGetValue(key, out var status)) { + if (_scanCache.TryGet(key, out var status)) { _log.LogTrace("Port scan cache hit for {HostName}:{Port}: {Status}", hostname, port, status); return status; } @@ -48,12 +115,12 @@ public virtual async Task CheckPort(string hostname, int port = 445, if (throwError) { throw new TimeoutException(ca.Error); } - PortScanCache.TryAdd(key, false); + _scanCache.Add(key, false); return false; } _log.LogTrace("CheckPort Succeeded for {HostName}:{Port}", hostname, port); - PortScanCache.TryAdd(key, true); + _scanCache.Add(key, true); return true; } catch (Exception e) { @@ -63,16 +130,12 @@ public virtual async Task CheckPort(string hostname, int port = 445, throw; } - PortScanCache.TryAdd(key, false); + _scanCache.Add(key, false); return false; } } - public static void ClearCache() { - PortScanCache.Clear(); - } - - private class PingCacheKey { + internal class PingCacheKey { internal string HostName { get; set; } internal int Port { get; set; } @@ -94,4 +157,4 @@ public override int GetHashCode() { } } } -} \ No newline at end of file +} diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 07107a12e..3eee539d9 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -93,6 +93,86 @@ public GPOLocalGroupProcessorTest(ITestOutputHelper testOutputHelper) { _testOutputHelper = testOutputHelper; } + [Fact] + public async Task GPOLocalGroupProcessorContext_Processors_QueryGPOOnce() { + var (mockLdapUtils, gpLink, linkDn) = CreateContextTestData(); + using var context = new GPOLocalGroupProcessorContext(); + var processors = Enumerable.Range(0, 50) + .Select(_ => context.CreateGPOLocalGroupProcessor(mockLdapUtils.Object)) + .ToArray(); + + await Task.WhenAll(processors.Select(processor => + processor.ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL"))); + + mockLdapUtils.Verify(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddAllObjects().GetFilter() && + parameters.SearchBase == linkDn), + It.IsAny()), Times.Once); + } + + [Fact] + public async Task GPOLocalGroupProcessorContext_Processors_DoNotShareCacheAcrossContexts() { + var (mockLdapUtils, gpLink, linkDn) = CreateContextTestData(); + using var firstContext = new GPOLocalGroupProcessorContext(); + using var secondContext = new GPOLocalGroupProcessorContext(); + + await Task.WhenAll( + firstContext.CreateGPOLocalGroupProcessor(mockLdapUtils.Object) + .ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL"), + secondContext.CreateGPOLocalGroupProcessor(mockLdapUtils.Object) + .ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL")); + + mockLdapUtils.Verify(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddAllObjects().GetFilter() && + parameters.SearchBase == linkDn), + It.IsAny()), Times.Exactly(2)); + } + + [Fact] + public async Task GPOLocalGroupProcessorContext_QueryFailure_IsRetried() { + var (mockLdapUtils, gpLink, linkDn) = CreateContextTestData(); + mockLdapUtils.SetupSequence(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddAllObjects().GetFilter()), + It.IsAny())) + .Returns(new[] { LdapResult.Fail() }.ToAsyncEnumerable) + .Returns(new[] { LdapResult.Ok(new Mock().Object) } + .ToAsyncEnumerable); + using var context = new GPOLocalGroupProcessorContext(); + var processor = context.CreateGPOLocalGroupProcessor(mockLdapUtils.Object); + + await processor.ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL"); + await processor.ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL"); + + mockLdapUtils.Verify(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddAllObjects().GetFilter() && + parameters.SearchBase == linkDn), + It.IsAny()), Times.Exactly(2)); + } + + [Fact] + public void GPOLocalGroupProcessorContext_CreateProcessor_AfterDispose_Throws() { + var context = new GPOLocalGroupProcessorContext(); + context.Dispose(); + + Assert.Throws(() => + context.CreateGPOLocalGroupProcessor(new MockLdapUtils())); + } + + [Fact] + public async Task GPOLocalGroupProcessorContext_Processor_AfterDispose_Throws() { + var (mockLdapUtils, gpLink, _) = CreateContextTestData(); + var context = new GPOLocalGroupProcessorContext(); + var processor = context.CreateGPOLocalGroupProcessor(mockLdapUtils.Object); + context.Dispose(); + + await Assert.ThrowsAsync(() => + processor.ReadGPOLocalGroups(gpLink, "DC=TEST,DC=LOCAL")); + } + [Fact] public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups_Null_GPLink() { var mockLDAPUtils = new Mock(); @@ -351,6 +431,29 @@ public async Task GPOLocalGroupProcess_ProcessGPOXMLFile_NoFile() { Assert.Empty(actual); } + private static (Mock LdapUtils, string GPLink, string LinkDn) CreateContextTestData() { + var mockLdapUtils = new Mock(); + var computerEntry = new Mock(); + var computerSid = $"S-1-5-21-{Random.Shared.Next()}-{Random.Shared.Next()}-{Random.Shared.Next()}-1000"; + computerEntry.Setup(x => x.TryGetSecurityIdentifier(out computerSid)).Returns(true); + var computerResults = new[] { LdapResult.Ok(computerEntry.Object) }; + var gpoResults = new[] { LdapResult.Ok(new Mock().Object) }; + + mockLdapUtils.Setup(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddComputersNoMSAs().GetFilter()), + It.IsAny())) + .Returns(computerResults.ToAsyncEnumerable); + mockLdapUtils.Setup(x => x.Query( + It.Is(parameters => + parameters.LDAPFilter == new LdapFilter().AddAllObjects().GetFilter()), + It.IsAny())) + .Returns(gpoResults.ToAsyncEnumerable); + + var linkDn = $"CN={Guid.NewGuid():N},CN=Policies,CN=System,DC=TEST,DC=LOCAL"; + return (mockLdapUtils, $"[LDAP://{linkDn};0]", linkDn); + } + [Fact] public async Task GPOLocalGroupProcess_ProcessGPOXMLFile_Disabled() { var mockLDAPUtils = new Mock(); @@ -467,4 +570,4 @@ public void GPOLocalGroupProcess_GroupAction() { str); } } -} \ No newline at end of file +} diff --git a/test/unit/PortScannerTest.cs b/test/unit/PortScannerTest.cs index 2e37a7e74..99eb11940 100644 --- a/test/unit/PortScannerTest.cs +++ b/test/unit/PortScannerTest.cs @@ -1,5 +1,6 @@ using System; using System.Diagnostics.CodeAnalysis; +using System.Threading.Tasks; using SharpHoundCommonLib.Processors; using Xunit; @@ -7,6 +8,46 @@ namespace CommonLibTest; [SuppressMessage("Interoperability", "CA1416:Validate platform compatibility")] public class PortScannerTest { + [Fact] + public async Task PortScannerContext_ScannersShareCache() { + using var context = new PortScannerContext(); + var firstScanner = context.CreatePortScanner(); + var secondScanner = context.CreatePortScanner(); + + // An invalid port is cached as false. A cache miss with throwError enabled would throw, + // so returning false demonstrates that the second scanner used the first scanner's result. + Assert.False(await firstScanner.CheckPort("localhost", -1)); + Assert.False(await secondScanner.CheckPort("localhost", -1, throwError: true)); + } + + [Fact] + public async Task PortScannerContext_ScannersDoNotShareCacheAcrossContexts() { + using var firstContext = new PortScannerContext(); + using var secondContext = new PortScannerContext(); + + Assert.False(await firstContext.CreatePortScanner().CheckPort("localhost", -1)); + // The second context has no cached result and therefore attempts the invalid scan. + await Assert.ThrowsAnyAsync(() => + secondContext.CreatePortScanner().CheckPort("localhost", -1, throwError: true)); + } + + [Fact] + public void PortScannerContext_CreateScanner_AfterDispose_Throws() { + var context = new PortScannerContext(); + context.Dispose(); + + Assert.Throws(() => context.CreatePortScanner()); + } + + [Fact] + public async Task PortScannerContext_Scanner_AfterDispose_Throws() { + var context = new PortScannerContext(); + var scanner = context.CreatePortScanner(); + context.Dispose(); + + await Assert.ThrowsAsync(() => scanner.CheckPort("localhost")); + } + //// Throws "no such host is known" exception // [Fact] // public void PortScanner_CheckPort_TimeoutException() { @@ -16,4 +57,4 @@ public class PortScannerTest { // var ex = Assert.ThrowsAsync(() => scanner.CheckPort(hostname, port, 1, true)); // Assert.Equal("Timed Out", ex.Result.Message); // } -} \ No newline at end of file +}