Don't look up the local system SID#20445
Merged
jheysel-r7 merged 1 commit intorapid7:masterfrom Aug 12, 2025
Merged
Conversation
Contributor
|
Looks great. TestingThere were significantly less LDAP queries being run and this does fix the issue described in the the PR. I'll note some unrelated behavior I noticed while testing. The SID ending in I'll push up a fix for this separately 👍 |
Contributor
Release NotesThis update improves the |
jheysel-r7
approved these changes
Aug 12, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes a small improvement to the new ActiveDirectory mixin when evaluating security descriptors. The local system SID
S-1-5-18SID won't be in AD and because it wasn't matched as a special case until now, it would trigger multiple lookups in AD. The cache wasn't helping here because the object was never found in LDAP so it wouldn't be added to the cache, causing it to be looked up in a query each time.This fixes the issue by adding the SID as a special case. It's placed after the SID we're testing for, so if the user wants to explicitly test for the SID they still can however if it's anything else, we know the ACE won't be applied so we just skip the lookup. This can be noticed when running the
ldap_esc_vulnerable_cert_findermodule with theVERBOSEoption set to true. Without this change there'd be a bunch of lookups as seen by the[*] Successfully queried (objectSID=S-1-5-18).lines in the logs. Now, there are no queries for that particular SID.Testing
ldap_esc_vulnerable_cert_finderwithVERBOSE=trueand see that there's not a bunch of unnecessary LDAP queries