Add PKI (ADCS) support #8

P-aLu · 2024-03-30T14:23:36Z

Add PKI (ADCS) support

Merged Certipy LDAP parsing

Related to Tw1sm/pyldapsearch#2 and trustedsec/CS-Situational-Awareness-BOF#118

Tw1sm · 2024-04-03T17:56:27Z

Hey thanks for the PR! Do the changes support BloodHound Community Edition (BHCE)? I know Certipy's BloodHound output worked with a fork of legacy BloodHound, but it's been a while since I examined either

…harphound)

P-aLu · 2024-04-08T08:39:16Z

Hello ! Thank you for your response.

You were right, the data was not corresponding to Sharphound (2024 April) data.

So I did update all data returned by Bofhound in order to make it correspond to the new data schema.

The output is now closer to SharpHound output and works in BH CE.

Tw1sm · 2024-05-30T15:08:25Z

Starting to review this. When I run the changes, it instantly throws this errror KeyError: 'service-principal-name'. Looks like line 775 in adds.py - is there a reason for the two changes to the parse_acl method?

Tw1sm · 2024-05-30T16:05:17Z

Planning to give the actual file changes a thorough review today/tomorrow, there are a lot of changes and after a first glance I don't understand the reason behind some of them (mostly changes to class definitions otherwise unaffected by ADCS integration). However this looks like a great start towards ADCS integration - thanks again for the PR.

Notes from initial comparison with SharpHound ADCS collection

(I'm using the GOAD lab essos.local domain that ships with a CA to test against)

SharpHound collection uploaded to BHCE shows the object counts below on the data quality page

Bofhound JSON files are currently only producing CertTemplate objects when uploaded

I've found this is usually indicative of expected attributes missing from objects in the JSON output. We should currently have AIACA, RootCA, EnterpriseCA, and CertTemplate objects getting loaded. Currently we're missing classes for IssuancePolicy and NTAuthStore objects.

bofhound/__main__.py

bofhound/ad/models/bloodhound_pki.py

Tw1sm · 2024-05-30T18:41:57Z

bofhound/ad/models/bloodhound_user.py

+            ### Not supported for the moment
+            self.Properties['hasspn'] = None
+            self.Properties['displayname'] = None
+            self.Properties['email'] = None
+            self.Properties['title'] = None
+            self.Properties['homedirectory'] = None
+            self.Properties['userpassword'] = None
+            self.Properties['unixpassword'] = None
+            self.Properties['unicodepassword'] = None
+            self.Properties['sfupassword'] = None
+            self.Properties['logonscript'] = None
+            self.Properties['sidhistory'] = []


hasspn is supported in the code above and this is always setting it to null in the JSON output

Is there a reason these properties are being explicitly set to null? Last I checked data was ingested fine without inclusion. If we want to add them here, we should support them and pull the value from the logs, otherwise we should just delete.

Yeah you are right. Moreover, hasspn value is set up earlier during the parsing, that's my bad.

For the rest, I wanted to mention that work had to be done on this part, maybe let them commented ?

bofhound/ad/models/bloodhound_user.py

Tw1sm · 2024-05-30T18:45:20Z

bofhound/ad/models/bloodhound_user.py

+        'samaccountname', 'distinguishedname', 'isdeleted', 'msds-groupmsamembership',
+        'serviceprincipalname', 'displayname',
        'lastlogon', 'lastlogontimestamp', 'pwdlastset', 'mail', 'title',
        'homedirectory', 'description', 'userpassword', 'admincount',
        'msds-allowedtodelegateto', 'sidhistory', 'whencreated', 'unicodepwd', 'unixuserpassword',
        'domainsid', 'allowedtodelegate', 'name', 'domain', 'admincount',
        'highvalue', 'unconstraineddelegation', 'passwordnotreqd', 'enabled',
        'dontreqpreauth', 'sensitive', 'trustedtoauth', 'pwdneverexpires',
        'dontreqpreauth', 'pwdneverexpires', 'sensitive',
-        'serviceprincipalnames', 'hasspn', 'email', 'memberof'
+        'serviceprincipalnames', 'isaclprotected',
+        'hasspn', 'displayname', 'email', 'title', 'homedirectory', 'userpassword', 'unixpassword',
+        'unicodepassword', 'sfupassword', 'logonscript', 'sidhistory'


See note later in this file about the property modifications. I explicitly do wish to keep memberof as a common attribute (something I find personally helpful when manually & iteratively enumerating AD). If you removed this due to UI clog, that can be an issue and I would be open to adding a new CLI flag that turns off memberof and member (for group objects) attribute inclusion in common properties.

I understand your point. My goal was to give a result as close as possible to SharpHound one, because the "memberof" value can be oversizing the Bloodhound GUI. I'll add this again in that case.

We could add --gui-only flag that only exports necessary information for BloodHound GUI. What do you think ?

I agree, a flag to opt out of them would be a nice addition

bofhound/ad/models/bloodhound_pki.py

Tw1sm · 2024-05-31T16:02:18Z

@Palu08 I completed a review, I'm happy to collaborate to add some of the missing pieces, make these edits and also work in some test cases to help with future additions

Tw1sm · 2024-06-28T12:31:52Z

I need to go back and delete commit c122e93 (you might encounter an error with it). My test machine must've had an old bloodhoundpy installed or something when I added that commit. At any rate, the current version of bloodhoundpy seems to return the correct trust attributes now for BHCE

P-aLu · 2024-07-09T22:26:55Z

Hi @Tw1sm

Thank you for your review !

The commit 487176 should resolve all issues adressed.

I will now try to add the support of an option (maybe properties_level ?) to let user chose between all_properties,common_properties and gui_properties

P-aLu · 2024-07-09T23:30:25Z

Added --properties-level / -p option (default to 2 -- Common Properties) and reviewed users and computers attributes in c466e26 commit

Tw1sm · 2024-08-23T17:48:57Z

@P-aLu Sorry for the lack of activity on this PR; been busy the last few months. I actually used this fork to parse out ADCS objects on a recent red team. Super helpful to have.

I think the last thing we need to clean up is the ACL parsing of ADCS objects. I think you brought over a specific function from Certipy that specially parses the template/CA/etc object DACLs. I've noted the the ACL abuse paths over ADCS objects generated by this fork are extremely different from those produced by SharpHound. I think we should remove the function special to ADCS object parsing and modify the main ACL parsing method to include the logic SharpHound uses here: https://github.com/BloodHoundAD/SharpHoundCommon/blob/v4/src/CommonLib/Processors/ACLProcessor.cs

There is some stuff we likely won't be able to account for, such as edges/attribute relying on registry values queried from CAs, but I think we can get the ACLs generated by bofhound to align a little more closely than they are now.

… 2 only

Tw1sm · 2024-10-20T20:19:17Z

Finally got around to addressing the ACL parsing. Added some stuff and fixed some bugs:

Change cert template name properties to use DN instead of Display Name (was causing a mismatch on published template on enterprise CA objects)
Process enroll ACLs on Enterprise CA objects
Removed the bloodhound_security file and merged ADCS object ACL parsing into the main ACL parsing function
- Added check for WriteProperty ACLs specific to PKI-Name-Flag and PKI-Enrollment-Flag (for WritePKIEnrollmentFlag and WritePKINameFlag abuse edges)
- We don't have ManageCA or ManageCertificates edges, but I don't think these are relied upon for abuse edges, and I'm pretty sure these are queried from the CA's registry
Modified the --properties-level flag to have values validated with an enum
- Also switched memberof and member attributes on groups to not be included in the lowest properties level

I just need to fix up the test cases now and I think it's close enough to data pulled with SharpHound to merge. Adding some test cases for the ADCS parsing would be nice, but I'll tackle that at a later time I think.

Paul SALADIN added 5 commits March 19, 2024 13:21

update pyproject

2cb5742

update pyproject

05105e6

update pyproject

7356b40

Add PKI (ADCS) support

b8809fb

Changelog.md updated

b851b10

This was referenced Mar 30, 2024

Add ADCS support and '-scope' '-output' options Tw1sm/pyldapsearch#2

Merged

ldapsearch (LDAPS + more modular options) trustedsec/CS-Situational-Awareness-BOF#118

Merged

Modified data parsing to correspond to Data version 6 (2024 April - S…

e0a390f

…harphound)

Tw1sm self-assigned this May 26, 2024

Tw1sm added the enhancement New feature or request label May 26, 2024

Output enhancement

92bce9a

fix indexing error

ffad642

Tw1sm reviewed May 31, 2024

View reviewed changes

Tw1sm added 12 commits May 31, 2024 13:20

require python ^3.10 for match statement

e226ef8

update lockfile

121d95e

remove assertion using deprecated method

abb78e0

add ntauthstore, issuancepolicy types; reorg util files

dd26919

fix bug with domain properties and trust attributes

c122e93

reorg pki classes into separate files

994eae6

remove unneeded code

efd0ccb

set methods metadata to 0

20b1fcb

remove bloodhound_pki file

fe956a5

handle cACertificate attr with 2+ certs

154d903

fix properties for BHCE; build ca cert chains

01dbad6

rename bloodhound_pki_template to bloodhound_certtemplate

5af410e

Addressed issues mentionned through MR review

4587176

Review properties level

c466e26

Tw1sm added 6 commits October 19, 2024 22:21

remove bad import

4794182

merge ADCS ACL parsing into single func

9e17579

fix bug causing published templates to be missed

a6d999c

process enroll ACLs on EnterpriseCAs

380f8a1

change --properties_level to --properties-level

7fb82f6

validate properties level with enum; member attribues on groups level…

067a1ae

… 2 only

adjust adds test case due to container parsing

76aae0e

Tw1sm merged commit 9a999e4 into coffeegist:main Oct 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PKI (ADCS) support #8

Add PKI (ADCS) support #8

P-aLu commented Mar 30, 2024 •

edited

Loading

Tw1sm commented Apr 3, 2024

P-aLu commented Apr 8, 2024

Tw1sm commented May 30, 2024

Tw1sm commented May 30, 2024

Tw1sm May 30, 2024

P-aLu Jun 28, 2024

Tw1sm May 30, 2024

P-aLu Jun 28, 2024

Tw1sm Jun 28, 2024

Tw1sm commented May 31, 2024

Tw1sm commented Jun 28, 2024

P-aLu commented Jul 9, 2024 •

edited

Loading

P-aLu commented Jul 9, 2024

Tw1sm commented Aug 23, 2024

Tw1sm commented Oct 20, 2024 •

edited

Loading

Add PKI (ADCS) support #8

Add PKI (ADCS) support #8

Conversation

P-aLu commented Mar 30, 2024 • edited Loading

Tw1sm commented Apr 3, 2024

P-aLu commented Apr 8, 2024

Tw1sm commented May 30, 2024

Tw1sm commented May 30, 2024

Notes from initial comparison with SharpHound ADCS collection

Tw1sm May 30, 2024

Choose a reason for hiding this comment

P-aLu Jun 28, 2024

Choose a reason for hiding this comment

Tw1sm May 30, 2024

Choose a reason for hiding this comment

P-aLu Jun 28, 2024

Choose a reason for hiding this comment

Tw1sm Jun 28, 2024

Choose a reason for hiding this comment

Tw1sm commented May 31, 2024

Tw1sm commented Jun 28, 2024

P-aLu commented Jul 9, 2024 • edited Loading

P-aLu commented Jul 9, 2024

Tw1sm commented Aug 23, 2024

Tw1sm commented Oct 20, 2024 • edited Loading

P-aLu commented Mar 30, 2024 •

edited

Loading

P-aLu commented Jul 9, 2024 •

edited

Loading

Tw1sm commented Oct 20, 2024 •

edited

Loading