feat: AdminSDHolder hash functions - BED-6242 #230
Conversation
WalkthroughThe changes introduce new functionality in the ACLProcessor to detect AdminSDHolder protection by hashing implicit ACEs and comparing them to a reference hash. Supporting methods and an internal class for ACE hashing are added, with new tests verifying hash calculation and protection detection. Minor whitespace and formatting adjustments are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ACLProcessor
participant SecurityDescriptor
Caller->>ACLProcessor: IsAdminSDHolderProtected(securityDescriptor, adminSdHolderHash)
ACLProcessor->>ACLProcessor: CalculateImplicitACLHash(securityDescriptor)
ACLProcessor->>SecurityDescriptor: Extract ACEs and DACL protection
SecurityDescriptor-->>ACLProcessor: ACEs, DACL protection
ACLProcessor->>ACLProcessor: Hash ACEs + DACL protection
ACLProcessor-->>Caller: Compare hash to adminSdHolderHash, return true/false/null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes 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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ 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 (
|
definitelynotagoblin
force-pushed
the
jsykora/BED-6178
branch
from
d575b2a to
015f099
Compare
![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 (4)
test/unit/ACLProcessorTest.cs (1)
1435-1459: Simplify test setup and remove unused variables.While this test correctly validates that different inputs produce unique hashes, it has some issues:
- Unused variables:
expectedPrincipalType,expectedPrincipalSID, andexpectedRightNameare defined but not used in assertions- Overly complex mock setup: For testing hash uniqueness, the extensive mocking seems unnecessary
Consider simplifying to focus on the core hash functionality:
[WindowsOnlyFact] public void ACLProcessor_CalculateImplicitACLHash_DifferentInputs_ProducesUniqueHashes() { // Setup - var expectedPrincipalType = Label.CertTemplate; - var expectedPrincipalSID = "S-1-5-21-3130019616-2776909439-2417379446-512"; - var expectedRightName = EdgeNames.WritePKINameFlag; var mockLDAPUtils = new Mock<ILdapUtils>(); var sd = new ActiveDirectorySecurityDescriptor(new ActiveDirectorySecurity()); mockLDAPUtils.Setup(x => x.MakeSecurityDescriptor()).Returns(sd); - mockLDAPUtils.Setup(x => x.ResolveIDAndType(It.IsAny<string>(), It.IsAny<string>())) - .ReturnsAsync((true, new TypedPrincipal(expectedPrincipalSID, expectedPrincipalType))); - mockLDAPUtils.Setup(x => x.PagedQuery(It.IsAny<LdapQueryParameters>(), It.IsAny<CancellationToken>())) - .Returns(Array.Empty<LdapResult<IDirectoryObject>>().ToAsyncEnumerable); var proc = new ACLProcessor(mockLDAPUtils.Object);src/CommonLib/Processors/ACLProcessor.cs (3)
294-304: Consider optimizing the sorting approach for large ACLsThe TODO comment correctly identifies potential inefficiency. Creating intermediate objects and sorting with multiple criteria could be expensive for objects with many ACEs.
Consider these optimizations:
- Use a custom IComparer to avoid multiple sorting passes
- Consider serializing directly to bytes instead of string concatenation
- Use a more memory-efficient approach for large ACLs
Example custom comparer:
private class ACEForHashingComparer : IComparer<ACEForHashing> { public int Compare(ACEForHashing x, ACEForHashing y) { int result = x.AccessControlType.CompareTo(y.AccessControlType); if (result != 0) return result; result = string.Compare(x.IdentityReference, y.IdentityReference, StringComparison.Ordinal); if (result != 0) return result; // ... continue for other properties return result; } }
312-312: Consider basing StringBuilder capacity on actual ACE string lengthThe hardcoded estimate of 50 characters per ACE might be too low or too high depending on the actual data.
Consider calculating a more accurate estimate:
-var stringBuilder = new StringBuilder(sortedAces.Count * 50); // Pre-allocate with estimated capacity TODO: is this a good estimate? +// Calculate more accurate capacity based on first ACE or use a conservative estimate +var estimatedCapacity = sortedAces.Count > 0 ? sortedAces[0].ToString().Length * sortedAces.Count * 1.2 : 1024; +var stringBuilder = new StringBuilder((int)estimatedCapacity);
847-847: Address TODO: Extract duplicated CA rights checks to shared methodThe TODO comment indicates code duplication between this method and ProcessRegistryEnrollmentPermissions.
Would you like me to help refactor these CA rights checks into a shared method to eliminate the duplication and improve maintainability?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/CommonLib/AdaptiveTimeout.cs(1 hunks)src/CommonLib/Processors/ACLProcessor.cs(6 hunks)src/CommonLib/Processors/LdapPropertyProcessor.cs(3 hunks)test/unit/ACLProcessorTest.cs(25 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/CommonLib/AdaptiveTimeout.cs (2)
Learnt from: definitelynotagoblin
PR: #222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
Learnt from: definitelynotagoblin
PR: #217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/CommonLib/AdaptiveTimeout.cs (1)
323-323: LGTM: Minor whitespace cleanup in comment block.The removal of the trailing blank line in the comment example improves consistency without affecting functionality.
src/CommonLib/Processors/LdapPropertyProcessor.cs (3)
123-123: LGTM: Formatting improvement.Removed extra space in the if condition for better code consistency.
368-370: LGTM: AdminCount property addition supports AdminSDHolder detection.The addition of the
admincountproperty with boolean conversion (ac != 0) correctly implements the LDAP AdminCount attribute retrieval for computer objects. This aligns with the PR's objective to support AdminSDHolder protection detection functionality.
945-945: LGTM: Minor whitespace cleanup.Removed unnecessary blank line in the enum declaration area for better code consistency.
test/unit/ACLProcessorTest.cs (2)
35-36: LGTM! AdminSDHolder test fixture properly defined.The new
AdminSDHolderSecurityDescriptorconstant follows the established pattern of other security descriptor constants in the test file and provides necessary test data for the AdminSDHolder functionality.
1417-1433: LGTM! Well-structured test for hash calculation.This test method effectively validates the
CalculateImplicitACLHashfunctionality with:
- Clear arrange-act-assert structure
- Appropriate use of
[WindowsOnlyFact]for platform-specific functionality- Proper mock setup and assertion against expected SHA1 hash value
src/CommonLib/Processors/ACLProcessor.cs (1)
47-63: Add null safety to ToString() methodThe
ToString()method doesn't handle null values forIdentityReference,ObjectType, orInheritedObjectType. This could lead to null reference exceptions during hash calculation.Apply this diff to add null safety:
public override string ToString() { - return $"{IdentityReference}|{Rights}|{AccessControlType}|{ObjectType}|{InheritedObjectType}|{InheritanceFlags}"; + return $"{IdentityReference ?? ""}|{Rights}|{AccessControlType}|{ObjectType ?? ""}|{InheritedObjectType ?? ""}|{InheritanceFlags}"; }Likely an incorrect or invalid review comment.
JimSycurity
force-pushed
the
jsykora/BED-6178
branch
from
b1fad1f to
18641e5
Compare
mykeelium
left a comment
mykeelium
left a comment
There was a problem hiding this comment.
Looks excellent!! Great work on this, it is fantastic how much you put into this
- Add class to represent ACE for hashing - Add method to determine if the SD hash of the provided object matches the AdminSDHolder SD hash and set 'adminsdholderprotected' if true - Add method to calculate SHA1 hash of authoratative SD - Add adminCount property to Comptuer nodes - Add mock tests Resolves BED-6221
JimSycurity
force-pushed
the
jsykora/BED-6178
branch
from
0bf150c to
4cc4846
Compare
|
Resolves BED-6242 |
JimSycurity
changed the title
Resolves BED-6221
Description
These SharpHoundCommon feature updates implement features required by SharpHound and SharpHoundEnterprise to produce the
adminsdholderprotectedproperty on user, computer, and group nodes as part of the AdminSDHolder design.Adds 2 methods to ACLProcessor class:
adminsdholderprotectedshould be set to true or falseAdded tests to ACLProcessorTests.
Added
admincountproperty to computer nodes as computers and computer-derived objects like (g/d)MSAs can be protected by AdminSDHolder.Motivation and Context
This code lays the foundation for SharpHound and SharpHoundEnterprise to set the
adminsdholderprotectedproperty, which will then enable graph expansion for AdminSDHolder in BloodHound and ultimately better findings in BHE.Issue: https://specterops.atlassian.net/browse/BED-6178
This PR needs to be merged into a SharpHoundCommon version update prior to corresponding PRs for SharpHound and SharpHoundEnterprise being merged. Those PRs should correspond to a BHCE and BHE version update with the graph expansion changes.
How Has This Been Tested?
Code was first tested using debug functionality in Rider.
Code was tested as part of custom builds of both SharpHound and SharpHound Enterprise from the corresponding BED-6178 branches in my home lab environment using multiple AD forests configured for AdminSDHolder research. Code was also tested as part of SharpHound FOSS by Jonas in the Dev GOAD Azure lab.
Tests in ACLProcessorTests are largely Windows-specific. All tests were performed on a Windows 11 PC in Rider and via dotnet test:
All tests were also performed in my WSL2 dev instance, but windows-specific tests were skipped:
I also tested this SharpHoundCommon version with the main branch of SharpHound FOSS of version 2.6.7 before rebasing to 2.7.0. In that test in my lab environment there were no negative effects of running this SHC version with a SharpHound FOSS version that does not explicitly support it as none of the methods were called.
I also tested SharpHoundEnterprise with a bhe-dev environment running on my BED-6178 branch. The SHDelegator is running in my home lab and provided accurate data to the bhe-dev build.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests