add support for WriteLogonScript #259
Conversation
WalkthroughSupport for a new ACE type "WriteLogonScript" is implemented by adding edge name and GUID constants, then extending the ACL processor to recognize and emit corresponding ACE edges when processing User objects. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CommonLib/Enums/EdgeNames.cs(1 hunks)src/CommonLib/Processors/ACEGuids.cs(1 hunks)src/CommonLib/Processors/ACLProcessor.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/CommonLib/Processors/ACLProcessor.cs (3)
src/CommonLib/Processors/ACEGuids.cs (1)
ACEGuids(3-25)src/CommonLib/OutputTypes/ACE.cs (1)
ACE(4-38)src/CommonLib/Enums/EdgeNames.cs (1)
EdgeNames(3-33)
🔇 Additional comments (2)
src/CommonLib/Enums/EdgeNames.cs (1)
21-21: LGTM!The edge name constant follows the established naming convention and is appropriately positioned among similar write permission edge types.
src/CommonLib/Processors/ACEGuids.cs (1)
13-13: The schema GUID for the scriptPath attribute is bf9679a8-0de6-11d0-a285-00aa003049e2, which matches exactly the value in theWriteLogonScriptconstant. The GUID is correct.
| else if (objectType == Label.User && aceType == ACEGuids.WriteLogonScript) | ||
| yield return new ACE | ||
| { | ||
| PrincipalType = resolvedPrincipal.ObjectType, | ||
| PrincipalSID = resolvedPrincipal.ObjectIdentifier, | ||
| IsInherited = inherited, | ||
| RightName = EdgeNames.WriteLogonScript, | ||
| InheritanceHash = aceInheritanceHash, | ||
| IsPermissionForOwnerRightsSid = isPermissionForOwnerRightsSid, | ||
| IsInheritedPermissionForOwnerRightsSid = isInheritedPermissionForOwnerRightsSid, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Implementation correctly follows established ACE processing patterns.
The WriteLogonScript ACE handling mirrors the existing WriteSPN implementation and properly includes all necessary properties (inheritance tracking, owner rights handling, etc.). The logic is correctly scoped to User objects only, which aligns with the scriptPath attribute's applicability.
However, please verify that corresponding tests and documentation have been added for this new edge type. Based on the PR objectives, the checklist items for tests and documentation are not marked as complete.
🏁 Script executed:
#!/bin/bash
# Description: Search for existing tests for similar ACE types (WriteSPN) to identify test patterns
# Expected: Find test files that should be updated to include WriteLogonScript coverage
echo "=== Searching for WriteSPN tests that should be replicated for WriteLogonScript ==="
rg -i "WriteSPN" --type cs -g "*test*" -C 3
echo -e "\n=== Checking if WriteLogonScript tests exist ==="
rg -i "WriteLogonScript" --type cs -g "*test*" -C 3
echo -e "\n=== Searching for ACLProcessor test files ==="
fd -e cs -i "test" | rg -i "acl"Length of output: 508
I found the test file. Let me search it directly for existing test patterns:
</script>🏁 Script executed:
#!/bin/bash
# Search the test file for WriteSPN tests (the reference pattern)
echo "=== WriteSPN tests in ACLProcessorTest.cs ==="
rg -i "WriteSPN" test/unit/ACLProcessorTest.cs -C 5
echo -e "\n=== WriteLogonScript tests in ACLProcessorTest.cs ==="
rg -i "WriteLogonScript" test/unit/ACLProcessorTest.cs -C 5
echo -e "\n=== Checking implementation file for documentation ==="
rg -i "WriteLogonScript" src/CommonLib/Processors/ACLProcessor.cs -C 3Length of output: 6071
Add unit tests and documentation for WriteLogonScript ACE handling.
WriteLogonScript tests are missing from test/unit/ACLProcessorTest.cs. WriteSPN—the referenced pattern—has multiple test cases (ACLProcessor_ProcessACL_GenericAll_WriteSPN, ACLProcessor_ProcessACL_GenericWrite_User_WriteSPN, etc.). Add equivalent test coverage for WriteLogonScript, including GenericAll and GenericWrite scenarios with User object type. Additionally, add inline documentation comments to the implementation at lines 759-769 in src/CommonLib/Processors/ACLProcessor.cs to align with established code documentation practices.
🤖 Prompt for AI Agents
In src/CommonLib/Processors/ACLProcessor.cs around lines 759 to 769, add an
inline XML comment above the else-if handling ACEGuids.WriteLogonScript that
documents the mapping: for User object type the ACEGuids.WriteLogonScript maps
to EdgeNames.WriteLogonScript and the produced ACE sets PrincipalType,
PrincipalSID, IsInherited, RightName, InheritanceHash,
IsPermissionForOwnerRightsSid, and IsInheritedPermissionForOwnerRightsSid; in
test/unit/ACLProcessorTest.cs add unit tests mirroring the existing WriteSPN
cases (e.g. ACLProcessor_ProcessACL_GenericAll_WriteLogonScript,
ACLProcessor_ProcessACL_GenericWrite_User_WriteLogonScript and any other
parallel scenarios), following the same arrangements and assertions as the
WriteSPN tests to verify the ACE is yielded and all fields (PrincipalType,
PrincipalSID, IsInherited, RightName == EdgeNames.WriteLogonScript,
InheritanceHash and the owner-rights flags) match expected values.
1 participant
Description
This pull request adds the ability to SharpHound to collect write privilege over a user's scriptPath property.
Motivation and Context
This PR follows SpecterOps/BloodHound#2046.
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