Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BED-4887: AZResetPassword edge false positive on a role-assignable group. #1151

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AD7ZJ
Copy link
Member

@AD7ZJ AD7ZJ commented Feb 14, 2025

Description

Before creating AZResetPassword from an AZRole to an AZUser, make sure the user is not a member of a role assignable group.

Motivation and Context

This PR addresses: https://specterops.atlassian.net/browse/BED-4887

How Has This Been Tested?

Ingest SpecterDev.

Run pathfinding between PARTNER TIER1 SUPPORT@SPECTEROPS DEVELOPMENT and ACHILES@SPECTERDEV.ONMICROSOFT.COM

You should get the following results (important part is that there is no AZPasswordReset edge directly between the two):
Screenshot from 2025-02-19 16-58-16

Prior to this fix, you would see the following:
Screenshot from 2025-02-19 16-53-39

This is not correct because ACHILES@SPECTERDEV.ONMICROSOFT.COM is a member of a role assignable group (ALL SPECTERDEV USERS@SPECTEROPS DEVELOPMENT)
image

Since it's a member of a role assignable group, only Global Administrator Role, Privileged Authentication Administrator Role, or Partner Tier2 Support Role can perform a reset password operation.

You can quickly disable my change by commenting out these lines I added in TenantRoleAssignments()
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

@AD7ZJ AD7ZJ self-assigned this Feb 14, 2025
populates this map of bitmaps, which is then used in
resetPasswordEndNodeBitmapForRole() to filter out users who have
membership in a role assignmable group.
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this missing error handling, this seems pretty sane to me. The loop doesn't appear to be a significant security concern on its face. If we ingested specterdev without noticing an impact, we're likely fine.

@AD7ZJ AD7ZJ requested a review from superlinkx February 21, 2025 19:13
…g the following edges:

azure.ResetPassword
azure.GlobalAdmin
azure.PrivilegedRoleAdmin
azure.PrivilegedAuthAdmin
azure.AddMembers
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise this looks good, let's review the results of performance regression testing, but I'm approving to remove blockers once that's complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants