[bug] Fix RBCD audit/find tree prefix using shadowed inner ACE index (#23)#24
Open
p0dalirius wants to merge 1 commit intomainfrom
Open
[bug] Fix RBCD audit/find tree prefix using shadowed inner ACE index (#23)#24p0dalirius wants to merge 1 commit intomainfrom
p0dalirius wants to merge 1 commit intomainfrom
Conversation
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.
Linked Issue
Closes #23
Root Cause
The inner loop over
ntSecurityDescriptor.DACL.Entriesreused the variable nameentryIndexfrom the outer loop oversearchResults, shadowing it inside the ACE loop body. The subsequent tree-prefix conditionalif entryIndex < len(searchResults)-1then read the inner ACE index rather than the outer search-result index, so the│vs. spaces choice for the left-hand prefix was driven by the wrong value. The outer variable was unreachable within the inner loop.This was a regression introduced by #18 (closed #17): before that change, the inner loop iterated over
valueswith a distinct name (valueIndex). #18 moved the inner loop ontoDACL.Entriesand reusedentryIndex, which is what produced the shadowing.Fix Description
Rename the inner loop variables from
entryIndex, entrytoaceIndex, acein bothcore/mode_audit/RessourceBasedConstrainedDelegations.goandcore/mode_find/RessourceBasedConstrainedDelegations.go. This removes the shadowing, so the outerentryIndex/entrybindings remain visible inside the inner loop body and the tree-prefix conditional now reads the outer search-result index as intended. The inner separator decision (├──vs.└──) keeps comparing againstlen(DACL.Entries)-1, now using the renamedaceIndex.Renaming is preferred over introducing a captured outer-index variable because it keeps the fix diff-minimal, matches the naming used elsewhere in the package, and makes the two nesting levels unambiguous at a glance.
How Verified
Static verification over the corrected code paths:
core/mode_audit/RessourceBasedConstrainedDelegations.go:82-107: inner loop variables renamed; outerentryIndexandlen(searchResults)are now the only referents for the│vs. spaces branch.core/mode_find/RessourceBasedConstrainedDelegations.go:82-107: same correction.Also verified by
go build ./...after the rename.Test Coverage
None. These audit/find functions are not unit-tested in the repository today; exercising them requires an LDAP session. The change is a rename-only correction inside a formatting path, and adding a test harness for that path is out of scope for this fix.
Scope of Change
core/mode_audit/RessourceBasedConstrainedDelegations.gocore/mode_find/RessourceBasedConstrainedDelegations.goRisk and Rollout
Trivial and local. Only affects the text layout of RBCD output; no LDAP writes, no security decisions, no state changes.