-
Notifications
You must be signed in to change notification settings - Fork 56
Custom Deny ACEs BED-8117 #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
JonasBK
wants to merge
14
commits into
v4
Choose a base branch
from
BED-8117-deny-aces
base: v4
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+502
−10
Draft
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
5082585
collect custom deny aces
JonasBK 6d2c4ad
disable DocFX by default on non-Windows
JonasBK 4490451
add SkipDenyAces flag
JonasBK fd53dac
mv customdenyaces out of LDAPProperties
JonasBK 686b781
coderabbit fixes
JonasBK 8fe0858
fix Windows only code
JonasBK 820d80d
fix AddCustomDenyAceProperty
JonasBK decd8a4
make CustomdDenyACEs a count
JonasBK d66c9c6
optimizations
JonasBK f7bb646
rename SkipDenyAces to SkipDenyAcesCount
JonasBK 9a8c875
Merge branch 'v4' into BED-8117-deny-aces
JonasBK 446c4fe
Merge branch 'v4' into BED-8117-deny-aces
JonasBK 71b8f77
move deny ace logic to ACLProcessor
JonasBK b3965b0
fix deny exclusion
JonasBK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,4 +53,4 @@ public override string ToString() { | |
| return sb.ToString(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<string, string[]> _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<string> 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 | ||
|
|
@@ -48,6 +57,44 @@ 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; | ||
| } | ||
|
|
||
| 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 { | ||
|
|
@@ -431,6 +478,24 @@ public IAsyncEnumerable<ACE> ProcessACL(ResolvedSearchResult result, IDirectoryO | |
| return ProcessACL(descriptor, result.Domain, result.ObjectType, searchResult.HasLAPS(), checkForOwnerRights, result.DisplayName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Processes the regular ACL edges and custom deny ACE counts in one ACL traversal. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Callers that do not want custom deny ACE counts should continue to call <see cref="ProcessACL(ResolvedSearchResult, IDirectoryObject, bool)"/>. | ||
| /// </remarks> | ||
| public Task<ACLProcessingResult> ProcessACLWithCustomDenyAces(ResolvedSearchResult result, | ||
| IDirectoryObject searchResult, bool checkForOwnerRights = true) { | ||
| if (!searchResult.TryGetByteProperty(LDAPProperties.SecurityDescriptor, out var descriptor)) { | ||
| return Task.FromResult(new ACLProcessingResult(Array.Empty<ACE>(), new CustomDenyAceCounts())); | ||
| } | ||
|
|
||
| searchResult.TryGetDistinguishedName(out var distinguishedName); | ||
| return ProcessACLWithCustomDenyAces(descriptor, result.Domain, result.ObjectType, searchResult.HasLAPS(), | ||
| checkForOwnerRights, distinguishedName, searchResult.IsMSA() || searchResult.IsGMSA(), | ||
| result.DisplayName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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 | ||
|
|
@@ -447,8 +512,25 @@ public IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, string obje | |
| return ProcessACL(ntSecurityDescriptor, objectDomain, objectType, hasLaps, true, objectName); | ||
| } | ||
|
|
||
| public async IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, string objectDomain, | ||
| public IAsyncEnumerable<ACE> ProcessACL(byte[] ntSecurityDescriptor, string objectDomain, | ||
| Label objectType, bool hasLaps, bool checkForOwnerRights, string objectName) { | ||
| return ProcessACLInternal(ntSecurityDescriptor, objectDomain, objectType, hasLaps, checkForOwnerRights, | ||
| objectName); | ||
| } | ||
|
|
||
| public async Task<ACLProcessingResult> 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<ACE> 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) { | ||
|
|
@@ -496,7 +578,19 @@ public async IAsyncEnumerable<ACE> 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; | ||
| } | ||
|
|
||
|
|
@@ -881,6 +975,164 @@ or Label.NTAuthStore | |
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would introduce a new function here that processes deny aces at the same time as the regular ACL data, that way we can do both in one call and then we can prioritize that call as the "canonical" version. |
||
| public Task<CustomDenyAceCounts> 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<CustomDenyAceCounts> GetCustomDenyAceCounts(byte[] ntSecurityDescriptor, | ||
| string objectDomain, Label objectType, string distinguishedName = null, bool isMSA = false, | ||
| string objectName = "") { | ||
| if (ntSecurityDescriptor == null) { | ||
| return new CustomDenyAceCounts(); | ||
| } | ||
|
|
||
| RawSecurityDescriptor descriptor; | ||
| try { | ||
| descriptor = new RawSecurityDescriptor(ntSecurityDescriptor, 0); | ||
| } | ||
| 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); | ||
| return new CustomDenyAceCounts(); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| if (descriptor.DiscretionaryAcl == null || descriptor.DiscretionaryAcl.Count == 0) { | ||
| return new CustomDenyAceCounts(); | ||
| } | ||
|
|
||
| var explicitCount = 0; | ||
| var inheritedCount = 0; | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| if ((ace.AceFlags & AceFlags.Inherited) == AceFlags.Inherited) { | ||
| inheritedCount++; | ||
| } else { | ||
| explicitCount++; | ||
| } | ||
| } | ||
|
|
||
| return new CustomDenyAceCounts(explicitCount, inheritedCount); | ||
| } | ||
|
|
||
| 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 (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, | ||
| 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<bool> 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 == (ActiveDirectoryRights.Delete | 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; | ||
| } | ||
|
JonasBK marked this conversation as resolved.
|
||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private async Task<bool> 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<string>(); | ||
| 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); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper function to use commonlib types and pass to ProcessGMSAReaders | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change for?