Custom Deny ACEs BED-8117#298
Open
JonasBK wants to merge 20 commits into
Open
Conversation
WalkthroughThis PR adds custom deny-ACE counting to ACLProcessor, distinguishing explicit vs. inherited deny ACEs while excluding Exchange trustees and well-known Everyone deny patterns. It introduces a shared EveryoneSid constant, an ExchangeLocation path constant, related unit tests, and minor formatting/config cleanup. ChangesCustom deny ACE counting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ACLProcessor
participant CustomDenyAceAccumulator
participant LdapUtils
Caller->>ACLProcessor: ProcessACLWithCustomDenyAces(securityDescriptor)
ACLProcessor->>ACLProcessor: Parse RawSecurityDescriptor DiscretionaryAcl
loop each ACE
alt Deny ACE
ACLProcessor->>ACLProcessor: ShouldExcludeCustomDenyAce check
ACLProcessor->>LdapUtils: ResolveAccountName (Exchange trustee lookup)
ACLProcessor->>CustomDenyAceAccumulator: CountCustomDenyAce (explicit/inherited)
else Non-deny ACE
ACLProcessor->>ACLProcessor: Build standard ACE edge
end
end
ACLProcessor-->>Caller: ACLProcessingResult(Aces, CustomDenyAceCounts)
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/CommonLib/LdapConfig.cs (1)
16-16: Log the new switch with the rest of the LDAP config.Line 16 adds a runtime collection switch, but
ToString()omits it even though LDAP config changes are logged. Including it makescustomdenyacessuppression easier to diagnose.Proposed observability tweak
sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); + sb.AppendLine($"SkipDenyAces: {SkipDenyAces}"); if (!string.IsNullOrWhiteSpace(Username)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/LdapConfig.cs` at line 16, The ToString() for class LdapConfig is missing the new SkipDenyAces property so runtime logs don't show the custom deny-aces switch; update the LdapConfig.ToString() method to include the SkipDenyAces value (use the same naming convention used for other settings—e.g., "SkipDenyAces" or "customdenyaces") so when an LdapConfig instance is logged it prints the boolean state of SkipDenyAces along with the other properties.src/CommonLib/ILdapUtils.cs (1)
160-162: Prefer a narrow read-only accessor for the new flag.Line 162 exposes the full mutable LDAP config just so callers can read
SkipDenyAces. SinceLdapUtils.SetLdapConfig()rebuilds connection state, callers mutating the returned config could bypass those side effects. Prefer a narrower accessor or immutable snapshot.Example narrower API
- LdapConfig GetLdapConfig(); + bool ShouldSkipDenyAces();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/ILdapUtils.cs` around lines 160 - 162, The interface currently exposes a mutable LdapConfig via ILdapUtils.GetLdapConfig(), which lets callers mutate config and bypass SetLdapConfig() side effects; replace this with a narrow read-only accessor (for example add a bool SkipDenyAces { get; } or a GetSkipDenyAces() method) or return an immutable snapshot type (e.g., ReadOnlyLdapConfig) instead of LdapConfig, update callers to read the specific flag from the new accessor, and ensure SetLdapConfig() remains the only way to change runtime config/connection state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 914-923: The try/catch around RawSecurityDescriptor construction
in ACLProcessor should also catch ArgumentException (not just OverflowException)
because RawSecurityDescriptor(byte[], int) can throw ArgumentException for
malformed self-relative SECURITY_DESCRIPTOR bytes; update the catch block that
wraps new RawSecurityDescriptor(ntSecurityDescriptor, 0) to handle both
OverflowException and ArgumentException (or add a separate catch for
ArgumentException) and log the same warning (including objectName) and return
Array.Empty<string>() so malformed descriptors from LDAP don't propagate an
unhandled exception.
- Around line 995-1011: The Everyone DeleteChild deny check is too narrow (only
Label.Domain); update the condition in ACLProcessor.cs inside the "Filter
default Everyone Deny ACEs" block so DeleteChild denies from Everyone are
treated as default accidental-deletion for container-like parents as well (e.g.,
Label.Domain OR Label.OU OR Label.Container) instead of only Label.Domain —
modify the objectType check that guards ActiveDirectoryRights.DeleteChild to
include those additional Label values and keep the existing return true behavior
so such default parent DeleteChild denies are filtered out.
In `@test/unit/LdapPropertyTests.cs`:
- Around line 154-157: Add a positive control run before the existing assertion:
call LdapPropertyProcessor.ReadOUProperties with the same mock/ldapUtils under
default configuration (i.e., without SkipDenyAces enabled) and assert that the
returned dictionary contains the "customdenyaces" key; then run the existing
test path that enables SkipDenyAces and assert that "customdenyaces" is not
present. Use the same mock variable and LdapPropertyProcessor instance creation
pattern so you verify the property is emitted normally and only suppressed when
SkipDenyAces is applied.
- Around line 139-143: The test method
LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt
constructs a SecurityIdentifier (Windows-only API) but lacks platform gating;
annotate the test to run only on Windows by replacing or supplementing the
[Fact] attribute with the Windows-only attributes used elsewhere (e.g., add
[SupportedOSPlatform("windows")] and use [WindowsOnlyFact] or equivalent) so the
SecurityIdentifier construction and related assertions are skipped on
non-Windows platforms.
---
Nitpick comments:
In `@src/CommonLib/ILdapUtils.cs`:
- Around line 160-162: The interface currently exposes a mutable LdapConfig via
ILdapUtils.GetLdapConfig(), which lets callers mutate config and bypass
SetLdapConfig() side effects; replace this with a narrow read-only accessor (for
example add a bool SkipDenyAces { get; } or a GetSkipDenyAces() method) or
return an immutable snapshot type (e.g., ReadOnlyLdapConfig) instead of
LdapConfig, update callers to read the specific flag from the new accessor, and
ensure SetLdapConfig() remains the only way to change runtime config/connection
state.
In `@src/CommonLib/LdapConfig.cs`:
- Line 16: The ToString() for class LdapConfig is missing the new SkipDenyAces
property so runtime logs don't show the custom deny-aces switch; update the
LdapConfig.ToString() method to include the SkipDenyAces value (use the same
naming convention used for other settings—e.g., "SkipDenyAces" or
"customdenyaces") so when an LdapConfig instance is logged it prints the boolean
state of SkipDenyAces along with the other properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 258ec52f-a3e7-470f-b1ac-bcebba1bd691
📒 Files selected for processing (11)
docfx/Docfx.csprojsrc/CommonLib/Enums/DirectoryPaths.cssrc/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cssrc/CommonLib/WellKnownPrincipal.cstest/unit/ACLProcessorTest.cstest/unit/Facades/MockLdapUtils.cstest/unit/LdapPropertyTests.cs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/unit/LdapPropertyTests.cs (1)
139-143:⚠️ Potential issue | 🟠 MajorGate this test as Windows-only.
Line 142 constructs a
SecurityIdentifier, while similar tests in this file use[SupportedOSPlatform("windows")]and[WindowsOnlyFact]. This can still break non-Windows test runs or platform analyzer enforcement.🛡️ Proposed fix
- [Fact] + [SupportedOSPlatform("windows")] + [WindowsOnlyFact] public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt()Run this read-only check to confirm remaining ungated
SecurityIdentifiertest usage:#!/bin/bash set -euo pipefail rg -n -C2 '\[Fact\]|\[Theory\]|\[WindowsOnlyFact\]|new SecurityIdentifier\(' --iglob '*.cs' test/unit rg -n '<TreatWarningsAsErrors|EnableNETAnalyzers|AnalysisLevel|SupportedOSPlatform' --iglob '*.csproj' --iglob '*.props' --iglob '*.targets'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` around lines 139 - 143, The test method LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt creates a SecurityIdentifier which is Windows-only; mark this test with the appropriate platform attributes (e.g., add [SupportedOSPlatform("windows")] and replace [Fact] with [WindowsOnlyFact] or add [WindowsOnlyFact] alongside [Fact] per project conventions) so the SecurityIdentifier construction is gated on Windows; update the method declaration attributes accordingly to match other tests in this file (refer to the method name and the SecurityIdentifier instantiation) to prevent non-Windows runs or analyzer failures.src/CommonLib/Processors/ACLProcessor.cs (1)
995-1011:⚠️ Potential issue | 🟠 MajorTighten the default Everyone deny filter.
These
HasFlagchecks suppress ACEs that contain the default bits plus additional custom deny rights, socustomdenyacescan silently miss relevant ACEs. Also, theDeleteChilddefault still only filters domains, leaving accidental-deletion parent ACEs on OUs/containers to be emitted.🛡️ Proposed fix
// 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)) { + HasOnlyRights(rights, ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree)) { return true; } if (isMSA && - rights.HasFlag(ActiveDirectoryRights.ExtendedRight) && + HasOnlyRights(rights, ActiveDirectoryRights.ExtendedRight) && objectAceType.Equals(new Guid(ACEGuids.UserForceChangePassword))) { return true; } - if (objectType == Label.Domain && rights.HasFlag(ActiveDirectoryRights.DeleteChild)) { + if ((objectType is Label.Domain or Label.OU or Label.Container or Label.Configuration) && + HasOnlyRights(rights, ActiveDirectoryRights.DeleteChild)) { return true; } }Add this helper near the filtering helpers:
private static bool HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) { return (actual & expected) == expected && (actual & ~expected) == 0; }Based on learnings, properties added to dictionaries returned by methods in SharpHoundCommon may be consumed by dependent projects like SharpHound and SharpHoundEnterprise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/ACLProcessor.cs` around lines 995 - 1011, The Everyone-deny filtering in ACLProcessor (the block that checks principalSid == WellKnownPrincipal.EveryoneSid and inspects objectType, rights, isMSA, and objectAceType) currently uses HasFlag which allows extra custom deny bits to be suppressed; add the proposed helper HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) and replace the HasFlag checks with calls to HasOnlyRights for the Delete/DeleteTree, ExtendedRight/UserForceChangePassword, and DeleteChild checks so only ACEs that match exactly the default deny bits are filtered; also broaden the DeleteChild default filter so it applies to OU and Container objectType values (not just Domain) to prevent accidental suppression of parent-deletion ACEs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CommonLib/Processors/LdapPropertyProcessor.cs`:
- Around line 670-683: The AddCustomDenyAceProperty method is passing the
distinguished name into both the 5th and 7th parameters of
_aclProcessor.AddCustomDenyAcesProperty (distinguishedName and objectName);
change the 7th argument to pass the object's actual name (e.g., sAMAccountName
retrieved via entry.TryGetStringProperty("sAMAccountName", out var objectName)
or the entry.TryGetSamAccountName helper if available) instead of using
distinguishedName, and handle the TryGetDistinguishedName return (use a safe
default like string.Empty if it returns false) so the 5th parameter never
unexpectedly becomes null. Ensure you update the call site in
AddCustomDenyAceProperty to pass the fetched objectName variable and not
distinguishedName.
---
Duplicate comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 995-1011: The Everyone-deny filtering in ACLProcessor (the block
that checks principalSid == WellKnownPrincipal.EveryoneSid and inspects
objectType, rights, isMSA, and objectAceType) currently uses HasFlag which
allows extra custom deny bits to be suppressed; add the proposed helper
HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) and
replace the HasFlag checks with calls to HasOnlyRights for the
Delete/DeleteTree, ExtendedRight/UserForceChangePassword, and DeleteChild checks
so only ACEs that match exactly the default deny bits are filtered; also broaden
the DeleteChild default filter so it applies to OU and Container objectType
values (not just Domain) to prevent accidental suppression of parent-deletion
ACEs.
In `@test/unit/LdapPropertyTests.cs`:
- Around line 139-143: The test method
LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt
creates a SecurityIdentifier which is Windows-only; mark this test with the
appropriate platform attributes (e.g., add [SupportedOSPlatform("windows")] and
replace [Fact] with [WindowsOnlyFact] or add [WindowsOnlyFact] alongside [Fact]
per project conventions) so the SecurityIdentifier construction is gated on
Windows; update the method declaration attributes accordingly to match other
tests in this file (refer to the method name and the SecurityIdentifier
instantiation) to prevent non-Windows runs or analyzer failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c6a129b-8e11-46d8-a9b6-54653652f8bf
📒 Files selected for processing (7)
src/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cstest/unit/Facades/MockLdapUtils.cstest/unit/LdapPropertyTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/CommonLib/LdapConfig.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/ILdapUtils.cs
This was referenced May 14, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/CommonLib/Processors/ACLProcessor.cs (1)
1026-1028:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroaden Everyone
DeleteChildfilter to non-Domain container parents.The Everyone
DeleteChilddeny ACE added by the "Protect object from accidental deletion" feature is written on the parent container, which can be an OU/Container/Configuration node, not only a Domain. With the check still restricted toLabel.Domain, child-protection denies on non-domain parents will be counted as custom deny ACEs.🛡️ Proposed fix
- if (objectType == Label.Domain && rights.HasFlag(ActiveDirectoryRights.DeleteChild)) { + if (objectType is Label.Domain or Label.OU or Label.Container or Label.Configuration && + rights.HasFlag(ActiveDirectoryRights.DeleteChild)) { return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/Processors/ACLProcessor.cs` around lines 1026 - 1028, The current check only treats DeleteChild denies as non-custom when objectType == Label.Domain; broaden this to detect DeleteChild denies on any container parent (e.g. domain, organizational unit, generic container, configuration partition) by changing the condition that uses objectType == Label.Domain to check for container-like labels (for example: objectType == Label.Domain || objectType == Label.OrganizationalUnit || objectType == Label.Container || objectType == Label.Configuration) while still checking rights.HasFlag(ActiveDirectoryRights.DeleteChild); update the if in ACLProcessor (the objectType/rights check) accordingly so the Everyone "Protect object from accidental deletion" ACE on non-domain parents is treated the same.
🧹 Nitpick comments (2)
src/CommonLib/LdapUtils.cs (1)
884-887: 💤 Low valueSimplify the
is varpattern.
sid.Value is var sidValueis an always-true declaration pattern; consider extracting to a local for clarity. Functionally equivalent, butvar sidValue = sid.Value;reads more idiomatically.♻️ Proposed refactor
- if (sid.Value is var sidValue && - (sidValue == WellKnownPrincipal.EveryoneSid || sidValue == "S-1-5-11")) { + var sidValue = sid.Value; + if (sidValue == WellKnownPrincipal.EveryoneSid || sidValue == "S-1-5-11") { return await GetWellKnownPrincipal(sid.Value, computerDomain); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/LdapUtils.cs` around lines 884 - 887, Replace the always-true declaration pattern "sid.Value is var sidValue" with an explicit local assignment to improve readability: introduce "var sidValue = sid.Value;" then use "sidValue == WellKnownPrincipal.EveryoneSid || sidValue == \"S-1-5-11\"" in the if and pass sid.Value (or sidValue) to GetWellKnownPrincipal as before; update the surrounding code in the LdapUtils class/method where the conditional and call to GetWellKnownPrincipal(computerDomain) occur to preserve identical behavior.src/CommonLib/Processors/ACLProcessor.cs (1)
1034-1060: 💤 Low valueConsider returning the canonical cached value after
TryAdd.If two threads concurrently miss the cache for the same domain, both perform the four
ResolveAccountNamelookups and oneTryAddwins. Functionally correct (both compute the same set), but you can avoid keeping a stale local view by reading back the value that ended up in the cache. Minor — leaving as-is is acceptable givenResolveAccountNameis itself cached.♻️ Optional refactor
- var exchangeTrusteeSids = resolvedSids.Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); - _exchangeTrusteeSidCache.TryAdd(objectDomain, exchangeTrusteeSids); - return exchangeTrusteeSids.Contains(principalSid, StringComparer.OrdinalIgnoreCase); + var exchangeTrusteeSids = resolvedSids.Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + var canonicalSids = _exchangeTrusteeSidCache.GetOrAdd(objectDomain, exchangeTrusteeSids); + return canonicalSids.Contains(principalSid, StringComparer.OrdinalIgnoreCase);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/Processors/ACLProcessor.cs` around lines 1034 - 1060, The IsExchangeTrustee method can race when two threads miss _exchangeTrusteeSidCache and both compute resolvedSids; after calling _exchangeTrusteeSidCache.TryAdd(objectDomain, exchangeTrusteeSids) read back the canonical cached value (e.g. via _exchangeTrusteeSidCache.TryGetValue(objectDomain, out var cachedSids) or the cache's indexer) and use that cached array to perform the final Contains check instead of the local exchangeTrusteeSids, ensuring you reference the actual stored value in the cache; keep all existing logic resolving ExchangeTrusteeNames and calling _utils.ResolveAccountName.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 1026-1028: The current check only treats DeleteChild denies as
non-custom when objectType == Label.Domain; broaden this to detect DeleteChild
denies on any container parent (e.g. domain, organizational unit, generic
container, configuration partition) by changing the condition that uses
objectType == Label.Domain to check for container-like labels (for example:
objectType == Label.Domain || objectType == Label.OrganizationalUnit ||
objectType == Label.Container || objectType == Label.Configuration) while still
checking rights.HasFlag(ActiveDirectoryRights.DeleteChild); update the if in
ACLProcessor (the objectType/rights check) accordingly so the Everyone "Protect
object from accidental deletion" ACE on non-domain parents is treated the same.
---
Nitpick comments:
In `@src/CommonLib/LdapUtils.cs`:
- Around line 884-887: Replace the always-true declaration pattern "sid.Value is
var sidValue" with an explicit local assignment to improve readability:
introduce "var sidValue = sid.Value;" then use "sidValue ==
WellKnownPrincipal.EveryoneSid || sidValue == \"S-1-5-11\"" in the if and pass
sid.Value (or sidValue) to GetWellKnownPrincipal as before; update the
surrounding code in the LdapUtils class/method where the conditional and call to
GetWellKnownPrincipal(computerDomain) occur to preserve identical behavior.
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 1034-1060: The IsExchangeTrustee method can race when two threads
miss _exchangeTrusteeSidCache and both compute resolvedSids; after calling
_exchangeTrusteeSidCache.TryAdd(objectDomain, exchangeTrusteeSids) read back the
canonical cached value (e.g. via
_exchangeTrusteeSidCache.TryGetValue(objectDomain, out var cachedSids) or the
cache's indexer) and use that cached array to perform the final Contains check
instead of the local exchangeTrusteeSids, ensuring you reference the actual
stored value in the cache; keep all existing logic resolving
ExchangeTrusteeNames and calling _utils.ResolveAccountName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c2ff32a-c2b0-48cb-8c6d-b8d08aeecbe2
📒 Files selected for processing (8)
src/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cstest/unit/ACLProcessorTest.cstest/unit/Facades/MockLdapUtils.cstest/unit/LdapPropertyTests.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- test/unit/Facades/MockLdapUtils.cs
- test/unit/ACLProcessorTest.cs
- test/unit/LdapPropertyTests.cs
- src/CommonLib/Processors/LdapPropertyProcessor.cs
rvazarkar
reviewed
May 21, 2026
rvazarkar
left a comment
rvazarkar
left a comment
Contributor
There was a problem hiding this comment.
Theres a few issues here, but the biggest one is extending the property processor. Yes it is a property, but this really belongs as part of ACL processing and should be contained there. I suggest moving all of the configuration logic up to the consumer (SharpHound/SHE) and letting those decide which functions to call
Contributor
Author
|
@rvazarkar I think I have fixed the things you mentioned in your comments now. I tested the updated Deny ACEs filters in my lab and confirmed they are working as expected There is a build error which I'm not really sure about though |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Adds support for two new BloodHound node properties derived from filtered deny ACEs in an AD object's
nTSecurityDescriptor:customexplicitdenyacescount: count of qualifying custom deny ACEs applied directly to the objectcustominheriteddenyacescount: count of qualifying custom deny ACEs inherited by the objectThe properties are emitted for ACL-backed AD objects only when at least one qualifying deny ACE remains after filtering. Known noisy/default deny ACEs are filtered out, including Exchange-related denies, deny ACEs under the Exchange configuration path, accidental deletion protection, and selected default AD deny patterns.
CommonLib now exposes ACLProcessor.ProcessACLWithCustomDenyAces(...), which returns normal ACL edges and custom deny ACE counts from a single ACL traversal. Consumers can choose whether to call the count-aware path.
The change also disables DocFX builds by default on non-Windows environments to avoid unrelated cross-platform build issues.
Motivation and Context
This adds auditing-only visibility into custom deny ACEs without affecting edge creation. Reporting counts instead of raw SDDL keeps the output smaller and distinguishes explicit denies from inherited denies.
This PR addresses: BED-8117
Related PRs:
BloodHound: SpecterOps/BloodHound#2779
SharpHound: SpecterOps/SharpHound#218
SharpHoundEnterprise: https://github.com/SpecterOps/sharphound-enterprise/pull/113
How Has This Been Tested?
Added and updated unit tests covering:
SkipDenyAcesCountsuppresses count property emissionAlso updated affected
LdapPropertyProcessorproperty-reader tests.Local verification:
DOTNET_ROLL_FORWARD=Major dotnet build test/unit/CommonLibTest.csproj -v minimalScreenshots
N/A
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores