secretsdump - Double DC Sync performance for DCs supporting SID lookups

[tomspencer]

Currently secretsdump calls DRSCrackNames to get object GUIDs and then calls DRSGetNCChanges with that GUID.

As mentioned in the code comments, this should not be necessary as DRSGetNCChanges can also accept SIDs, which we already have or can get without another RPC / network call.

This pull switches to using SIDs for DRSGetNCChanges which eliminates the extra RPC call, doubles sync performance, and halves load / noise on the Domain Controller.

Syncs of just one user (-just-dc-user) still require a call to DRSCrackNames to resolve, so the only change there is to get back a SID instead of a GUID to match the new expected SID argument format of DRSGetNCChanges.

[mubix]

Didn't seem to work for me, I'll try a few other configurations. Without this pull the dump works just fine.

[2023-07-10 21:26:29] [+] Calling DRSGetNCChanges for S-1-5-21-DOMAIN-SID-HERE-500
Traceback (most recent call last):
  File "secretsdump.py", line 279, in dump
    self.__NTDSHashes.dump()
  File "/usr/local/lib/python3.8/dist-packages/impacket-0.10.1.dev1+20230629.121115.b5dab2df-py3.8.egg/impacket/examples/secretsdump.py", line 2671, in dump
    userRecord = self.__remoteOps.DRSGetNCChanges(userSid)
  File "/usr/local/lib/python3.8/dist-packages/impacket-0.10.1.dev1+20230629.121115.b5dab2df-py3.8.egg/impacket/examples/secretsdump.py", line 591, in DRSGetNCChanges
    return self.__drsr.request(request)
  File "/usr/local/lib/python3.8/dist-packages/impacket-0.10.1.dev1+20230629.121115.b5dab2df-py3.8.egg/impacket/dcerpc/v5/rpcrt.py", line 880, in request
    raise exception
impacket.dcerpc.v5.drsuapi.DCERPCSessionError: DRSR SessionError: code: 0x20f8 - ERROR_DS_DRA_BAD_NC - The naming context specified for this replication operation is invalid.
[2023-07-10 21:26:29] [-] DRSR SessionError: code: 0x20f8 - ERROR_DS_DRA_BAD_NC - The naming context specified for this replication operation is invalid.
[2023-07-10 21:26:29] [*] Something went wrong with the DRSUAPI approach. Try again with -use-vss parameter

[tomspencer]

Interesting. So I just tested it on more DCs in the same domain and was able to replicate that error.

I tested it previously (and again now) on two 2022 DCs and it ran great on both, but running it against two 2016 DCs both resulted in the same "ERROR_DS_DRA_BAD_NC" you had. Not sure why it would be OS-specific though.

I don't have a 2019 DC handy to test with.

I suppose if it does end up being OS version specific with newer ones (and presumably future ones) supporting this I could just wrap this in a try and fallback to the old behavior for that and all future requests if it fails.

[mubix]

I tested against a 2012 DC that I have in my lab, I can try on a 2019 when I have a moment in the next few days.

[tomspencer]

I created a new forest/domain with a 2019 DC and a 2022 DC. Again the 2022 DC worked fine, but the 2019 DC had the same ERROR_DS_DRA_BAD_NC behavior.

Given the compatibility issues, I'm going to update my pull request to try the faster SID method first but fall back gracefully to the existing behavior if it doesn't work.

[tomspencer]

Okay, amended the logic to support graceful fallback.

If SID lookups/syncs are successful it will use those and enjoy the performance/load/noise benefits, but if it fails it will stop trying SID lookups and fallback to the existing DSCrackNames/GUID lookup logic.

Support for this has been limited to 2022 DCs so far in my testing.

[tomspencer]

Hi Adrian, I had the same concern, so this PR will never try a SID lookup again for the rest of the run if a single one fails. That way, if the DC doesn't support SID lookups then worst case the RPC call count will be 2n+1 (for n entries) instead of the current 2n, and if it does support lookups it will just be n. That way the unsupported case is basically unchanged (just one total extra call for the whole run) but the supported case still gets the benefits. Btw, for syncing a single user with ("-just-dc-user") i have just kept the existing behavior and not attempted a SID lookup. That flow requires the call to DSCrackNames regardless, so attempting a SID resolution would not be beneficial. Thanks, Tom

…

On Tue, Jul 18, 2023, 3:12 PM adrian manrique ***@***.***> wrote: Hi, thanks for the PR. My main concern with these changes is the impact in performance in scenarios != Win2022 given that we are now doing 3 rpc calls instead of 2. — Reply to this email directly, view it on GitHub <#1578 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZN2INVM4G3O7JJUCTHML3XQ2DSNANCNFSM6AAAAAA2COHZAY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[tomspencer]

Put this together to clarify the RPC load impact:

# of RPC calls	Existing behavior	With this pull	Notes
--just-dc-user sync	2	2	No behavior change
Sync of n users (pre-2022 DCs)	2n	2n + 1	One SID lookup fails, existing behavior restored for all remaining users
Sync of n users (2022+ DCs)	2n	n	SID lookups cut total RPC lookups in half

The below flow (with sanitized names, hashes, etc.) shows the worst-case with a SID lookup failure occurring once and the code falling back to DRSCrackNames/GUID lookups without attempting any further SID-based lookups.

[2023-07-23 19:50:46] [+] Trying to connect to KDC at SUB.EXAMPLE.COM
[2023-07-23 19:50:48] [+] Calling DRSGetNCChanges for S-1-5-21-4895789237-5729438464-1993826746-502
[2023-07-23 19:50:48] [+] SID lookup unsuccessful, falling back to DRSCrackNames/GUID lookups
[2023-07-23 19:50:48] [+] Calling DRSCrackNames for SUB\krbtgt
[2023-07-23 19:50:48] [+] Calling DRSGetNCChanges for {49bfde33-0576-4b48-8a7b-6b19fbb7b6dc}
[2023-07-23 19:50:48] [+] Entering NTDSHashes.__decryptHash
[2023-07-23 19:50:48] [+] Decrypting hash for user: CN=krbtgt,CN=Users,DC=SUB,DC=EXAMPLE,DC=COM
krbtgt:502:aad3b435b51404eeaad3b435b51404ee:098432089d44b298483ce103093eda28:::
[2023-07-23 19:50:48] [+] Leaving NTDSHashes.__decryptHash
[2023-07-23 19:50:48] [+] Calling DRSCrackNames for SUB\Administrator
[2023-07-23 19:50:48] [+] Calling DRSGetNCChanges for {9b169cdc-b3e3-4fbe-bea9-ee53a7153b44}
[2023-07-23 19:50:49] [+] Entering NTDSHashes.__decryptHash
[2023-07-23 19:50:49] [+] Decrypting hash for user: CN=Administrator,CN=Users,DC=SUB,DC=EXAMPLE,DC=COM
SUB.EXAMPLE.COM\Administrator:500:aad3b435b51404eeaad3b435b51404ee:8846f7eaee8fb117ad06bdd830b7586c:::
[2023-07-23 19:50:49] [+] Leaving NTDSHashes.__decryptHash
[2023-07-23 19:50:49] [+] Calling DRSCrackNames for SUB\Bernard

[anadrianmanrique]

Thanks for clarifying @tomspencer . I realized that after looking at the changes for the second time, that's why I erased my original question. I'll be testing these changes and let you know about it. Thanks!

[tomspencer]

Just checking in Anadrian, any news on the testing?

[anadrianmanrique]

Hi @tomspencer! We are planning to integrate these days, soon after the release ( after finishing with testing ). Thanks!

[anadrianmanrique]

It's now merged in master, thanks!

[tomspencer]

Appreciate it, @anadrianmanrique!