Fix a bug in ACE processing when searching for ESC vulnerabilities #17968
Conversation
This file contains 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
zeroSteiner
commented
May 5, 2023
Comment on lines
-85
to
-91
| if (ace_body.access_mask.protocol & CONTROL_ACCESS) != 0 && (object_type == CERTIFICATE_ENROLLMENT_EXTENDED_RIGHT || object_type == CERTIFICATE_AUTOENROLLMENT_EXTENDED_RIGHT) | ||
| if ace_string.match(/DENIED/) | ||
| flag_allowed_to_enroll = false | ||
| elsif ace_string.match(/ALLOWED/) | ||
| flag_allowed_to_enroll = true | ||
| allowed_sids << ace_body[:sid].to_s | ||
| end |
There was a problem hiding this comment.
This logic was causing the last flag_allowed_to_enroll value to be returned. That means that if the ACL ended in an ACE denying permissions to some account, the entire thing would be ignored. Now the update only processes SIDs that are allowed to enroll. This does contain an issue (that existed prior to these changes) in that if there is an explicit deny of a SID higher in the ACL, it will be ignored. I'm not sure how common this is. The solution I'd propose if this is a concern would be to create a second array of SIDs that are explicitly denied access and print them both for the user so they at least see the information. We don't really do any evaluation of the SIDs so it doesn't seem worth while to pursue the non-trivial endeavor of processing the ACEs in order to check if a particular SID (such as the currently authenticated user) matches an entry with a result of either allow or deny. This would mostly be complicated because of the nested groups that AD supports.
Anyways for now, let's just list the SIDs that are allowed and check if that list is empty (which is done by the caller instead of checking flag_allowed_to_enroll).
gwillcox-r7
reviewed
May 8, 2023
|
Before: |
gwillcox-r7
reviewed
May 8, 2023
There was an issue in the ACE processing where only ACEs corresponding to an object were processed for SIDs with enrollment rights. The processing should also process ACEs that grant the enrollment right and are not related to any objects. In other words, only ACEs associated with an object that is neither the CERTIFICATE_ENROLLMENT_EXTENDED_RIGHT or CERTIFICATE_AUTOENROLLMENT_EXTENDED_RIGHT right should be ignored.
gwillcox-r7
force-pushed
the
fix/mod/esc-finder
branch
from
526dd37
to
0441c1f
Compare
gwillcox-r7
force-pushed
the
fix/mod/esc-finder
branch
from
0441c1f
to
bc25907
Compare
|
With updates: Shows that we can't access the Now to test the new updates: |
|
Everything looks good, will get this landed so long now! |
Release NotesA bug has been fixed where Certificate Templates were not being identified as vulnerable when there was an ACE that granted enrollment rights but did not correspond to any object types. The logic has now been updated so that only ACEs associated with an object that is neither the |
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 fixes a bug I noticed while working on #17965 where the Certificate Template was not being identified as vulnerable after it had been updated.
The issue was in the ACE processing where only ACEs corresponding to an object were processed for SIDs with enrollment rights. The processing should also process ACEs that grant the enrollment right and are not related to any objects. In other words, only ACEs associated with an object that is neither the CERTIFICATE_ENROLLMENT_EXTENDED_RIGHT or CERTIFICATE_AUTOENROLLMENT_EXTENDED_RIGHT right should be ignored.
Verification
Using the changes from #17965:
ESC4-Test) has been updated, run this moduleBoth of these qualities can be checked with the
READaction from the newad_cs_cert_templatemodule in the aforementioned PRCT_FLAG_ENROLLEE_SUPPLIES_SUBJECTflag is setpKIExtendedUsagefield is emptyDemo (New and Improved)
Without these changes, the
ESC4-Templatewould not have been identified as being vulnerable to any of the attacks.