Add local privilege collection #256
Conversation
WalkthroughThis pull request adds GPO-based user rights collection capability to the codebase. Changes include a new collection method enum value, expanded privilege definitions, updated LDAP query generation, new output types for storing GPO user rights data, and a processor that extracts privilege assignments from Group Policy template files and maps them to affected computers. Changes
Sequence DiagramsequenceDiagram
actor Consumer
participant GOURP as GPOUserRights<br/>Processor
participant LDAP as LDAP Utils
participant FS as File System
participant Cache as GPO Cache
Consumer->>GOURP: ReadGPOUserRights(entry)
GOURP->>LDAP: Extract GPLink & OUs
GOURP->>LDAP: Query computers under OU
alt No computers found
GOURP-->>Consumer: Return empty result
end
GOURP->>GOURP: Parse GPLink (unenforced + enforced)
loop For each GPO link
GOURP->>Cache: Check cached privileges
alt Not cached
GOURP->>FS: Read GptTmpl.inf
GOURP->>GOURP: Parse [Privilege Rights]
loop For each privilege entry
GOURP->>LDAP: Resolve SIDs/accounts
GOURP->>GOURP: Yield PrivilegeAction
end
GOURP->>Cache: Store results
end
end
GOURP->>GOURP: Aggregate privileges (last-write-wins)
GOURP->>Consumer: Return ResultingGPOUserRights
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Multiple files with diverse responsibilities (enums, query generation, output types, complex processor logic) requiring separate reasoning for each area. The GPOUserRightsAssignmentProcessor introduces substantial new logic for template file parsing, SID resolution, caching, and aggregation semantics that demands careful review. Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (1)
60-60: Remove trailing whitespace.The method signature has trailing whitespace that should be removed for consistency with coding standards.
src/CommonLib/Processors/GPOUserRightsProcessor.cs (1)
125-159: Consider caching LDAP query failures to avoid repeated retries.Lines 139-141 skip caching when LDAP queries fail, causing the same GPO to be re-queried on subsequent calls. While not a correctness issue, caching failed results could improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/CommonLib/Enums/CollectionMethod.cs(1 hunks)src/CommonLib/Enums/LSAPrivileges.cs(1 hunks)src/CommonLib/LdapProducerQueryGenerator.cs(2 hunks)src/CommonLib/LdapQueries/CommonProperties.cs(1 hunks)src/CommonLib/OutputTypes/OU.cs(1 hunks)src/CommonLib/OutputTypes/ResultingGPOUserRights.cs(1 hunks)src/CommonLib/Processors/GPOLocalGroupProcessor.cs(1 hunks)src/CommonLib/Processors/GPOUserRightsProcessor.cs(1 hunks)src/CommonLib/Processors/LdapPropertyProcessor.cs(1 hunks)src/CommonLib/Processors/UserRightsAssignmentProcessor.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (1)
src/CommonLib/OutputTypes/ResultingGPOChanges.cs (1)
ResultingGPOChanges(5-12)
src/CommonLib/OutputTypes/OU.cs (3)
src/CommonLib/OutputTypes/OutputBase.cs (1)
OutputBase(9-17)src/CommonLib/OutputTypes/ResultingGPOChanges.cs (1)
ResultingGPOChanges(5-12)src/CommonLib/OutputTypes/ResultingGPOUserRights.cs (1)
ResultingGPOUserRights(6-13)
src/CommonLib/OutputTypes/ResultingGPOUserRights.cs (1)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (1)
TypedPrincipal(514-519)
src/CommonLib/LdapQueries/CommonProperties.cs (1)
src/CommonLib/Enums/LDAPProperties.cs (1)
LDAPProperties(3-99)
src/CommonLib/Enums/LSAPrivileges.cs (1)
src/CommonLib/Impersonate.cs (1)
Impersonate(131-164)
src/CommonLib/Processors/LdapPropertyProcessor.cs (1)
src/CommonLib/LdapQueries/CommonProperties.cs (1)
CommonProperties(3-105)
src/CommonLib/LdapProducerQueryGenerator.cs (1)
src/CommonLib/LdapQueries/CommonProperties.cs (1)
CommonProperties(3-105)
src/CommonLib/Processors/GPOUserRightsProcessor.cs (5)
src/CommonLib/Logging/Logging.cs (3)
Logging(7-20)ILogger(31-34)LogProvider(22-35)src/CommonLib/OutputTypes/ResultingGPOUserRights.cs (1)
ResultingGPOUserRights(6-13)src/CommonLib/Enums/LDAPProperties.cs (1)
LDAPProperties(3-99)src/CommonLib/Helpers.cs (1)
DistinguishedNameToDomain(135-151)src/CommonLib/LdapQueries/CommonProperties.cs (1)
CommonProperties(3-105)
🔇 Additional comments (15)
src/CommonLib/Enums/CollectionMethod.cs (1)
30-30: LGTM! GPOUserRights flag is correctly integrated.The new collection method flag is properly defined with a unique bit position (1 << 24) and correctly included in both the
DCOnlyandAllcomposite flags, following the established pattern for GPO-related collection methods.Also applies to: 35-35, 40-41
src/CommonLib/Processors/LdapPropertyProcessor.cs (1)
34-34: LGTM! Reserved attribute registration is correct.The GPOUserRights property is properly registered as a reserved attribute, ensuring it's excluded from generic property parsing. This follows the established pattern for special-purpose LDAP properties.
src/CommonLib/Processors/UserRightsAssignmentProcessor.cs (1)
216-216: Formatting-only change.This is a minor formatting adjustment with no functional impact.
src/CommonLib/OutputTypes/OU.cs (1)
9-9: LGTM! New field follows established pattern.The
GPOUserRightsfield is properly added alongside the existingGPOChangesfield, maintaining consistency in how GPO-related results are stored in OUs.src/CommonLib/LdapQueries/CommonProperties.cs (1)
89-91: LGTM! Property set correctly defined.The
GPOUserRightsproperty array is properly defined with the necessary LDAP properties (GPLinkandName). Using the same properties asGPOLocalGroupPropsis appropriate since both features read from GPO configurations.src/CommonLib/LdapProducerQueryGenerator.cs (2)
47-48: LGTM! GPOUserRights correctly integrated into comprehensive collection path.The GPOUserRights properties are properly added when the flag is set, following the same pattern as GPOLocalGroup.
86-89: LGTM! GPOUserRights correctly integrated into targeted collection path.The filter and properties are properly configured for GPOUserRights-only collection, mirroring the established pattern for GPOLocalGroup collection.
src/CommonLib/OutputTypes/ResultingGPOUserRights.cs (1)
6-13: LGTM! Well-designed data container.The
ResultingGPOUserRightsclass provides a clean structure for GPO user rights collection results. The design follows the established pattern ofResultingGPOChanges, with appropriate default initializations to prevent null reference issues. The inline comment helpfully clarifies the dictionary's purpose.src/CommonLib/Enums/LSAPrivileges.cs (1)
50-50: LGTM: Privilege expansion aligns with GPO user rights collection.The expanded privilege set includes security-critical rights appropriate for monitoring and analysis. All referenced constants are properly defined.
src/CommonLib/Processors/GPOUserRightsProcessor.cs (6)
17-36: LGTM: Well-structured processor initialization.The use of compiled regex patterns, thread-safe caching, and dependency injection with logger fallback follows best practices.
72-98: LGTM: Good optimization to check for affected computers early.Querying computers first and returning early if none exist avoids unnecessary GPO processing.
100-118: GPLink ordering correctly implements precedence rules.The code properly orders unenforced GPOs before enforced ones, ensuring enforced GPOs override unenforced ones with the last-write-wins processing at line 175.
196-266: LGTM: Template file parsing correctly handles GptTmpl.inf format.The method properly extracts privilege rights sections, parses the INI-style key=value format, handles asterisk-prefixed SIDs, and resolves principals with appropriate error handling.
268-304: LGTM: SID resolution handles multiple account formats correctly.The method appropriately handles DOMAIN\user format, plain usernames, computer account fallback (with $ suffix), and direct SID resolution.
182-185: No issues found—TypedPrincipal equality implementation correctly supports Distinct().The
TypedPrincipalclass properly implements bothEquals()andGetHashCode()with consistent logic. TheEquals()method comparesObjectIdentifierandObjectType, and theGetHashCode()method combines their hashes using a standard pattern. This implementation correctly supports deduplication viaDistinct().
| @@ -1,10 +1,12 @@ | |||
| using System; | |||
| using SharpHoundCommonLib.Processors; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary using directive.
The using SharpHoundCommonLib.Processors directive appears unused. ResultingGPOUserRights is in the SharpHoundCommonLib.OutputTypes namespace (same as this file), so this using directive is not needed.
🤖 Prompt for AI Agents
In src/CommonLib/OutputTypes/OU.cs around lines 1 to 1, remove the unused using
directive "using SharpHoundCommonLib.Processors" because ResultingGPOUserRights
resides in the same SharpHoundCommonLib.OutputTypes namespace; simply delete
that using line from the top of the file so only necessary directives remain.
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Extensions.Logging; | ||
| using SharpHoundCommonLib.Enums; | ||
| using SharpHoundCommonLib.LDAPQueries; | ||
| using SharpHoundCommonLib.OutputTypes; | ||
| using System.DirectoryServices.Protocols; | ||
| using System.Drawing.Printing; | ||
|
|
There was a problem hiding this comment.
Remove unused import.
The System.Drawing.Printing namespace (Line 13) is unused and unrelated to GPO user rights processing.
Apply this diff:
using System.DirectoryServices.Protocols;
-using System.Drawing.Printing;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using System; | |
| using System.Collections.Concurrent; | |
| using System.Collections.Generic; | |
| using System.IO; | |
| using System.Linq; | |
| using System.Text.RegularExpressions; | |
| using System.Threading.Tasks; | |
| using Microsoft.Extensions.Logging; | |
| using SharpHoundCommonLib.Enums; | |
| using SharpHoundCommonLib.LDAPQueries; | |
| using SharpHoundCommonLib.OutputTypes; | |
| using System.DirectoryServices.Protocols; | |
| using System.Drawing.Printing; | |
| using System; | |
| using System.Collections.Concurrent; | |
| using System.Collections.Generic; | |
| using System.IO; | |
| using System.Linq; | |
| using System.Text.RegularExpressions; | |
| using System.Threading.Tasks; | |
| using Microsoft.Extensions.Logging; | |
| using SharpHoundCommonLib.Enums; | |
| using SharpHoundCommonLib.LDAPQueries; | |
| using SharpHoundCommonLib.OutputTypes; | |
| using System.DirectoryServices.Protocols; | |
🤖 Prompt for AI Agents
In src/CommonLib/Processors/GPOUserRightsProcessor.cs around lines 1 to 14,
remove the unused and unrelated using directive for System.Drawing.Printing
(line 13). Edit the top-of-file using list to delete that import so only
required namespaces remain; run a build or IDE analyzer to confirm no other
references to that namespace exist.
1 participant
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: