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());