Skip to content
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

Fix BUG #70 #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix BUG #70 #71

wants to merge 1 commit into from

Conversation

godylockz
Copy link

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
Copy link

github-actions bot commented Sep 13, 2023

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

@godylockz
Copy link
Author

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

@JonasBK
Copy link
Collaborator

JonasBK commented Sep 13, 2023

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
Copy link
Author

godylockz commented Sep 13, 2023

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
Copy link
Collaborator

JonasBK commented Jan 23, 2024

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 :)

leechristensen pushed a commit to leechristensen/SharpHoundCommon-1 that referenced this pull request Jul 12, 2024
…update

BED-3868: Build contains edges from DNs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants