-
Notifications
You must be signed in to change notification settings - Fork 220
Add local privilege collection #184
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
base: 2.X
Are you sure you want to change the base?
Conversation
WalkthroughThis PR adds support for a new collection method called GPOUserRights. The changes include updating the enum definition, CLI help text, PowerShell parameter validation, and runtime processing logic to handle this new collection method, with specific processing for OU objects. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Runtime/ObjectProcessors.cs (1)
365-365: Remove commented-out code.These commented-out code blocks should be removed to maintain code cleanliness. If these represent alternative implementations being evaluated, consider using feature flags or creating a separate development branch.
Apply this diff to remove the commented-out code:
if (_methods.HasFlag(CollectionMethod.SmbInfo)) { ret.SmbInfo = await _smbProcessor.Scan(apiName, resolvedSearchResult.DomainSid); - //ret.SmbInfo = await _smbProcessor.Scan(apiName); }if (ldapServices.IsSigningRequired.Collected) { ret.Properties.Add("ldapsigning", ldapServices.IsSigningRequired.Result); } - //var ldapServices = await dcLdapProcessor.Scan(resolvedSearchResult.DisplayName); - //ret.Properties.Add("ldapavailable", ldapServices.HasLdap); - //ret.Properties.Add("ldapsavailable", ldapServices.HasLdaps); - //if (ldapServices.IsChannelBindingDisabled.Collected) { - // ret.Properties.Add("ldapsepa", !ldapServices.IsChannelBindingDisabled.Result); - //} - - //if (ldapServices.IsSigningRequired.Collected) { - // ret.Properties.Add("ldapsigning", ldapServices.IsSigningRequired.Result); - //} }Also applies to: 431-440
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)src/Client/Enums.cs(1 hunks)src/Options.cs(2 hunks)src/PowerShell/Template.ps1(1 hunks)src/Runtime/ObjectProcessors.cs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Options.cs (3)
src/BaseContext.cs (1)
CollectionMethod(95-102)src/Client/Context.cs (1)
CollectionMethod(75-75)src/Extensions.cs (1)
CollectionMethod(114-119)
🔇 Additional comments (7)
src/Client/Enums.cs (1)
22-22: LGTM! Enum member positioned logically.The new
GPOUserRightsenum member is appropriately placed alongside other GPO-related collection methods.README.md (1)
38-38: LGTM! Documentation updated correctly.The CLI help text accurately reflects the new collection method.
src/PowerShell/Template.ps1 (1)
30-30: LGTM! PowerShell parameter documentation updated.The description clearly explains the new collection method's purpose.
src/Options.cs (2)
17-17: LGTM! Help text includes new collection method.The help text correctly lists GPOUserRights among available collection methods.
199-199: Mapping is correct; composite method verification is external to this file.The enum mapping at line 199 correctly maps
CollectionMethodOptions.GPOUserRightstoCollectionMethod.GPOUserRights, consistent with all other option mappings in the switch expression.However, verification of whether
GPOUserRightsis appropriately included in composite methods (Default,All,DCOnly) is not applicable here. Those composite values are defined in the externalSharpHoundCommonLib.Enums.CollectionMethodenum, not in this file. The code inOptions.cssimply maps individual user-specified options to their corresponding enum values using bitwise OR operations—it does not construct or define composite methods.The mapping itself contains no issues: it handles the new
GPOUserRightsoption consistently with existing collection methods.src/Runtime/ObjectProcessors.cs (2)
35-35: LGTM! Processor field and initialization follow established pattern.The
GPOUserRightsAssignmentProcessoris correctly declared and initialized, mirroring the approach used forGPOLocalGroupProcessor.Also applies to: 60-60
615-617: Verify OU class has GPOUserRights property.The implementation correctly processes GPO user rights for OUs when the collection method is enabled. Ensure that the
OUclass (in SharpHoundCommonLib.OutputTypes) has a correspondingGPOUserRightsproperty defined to receive this data.
Description
This pull request adds the ability to SharpHound to collect user privileges using GPOs and increases of the scope of the collected privileges.
Motivation and Context
This PR follows SpecterOps/BloodHound#1999.
How Has This Been Tested?
The changes have been tested manually on a lab environment.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit