feat: AdminSDHolder updates - BED-6221 #165
Conversation
- Added AdminSDHolderHash dictionary to contexts - Collect AdminSDHolder data if ACL collection is enabled in LdapProducer - Compare SD hash of users, computers, groups with AdminSDHolder hash for that domain to determine status of adminsdholderprotected property Resolves BED-6221
WalkthroughA new property, Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as LdapProducer
participant Directory as LDAP Directory
participant Context as Context
participant ACLProc as ACLProcessor
Producer->>Directory: Query AdminSDHolder container (LDAP)
Directory-->>Producer: Return nTSecurityDescriptor
Producer->>ACLProc: Compute ACL hash from security descriptor
ACLProc-->>Producer: Return hash
Producer->>Context: Store hash in AdminSDHolderHash[domain]
Producer->>Directory: Continue with main LDAP queries
Directory-->>Producer: Return objects
Producer->>Context: Process and output objects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
JimSycurity
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Runtime/ObjectProcessors.cs (1)
427-449: Consider extracting common AdminSDHolder logicThe implementation is correct and consistent with User and Computer processing. However, the AdminSDHolder hash retrieval logic is duplicated across all three methods.
Consider extracting the common logic into a helper method:
+private string GetAdminSDHolderHash(string domain) +{ + if (_context.AdminSDHolderHash != null && + _context.AdminSDHolderHash.TryGetValue(domain, out var hash)) + { + return hash; + } + return null; +}Then simplify each occurrence to:
-string adminSdHolderHash = null; -if (_context.AdminSDHolderHash != null && - _context.AdminSDHolderHash.TryGetValue(resolvedSearchResult.Domain, out var hash)) -{ - adminSdHolderHash = hash; -} +var adminSdHolderHash = GetAdminSDHolderHash(resolvedSearchResult.Domain);src/Producers/LdapProducer.cs (2)
94-94: Consider reusing ACLProcessor instanceA new
ACLProcessorinstance is created here, but theObjectProcessorsclass already has an_aclProcessorfield. Consider if this instance could be reused or injected.
100-102: Initialize AdminSDHolderHash in constructorThe dictionary is initialized on first use. Consider initializing it in the
BaseContextconstructor for cleaner code.In
BaseContext.csconstructor:public BaseContext(ILogger logger, LdapConfig ldapConfig, Flags flags) { Logger = logger; Flags = flags; LDAPUtils = new LdapUtils(); LDAPUtils.SetLdapConfig(ldapConfig); CancellationTokenSource = new CancellationTokenSource(); + AdminSDHolderHash = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/BaseContext.cs(1 hunks)src/Client/Context.cs(1 hunks)src/Producers/LdapProducer.cs(4 hunks)src/Runtime/ObjectProcessors.cs(6 hunks)
🔇 Additional comments (6)
src/BaseContext.cs (1)
125-128: LGTM!The property addition is clean and follows the existing code patterns. The XML documentation clearly describes its purpose.
src/Client/Context.cs (1)
80-84: LGTM!The interface extension is properly documented and consistent with the implementation in
BaseContext.cs.src/Runtime/ObjectProcessors.cs (2)
137-161: LGTM!The AdminSDHolder protection check is properly implemented with appropriate null checks and only executes when ACL collection is enabled.
214-236: Good consistency with User processingThe implementation correctly mirrors the User object processing, maintaining consistency across security principal types.
src/Producers/LdapProducer.cs (2)
58-75: LGTM!The AdminSDHolder query setup is correct with appropriate attributes and security descriptor inclusion.
111-124: Good error handlingThe error handling appropriately logs failures without interrupting the collection process for other domains.
JimSycurity
force-pushed
the
jsykora/BED-6178
branch
from
4427104 to
6dccfac
Compare
JimSycurity
changed the title
Description
Resolves BED-6221
Motivation and Context
This code expands upon the SharpHoundCommon work in SpecterOps/SharpHoundCommon#230 to produce the
adminsdholderprotectedproperty on user, computer, and group nodes as part of the AdminSDHolder design which will then enable graph expansion for AdminSDHolder in BloodHound and ultimately better findings in BHE.In order to determine if an object is AdminSDHolder protected, my research has determined that the most reliable method across all scenarios is to compare the authoritative security descriptor of the AdminSDHolder container for the domain with the authoritative SD of security principals in the domain. When there is a match, the object is effectively protected.
Issue: https://specterops.atlassian.net/browse/BED-6178
This PR should ideally correspond to a BHCE and BHE version update with the graph expansion changes.
How Has This Been Tested?
A build of this code with these changes and the underlying SHC updates was tested in my home lab environment using multiple AD forests configured for AdminSDHolder research.
A SharpHound FOSS build with these changes and the underlying SHC updates was also tested by Jonas in the Dev GOAD Azure lab.
Data from these captures has been ingested into both BHCE and BHE dev instances based on my BED-6178 branches. Data ingestion has been consistent and appropriate.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit