BloodHoundAD / SharpHoundCommon

Common library used by SharpHound.
GNU General Public License v3.0
75 stars 47 forks source link

Fix BUG #70 #71

Open godylockz opened 12 months ago

godylockz commented 12 months ago

Description

See BUG #70

Motivation and Context

Properly make the "AddSelf" edge

How Has This Been Tested?

Tested on a OU that has the AddSelf edge is created if you grant "Add/remove self as member" only, but if you select "All validated writes", then the edge is not created.

Types of changes

The self ACL does not require the ACL for WriteMember.

github-actions[bot] commented 12 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

godylockz commented 12 months ago

I have read the CLA Document and I hereby sign the CLA

JonasBK commented 12 months ago

Thanks for your contribution @godylockz!

I think the fix could produce false positives if a principal has been granted one of the other validated writes such as Validated-DNS-Host-Name.

Hence we should check if the principal has been granted Self-Membership specifically or all validated writes.

But I think it would make sense to introduce a new edge for all validated writes instead. I will confirm that with the team and get back to you.

godylockz commented 12 months ago

I will be awaiting your response, thank you!

In the meantime if anyone feels the need to use these new binaries, posting them here. SharpHound-SelfMembership.zip

Just FYI, I couldn't find much documentation on how to compile SharpHoundCommon with SharpHound. Obviously had to uncomment and fetch the new version of the .dll file.

<!--        <Reference Include="SharpHoundCommonLib, Version=2.0.15.0, Culture=neutral, PublicKeyToken=null">-->
<!--            <HintPath>..\SharpHoundCommon\src\CommonLib\bin\Debug\net462\SharpHoundCommonLib.dll</HintPath>-->
<!--        </Reference>-->
JonasBK commented 7 months ago

Hi @godylockz,

Sorry for having you waiting so long.

I have talked to the team and we have decided that it would make the most sense to implement a new edge type named AllValidatedWrites to cover the scenario where a principal has an ACE with ActiveDirectoryRights : Self and ObjectType : 00000000-0000-0000-0000-000000000000 (All) on Groups, as that is a different from having the specific permission of AddSelf.

Would you be up for updating your PR to create that edge and create the necessary BloodHound change as well? I will be available to guide you :)