From 508258539ca09102984359f56d076f31e4413b13 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 13:11:20 +0200 Subject: [PATCH 01/13] collect custom deny aces --- src/CommonLib/Enums/DirectoryPaths.cs | 3 +- src/CommonLib/Enums/LDAPProperties.cs | 1 + src/CommonLib/LdapUtils.cs | 5 +- src/CommonLib/Processors/ACLProcessor.cs | 178 ++++++++++++++++++ .../Processors/LdapPropertyProcessor.cs | 50 ++++- src/CommonLib/WellKnownPrincipal.cs | 4 +- test/unit/ACLProcessorTest.cs | 170 ++++++++++++++++- test/unit/LdapPropertyTests.cs | 50 +++-- 8 files changed, 428 insertions(+), 33 deletions(-) diff --git a/src/CommonLib/Enums/DirectoryPaths.cs b/src/CommonLib/Enums/DirectoryPaths.cs index 878feef23..c7421afac 100644 --- a/src/CommonLib/Enums/DirectoryPaths.cs +++ b/src/CommonLib/Enums/DirectoryPaths.cs @@ -8,7 +8,8 @@ public static class DirectoryPaths public const string CertTemplateLocation = "CN=Certificate Templates,CN=Public Key Services,CN=Services"; public const string NTAuthStoreLocation = "CN=NTAuthCertificates,CN=Public Key Services,CN=Services"; public const string PKILocation = "CN=Public Key Services,CN=Services"; + public const string ExchangeLocation = "CN=Microsoft Exchange,CN=Services,CN=Configuration"; public const string ConfigLocation = "CN=Configuration"; public const string OIDContainerLocation = "CN=OID,CN=Public Key Services,CN=Services"; } -} \ No newline at end of file +} diff --git a/src/CommonLib/Enums/LDAPProperties.cs b/src/CommonLib/Enums/LDAPProperties.cs index 0bf6b726e..89ba25462 100644 --- a/src/CommonLib/Enums/LDAPProperties.cs +++ b/src/CommonLib/Enums/LDAPProperties.cs @@ -96,5 +96,6 @@ public static class LDAPProperties public const string LockOutObservationWindow = "lockoutobservationwindow"; public const string PrincipalName = "msds-principalname"; public const string GroupType = "grouptype"; + public const string CustomDenyAces = "customdenyaces"; } } diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 9db6d7ec0..98ec52692 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -881,7 +881,8 @@ public ActiveDirectorySecurityDescriptor MakeSecurityDescriptor() { string computerDomainSid, string computerDomain) { if (!WellKnownPrincipal.GetWellKnownPrincipal(sid.Value, out var common)) return (false, null); //The "Everyone" and "Authenticated Users" principals are special and will be converted to the domain equivalent - if (sid.Value is "S-1-1-0" or "S-1-5-11") { + if (sid.Value is var sidValue && + (sidValue == WellKnownPrincipal.EveryoneSid || sidValue == "S-1-5-11")) { return await GetWellKnownPrincipal(sid.Value, computerDomain); } @@ -1418,4 +1419,4 @@ private static string ComputeDisplayName(IDirectoryObject directoryObject, strin return displayName.ToUpper(); } } -} \ No newline at end of file +} diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index da3a615b4..59a44e5b3 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -10,6 +10,7 @@ using Microsoft.Extensions.Logging; using SharpHoundCommonLib.DirectoryObjects; using SharpHoundCommonLib.Enums; +using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using System.Linq; @@ -20,7 +21,15 @@ public class ACLProcessor { 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", + "Exchange Trusted Subsystem", + "Exchange Servers", + "Organization Management" + }; static ACLProcessor() { //Create a dictionary with the base GUIDs of each object type @@ -881,6 +890,175 @@ or Label.NTAuthStore } } + public Task GetCustomDenyAces(ResolvedSearchResult result, IDirectoryObject searchResult) { + if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { + return Task.FromResult(Array.Empty()); + } + + searchResult.TryGetDistinguishedName(out var distinguishedName); + return GetCustomDenyAces( + descriptor, + result.Domain, + result.ObjectType, + distinguishedName, + searchResult.IsMSA() || searchResult.IsGMSA(), + result.DisplayName); + } + + public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, string objectDomain, + Label objectType, string distinguishedName = null, bool isMSA = false, string objectName = "") { + if (ntSecurityDescriptor == null) { + return Array.Empty(); + } + + RawSecurityDescriptor descriptor; + try { + descriptor = new RawSecurityDescriptor(ntSecurityDescriptor, 0); + } + catch (OverflowException) { + _log.LogWarning( + "Security descriptor on object {Name} exceeds maximum allowable length. Unable to process custom deny ACEs", + objectName); + return Array.Empty(); + } + + if (descriptor.DiscretionaryAcl == null || descriptor.DiscretionaryAcl.Count == 0) { + return Array.Empty(); + } + + var results = new List(); + + // Walk the raw DACL so we can preserve deny ACE ordering and serialize each ACE back to SDDL verbatim. + foreach (GenericAce ace in descriptor.DiscretionaryAcl) { + if (!TryGetDenyAceData(ace, out var principalSid, out var rights, out var objectAceType)) { + continue; + } + + if (await ShouldExcludeCustomDenyAce(principalSid, rights, objectAceType, objectDomain, objectType, + distinguishedName, isMSA)) { + continue; + } + + var sddl = SerializeAceToSddl(ace); + if (!string.IsNullOrWhiteSpace(sddl)) { + results.Add(sddl); + } + } + + return results.Count == 0 ? Array.Empty() : results.ToArray(); + } + + public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, + string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, + string objectName = "") { + var customDenyAces = await GetCustomDenyAces(ntSecurityDescriptor, objectDomain, objectType, + distinguishedName, isMSA, objectName); + + if (customDenyAces.Length > 0) { + props[LDAPProperties.CustomDenyAces] = customDenyAces; + } + } + + private static bool TryGetDenyAceData(GenericAce ace, out string principalSid, out ActiveDirectoryRights rights, + out Guid objectAceType) { + principalSid = null; + rights = 0; + objectAceType = Guid.Empty; + + switch (ace) { + case CommonAce commonAce when commonAce.AceQualifier == AceQualifier.AccessDenied: + principalSid = commonAce.SecurityIdentifier?.Value; + rights = (ActiveDirectoryRights)commonAce.AccessMask; + return !string.IsNullOrWhiteSpace(principalSid); + case ObjectAce objectAce when objectAce.AceQualifier == AceQualifier.AccessDenied: + principalSid = objectAce.SecurityIdentifier?.Value; + rights = (ActiveDirectoryRights)objectAce.AccessMask; + objectAceType = objectAce.ObjectAceType; + return !string.IsNullOrWhiteSpace(principalSid); + default: + return false; + } + } + + private async Task ShouldExcludeCustomDenyAce(string principalSid, ActiveDirectoryRights rights, + Guid objectAceType, string objectDomain, Label objectType, string distinguishedName, bool isMSA) { + // Filter Exchange Deny ACEs + if (!string.IsNullOrWhiteSpace(distinguishedName) && + distinguishedName.IndexOf(DirectoryPaths.ExchangeLocation, StringComparison.OrdinalIgnoreCase) >= 0) { + return true; + } + + if (await IsExchangeTrustee(principalSid, objectDomain)) { + return true; + } + + // Filter default Everyone Deny ACEs + if (principalSid.Equals(WellKnownPrincipal.EveryoneSid, StringComparison.OrdinalIgnoreCase)) { + if ((objectType is Label.OU or Label.Container) && + rights.HasFlag(ActiveDirectoryRights.Delete) && + rights.HasFlag(ActiveDirectoryRights.DeleteTree)) { + return true; + } + + if (isMSA && + rights.HasFlag(ActiveDirectoryRights.ExtendedRight) && + objectAceType.Equals(new Guid(ACEGuids.UserForceChangePassword))) { + return true; + } + + if (objectType == Label.Domain && rights.HasFlag(ActiveDirectoryRights.DeleteChild)) { + return true; + } + } + + return false; + } + + private async Task IsExchangeTrustee(string principalSid, string objectDomain) { + if (string.IsNullOrWhiteSpace(principalSid) || string.IsNullOrWhiteSpace(objectDomain)) { + return false; + } + + if (_exchangeTrusteeSidCache.TryGetValue(objectDomain, out var cachedSids)) { + return cachedSids.Contains(principalSid, StringComparer.OrdinalIgnoreCase); + } + + // Well-known principals never match the Exchange groups we are suppressing. + if (WellKnownPrincipal.GetWellKnownPrincipal(principalSid, out _)) { + return false; + } + + // Resolve the small fixed set of Exchange trustee names once per domain using the shared name -> ID cache path. + var resolvedSids = new List(); + foreach (var trusteeName in ExchangeTrusteeNames) { + if (await _utils.ResolveAccountName(trusteeName, objectDomain) is (true, var principal) && + !string.IsNullOrWhiteSpace(principal.ObjectIdentifier)) { + resolvedSids.Add(principal.ObjectIdentifier); + } + } + + var exchangeTrusteeSids = resolvedSids.Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + _exchangeTrusteeSidCache.TryAdd(objectDomain, exchangeTrusteeSids); + return exchangeTrusteeSids.Contains(principalSid, StringComparer.OrdinalIgnoreCase); + } + + private static string SerializeAceToSddl(GenericAce ace) { + // Rehydrate the ACE inside a one-entry DACL and let the framework emit the canonical ACE SDDL for us. + var acl = new RawAcl(ace is ObjectAce ? GenericAcl.AclRevisionDS : GenericAcl.AclRevision, 1); + acl.InsertAce(0, CloneAce(ace)); + + var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); + var sddl = descriptor.GetSddlForm(AccessControlSections.Access); + + return sddl.StartsWith("D:", StringComparison.OrdinalIgnoreCase) ? sddl.Substring(2) : sddl; + } + + private static GenericAce CloneAce(GenericAce ace) { + var buffer = new byte[ace.BinaryLength]; + ace.GetBinaryForm(buffer, 0); + return GenericAce.CreateFromBinaryForm(buffer, 0); + } + /// /// Helper function to use commonlib types and pass to ProcessGMSAReaders diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index 14cd0e6f4..a722222d1 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -8,6 +8,7 @@ using System.Security.Principal; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using SharpHoundCommonLib.DirectoryObjects; using SharpHoundCommonLib.Enums; using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; @@ -37,10 +38,12 @@ static LdapPropertyProcessor() { private readonly ILdapUtils _utils; private readonly ILogger _log; + private readonly ACLProcessor _aclProcessor; public LdapPropertyProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; _log = log ?? Logging.LogProvider.CreateLogger(nameof(LdapPropertyProcessor)); + _aclProcessor = new ACLProcessor(utils, _log); } private static Dictionary GetCommonProps(IDirectoryObject entry) { @@ -78,6 +81,7 @@ private static Dictionary GetCommonProps(IDirectoryObject entry) /// public async Task> ReadDomainProperties(IDirectoryObject entry, string domain) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, domain, Label.Domain); if (entry.TryGetProperty(LDAPProperties.ExpirePasswordsOnSmartCardOnlyAccounts, out var expirePassword) && bool.TryParse(expirePassword, out var expirePasswordBool)) { @@ -178,8 +182,9 @@ public static string FunctionalLevelToString(int level) { /// /// /// - public static Dictionary ReadGPOProperties(IDirectoryObject entry) { + public async Task> ReadGPOProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.GPO); entry.TryGetProperty(LDAPProperties.GPCFileSYSPath, out var path); props.Add("gpcpath", path.ToUpper()); entry.TryGetProperty(LDAPProperties.Flags, out var flags); @@ -192,8 +197,9 @@ public static Dictionary ReadGPOProperties(IDirectoryObject entr /// /// /// - public static Dictionary ReadOUProperties(IDirectoryObject entry) { + public async Task> ReadOUProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.OU); return props; } @@ -212,6 +218,7 @@ public async Task ReadGroupPropertiesAsync(IDirectoryObject ent { var groupProperties = new GroupProperties(); var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, domain, Label.Group); entry.TryGetLongProperty(LDAPProperties.AdminCount, out var ac); props.Add("admincount", ac != 0); entry.TryGetLongProperty(LDAPProperties.GroupType, out var groupType); @@ -230,8 +237,10 @@ public async Task ReadGroupPropertiesAsync(IDirectoryObject ent /// /// /// - public static Dictionary ReadContainerProperties(IDirectoryObject entry) { + public async Task> ReadContainerProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + var objectType = entry.GetLabel(out var label) ? label : Label.Container; + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), objectType); return props; } @@ -249,6 +258,7 @@ public Task public async Task ReadUserProperties(IDirectoryObject entry, string domain) { var userProps = new UserProperties(); var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, domain, Label.User); if (entry.TryGetLongProperty(LDAPProperties.UserAccountControl, out var uac)) { var uacFlags = (UacFlags)uac; @@ -364,6 +374,7 @@ public Task ReadComputerProperties(IDirectoryObject entry, public async Task ReadComputerProperties(IDirectoryObject entry, string domain) { var compProps = new ComputerProperties(); var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, domain, Label.Computer); var flags = (UacFlags)0; if (entry.TryGetLongProperty(LDAPProperties.UserAccountControl, out var uac)) { @@ -468,8 +479,9 @@ await SendComputerStatus(new CSVComputerStatus { /// /// /// Returns a dictionary with the common properties of the RootCA - public static Dictionary ReadRootCAProperties(IDirectoryObject entry) { + public async Task> ReadRootCAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.RootCA); // Certificate if (entry.TryGetByteProperty(LDAPProperties.CACertificate, out var rawCertificate) && HasBytes(rawCertificate)) { @@ -489,8 +501,9 @@ public static Dictionary ReadRootCAProperties(IDirectoryObject e /// /// /// Returns a dictionary with the common properties and the crosscertificatepair property of the AICA - public static Dictionary ReadAIACAProperties(IDirectoryObject entry) { + public async Task> ReadAIACAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.AIACA); entry.TryGetByteArrayProperty(LDAPProperties.CrossCertificatePair, out var crossCertificatePair); var hasCrossCertificatePair = crossCertificatePair.Length > 0; @@ -515,8 +528,9 @@ public static Dictionary ReadAIACAProperties(IDirectoryObject en /// /// /// Returns a dictionary with the common properties and the caname, hostname, and flags properties of the EnterpriseCA - public static Dictionary ReadEnterpriseCAProperties(IDirectoryObject entry) { + public async Task> ReadEnterpriseCAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.EnterpriseCA); if (entry.TryGetLongProperty("flags", out var flags)) props.Add("flags", (PKICertificateAuthorityFlags)flags); props.Add("caname", entry.GetProperty(LDAPProperties.Name)); @@ -540,8 +554,9 @@ public static Dictionary ReadEnterpriseCAProperties(IDirectoryOb /// /// /// Returns a dictionary with the common properties of the NTAuthStore - public static Dictionary ReadNTAuthStoreProperties(IDirectoryObject entry) { + public async Task> ReadNTAuthStoreProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.NTAuthStore); return props; } @@ -550,8 +565,9 @@ public static Dictionary ReadNTAuthStoreProperties(IDirectoryObj /// /// /// Returns a dictionary associated with the CertTemplate properties that were read - public static Dictionary ReadCertTemplateProperties(IDirectoryObject entry) { + public async Task> ReadCertTemplateProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.CertTemplate); props.Add("validityperiod", ConvertPKIPeriod(entry.GetByteProperty(LDAPProperties.PKIExpirationPeriod))); props.Add("renewalperiod", ConvertPKIPeriod(entry.GetByteProperty(LDAPProperties.PKIOverlappedPeriod))); @@ -636,6 +652,7 @@ public static Dictionary ReadCertTemplateProperties(IDirectoryOb public async Task ReadIssuancePolicyProperties(IDirectoryObject entry) { var ret = new IssuancePolicyProperties(); var props = GetCommonProps(entry); + await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.IssuancePolicy); props.Add("displayname", entry.GetProperty(LDAPProperties.DisplayName)); props.Add("certtemplateoid", entry.GetProperty(LDAPProperties.CertTemplateOID)); @@ -650,6 +667,23 @@ public async Task ReadIssuancePolicyProperties(IDirect return ret; } + private async Task AddCustomDenyAceProperty(Dictionary props, IDirectoryObject entry, + string domain, Label objectType) { + if (!entry.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var ntSecurityDescriptor)) { + return; + } + + entry.TryGetDistinguishedName(out var distinguishedName); + await _aclProcessor.AddCustomDenyAcesProperty(props, ntSecurityDescriptor, domain, objectType, + distinguishedName, entry.IsMSA() || entry.IsGMSA(), distinguishedName ?? string.Empty); + } + + private static string GetEntryDomain(IDirectoryObject entry) { + return entry.TryGetDistinguishedName(out var distinguishedName) + ? Helpers.DistinguishedNameToDomain(distinguishedName) + : string.Empty; + } + /// /// Attempts to parse all LDAP attributes outside of the ones already collected and converts them to a human readable /// format using a best guess diff --git a/src/CommonLib/WellKnownPrincipal.cs b/src/CommonLib/WellKnownPrincipal.cs index 4185dc8bb..5c8f661ea 100644 --- a/src/CommonLib/WellKnownPrincipal.cs +++ b/src/CommonLib/WellKnownPrincipal.cs @@ -5,6 +5,8 @@ namespace SharpHoundCommonLib { public static class WellKnownPrincipal { + public const string EveryoneSid = "S-1-1-0"; + /// /// Gets the principal associated with a well known SID /// @@ -18,7 +20,7 @@ public static bool GetWellKnownPrincipal(string sid, out TypedPrincipal commonPr "S-1-0" => new TypedPrincipal("Null Authority", Label.User), "S-1-0-0" => new TypedPrincipal("Nobody", Label.User), "S-1-1" => new TypedPrincipal("World Authority", Label.User), - "S-1-1-0" => new TypedPrincipal("Everyone", Label.Group), + EveryoneSid => new TypedPrincipal("Everyone", Label.Group), "S-1-2" => new TypedPrincipal("Local Authority", Label.User), "S-1-2-0" => new TypedPrincipal("Local", Label.Group), "S-1-2-1" => new TypedPrincipal("Console Logon", Label.Group), diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index f6907fd34..40f1f2b18 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -2,9 +2,11 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.DirectoryServices; +using System.Collections; using System.Linq; using System.Runtime.Versioning; using System.Security.AccessControl; +using System.Security.Principal; using System.Threading; using System.Threading.Tasks; using CommonLibTest.Facades; @@ -12,6 +14,7 @@ using Newtonsoft.Json; using SharpHoundCommonLib; using SharpHoundCommonLib.Enums; +using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; using SharpHoundCommonLib.Processors; using Xunit; @@ -2135,5 +2138,170 @@ public async Task ACLProcessor_ProcessACL_EnterpriseCA_Enroll() Assert.False(actual.IsInherited); Assert.Equal(actual.RightName, expectedRightName); } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_EmitsQualifyingDenyAce() { + var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2500", + ActiveDirectoryRights.Delete); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Single(result); + Assert.Equal(SerializeAce(ace), result[0]); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeTrusteeDenyAce() { + var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2600", + ActiveDirectoryRights.Delete); + var processor = CreateCustomDenyAceProcessor(("S-1-5-21-3130019616-2776909439-2417379446-2600", + "Exchange Windows Permissions")); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Empty(result); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_SkipsOrganizationManagementDenyAce() { + var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2601", + ActiveDirectoryRights.Delete); + var processor = CreateCustomDenyAceProcessor(("S-1-5-21-3130019616-2776909439-2417379446-2601", + "Organization Management")); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Empty(result); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeConfigurationPath() { + var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2700", + ActiveDirectoryRights.Delete); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.Container, + "CN=Mailbox Database,CN=Microsoft Exchange,CN=Services,CN=Configuration,DC=TESTLAB,DC=LOCAL"); + + Assert.Empty(result); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_SkipsAccidentalDeletionProtection() { + var ace = CreateCommonDenyAce("S-1-1-0", + ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); + + Assert.Empty(result); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_SkipsDefaultAdDenyPatterns() { + var msaAce = CreateObjectDenyAce("S-1-1-0", ActiveDirectoryRights.ExtendedRight, + new Guid(ACEGuids.UserForceChangePassword)); + var domainAce = CreateCommonDenyAce("S-1-1-0", ActiveDirectoryRights.DeleteChild); + var processor = CreateCustomDenyAceProcessor(); + + var msaResult = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(msaAce), _testDomainName, + Label.User, "CN=TEST MSA,CN=Managed Service Accounts,DC=TESTLAB,DC=LOCAL", true); + var domainResult = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(domainAce), + _testDomainName, Label.Domain, "DC=TESTLAB,DC=LOCAL"); + + Assert.Empty(msaResult); + Assert.Empty(domainResult); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_EmitsMultipleQualifyingAces() { + var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2800", + ActiveDirectoryRights.Delete); + var ace2 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2801", + ActiveDirectoryRights.DeleteChild); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace1, ace2), _testDomainName, + Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Equal(2, result.Length); + Assert.Equal(SerializeAce(ace1), result[0]); + Assert.Equal(SerializeAce(ace2), result[1]); + } + + [Fact] + public async Task ACLProcessor_GetCustomDenyAces_PreservesDeterministicOrdering() { + var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2901", + ActiveDirectoryRights.DeleteChild); + var ace2 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2900", + ActiveDirectoryRights.Delete); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace1, ace2), _testDomainName, + Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Equal(new[] { SerializeAce(ace1), SerializeAce(ace2) }, result); + } + + [Fact] + public async Task ACLProcessor_AddCustomDenyAcesProperty_DoesNotEmitWhenEmpty() { + var props = new Dictionary(); + var ace = CreateCommonDenyAce("S-1-1-0", + ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree); + var processor = CreateCustomDenyAceProcessor(); + + await processor.AddCustomDenyAcesProperty(props, CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); + + Assert.DoesNotContain("customdenyaces", props.Keys); + } + + private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Name)[] principals) { + var mockLdapUtils = new Mock(MockBehavior.Strict); + mockLdapUtils.Setup(x => x.ResolveAccountName(It.IsAny(), It.IsAny())) + .ReturnsAsync((string name, string _) => { + var match = principals.FirstOrDefault(x => x.Name.Equals(name, StringComparison.OrdinalIgnoreCase)); + return string.IsNullOrWhiteSpace(match.Sid) + ? (false, null) + : (true, new TypedPrincipal(match.Sid, Label.Group)); + }); + + return new ACLProcessor(mockLdapUtils.Object); + } + + private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) { + var acl = new RawAcl(GenericAcl.AclRevisionDS, aces.Length); + for (var i = 0; i < aces.Length; i++) { + acl.InsertAce(i, aces[i]); + } + + var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); + var buffer = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(buffer, 0); + return buffer; + } + + private static CommonAce CreateCommonDenyAce(string sid, ActiveDirectoryRights rights) { + return new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)rights, + new SecurityIdentifier(sid), false, null); + } + + private static ObjectAce CreateObjectDenyAce(string sid, ActiveDirectoryRights rights, Guid objectType) { + return new ObjectAce(AceFlags.None, AceQualifier.AccessDenied, (int)rights, + new SecurityIdentifier(sid), ObjectAceFlags.ObjectAceTypePresent, objectType, Guid.Empty, false, null); + } + + private static string SerializeAce(GenericAce ace) { + var acl = new RawAcl(GenericAcl.AclRevisionDS, 1); + acl.InsertAce(0, ace); + var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); + return descriptor.GetSddlForm(AccessControlSections.Access).Substring(2); + } } -} \ No newline at end of file +} diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 6fe094509..18ae40118 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -82,7 +82,7 @@ public void LDAPPropertyProcessor_FunctionalLevelToString_TestFunctionalLevels() } [Fact] - public void LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() + public async Task LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() { var mock = new MockDirectoryObject( "CN\u003d{94DD0260-38B5-497E-8876-10E7A96E80D0},CN\u003dPolicies,CN\u003dSystem,DC\u003dtestlab,DC\u003dlocal", @@ -96,7 +96,8 @@ public void LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() {"description", "Test"} }, "S-1-5-21-3130019616-2776909439-2417379446",""); - var test = LdapPropertyProcessor.ReadGPOProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadGPOProperties(mock); Assert.Contains("description", test.Keys); Assert.Equal("Test", test["description"] as string); @@ -106,7 +107,7 @@ public void LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() } [Fact] - public void LDAPPropertyProcessor_ReadOUProperties_TestGoodData() + public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() { var mock = new MockDirectoryObject("OU\u003dTestOU,DC\u003dtestlab,DC\u003dlocal", new Dictionary @@ -114,7 +115,8 @@ public void LDAPPropertyProcessor_ReadOUProperties_TestGoodData() {"description", "Test"} },"", "2A374493-816A-4193-BEFD-D2F4132C6DCA"); - var test = LdapPropertyProcessor.ReadOUProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadOUProperties(mock); Assert.Contains("description", test.Keys); Assert.Equal("Test", test["description"] as string); } @@ -681,7 +683,7 @@ public async Task LDAPPropertyProcessor_ReadComputerProperties_TestDumpSMSAPassw } [Fact] - public void LDAPPropertyProcessor_ReadRootCAProperties() { + public async Task LDAPPropertyProcessor_ReadRootCAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -699,7 +701,8 @@ public void LDAPPropertyProcessor_ReadRootCAProperties() { {LDAPProperties.CACertificate, bytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadRootCAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadRootCAProperties(mock); var keys = test.Keys; //These are not common properties @@ -718,7 +721,7 @@ public void LDAPPropertyProcessor_ReadRootCAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { + public async Task LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -731,7 +734,8 @@ public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CA {LDAPProperties.CACertificate, CACertBytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadRootCAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadRootCAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -745,7 +749,7 @@ public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CA } [Fact] - public void LDAPPropertyProcessor_ReadAIACAProperties() { + public async Task LDAPPropertyProcessor_ReadAIACAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -764,7 +768,8 @@ public void LDAPPropertyProcessor_ReadAIACAProperties() { {LDAPProperties.CACertificate, bytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadAIACAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadAIACAProperties(mock); var keys = test.Keys; //These are not common properties @@ -787,7 +792,7 @@ public void LDAPPropertyProcessor_ReadAIACAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public void LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CACertBytes) { + public async Task LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -801,7 +806,8 @@ public void LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CAC {LDAPProperties.CACertificate, CACertBytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadAIACAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadAIACAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -817,7 +823,7 @@ public void LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CAC } [Fact] - public void LDAPPropertyProcessor_ReadEnterpriseCAProperties() { + public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -836,7 +842,8 @@ public void LDAPPropertyProcessor_ReadEnterpriseCAProperties() { {"flags", 1} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadEnterpriseCAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadEnterpriseCAProperties(mock); var keys = test.Keys; //These are not common properties @@ -860,7 +867,7 @@ public void LDAPPropertyProcessor_ReadEnterpriseCAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public void LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byte[] CACertBytes) { + public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -874,7 +881,8 @@ public void LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byt {"flags", 1} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadEnterpriseCAProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadEnterpriseCAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -899,7 +907,7 @@ public void LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byt }; [Fact] - public void LDAPPropertyProcessor_ReadNTAuthStoreProperties() + public async Task LDAPPropertyProcessor_ReadNTAuthStoreProperties() { var mock = new MockDirectoryObject("CN\u003dNTAUTHCERTIFICATES,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -911,7 +919,8 @@ public void LDAPPropertyProcessor_ReadNTAuthStoreProperties() {"whencreated", 1683986131}, }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadNTAuthStoreProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadNTAuthStoreProperties(mock); var keys = test.Keys; //These are not common properties @@ -923,7 +932,7 @@ public void LDAPPropertyProcessor_ReadNTAuthStoreProperties() } [Fact] - public void LDAPPropertyProcessor_ReadCertTemplateProperties() + public async Task LDAPPropertyProcessor_ReadCertTemplateProperties() { var mock = new MockDirectoryObject("CN\u003dWORKSTATION,CN\u003dCERTIFICATE TEMPLATES,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dEXTERNAL,DC\u003dLOCAL", new Dictionary @@ -961,7 +970,8 @@ public void LDAPPropertyProcessor_ReadCertTemplateProperties() {LDAPProperties.PKIPrivateKeyFlag, 256}, }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var test = LdapPropertyProcessor.ReadCertTemplateProperties(mock); + var processor = new LdapPropertyProcessor(new MockLdapUtils()); + var test = await processor.ReadCertTemplateProperties(mock); var keys = test.Keys; //These are not common properties From 6d2c4ad30b006dddc80a9a734ede435f6f80cf20 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 13:25:30 +0200 Subject: [PATCH 02/13] disable DocFX by default on non-Windows --- docfx/Docfx.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/docfx/Docfx.csproj b/docfx/Docfx.csproj index e8d74dd37..6133e54fe 100644 --- a/docfx/Docfx.csproj +++ b/docfx/Docfx.csproj @@ -2,6 +2,7 @@ net5.0 false + false $([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), `..`, `docs`)) 8002 $(MSBuildThisFileDirectory)docfx.log From 4490451cd4fc5b9ff60dd527dac0be5897967677 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 14:23:40 +0200 Subject: [PATCH 03/13] add SkipDenyAces flag --- src/CommonLib/ILdapUtils.cs | 6 +++- src/CommonLib/LdapConfig.cs | 3 +- src/CommonLib/LdapUtils.cs | 4 +++ .../Processors/LdapPropertyProcessor.cs | 4 +++ test/unit/Facades/MockLdapUtils.cs | 9 +++-- test/unit/LdapPropertyTests.cs | 36 +++++++++++++++++++ 6 files changed, 58 insertions(+), 4 deletions(-) diff --git a/src/CommonLib/ILdapUtils.cs b/src/CommonLib/ILdapUtils.cs index 753e53bd1..56faa21fc 100644 --- a/src/CommonLib/ILdapUtils.cs +++ b/src/CommonLib/ILdapUtils.cs @@ -157,6 +157,10 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// The new ldap config void SetLdapConfig(LdapConfig config); /// + /// Gets the current ldap config for this utils instance + /// + LdapConfig GetLdapConfig(); + /// /// Tests if a LDAP connection can be made successfully to a domain /// /// The domain to test @@ -175,4 +179,4 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// void ResetUtils(); } -} \ No newline at end of file +} diff --git a/src/CommonLib/LdapConfig.cs b/src/CommonLib/LdapConfig.cs index 3f3e84e40..81da1541a 100644 --- a/src/CommonLib/LdapConfig.cs +++ b/src/CommonLib/LdapConfig.cs @@ -13,6 +13,7 @@ public class LdapConfig public bool ForceSSL { get; set; } = false; public bool DisableSigning { get; set; } = false; public bool DisableCertVerification { get; set; } = false; + public bool SkipDenyAces { get; set; } = false; public AuthType AuthType { get; set; } = AuthType.Kerberos; public int MaxConcurrentQueries { get; set; } = 15; @@ -53,4 +54,4 @@ public override string ToString() { return sb.ToString(); } } -} \ No newline at end of file +} diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 98ec52692..6d0ee17ad 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1077,6 +1077,10 @@ public void SetLdapConfig(LdapConfig config) { _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); } + public LdapConfig GetLdapConfig() { + return _ldapConfig; + } + public Task<(bool Success, string Message)> TestLdapConnection(string domain) { return _connectionPool.TestDomainConnection(domain, false); } diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index a722222d1..2f38fe19f 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -669,6 +669,10 @@ public async Task ReadIssuancePolicyProperties(IDirect private async Task AddCustomDenyAceProperty(Dictionary props, IDirectoryObject entry, string domain, Label objectType) { + if (_utils.GetLdapConfig().SkipDenyAces) { + return; + } + if (!entry.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var ntSecurityDescriptor)) { return; } diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index 1e5f1194d..e3b6a56e7 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -20,6 +20,7 @@ namespace CommonLibTest.Facades [SuppressMessage("Interoperability", "CA1416:Validate platform compatibility")] public class MockLdapUtils : ILdapUtils { + private LdapConfig _ldapConfig = new(); private readonly ConcurrentDictionary _domainControllers = new(); private readonly Forest _forest; private readonly ConcurrentDictionary _seenWellKnownPrincipals = new(); @@ -1007,7 +1008,11 @@ public IAsyncEnumerable GetWellKnownPrincipalOutput() { } public void SetLdapConfig(LdapConfig config) { - throw new NotImplementedException(); + _ldapConfig = config; + } + + public LdapConfig GetLdapConfig() { + return _ldapConfig; } public Task<(bool Success, string Message)> TestLdapConnection(string domain) { @@ -1127,4 +1132,4 @@ public void Dispose() { return (true, "0"); } } -} \ No newline at end of file +} diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 18ae40118..7862df516 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.DirectoryServices; using System.Runtime.Versioning; +using System.Security.AccessControl; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Security.Principal; @@ -30,6 +31,20 @@ public LdapPropertyTests(ITestOutputHelper testOutputHelper) _testOutputHelper = testOutputHelper; } + private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) + { + var acl = new RawAcl(GenericAcl.AclRevisionDS, aces.Length); + for (var i = 0; i < aces.Length; i++) + { + acl.InsertAce(i, aces[i]); + } + + var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); + var buffer = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(buffer, 0); + return buffer; + } + [Fact] public async void LDAPPropertyProcessor_ReadDomainProperties_TestGoodData() { @@ -121,6 +136,27 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() Assert.Equal("Test", test["description"] as string); } + [Fact] + public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt() + { + var denyAce = new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)ActiveDirectoryRights.Delete, + new SecurityIdentifier("S-1-5-21-3130019616-2776909439-2417379446-2500"), false, null); + var mock = new MockDirectoryObject("OU\u003dTestOU,DC\u003dtestlab,DC\u003dlocal", + new Dictionary + { + {LDAPProperties.SecurityDescriptor, CreateSecurityDescriptorBytes(denyAce)} + }, "", "2A374493-816A-4193-BEFD-D2F4132C6DCA"); + var ldapUtils = new MockLdapUtils(); + ldapUtils.SetLdapConfig(new LdapConfig { + SkipDenyAces = true + }); + + var processor = new LdapPropertyProcessor(ldapUtils); + var test = await processor.ReadOUProperties(mock); + + Assert.DoesNotContain(LDAPProperties.CustomDenyAces, test.Keys); + } + [Fact] public async Task LDAPPropertyProcessor_ReadGroupProperties_TestGoodData() { From fd53dac85f7d2f045f5cb1c336e40364752d9a1a Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 14:44:13 +0200 Subject: [PATCH 04/13] mv customdenyaces out of LDAPProperties --- src/CommonLib/Enums/LDAPProperties.cs | 1 - src/CommonLib/Processors/ACLProcessor.cs | 2 +- test/unit/LdapPropertyTests.cs | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/CommonLib/Enums/LDAPProperties.cs b/src/CommonLib/Enums/LDAPProperties.cs index 89ba25462..0bf6b726e 100644 --- a/src/CommonLib/Enums/LDAPProperties.cs +++ b/src/CommonLib/Enums/LDAPProperties.cs @@ -96,6 +96,5 @@ public static class LDAPProperties public const string LockOutObservationWindow = "lockoutobservationwindow"; public const string PrincipalName = "msds-principalname"; public const string GroupType = "grouptype"; - public const string CustomDenyAces = "customdenyaces"; } } diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 59a44e5b3..08f9af12b 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -955,7 +955,7 @@ public async Task AddCustomDenyAcesProperty(Dictionary props, by distinguishedName, isMSA, objectName); if (customDenyAces.Length > 0) { - props[LDAPProperties.CustomDenyAces] = customDenyAces; + props["customdenyaces"] = customDenyAces; } } diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 7862df516..5dfaa9d32 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -154,7 +154,7 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_Whe var processor = new LdapPropertyProcessor(ldapUtils); var test = await processor.ReadOUProperties(mock); - Assert.DoesNotContain(LDAPProperties.CustomDenyAces, test.Keys); + Assert.DoesNotContain("customdenyaces", test.Keys); } [Fact] From 686b7814a820bbb0ea066bc6c157ce59023a3927 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 18:07:53 +0200 Subject: [PATCH 05/13] coderabbit fixes --- src/CommonLib/ILdapUtils.cs | 4 ++-- src/CommonLib/LdapConfig.cs | 1 + src/CommonLib/LdapUtils.cs | 4 +--- src/CommonLib/Processors/ACLProcessor.cs | 2 +- src/CommonLib/Processors/LdapPropertyProcessor.cs | 2 +- test/unit/Facades/MockLdapUtils.cs | 4 +--- test/unit/LdapPropertyTests.cs | 5 +++++ 7 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/CommonLib/ILdapUtils.cs b/src/CommonLib/ILdapUtils.cs index 56faa21fc..2fc7a6b32 100644 --- a/src/CommonLib/ILdapUtils.cs +++ b/src/CommonLib/ILdapUtils.cs @@ -157,9 +157,9 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// The new ldap config void SetLdapConfig(LdapConfig config); /// - /// Gets the current ldap config for this utils instance + /// Gets whether custom deny ACE collection is disabled for this utils instance /// - LdapConfig GetLdapConfig(); + bool SkipDenyAces { get; } /// /// Tests if a LDAP connection can be made successfully to a domain /// diff --git a/src/CommonLib/LdapConfig.cs b/src/CommonLib/LdapConfig.cs index 81da1541a..5242d02b2 100644 --- a/src/CommonLib/LdapConfig.cs +++ b/src/CommonLib/LdapConfig.cs @@ -42,6 +42,7 @@ public override string ToString() { sb.AppendLine($"LdapPort: {GetPort(false)}"); sb.AppendLine($"LdapSSLPort: {GetPort(true)}"); sb.AppendLine($"ForceSSL: {ForceSSL}"); + sb.AppendLine($"SkipDenyAces: {SkipDenyAces}"); sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); if (!string.IsNullOrWhiteSpace(Username)) { diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 6d0ee17ad..cd416823d 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1077,9 +1077,7 @@ public void SetLdapConfig(LdapConfig config) { _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); } - public LdapConfig GetLdapConfig() { - return _ldapConfig; - } + public bool SkipDenyAces => _ldapConfig.SkipDenyAces; public Task<(bool Success, string Message)> TestLdapConnection(string domain) { return _connectionPool.TestDomainConnection(domain, false); diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 08f9af12b..44edeb9b4 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -915,7 +915,7 @@ public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, strin try { descriptor = new RawSecurityDescriptor(ntSecurityDescriptor, 0); } - catch (OverflowException) { + catch (Exception e) when (e is OverflowException or ArgumentException) { _log.LogWarning( "Security descriptor on object {Name} exceeds maximum allowable length. Unable to process custom deny ACEs", objectName); diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index 2f38fe19f..2d1aac4d1 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -669,7 +669,7 @@ public async Task ReadIssuancePolicyProperties(IDirect private async Task AddCustomDenyAceProperty(Dictionary props, IDirectoryObject entry, string domain, Label objectType) { - if (_utils.GetLdapConfig().SkipDenyAces) { + if (_utils.SkipDenyAces) { return; } diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index e3b6a56e7..efe0c0518 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -1011,9 +1011,7 @@ public void SetLdapConfig(LdapConfig config) { _ldapConfig = config; } - public LdapConfig GetLdapConfig() { - return _ldapConfig; - } + public bool SkipDenyAces => _ldapConfig.SkipDenyAces; public Task<(bool Success, string Message)> TestLdapConnection(string domain) { throw new NotImplementedException(); diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 5dfaa9d32..966a6cd93 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -146,6 +146,11 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_Whe { {LDAPProperties.SecurityDescriptor, CreateSecurityDescriptorBytes(denyAce)} }, "", "2A374493-816A-4193-BEFD-D2F4132C6DCA"); + + var baselineProcessor = new LdapPropertyProcessor(new MockLdapUtils()); + var baseline = await baselineProcessor.ReadOUProperties(mock); + Assert.Contains("customdenyaces", baseline.Keys); + var ldapUtils = new MockLdapUtils(); ldapUtils.SetLdapConfig(new LdapConfig { SkipDenyAces = true From 8fe085885511b74a271d009fb0f283bc8a93c045 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 18:15:48 +0200 Subject: [PATCH 06/13] fix Windows only code --- test/unit/LdapPropertyTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 966a6cd93..2f8372f16 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -31,6 +31,7 @@ public LdapPropertyTests(ITestOutputHelper testOutputHelper) _testOutputHelper = testOutputHelper; } + [SupportedOSPlatform("windows")] private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) { var acl = new RawAcl(GenericAcl.AclRevisionDS, aces.Length); @@ -136,7 +137,8 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() Assert.Equal("Test", test["description"] as string); } - [Fact] + [SupportedOSPlatform("windows")] + [WindowsOnlyFact] public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt() { var denyAce = new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)ActiveDirectoryRights.Delete, From 820d80d2ca977fd2031bd3824d5f835c6ea6c70c Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 23 Apr 2026 18:45:04 +0200 Subject: [PATCH 07/13] fix AddCustomDenyAceProperty --- src/CommonLib/Processors/LdapPropertyProcessor.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index 2d1aac4d1..11e11f84e 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -677,9 +677,12 @@ private async Task AddCustomDenyAceProperty(Dictionary props, ID return; } - entry.TryGetDistinguishedName(out var distinguishedName); + var distinguishedName = entry.TryGetDistinguishedName(out var dn) ? dn : string.Empty; + var objectName = entry.TryGetProperty(LDAPProperties.SAMAccountName, out var samAccountName) + ? samAccountName + : distinguishedName; await _aclProcessor.AddCustomDenyAcesProperty(props, ntSecurityDescriptor, domain, objectType, - distinguishedName, entry.IsMSA() || entry.IsGMSA(), distinguishedName ?? string.Empty); + distinguishedName, entry.IsMSA() || entry.IsGMSA(), objectName); } private static string GetEntryDomain(IDirectoryObject entry) { From decd8a421c589c09e68986c3ceae096f52c196ca Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 14 May 2026 19:55:00 +0200 Subject: [PATCH 08/13] make CustomdDenyACEs a count --- src/CommonLib/Processors/ACLProcessor.cs | 93 +++++++++++++++++++----- test/unit/ACLProcessorTest.cs | 58 ++++++++++++--- test/unit/LdapPropertyTests.cs | 8 +- 3 files changed, 130 insertions(+), 29 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 44edeb9b4..651014f3b 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -57,6 +57,17 @@ public ACLProcessor(ILdapUtils utils, ILogger log = null) _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); } + public readonly struct CustomDenyAceCounts { + public CustomDenyAceCounts(int explicitCount, int inheritedCount) { + ExplicitCount = explicitCount; + InheritedCount = inheritedCount; + } + + public int ExplicitCount { get; } + public int InheritedCount { get; } + public int Total => ExplicitCount + InheritedCount; + } + /// Represents a lightweight Access Control Entry (ACE) used to compute hash values /// for AdminSDHolder purposes internal class ACEForHashing { @@ -905,10 +916,55 @@ public Task GetCustomDenyAces(ResolvedSearchResult result, IDirectoryO result.DisplayName); } + public Task GetCustomDenyAceCounts(ResolvedSearchResult result, + IDirectoryObject searchResult) { + if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { + return Task.FromResult(new CustomDenyAceCounts()); + } + + searchResult.TryGetDistinguishedName(out var distinguishedName); + return GetCustomDenyAceCounts( + descriptor, + result.Domain, + result.ObjectType, + distinguishedName, + searchResult.IsMSA() || searchResult.IsGMSA(), + result.DisplayName); + } + public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, string objectName = "") { + var customDenyAces = await GetCustomDenyAceData(ntSecurityDescriptor, objectDomain, objectType, + distinguishedName, isMSA, objectName, true); + + return customDenyAces.Sddl.Count == 0 ? Array.Empty() : customDenyAces.Sddl.ToArray(); + } + + public async Task GetCustomDenyAceCounts(byte[] ntSecurityDescriptor, + string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, + string objectName = "") { + var customDenyAces = await GetCustomDenyAceData(ntSecurityDescriptor, objectDomain, objectType, + distinguishedName, isMSA, objectName, false); + + return new CustomDenyAceCounts(customDenyAces.ExplicitCount, customDenyAces.InheritedCount); + } + + public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, + string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, + string objectName = "") { + var counts = await GetCustomDenyAceCounts(ntSecurityDescriptor, objectDomain, objectType, + distinguishedName, isMSA, objectName); + + if (counts.Total > 0) { + props["customexplicitdenyacescount"] = counts.ExplicitCount; + props["custominheriteddenyacescount"] = counts.InheritedCount; + } + } + + private async Task GetCustomDenyAceData(byte[] ntSecurityDescriptor, string objectDomain, + Label objectType, string distinguishedName, bool isMSA, string objectName, bool collectSddl) { if (ntSecurityDescriptor == null) { - return Array.Empty(); + return new CustomDenyAceData(); } RawSecurityDescriptor descriptor; @@ -919,14 +975,14 @@ public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, strin _log.LogWarning( "Security descriptor on object {Name} exceeds maximum allowable length. Unable to process custom deny ACEs", objectName); - return Array.Empty(); + return new CustomDenyAceData(); } if (descriptor.DiscretionaryAcl == null || descriptor.DiscretionaryAcl.Count == 0) { - return Array.Empty(); + return new CustomDenyAceData(); } - var results = new List(); + var results = new CustomDenyAceData(); // Walk the raw DACL so we can preserve deny ACE ordering and serialize each ACE back to SDDL verbatim. foreach (GenericAce ace in descriptor.DiscretionaryAcl) { @@ -939,24 +995,27 @@ public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, strin continue; } - var sddl = SerializeAceToSddl(ace); - if (!string.IsNullOrWhiteSpace(sddl)) { - results.Add(sddl); + if ((ace.AceFlags & AceFlags.Inherited) == AceFlags.Inherited) { + results.InheritedCount++; + } else { + results.ExplicitCount++; + } + + if (collectSddl) { + var sddl = SerializeAceToSddl(ace); + if (!string.IsNullOrWhiteSpace(sddl)) { + results.Sddl.Add(sddl); + } } } - return results.Count == 0 ? Array.Empty() : results.ToArray(); + return results; } - public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, - string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, - string objectName = "") { - var customDenyAces = await GetCustomDenyAces(ntSecurityDescriptor, objectDomain, objectType, - distinguishedName, isMSA, objectName); - - if (customDenyAces.Length > 0) { - props["customdenyaces"] = customDenyAces; - } + private class CustomDenyAceData { + public List Sddl { get; } = new(); + public int ExplicitCount { get; set; } + public int InheritedCount { get; set; } } private static bool TryGetDenyAceData(GenericAce ace, out string principalSid, out ActiveDirectoryRights rights, diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index 40f1f2b18..d9e184f34 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -2139,7 +2139,7 @@ public async Task ACLProcessor_ProcessACL_EnterpriseCA_Enroll() Assert.Equal(actual.RightName, expectedRightName); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_EmitsQualifyingDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2500", ActiveDirectoryRights.Delete); @@ -2152,7 +2152,7 @@ public async Task ACLProcessor_GetCustomDenyAces_EmitsQualifyingDenyAce() { Assert.Equal(SerializeAce(ace), result[0]); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeTrusteeDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2600", ActiveDirectoryRights.Delete); @@ -2165,7 +2165,7 @@ public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeTrusteeDenyAce() { Assert.Empty(result); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_SkipsOrganizationManagementDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2601", ActiveDirectoryRights.Delete); @@ -2178,7 +2178,7 @@ public async Task ACLProcessor_GetCustomDenyAces_SkipsOrganizationManagementDeny Assert.Empty(result); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeConfigurationPath() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2700", ActiveDirectoryRights.Delete); @@ -2191,7 +2191,7 @@ public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeConfigurationPath( Assert.Empty(result); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_SkipsAccidentalDeletionProtection() { var ace = CreateCommonDenyAce("S-1-1-0", ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree); @@ -2203,7 +2203,7 @@ public async Task ACLProcessor_GetCustomDenyAces_SkipsAccidentalDeletionProtecti Assert.Empty(result); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_SkipsDefaultAdDenyPatterns() { var msaAce = CreateObjectDenyAce("S-1-1-0", ActiveDirectoryRights.ExtendedRight, new Guid(ACEGuids.UserForceChangePassword)); @@ -2219,7 +2219,7 @@ public async Task ACLProcessor_GetCustomDenyAces_SkipsDefaultAdDenyPatterns() { Assert.Empty(domainResult); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_EmitsMultipleQualifyingAces() { var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2800", ActiveDirectoryRights.Delete); @@ -2235,7 +2235,7 @@ public async Task ACLProcessor_GetCustomDenyAces_EmitsMultipleQualifyingAces() { Assert.Equal(SerializeAce(ace2), result[1]); } - [Fact] + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAces_PreservesDeterministicOrdering() { var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2901", ActiveDirectoryRights.DeleteChild); @@ -2249,7 +2249,40 @@ public async Task ACLProcessor_GetCustomDenyAces_PreservesDeterministicOrdering( Assert.Equal(new[] { SerializeAce(ace1), SerializeAce(ace2) }, result); } - [Fact] + [WindowsOnlyFact] + public async Task ACLProcessor_GetCustomDenyAceCounts_SplitsExplicitAndInheritedAces() { + var explicitAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3000", + ActiveDirectoryRights.Delete); + var inheritedAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3001", + ActiveDirectoryRights.DeleteChild, AceFlags.Inherited); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(explicitAce, inheritedAce), + _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.Equal(1, result.ExplicitCount); + Assert.Equal(1, result.InheritedCount); + Assert.Equal(2, result.Total); + } + + [WindowsOnlyFact] + public async Task ACLProcessor_AddCustomDenyAcesProperty_EmitsExplicitAndInheritedCounts() { + var props = new Dictionary(); + var explicitAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3100", + ActiveDirectoryRights.Delete); + var inheritedAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3101", + ActiveDirectoryRights.DeleteChild, AceFlags.Inherited); + var processor = CreateCustomDenyAceProcessor(); + + await processor.AddCustomDenyAcesProperty(props, CreateSecurityDescriptorBytes(explicitAce, inheritedAce), + _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + + Assert.DoesNotContain("customdenyaces", props.Keys); + Assert.Equal(1, props["customexplicitdenyacescount"]); + Assert.Equal(1, props["custominheriteddenyacescount"]); + } + + [WindowsOnlyFact] public async Task ACLProcessor_AddCustomDenyAcesProperty_DoesNotEmitWhenEmpty() { var props = new Dictionary(); var ace = CreateCommonDenyAce("S-1-1-0", @@ -2260,6 +2293,8 @@ await processor.AddCustomDenyAcesProperty(props, CreateSecurityDescriptorBytes(a Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); Assert.DoesNotContain("customdenyaces", props.Keys); + Assert.DoesNotContain("customexplicitdenyacescount", props.Keys); + Assert.DoesNotContain("custominheriteddenyacescount", props.Keys); } private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Name)[] principals) { @@ -2287,8 +2322,9 @@ private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) { return buffer; } - private static CommonAce CreateCommonDenyAce(string sid, ActiveDirectoryRights rights) { - return new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)rights, + private static CommonAce CreateCommonDenyAce(string sid, ActiveDirectoryRights rights, + AceFlags aceFlags = AceFlags.None) { + return new CommonAce(aceFlags, AceQualifier.AccessDenied, (int)rights, new SecurityIdentifier(sid), false, null); } diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 2f8372f16..dac0ef801 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -151,7 +151,11 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_Whe var baselineProcessor = new LdapPropertyProcessor(new MockLdapUtils()); var baseline = await baselineProcessor.ReadOUProperties(mock); - Assert.Contains("customdenyaces", baseline.Keys); + Assert.DoesNotContain("customdenyaces", baseline.Keys); + Assert.Contains("customexplicitdenyacescount", baseline.Keys); + Assert.Contains("custominheriteddenyacescount", baseline.Keys); + Assert.Equal(1, baseline["customexplicitdenyacescount"]); + Assert.Equal(0, baseline["custominheriteddenyacescount"]); var ldapUtils = new MockLdapUtils(); ldapUtils.SetLdapConfig(new LdapConfig { @@ -162,6 +166,8 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_Whe var test = await processor.ReadOUProperties(mock); Assert.DoesNotContain("customdenyaces", test.Keys); + Assert.DoesNotContain("customexplicitdenyacescount", test.Keys); + Assert.DoesNotContain("custominheriteddenyacescount", test.Keys); } [Fact] From d66c9c6baec0ef04f43a694282c24ba922dd89f4 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 14 May 2026 20:09:58 +0200 Subject: [PATCH 09/13] optimizations --- src/CommonLib/Processors/ACLProcessor.cs | 100 +++++------------------ test/unit/ACLProcessorTest.cs | 87 ++++++++------------ test/unit/LdapPropertyTests.cs | 2 +- 3 files changed, 55 insertions(+), 134 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 651014f3b..de117776b 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -23,6 +23,8 @@ public class ACLProcessor { private readonly ConcurrentHashSet _builtDomainCaches = new(StringComparer.OrdinalIgnoreCase); private readonly ConcurrentDictionary _exchangeTrusteeSidCache = new(StringComparer.OrdinalIgnoreCase); private readonly object _lock = new(); + private const string CustomExplicitDenyAcesCountProperty = "customexplicitdenyacescount"; + private const string CustomInheritedDenyAcesCountProperty = "custominheriteddenyacescount"; // These Exchange principals commonly carry product-added deny ACEs that we intentionally suppress. private static readonly HashSet ExchangeTrusteeNames = new(StringComparer.OrdinalIgnoreCase) { "Exchange Windows Permissions", @@ -901,21 +903,6 @@ or Label.NTAuthStore } } - public Task GetCustomDenyAces(ResolvedSearchResult result, IDirectoryObject searchResult) { - if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { - return Task.FromResult(Array.Empty()); - } - - searchResult.TryGetDistinguishedName(out var distinguishedName); - return GetCustomDenyAces( - descriptor, - result.Domain, - result.ObjectType, - distinguishedName, - searchResult.IsMSA() || searchResult.IsGMSA(), - result.DisplayName); - } - public Task GetCustomDenyAceCounts(ResolvedSearchResult result, IDirectoryObject searchResult) { if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { @@ -932,39 +919,11 @@ public Task GetCustomDenyAceCounts(ResolvedSearchResult res result.DisplayName); } - public async Task GetCustomDenyAces(byte[] ntSecurityDescriptor, string objectDomain, - Label objectType, string distinguishedName = null, bool isMSA = false, string objectName = "") { - var customDenyAces = await GetCustomDenyAceData(ntSecurityDescriptor, objectDomain, objectType, - distinguishedName, isMSA, objectName, true); - - return customDenyAces.Sddl.Count == 0 ? Array.Empty() : customDenyAces.Sddl.ToArray(); - } - public async Task GetCustomDenyAceCounts(byte[] ntSecurityDescriptor, string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, string objectName = "") { - var customDenyAces = await GetCustomDenyAceData(ntSecurityDescriptor, objectDomain, objectType, - distinguishedName, isMSA, objectName, false); - - return new CustomDenyAceCounts(customDenyAces.ExplicitCount, customDenyAces.InheritedCount); - } - - public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, - string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, - string objectName = "") { - var counts = await GetCustomDenyAceCounts(ntSecurityDescriptor, objectDomain, objectType, - distinguishedName, isMSA, objectName); - - if (counts.Total > 0) { - props["customexplicitdenyacescount"] = counts.ExplicitCount; - props["custominheriteddenyacescount"] = counts.InheritedCount; - } - } - - private async Task GetCustomDenyAceData(byte[] ntSecurityDescriptor, string objectDomain, - Label objectType, string distinguishedName, bool isMSA, string objectName, bool collectSddl) { if (ntSecurityDescriptor == null) { - return new CustomDenyAceData(); + return new CustomDenyAceCounts(); } RawSecurityDescriptor descriptor; @@ -975,16 +934,16 @@ private async Task GetCustomDenyAceData(byte[] ntSecurityDesc _log.LogWarning( "Security descriptor on object {Name} exceeds maximum allowable length. Unable to process custom deny ACEs", objectName); - return new CustomDenyAceData(); + return new CustomDenyAceCounts(); } if (descriptor.DiscretionaryAcl == null || descriptor.DiscretionaryAcl.Count == 0) { - return new CustomDenyAceData(); + return new CustomDenyAceCounts(); } - var results = new CustomDenyAceData(); + var explicitCount = 0; + var inheritedCount = 0; - // Walk the raw DACL so we can preserve deny ACE ordering and serialize each ACE back to SDDL verbatim. foreach (GenericAce ace in descriptor.DiscretionaryAcl) { if (!TryGetDenyAceData(ace, out var principalSid, out var rights, out var objectAceType)) { continue; @@ -996,26 +955,25 @@ private async Task GetCustomDenyAceData(byte[] ntSecurityDesc } if ((ace.AceFlags & AceFlags.Inherited) == AceFlags.Inherited) { - results.InheritedCount++; + inheritedCount++; } else { - results.ExplicitCount++; - } - - if (collectSddl) { - var sddl = SerializeAceToSddl(ace); - if (!string.IsNullOrWhiteSpace(sddl)) { - results.Sddl.Add(sddl); - } + explicitCount++; } } - return results; + return new CustomDenyAceCounts(explicitCount, inheritedCount); } - private class CustomDenyAceData { - public List Sddl { get; } = new(); - public int ExplicitCount { get; set; } - public int InheritedCount { get; set; } + public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, + string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, + string objectName = "") { + var counts = await GetCustomDenyAceCounts(ntSecurityDescriptor, objectDomain, objectType, + distinguishedName, isMSA, objectName); + + if (counts.Total > 0) { + props[CustomExplicitDenyAcesCountProperty] = counts.ExplicitCount; + props[CustomInheritedDenyAcesCountProperty] = counts.InheritedCount; + } } private static bool TryGetDenyAceData(GenericAce ace, out string principalSid, out ActiveDirectoryRights rights, @@ -1101,24 +1059,6 @@ private async Task IsExchangeTrustee(string principalSid, string objectDom return exchangeTrusteeSids.Contains(principalSid, StringComparer.OrdinalIgnoreCase); } - private static string SerializeAceToSddl(GenericAce ace) { - // Rehydrate the ACE inside a one-entry DACL and let the framework emit the canonical ACE SDDL for us. - var acl = new RawAcl(ace is ObjectAce ? GenericAcl.AclRevisionDS : GenericAcl.AclRevision, 1); - acl.InsertAce(0, CloneAce(ace)); - - var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); - var sddl = descriptor.GetSddlForm(AccessControlSections.Access); - - return sddl.StartsWith("D:", StringComparison.OrdinalIgnoreCase) ? sddl.Substring(2) : sddl; - } - - private static GenericAce CloneAce(GenericAce ace) { - var buffer = new byte[ace.BinaryLength]; - ace.GetBinaryForm(buffer, 0); - return GenericAce.CreateFromBinaryForm(buffer, 0); - } - - /// /// Helper function to use commonlib types and pass to ProcessGMSAReaders /// diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index d9e184f34..3a9d62a1d 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -2140,117 +2140,100 @@ public async Task ACLProcessor_ProcessACL_EnterpriseCA_Enroll() } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_EmitsQualifyingDenyAce() { + public async Task ACLProcessor_GetCustomDenyAceCounts_CountsQualifyingExplicitDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2500", ActiveDirectoryRights.Delete); var processor = CreateCustomDenyAceProcessor(); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - Assert.Single(result); - Assert.Equal(SerializeAce(ace), result[0]); + AssertCustomDenyAceCounts(result, 1, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeTrusteeDenyAce() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsExchangeTrusteeDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2600", ActiveDirectoryRights.Delete); var processor = CreateCustomDenyAceProcessor(("S-1-5-21-3130019616-2776909439-2417379446-2600", "Exchange Windows Permissions")); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - Assert.Empty(result); + AssertCustomDenyAceCounts(result, 0, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_SkipsOrganizationManagementDenyAce() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsOrganizationManagementDenyAce() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2601", ActiveDirectoryRights.Delete); var processor = CreateCustomDenyAceProcessor(("S-1-5-21-3130019616-2776909439-2417379446-2601", "Organization Management")); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - Assert.Empty(result); + AssertCustomDenyAceCounts(result, 0, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_SkipsExchangeConfigurationPath() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsExchangeConfigurationPath() { var ace = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2700", ActiveDirectoryRights.Delete); var processor = CreateCustomDenyAceProcessor(); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, Label.Container, "CN=Mailbox Database,CN=Microsoft Exchange,CN=Services,CN=Configuration,DC=TESTLAB,DC=LOCAL"); - Assert.Empty(result); + AssertCustomDenyAceCounts(result, 0, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_SkipsAccidentalDeletionProtection() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsAccidentalDeletionProtection() { var ace = CreateCommonDenyAce("S-1-1-0", ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree); var processor = CreateCustomDenyAceProcessor(); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace), _testDomainName, + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); - Assert.Empty(result); + AssertCustomDenyAceCounts(result, 0, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_SkipsDefaultAdDenyPatterns() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsDefaultAdDenyPatterns() { var msaAce = CreateObjectDenyAce("S-1-1-0", ActiveDirectoryRights.ExtendedRight, new Guid(ACEGuids.UserForceChangePassword)); var domainAce = CreateCommonDenyAce("S-1-1-0", ActiveDirectoryRights.DeleteChild); var processor = CreateCustomDenyAceProcessor(); - var msaResult = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(msaAce), _testDomainName, - Label.User, "CN=TEST MSA,CN=Managed Service Accounts,DC=TESTLAB,DC=LOCAL", true); - var domainResult = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(domainAce), + var msaResult = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(msaAce), + _testDomainName, Label.User, "CN=TEST MSA,CN=Managed Service Accounts,DC=TESTLAB,DC=LOCAL", true); + var domainResult = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(domainAce), _testDomainName, Label.Domain, "DC=TESTLAB,DC=LOCAL"); - Assert.Empty(msaResult); - Assert.Empty(domainResult); + AssertCustomDenyAceCounts(msaResult, 0, 0); + AssertCustomDenyAceCounts(domainResult, 0, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_EmitsMultipleQualifyingAces() { + public async Task ACLProcessor_GetCustomDenyAceCounts_CountsMultipleExplicitDenyAces() { var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2800", ActiveDirectoryRights.Delete); var ace2 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2801", ActiveDirectoryRights.DeleteChild); var processor = CreateCustomDenyAceProcessor(); - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace1, ace2), _testDomainName, - Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - - Assert.Equal(2, result.Length); - Assert.Equal(SerializeAce(ace1), result[0]); - Assert.Equal(SerializeAce(ace2), result[1]); - } - - [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAces_PreservesDeterministicOrdering() { - var ace1 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2901", - ActiveDirectoryRights.DeleteChild); - var ace2 = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-2900", - ActiveDirectoryRights.Delete); - var processor = CreateCustomDenyAceProcessor(); - - var result = await processor.GetCustomDenyAces(CreateSecurityDescriptorBytes(ace1, ace2), _testDomainName, - Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace1, ace2), + _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - Assert.Equal(new[] { SerializeAce(ace1), SerializeAce(ace2) }, result); + AssertCustomDenyAceCounts(result, 2, 0); } [WindowsOnlyFact] - public async Task ACLProcessor_GetCustomDenyAceCounts_SplitsExplicitAndInheritedAces() { + public async Task ACLProcessor_GetCustomDenyAceCounts_SplitsExplicitAndInheritedDenyAces() { var explicitAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3000", ActiveDirectoryRights.Delete); var inheritedAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3001", @@ -2260,9 +2243,7 @@ public async Task ACLProcessor_GetCustomDenyAceCounts_SplitsExplicitAndInherited var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(explicitAce, inheritedAce), _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); - Assert.Equal(1, result.ExplicitCount); - Assert.Equal(1, result.InheritedCount); - Assert.Equal(2, result.Total); + AssertCustomDenyAceCounts(result, 1, 1); } [WindowsOnlyFact] @@ -2310,6 +2291,13 @@ private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Nam return new ACLProcessor(mockLdapUtils.Object); } + private static void AssertCustomDenyAceCounts(ACLProcessor.CustomDenyAceCounts result, + int expectedExplicitCount, int expectedInheritedCount) { + Assert.Equal(expectedExplicitCount, result.ExplicitCount); + Assert.Equal(expectedInheritedCount, result.InheritedCount); + Assert.Equal(expectedExplicitCount + expectedInheritedCount, result.Total); + } + private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) { var acl = new RawAcl(GenericAcl.AclRevisionDS, aces.Length); for (var i = 0; i < aces.Length; i++) { @@ -2332,12 +2320,5 @@ private static ObjectAce CreateObjectDenyAce(string sid, ActiveDirectoryRights r return new ObjectAce(AceFlags.None, AceQualifier.AccessDenied, (int)rights, new SecurityIdentifier(sid), ObjectAceFlags.ObjectAceTypePresent, objectType, Guid.Empty, false, null); } - - private static string SerializeAce(GenericAce ace) { - var acl = new RawAcl(GenericAcl.AclRevisionDS, 1); - acl.InsertAce(0, ace); - var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); - return descriptor.GetSddlForm(AccessControlSections.Access).Substring(2); - } } } diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index dac0ef801..278833ebc 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -139,7 +139,7 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() [SupportedOSPlatform("windows")] [WindowsOnlyFact] - public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt() + public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAceCounts_WhenLdapConfigRequestsIt() { var denyAce = new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)ActiveDirectoryRights.Delete, new SecurityIdentifier("S-1-5-21-3130019616-2776909439-2417379446-2500"), false, null); From f7bb64631f144806bddc228353b0efd5027df5cd Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 14 May 2026 20:17:32 +0200 Subject: [PATCH 10/13] rename SkipDenyAces to SkipDenyAcesCount --- src/CommonLib/ILdapUtils.cs | 4 ++-- src/CommonLib/LdapConfig.cs | 4 ++-- src/CommonLib/LdapUtils.cs | 2 +- src/CommonLib/Processors/LdapPropertyProcessor.cs | 2 +- test/unit/Facades/MockLdapUtils.cs | 2 +- test/unit/LdapPropertyTests.cs | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/CommonLib/ILdapUtils.cs b/src/CommonLib/ILdapUtils.cs index 2fc7a6b32..63ded6d4a 100644 --- a/src/CommonLib/ILdapUtils.cs +++ b/src/CommonLib/ILdapUtils.cs @@ -157,9 +157,9 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// The new ldap config void SetLdapConfig(LdapConfig config); /// - /// Gets whether custom deny ACE collection is disabled for this utils instance + /// Gets whether custom deny ACE count collection is disabled for this utils instance /// - bool SkipDenyAces { get; } + bool SkipDenyAcesCount { get; } /// /// Tests if a LDAP connection can be made successfully to a domain /// diff --git a/src/CommonLib/LdapConfig.cs b/src/CommonLib/LdapConfig.cs index 5242d02b2..587cda593 100644 --- a/src/CommonLib/LdapConfig.cs +++ b/src/CommonLib/LdapConfig.cs @@ -13,7 +13,7 @@ public class LdapConfig public bool ForceSSL { get; set; } = false; public bool DisableSigning { get; set; } = false; public bool DisableCertVerification { get; set; } = false; - public bool SkipDenyAces { get; set; } = false; + public bool SkipDenyAcesCount { get; set; } = false; public AuthType AuthType { get; set; } = AuthType.Kerberos; public int MaxConcurrentQueries { get; set; } = 15; @@ -42,7 +42,7 @@ public override string ToString() { sb.AppendLine($"LdapPort: {GetPort(false)}"); sb.AppendLine($"LdapSSLPort: {GetPort(true)}"); sb.AppendLine($"ForceSSL: {ForceSSL}"); - sb.AppendLine($"SkipDenyAces: {SkipDenyAces}"); + sb.AppendLine($"SkipDenyAcesCount: {SkipDenyAcesCount}"); sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); if (!string.IsNullOrWhiteSpace(Username)) { diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index cd416823d..087ac57e1 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1077,7 +1077,7 @@ public void SetLdapConfig(LdapConfig config) { _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); } - public bool SkipDenyAces => _ldapConfig.SkipDenyAces; + public bool SkipDenyAcesCount => _ldapConfig.SkipDenyAcesCount; public Task<(bool Success, string Message)> TestLdapConnection(string domain) { return _connectionPool.TestDomainConnection(domain, false); diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index 11e11f84e..f1af4a23a 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -669,7 +669,7 @@ public async Task ReadIssuancePolicyProperties(IDirect private async Task AddCustomDenyAceProperty(Dictionary props, IDirectoryObject entry, string domain, Label objectType) { - if (_utils.SkipDenyAces) { + if (_utils.SkipDenyAcesCount) { return; } diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index efe0c0518..7eedc616a 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -1011,7 +1011,7 @@ public void SetLdapConfig(LdapConfig config) { _ldapConfig = config; } - public bool SkipDenyAces => _ldapConfig.SkipDenyAces; + public bool SkipDenyAcesCount => _ldapConfig.SkipDenyAcesCount; public Task<(bool Success, string Message)> TestLdapConnection(string domain) { throw new NotImplementedException(); diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 278833ebc..7c06b87dc 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -159,7 +159,7 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAceCount var ldapUtils = new MockLdapUtils(); ldapUtils.SetLdapConfig(new LdapConfig { - SkipDenyAces = true + SkipDenyAcesCount = true }); var processor = new LdapPropertyProcessor(ldapUtils); From 71b8f778569d57ac2a40ab8f3f251684573598c1 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Wed, 1 Jul 2026 16:09:35 +0200 Subject: [PATCH 11/13] move deny ace logic to ACLProcessor --- .codex | 0 src/CommonLib/ILdapUtils.cs | 4 - src/CommonLib/LdapConfig.cs | 2 - src/CommonLib/LdapUtils.cs | 2 - src/CommonLib/Processors/ACLProcessor.cs | 100 +++++++++++++++--- .../Processors/LdapPropertyProcessor.cs | 57 ++-------- test/unit/ACLProcessorTest.cs | 71 +++++++++---- test/unit/Facades/MockLdapUtils.cs | 5 +- test/unit/LdapPropertyTests.cs | 99 ++++------------- 9 files changed, 165 insertions(+), 175 deletions(-) create mode 100644 .codex diff --git a/.codex b/.codex new file mode 100644 index 000000000..e69de29bb diff --git a/src/CommonLib/ILdapUtils.cs b/src/CommonLib/ILdapUtils.cs index 63ded6d4a..9ddba7c55 100644 --- a/src/CommonLib/ILdapUtils.cs +++ b/src/CommonLib/ILdapUtils.cs @@ -157,10 +157,6 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// The new ldap config void SetLdapConfig(LdapConfig config); /// - /// Gets whether custom deny ACE count collection is disabled for this utils instance - /// - bool SkipDenyAcesCount { get; } - /// /// Tests if a LDAP connection can be made successfully to a domain /// /// The domain to test diff --git a/src/CommonLib/LdapConfig.cs b/src/CommonLib/LdapConfig.cs index 587cda593..745a19b6e 100644 --- a/src/CommonLib/LdapConfig.cs +++ b/src/CommonLib/LdapConfig.cs @@ -13,7 +13,6 @@ public class LdapConfig public bool ForceSSL { get; set; } = false; public bool DisableSigning { get; set; } = false; public bool DisableCertVerification { get; set; } = false; - public bool SkipDenyAcesCount { get; set; } = false; public AuthType AuthType { get; set; } = AuthType.Kerberos; public int MaxConcurrentQueries { get; set; } = 15; @@ -42,7 +41,6 @@ public override string ToString() { sb.AppendLine($"LdapPort: {GetPort(false)}"); sb.AppendLine($"LdapSSLPort: {GetPort(true)}"); sb.AppendLine($"ForceSSL: {ForceSSL}"); - sb.AppendLine($"SkipDenyAcesCount: {SkipDenyAcesCount}"); sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); if (!string.IsNullOrWhiteSpace(Username)) { diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 47f1c3210..1ba4ae951 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1077,8 +1077,6 @@ public void SetLdapConfig(LdapConfig config) { _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); } - public bool SkipDenyAcesCount => _ldapConfig.SkipDenyAcesCount; - public Task<(bool Success, string Message)> TestLdapConnection(string domain) { return _connectionPool.TestDomainConnection(domain, false); } diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index de117776b..4fd00d956 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -23,8 +23,6 @@ public class ACLProcessor { private readonly ConcurrentHashSet _builtDomainCaches = new(StringComparer.OrdinalIgnoreCase); private readonly ConcurrentDictionary _exchangeTrusteeSidCache = new(StringComparer.OrdinalIgnoreCase); private readonly object _lock = new(); - private const string CustomExplicitDenyAcesCountProperty = "customexplicitdenyacescount"; - private const string CustomInheritedDenyAcesCountProperty = "custominheriteddenyacescount"; // These Exchange principals commonly carry product-added deny ACEs that we intentionally suppress. private static readonly HashSet ExchangeTrusteeNames = new(StringComparer.OrdinalIgnoreCase) { "Exchange Windows Permissions", @@ -70,6 +68,33 @@ public CustomDenyAceCounts(int explicitCount, int inheritedCount) { public int Total => ExplicitCount + InheritedCount; } + public sealed class ACLProcessingResult { + public ACLProcessingResult(ACE[] aces, CustomDenyAceCounts customDenyAceCounts) { + Aces = aces; + CustomDenyAceCounts = customDenyAceCounts; + } + + public ACE[] Aces { get; } + public CustomDenyAceCounts CustomDenyAceCounts { get; } + } + + private sealed class CustomDenyAceAccumulator { + private int _explicitCount; + private int _inheritedCount; + + public void Add(bool inherited) { + if (inherited) { + _inheritedCount++; + } else { + _explicitCount++; + } + } + + public CustomDenyAceCounts ToCounts() { + return new CustomDenyAceCounts(_explicitCount, _inheritedCount); + } + } + /// Represents a lightweight Access Control Entry (ACE) used to compute hash values /// for AdminSDHolder purposes internal class ACEForHashing { @@ -453,6 +478,24 @@ public IAsyncEnumerable ProcessACL(ResolvedSearchResult result, IDirectoryO return ProcessACL(descriptor, result.Domain, result.ObjectType, searchResult.HasLAPS(), checkForOwnerRights, result.DisplayName); } + /// + /// Processes the regular ACL edges and custom deny ACE counts in one ACL traversal. + /// + /// + /// Callers that do not want custom deny ACE counts should continue to call . + /// + public Task ProcessACLWithCustomDenyAces(ResolvedSearchResult result, + IDirectoryObject searchResult, bool checkForOwnerRights = true) { + if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { + return Task.FromResult(new ACLProcessingResult(Array.Empty(), new CustomDenyAceCounts())); + } + + searchResult.TryGetDistinguishedName(out var distinguishedName); + return ProcessACLWithCustomDenyAces(descriptor, result.Domain, result.ObjectType, searchResult.HasLAPS(), + checkForOwnerRights, distinguishedName, searchResult.IsMSA() || searchResult.IsGMSA(), + result.DisplayName); + } + /// /// Read's a raw ntSecurityDescriptor and processes the ACEs in the ACL, filtering out ACEs that /// BloodHound is not interested in as well as principals we don't care about @@ -469,8 +512,25 @@ public IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, string obje return ProcessACL(ntSecurityDescriptor, objectDomain, objectType, hasLaps, true, objectName); } - public async IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, string objectDomain, + public IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, string objectDomain, Label objectType, bool hasLaps, bool checkForOwnerRights, string objectName) { + return ProcessACLInternal(ntSecurityDescriptor, objectDomain, objectType, hasLaps, checkForOwnerRights, + objectName); + } + + public async Task ProcessACLWithCustomDenyAces(byte[] ntSecurityDescriptor, + string objectDomain, Label objectType, bool hasLaps, bool checkForOwnerRights = true, + string distinguishedName = null, bool isMSA = false, string objectName = "") { + var accumulator = new CustomDenyAceAccumulator(); + var aces = await ProcessACLInternal(ntSecurityDescriptor, objectDomain, objectType, hasLaps, + checkForOwnerRights, objectName, accumulator, distinguishedName, isMSA).ToArrayAsync(); + return new ACLProcessingResult(aces, accumulator.ToCounts()); + } + + private async IAsyncEnumerable ProcessACLInternal(byte[] ntSecurityDescriptor, string objectDomain, + Label objectType, bool hasLaps, bool checkForOwnerRights, string objectName, + CustomDenyAceAccumulator customDenyAceAccumulator = null, string distinguishedName = null, + bool isMSA = false) { await BuildGuidCache(objectDomain); if (ntSecurityDescriptor == null) { @@ -518,7 +578,19 @@ public async IAsyncEnumerable ProcessACL(byte[] ntSecurityDescriptor, strin bool isPermissionForOwnerRightsSid = false; bool isInheritedPermissionForOwnerRightsSid = false; - if (ace == null || ace.AccessControlType() == AccessControlType.Deny || !ace.IsAceInheritedFrom(BaseGuids[objectType])) { + if (ace == null) { + continue; + } + + if (ace.AccessControlType() == AccessControlType.Deny) { + if (customDenyAceAccumulator != null) { + await CountCustomDenyAce(ace, customDenyAceAccumulator, objectDomain, objectType, + distinguishedName, isMSA); + } + continue; + } + + if (!ace.IsAceInheritedFrom(BaseGuids[objectType])) { continue; } @@ -964,16 +1036,20 @@ public async Task GetCustomDenyAceCounts(byte[] ntSecurityD return new CustomDenyAceCounts(explicitCount, inheritedCount); } - public async Task AddCustomDenyAcesProperty(Dictionary props, byte[] ntSecurityDescriptor, - string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, - string objectName = "") { - var counts = await GetCustomDenyAceCounts(ntSecurityDescriptor, objectDomain, objectType, - distinguishedName, isMSA, objectName); + private async Task CountCustomDenyAce(ActiveDirectoryRuleDescriptor ace, + CustomDenyAceAccumulator accumulator, string objectDomain, Label objectType, string distinguishedName, + bool isMSA) { + var principalSid = ace.IdentityReference(); + if (string.IsNullOrWhiteSpace(principalSid)) { + return; + } - if (counts.Total > 0) { - props[CustomExplicitDenyAcesCountProperty] = counts.ExplicitCount; - props[CustomInheritedDenyAcesCountProperty] = counts.InheritedCount; + if (await ShouldExcludeCustomDenyAce(principalSid, ace.ActiveDirectoryRights(), ace.ObjectType(), + objectDomain, objectType, distinguishedName, isMSA)) { + return; } + + accumulator.Add(ace.IsInherited()); } private static bool TryGetDenyAceData(GenericAce ace, out string principalSid, out ActiveDirectoryRights rights, diff --git a/src/CommonLib/Processors/LdapPropertyProcessor.cs b/src/CommonLib/Processors/LdapPropertyProcessor.cs index 35c3552d6..2625b62f0 100644 --- a/src/CommonLib/Processors/LdapPropertyProcessor.cs +++ b/src/CommonLib/Processors/LdapPropertyProcessor.cs @@ -8,7 +8,6 @@ using System.Security.Principal; using System.Threading.Tasks; using Microsoft.Extensions.Logging; -using SharpHoundCommonLib.DirectoryObjects; using SharpHoundCommonLib.Enums; using SharpHoundCommonLib.LDAPQueries; using SharpHoundCommonLib.OutputTypes; @@ -38,12 +37,10 @@ static LdapPropertyProcessor() { private readonly ILdapUtils _utils; private readonly ILogger _log; - private readonly ACLProcessor _aclProcessor; public LdapPropertyProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; _log = log ?? Logging.LogProvider.CreateLogger(nameof(LdapPropertyProcessor)); - _aclProcessor = new ACLProcessor(utils, _log); } private static Dictionary GetCommonProps(IDirectoryObject entry) { @@ -81,7 +78,6 @@ private static Dictionary GetCommonProps(IDirectoryObject entry) /// public async Task> ReadDomainProperties(IDirectoryObject entry, string domain) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, domain, Label.Domain); if (entry.TryGetProperty(LDAPProperties.ExpirePasswordsOnSmartCardOnlyAccounts, out var expirePassword) && bool.TryParse(expirePassword, out var expirePasswordBool)) { @@ -182,9 +178,8 @@ public static string FunctionalLevelToString(int level) { /// /// /// - public async Task> ReadGPOProperties(IDirectoryObject entry) { + public static Dictionary ReadGPOProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.GPO); entry.TryGetProperty(LDAPProperties.GPCFileSYSPath, out var path); props.Add("gpcpath", path.ToUpper()); entry.TryGetProperty(LDAPProperties.Flags, out var flags); @@ -197,9 +192,8 @@ public async Task> ReadGPOProperties(IDirectoryObject /// /// /// - public async Task> ReadOUProperties(IDirectoryObject entry) { + public static Dictionary ReadOUProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.OU); return props; } @@ -218,7 +212,6 @@ public async Task ReadGroupPropertiesAsync(IDirectoryObject ent { var groupProperties = new GroupProperties(); var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, domain, Label.Group); entry.TryGetLongProperty(LDAPProperties.AdminCount, out var ac); props.Add("admincount", ac != 0); entry.TryGetLongProperty(LDAPProperties.GroupType, out var groupType); @@ -237,10 +230,8 @@ public async Task ReadGroupPropertiesAsync(IDirectoryObject ent /// /// /// - public async Task> ReadContainerProperties(IDirectoryObject entry) { + public static Dictionary ReadContainerProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - var objectType = entry.GetLabel(out var label) ? label : Label.Container; - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), objectType); return props; } @@ -258,7 +249,6 @@ public Task public async Task ReadUserProperties(IDirectoryObject entry, string domain) { var userProps = new UserProperties(); var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, domain, Label.User); if (entry.TryGetLongProperty(LDAPProperties.UserAccountControl, out var uac)) { var uacFlags = (UacFlags)uac; @@ -373,7 +363,6 @@ public Task ReadComputerProperties(IDirectoryObject entry, public async Task ReadComputerProperties(IDirectoryObject entry, string domain) { var compProps = new ComputerProperties(); var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, domain, Label.Computer); var flags = (UacFlags)0; if (entry.TryGetLongProperty(LDAPProperties.UserAccountControl, out var uac)) { @@ -477,9 +466,8 @@ await SendComputerStatus(new CSVComputerStatus { /// /// /// Returns a dictionary with the common properties of the RootCA - public async Task> ReadRootCAProperties(IDirectoryObject entry) { + public static Dictionary ReadRootCAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.RootCA); // Certificate if (entry.TryGetByteProperty(LDAPProperties.CACertificate, out var rawCertificate) && HasBytes(rawCertificate)) { @@ -499,9 +487,8 @@ public async Task> ReadRootCAProperties(IDirectoryObj /// /// /// Returns a dictionary with the common properties and the crosscertificatepair property of the AICA - public async Task> ReadAIACAProperties(IDirectoryObject entry) { + public static Dictionary ReadAIACAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.AIACA); entry.TryGetByteArrayProperty(LDAPProperties.CrossCertificatePair, out var crossCertificatePair); var hasCrossCertificatePair = crossCertificatePair.Length > 0; @@ -526,9 +513,8 @@ public async Task> ReadAIACAProperties(IDirectoryObje /// /// /// Returns a dictionary with the common properties and the caname, hostname, and flags properties of the EnterpriseCA - public async Task> ReadEnterpriseCAProperties(IDirectoryObject entry) { + public static Dictionary ReadEnterpriseCAProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.EnterpriseCA); if (entry.TryGetLongProperty("flags", out var flags)) props.Add("flags", (PKICertificateAuthorityFlags)flags); props.Add("caname", entry.GetProperty(LDAPProperties.Name)); @@ -552,9 +538,8 @@ public async Task> ReadEnterpriseCAProperties(IDirect /// /// /// Returns a dictionary with the common properties of the NTAuthStore - public async Task> ReadNTAuthStoreProperties(IDirectoryObject entry) { + public static Dictionary ReadNTAuthStoreProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.NTAuthStore); return props; } @@ -563,9 +548,8 @@ public async Task> ReadNTAuthStoreProperties(IDirecto /// /// /// Returns a dictionary associated with the CertTemplate properties that were read - public async Task> ReadCertTemplateProperties(IDirectoryObject entry) { + public static Dictionary ReadCertTemplateProperties(IDirectoryObject entry) { var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.CertTemplate); props.Add("validityperiod", ConvertPKIPeriod(entry.GetByteProperty(LDAPProperties.PKIExpirationPeriod))); props.Add("renewalperiod", ConvertPKIPeriod(entry.GetByteProperty(LDAPProperties.PKIOverlappedPeriod))); @@ -650,7 +634,6 @@ public async Task> ReadCertTemplateProperties(IDirect public async Task ReadIssuancePolicyProperties(IDirectoryObject entry) { var ret = new IssuancePolicyProperties(); var props = GetCommonProps(entry); - await AddCustomDenyAceProperty(props, entry, GetEntryDomain(entry), Label.IssuancePolicy); props.Add("displayname", entry.GetProperty(LDAPProperties.DisplayName)); props.Add("certtemplateoid", entry.GetProperty(LDAPProperties.CertTemplateOID)); @@ -665,30 +648,6 @@ public async Task ReadIssuancePolicyProperties(IDirect return ret; } - private async Task AddCustomDenyAceProperty(Dictionary props, IDirectoryObject entry, - string domain, Label objectType) { - if (_utils.SkipDenyAcesCount) { - return; - } - - if (!entry.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var ntSecurityDescriptor)) { - return; - } - - var distinguishedName = entry.TryGetDistinguishedName(out var dn) ? dn : string.Empty; - var objectName = entry.TryGetProperty(LDAPProperties.SAMAccountName, out var samAccountName) - ? samAccountName - : distinguishedName; - await _aclProcessor.AddCustomDenyAcesProperty(props, ntSecurityDescriptor, domain, objectType, - distinguishedName, entry.IsMSA() || entry.IsGMSA(), objectName); - } - - private static string GetEntryDomain(IDirectoryObject entry) { - return entry.TryGetDistinguishedName(out var distinguishedName) - ? Helpers.DistinguishedNameToDomain(distinguishedName) - : string.Empty; - } - /// /// Attempts to parse all LDAP attributes outside of the ones already collected and converts them to a human readable /// format using a best guess diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index a2cc6034d..b13e26a58 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -2247,36 +2247,34 @@ public async Task ACLProcessor_GetCustomDenyAceCounts_SplitsExplicitAndInherited AssertCustomDenyAceCounts(result, 1, 1); } - [WindowsOnlyFact] - public async Task ACLProcessor_AddCustomDenyAcesProperty_EmitsExplicitAndInheritedCounts() { - var props = new Dictionary(); - var explicitAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3100", - ActiveDirectoryRights.Delete); - var inheritedAce = CreateCommonDenyAce("S-1-5-21-3130019616-2776909439-2417379446-3101", - ActiveDirectoryRights.DeleteChild, AceFlags.Inherited); - var processor = CreateCustomDenyAceProcessor(); + [Fact] + public async Task ACLProcessor_ProcessACLWithCustomDenyAces_ReturnsRegularAcesAndDenyCounts() { + var denyRule = CreateRuleDescriptor("S-1-5-21-3130019616-2776909439-2417379446-3100", + AccessControlType.Deny, ActiveDirectoryRights.Delete); + var allowRule = CreateRuleDescriptor("S-1-5-21-3130019616-2776909439-2417379446-3101", + AccessControlType.Allow, ActiveDirectoryRights.WriteDacl); + allowRule.Setup(x => x.IsAceInheritedFrom(It.IsAny())).Returns(true); - await processor.AddCustomDenyAcesProperty(props, CreateSecurityDescriptorBytes(explicitAce, inheritedAce), - _testDomainName, Label.User, "CN=TEST USER,CN=USERS,DC=TESTLAB,DC=LOCAL"); + var processor = CreateCombinedAclProcessor(new[] { denyRule.Object, allowRule.Object }); + var result = await processor.ProcessACLWithCustomDenyAces(new byte[] { 1 }, _testDomainName, Label.User, + false); - Assert.DoesNotContain("customdenyaces", props.Keys); - Assert.Equal(1, props["customexplicitdenyacescount"]); - Assert.Equal(1, props["custominheriteddenyacescount"]); + Assert.Single(result.Aces); + Assert.Equal(EdgeNames.WriteDacl, result.Aces[0].RightName); + AssertCustomDenyAceCounts(result.CustomDenyAceCounts, 1, 0); } - [WindowsOnlyFact] - public async Task ACLProcessor_AddCustomDenyAcesProperty_DoesNotEmitWhenEmpty() { - var props = new Dictionary(); - var ace = CreateCommonDenyAce("S-1-1-0", + [Fact] + public async Task ACLProcessor_ProcessACLWithCustomDenyAces_DoesNotCountExcludedDenyAces() { + var denyRule = CreateRuleDescriptor(WellKnownPrincipal.EveryoneSid, AccessControlType.Deny, ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree); - var processor = CreateCustomDenyAceProcessor(); - await processor.AddCustomDenyAcesProperty(props, CreateSecurityDescriptorBytes(ace), _testDomainName, - Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); + var processor = CreateCombinedAclProcessor(new[] { denyRule.Object }); + var result = await processor.ProcessACLWithCustomDenyAces(new byte[] { 1 }, _testDomainName, Label.OU, + false); - Assert.DoesNotContain("customdenyaces", props.Keys); - Assert.DoesNotContain("customexplicitdenyacescount", props.Keys); - Assert.DoesNotContain("custominheriteddenyacescount", props.Keys); + Assert.Empty(result.Aces); + AssertCustomDenyAceCounts(result.CustomDenyAceCounts, 0, 0); } private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Name)[] principals) { @@ -2292,6 +2290,33 @@ private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Nam return new ACLProcessor(mockLdapUtils.Object); } + private ACLProcessor CreateCombinedAclProcessor(IEnumerable rules) { + var mockLdapUtils = new Mock(); + var mockSecurityDescriptor = new Mock(MockBehavior.Loose, null); + mockSecurityDescriptor.Setup(x => x.GetOwner(It.IsAny())).Returns((string)null); + mockSecurityDescriptor.Setup(x => x.GetAccessRules(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(rules.ToList()); + mockLdapUtils.Setup(x => x.MakeSecurityDescriptor()).Returns(mockSecurityDescriptor.Object); + mockLdapUtils.Setup(x => x.PagedQuery(It.IsAny(), It.IsAny())) + .Returns(AsyncEnumerable.Empty>()); + mockLdapUtils.Setup(x => x.ResolveAccountName(It.IsAny(), It.IsAny())) + .ReturnsAsync((false, null)); + mockLdapUtils.Setup(x => x.ResolveIDAndType(It.IsAny(), It.IsAny())) + .ReturnsAsync((string sid, string _) => (true, new TypedPrincipal(sid, Label.User))); + return new ACLProcessor(mockLdapUtils.Object); + } + + private static Mock CreateRuleDescriptor(string sid, + AccessControlType accessControlType, ActiveDirectoryRights rights, bool inherited = false) { + var rule = new Mock(MockBehavior.Loose, null); + rule.Setup(x => x.IdentityReference()).Returns(sid); + rule.Setup(x => x.AccessControlType()).Returns(accessControlType); + rule.Setup(x => x.ActiveDirectoryRights()).Returns(rights); + rule.Setup(x => x.ObjectType()).Returns(Guid.Empty); + rule.Setup(x => x.IsInherited()).Returns(inherited); + return rule; + } + private static void AssertCustomDenyAceCounts(ACLProcessor.CustomDenyAceCounts result, int expectedExplicitCount, int expectedInheritedCount) { Assert.Equal(expectedExplicitCount, result.ExplicitCount); diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index 165c99ecb..7cc463eaa 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -20,7 +20,6 @@ namespace CommonLibTest.Facades [SuppressMessage("Interoperability", "CA1416:Validate platform compatibility")] public class MockLdapUtils : ILdapUtils { - private LdapConfig _ldapConfig = new(); private readonly ConcurrentDictionary _domainControllers = new(); private readonly Forest _forest; private readonly ConcurrentDictionary _seenWellKnownPrincipals = new(); @@ -1008,11 +1007,9 @@ public IAsyncEnumerable GetWellKnownPrincipalOutput() { } public void SetLdapConfig(LdapConfig config) { - _ldapConfig = config; + throw new NotImplementedException(); } - public bool SkipDenyAcesCount => _ldapConfig.SkipDenyAcesCount; - public Task<(bool Success, string Message)> TestLdapConnection(string domain) { throw new NotImplementedException(); } diff --git a/test/unit/LdapPropertyTests.cs b/test/unit/LdapPropertyTests.cs index 5cee8fcb0..881c2433b 100644 --- a/test/unit/LdapPropertyTests.cs +++ b/test/unit/LdapPropertyTests.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.DirectoryServices; using System.Runtime.Versioning; -using System.Security.AccessControl; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Security.Principal; @@ -31,21 +30,6 @@ public LdapPropertyTests(ITestOutputHelper testOutputHelper) _testOutputHelper = testOutputHelper; } - [SupportedOSPlatform("windows")] - private static byte[] CreateSecurityDescriptorBytes(params GenericAce[] aces) - { - var acl = new RawAcl(GenericAcl.AclRevisionDS, aces.Length); - for (var i = 0; i < aces.Length; i++) - { - acl.InsertAce(i, aces[i]); - } - - var descriptor = new RawSecurityDescriptor(ControlFlags.DiscretionaryAclPresent, null, null, null, acl); - var buffer = new byte[descriptor.BinaryLength]; - descriptor.GetBinaryForm(buffer, 0); - return buffer; - } - [Fact] public async void LDAPPropertyProcessor_ReadDomainProperties_TestGoodData() { @@ -98,7 +82,7 @@ public void LDAPPropertyProcessor_FunctionalLevelToString_TestFunctionalLevels() } [Fact] - public async Task LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() + public void LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() { var mock = new MockDirectoryObject( "CN\u003d{94DD0260-38B5-497E-8876-10E7A96E80D0},CN\u003dPolicies,CN\u003dSystem,DC\u003dtestlab,DC\u003dlocal", @@ -112,8 +96,7 @@ public async Task LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() {"description", "Test"} }, "S-1-5-21-3130019616-2776909439-2417379446",""); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadGPOProperties(mock); + var test = LdapPropertyProcessor.ReadGPOProperties(mock); Assert.Contains("description", test.Keys); Assert.Equal("Test", test["description"] as string); @@ -123,7 +106,7 @@ public async Task LDAPPropertyProcessor_ReadGPOProperties_TestGoodData() } [Fact] - public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() + public void LDAPPropertyProcessor_ReadOUProperties_TestGoodData() { var mock = new MockDirectoryObject("OU\u003dTestOU,DC\u003dtestlab,DC\u003dlocal", new Dictionary @@ -131,45 +114,11 @@ public async Task LDAPPropertyProcessor_ReadOUProperties_TestGoodData() {"description", "Test"} },"", "2A374493-816A-4193-BEFD-D2F4132C6DCA"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadOUProperties(mock); + var test = LdapPropertyProcessor.ReadOUProperties(mock); Assert.Contains("description", test.Keys); Assert.Equal("Test", test["description"] as string); } - [SupportedOSPlatform("windows")] - [WindowsOnlyFact] - public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAceCounts_WhenLdapConfigRequestsIt() - { - var denyAce = new CommonAce(AceFlags.None, AceQualifier.AccessDenied, (int)ActiveDirectoryRights.Delete, - new SecurityIdentifier("S-1-5-21-3130019616-2776909439-2417379446-2500"), false, null); - var mock = new MockDirectoryObject("OU\u003dTestOU,DC\u003dtestlab,DC\u003dlocal", - new Dictionary - { - {LDAPProperties.SecurityDescriptor, CreateSecurityDescriptorBytes(denyAce)} - }, "", "2A374493-816A-4193-BEFD-D2F4132C6DCA"); - - var baselineProcessor = new LdapPropertyProcessor(new MockLdapUtils()); - var baseline = await baselineProcessor.ReadOUProperties(mock); - Assert.DoesNotContain("customdenyaces", baseline.Keys); - Assert.Contains("customexplicitdenyacescount", baseline.Keys); - Assert.Contains("custominheriteddenyacescount", baseline.Keys); - Assert.Equal(1, baseline["customexplicitdenyacescount"]); - Assert.Equal(0, baseline["custominheriteddenyacescount"]); - - var ldapUtils = new MockLdapUtils(); - ldapUtils.SetLdapConfig(new LdapConfig { - SkipDenyAcesCount = true - }); - - var processor = new LdapPropertyProcessor(ldapUtils); - var test = await processor.ReadOUProperties(mock); - - Assert.DoesNotContain("customdenyaces", test.Keys); - Assert.DoesNotContain("customexplicitdenyacescount", test.Keys); - Assert.DoesNotContain("custominheriteddenyacescount", test.Keys); - } - [Fact] public async Task LDAPPropertyProcessor_ReadGroupProperties_TestGoodData() { @@ -732,7 +681,7 @@ public async Task LDAPPropertyProcessor_ReadComputerProperties_TestDumpSMSAPassw } [Fact] - public async Task LDAPPropertyProcessor_ReadRootCAProperties() { + public void LDAPPropertyProcessor_ReadRootCAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -750,8 +699,7 @@ public async Task LDAPPropertyProcessor_ReadRootCAProperties() { {LDAPProperties.CACertificate, bytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadRootCAProperties(mock); + var test = LdapPropertyProcessor.ReadRootCAProperties(mock); var keys = test.Keys; //These are not common properties @@ -770,7 +718,7 @@ public async Task LDAPPropertyProcessor_ReadRootCAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public async Task LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { + public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -783,8 +731,7 @@ public async Task LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byt {LDAPProperties.CACertificate, CACertBytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadRootCAProperties(mock); + var test = LdapPropertyProcessor.ReadRootCAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -798,7 +745,7 @@ public async Task LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byt } [Fact] - public async Task LDAPPropertyProcessor_ReadAIACAProperties() { + public void LDAPPropertyProcessor_ReadAIACAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -817,8 +764,7 @@ public async Task LDAPPropertyProcessor_ReadAIACAProperties() { {LDAPProperties.CACertificate, bytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadAIACAProperties(mock); + var test = LdapPropertyProcessor.ReadAIACAProperties(mock); var keys = test.Keys; //These are not common properties @@ -841,7 +787,7 @@ public async Task LDAPPropertyProcessor_ReadAIACAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public async Task LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CACertBytes) { + public void LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -855,8 +801,7 @@ public async Task LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte {LDAPProperties.CACertificate, CACertBytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadAIACAProperties(mock); + var test = LdapPropertyProcessor.ReadAIACAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -872,7 +817,7 @@ public async Task LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate(byte } [Fact] - public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties() { + public void LDAPPropertyProcessor_ReadEnterpriseCAProperties() { var ecdsa = ECDsa.Create(); var req = new CertificateRequest("cn=foobar", ecdsa, HashAlgorithmName.SHA256); var cert = req.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(5)); @@ -891,8 +836,7 @@ public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties() { {"flags", 1} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadEnterpriseCAProperties(mock); + var test = LdapPropertyProcessor.ReadEnterpriseCAProperties(mock); var keys = test.Keys; //These are not common properties @@ -916,7 +860,7 @@ public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties() { [Theory] [MemberData(nameof(EmptyCertBytes))] - public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byte[] CACertBytes) { + public void LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate(byte[] CACertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -930,8 +874,7 @@ public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertifica {"flags", 1} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadEnterpriseCAProperties(mock); + var test = LdapPropertyProcessor.ReadEnterpriseCAProperties(mock); var keys = test.Keys; //These are cert derived properties @@ -956,7 +899,7 @@ public async Task LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertifica }; [Fact] - public async Task LDAPPropertyProcessor_ReadNTAuthStoreProperties() + public void LDAPPropertyProcessor_ReadNTAuthStoreProperties() { var mock = new MockDirectoryObject("CN\u003dNTAUTHCERTIFICATES,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary @@ -968,8 +911,7 @@ public async Task LDAPPropertyProcessor_ReadNTAuthStoreProperties() {"whencreated", 1683986131}, }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadNTAuthStoreProperties(mock); + var test = LdapPropertyProcessor.ReadNTAuthStoreProperties(mock); var keys = test.Keys; //These are not common properties @@ -981,7 +923,7 @@ public async Task LDAPPropertyProcessor_ReadNTAuthStoreProperties() } [Fact] - public async Task LDAPPropertyProcessor_ReadCertTemplateProperties() + public void LDAPPropertyProcessor_ReadCertTemplateProperties() { var mock = new MockDirectoryObject("CN\u003dWORKSTATION,CN\u003dCERTIFICATE TEMPLATES,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dEXTERNAL,DC\u003dLOCAL", new Dictionary @@ -1019,8 +961,7 @@ public async Task LDAPPropertyProcessor_ReadCertTemplateProperties() {LDAPProperties.PKIPrivateKeyFlag, 256}, }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9"); - var processor = new LdapPropertyProcessor(new MockLdapUtils()); - var test = await processor.ReadCertTemplateProperties(mock); + var test = LdapPropertyProcessor.ReadCertTemplateProperties(mock); var keys = test.Keys; //These are not common properties From b3965b0d28d145fd546de62362202650d04a1a7b Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 2 Jul 2026 09:27:52 +0200 Subject: [PATCH 12/13] fix deny exclusion --- src/CommonLib/Processors/ACLProcessor.cs | 3 +-- test/unit/ACLProcessorTest.cs | 25 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/CommonLib/Processors/ACLProcessor.cs b/src/CommonLib/Processors/ACLProcessor.cs index 4fd00d956..b9a1c5ede 100644 --- a/src/CommonLib/Processors/ACLProcessor.cs +++ b/src/CommonLib/Processors/ACLProcessor.cs @@ -1088,8 +1088,7 @@ private async Task ShouldExcludeCustomDenyAce(string principalSid, ActiveD // Filter default Everyone Deny ACEs if (principalSid.Equals(WellKnownPrincipal.EveryoneSid, StringComparison.OrdinalIgnoreCase)) { if ((objectType is Label.OU or Label.Container) && - rights.HasFlag(ActiveDirectoryRights.Delete) && - rights.HasFlag(ActiveDirectoryRights.DeleteTree)) { + rights == (ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree)) { return true; } diff --git a/test/unit/ACLProcessorTest.cs b/test/unit/ACLProcessorTest.cs index b13e26a58..b383c78fd 100644 --- a/test/unit/ACLProcessorTest.cs +++ b/test/unit/ACLProcessorTest.cs @@ -2203,6 +2203,18 @@ public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsAccidentalDeletionPro AssertCustomDenyAceCounts(result, 0, 0); } + [WindowsOnlyFact] + public async Task ACLProcessor_GetCustomDenyAceCounts_CountsAccidentalDeletionProtectionWithAdditionalRights() { + var ace = CreateCommonDenyAce("S-1-1-0", + ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree | ActiveDirectoryRights.WriteDacl); + var processor = CreateCustomDenyAceProcessor(); + + var result = await processor.GetCustomDenyAceCounts(CreateSecurityDescriptorBytes(ace), _testDomainName, + Label.OU, "OU=TEST,DC=TESTLAB,DC=LOCAL"); + + AssertCustomDenyAceCounts(result, 1, 0); + } + [WindowsOnlyFact] public async Task ACLProcessor_GetCustomDenyAceCounts_SkipsDefaultAdDenyPatterns() { var msaAce = CreateObjectDenyAce("S-1-1-0", ActiveDirectoryRights.ExtendedRight, @@ -2277,6 +2289,19 @@ public async Task ACLProcessor_ProcessACLWithCustomDenyAces_DoesNotCountExcluded AssertCustomDenyAceCounts(result.CustomDenyAceCounts, 0, 0); } + [Fact] + public async Task ACLProcessor_ProcessACLWithCustomDenyAces_CountsAccidentalDeletionProtectionWithAdditionalRights() { + var denyRule = CreateRuleDescriptor(WellKnownPrincipal.EveryoneSid, AccessControlType.Deny, + ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree | ActiveDirectoryRights.WriteDacl); + + var processor = CreateCombinedAclProcessor(new[] { denyRule.Object }); + var result = await processor.ProcessACLWithCustomDenyAces(new byte[] { 1 }, _testDomainName, Label.OU, + false); + + Assert.Empty(result.Aces); + AssertCustomDenyAceCounts(result.CustomDenyAceCounts, 1, 0); + } + private ACLProcessor CreateCustomDenyAceProcessor(params (string Sid, string Name)[] principals) { var mockLdapUtils = new Mock(MockBehavior.Strict); mockLdapUtils.Setup(x => x.ResolveAccountName(It.IsAny(), It.IsAny())) From bfcbd5b3c5771d807225760aef90e71c7439e919 Mon Sep 17 00:00:00 2001 From: JonasBK Date: Thu, 2 Jul 2026 09:47:21 +0200 Subject: [PATCH 13/13] simplify code --- src/CommonLib/LdapUtils.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 1ba4ae951..f637130ea 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -881,8 +881,7 @@ public ActiveDirectorySecurityDescriptor MakeSecurityDescriptor() { string computerDomainSid, string computerDomain) { if (!WellKnownPrincipal.GetWellKnownPrincipal(sid.Value, out var common)) return (false, null); //The "Everyone" and "Authenticated Users" principals are special and will be converted to the domain equivalent - if (sid.Value is var sidValue && - (sidValue == WellKnownPrincipal.EveryoneSid || sidValue == "S-1-5-11")) { + if (sid.Value is WellKnownPrincipal.EveryoneSid or "S-1-5-11") { return await GetWellKnownPrincipal(sid.Value, computerDomain); }