Add high risk application/service principal permissions into ScubaResults.json

[mitchelbaker-cisa]

🗣 Description

Adds a new module AADRiskyPermissionsHelper.psm1 which has the following functions:

• Get-ApplicationsWithRiskyPermissions
• Get-ServicePrincipalsWithRiskyPermissions
• Get-FirstPartyRiskyApplications
• Get-ThirdPartyRiskyServicePrincipals
• Format-RiskyPermission
• Format-Credentials
• Merge-Credentials

These functions serve the purpose of parsing application and service principal objects for risky permissions. The result can be found in ScubaResults.json with the following keys: first_party_risky_applications and third_party_risky_service_principals.

First-party applications refer to applications created within a local tenant, while third-party service principals refer to service principals owned by an external tenant.

New Microsoft commandlets added:

• Get-MgBetaApplication
• Get-MgBetaApplicationFederatedIdentityCredential
• Get-MgBetaServicePrincipal
• Get-MgBetaServicePrincipalAppRoleAssignment

💭 Motivation and context

Closes #1327
Part of ongoing work for epic #1073

Created #1463 as a follow-up issue

🧪 Testing

How to run Pester tests

Checkout this PR's feature branch locally, then from the the root directory run the following commands:

Invoke-Pester -Output 'Detailed' -Path '.\PowerShell\ScubaGear\Testing\Unit\PowerShell\Providers\AADProvider\AADRiskyPermissionsHelper\'

Export-AADProvider.Tests.ps1 was modified since we call the above functions in Export-AADProvider.psm1. Run all AAD provider tests to ensure they pass:

Invoke-Pester -Output 'Detailed' -Path '.\PowerShell\ScubaGear\Testing\Unit\PowerShell\Providers\AADProvider\

Create a new report, verify first_party_risky_applications and third_party_risky_service_principals are added in ScubaResults.json.

✅ Pre-approval checklist

• This PR has an informative and human-readable title.
• PR targets the correct parent branch (e.g., main or release-name) for merge.
• Changes are limited to a single goal - eschew scope creep!
• Changes are sized such that they do not touch excessive number of files.
• All future TODOs are captured in issues, which are referenced in code comments.
• These code changes follow the ScubaGear content style guide.
• Related issues these changes resolve are linked preferably via closing keywords.
• All relevant type-of-change labels added.
• All relevant project fields are set.
• All relevant repo and/or project documentation updated to reflect these changes.
• Unit tests added/updated to cover PowerShell and Rego changes.
• Functional tests added/updated to cover PowerShell and Rego changes.
• All relevant functional tests passed.
• All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ Pre-merge checklist

• PR passed smoke test check.

• Feature branch has been rebased against changes from parent branch, as needed

  Use Rebase branch button below or use this reference to rebase from the command line.

• Resolved all merge conflicts on branch

• Notified merge coordinator that PR is ready for merge via comment mention

• Demonstrate changes to the team for questions and comments.
  (Note: Only required for issues of size Medium or larger)

✅ Post-merge checklist

• Feature branch deleted after merge to clean up repository.
• Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

[tkol2022]

I noticed code at the top of the AADRiskyPermissionsHelper that is not part of an obvious code block. It is just floating there. I would prefer if we move it into a block of code. Is there somewhere where we load up global variables or something like that?

Addam's input

I asked @schrolla for his opinion and he said:

Either repeat or turn it into a function of it's own and call it within the others.
So maybe a GetPermissionsJson private function and then just call it where needed in each function to do $PermissionsJson = Get-PermissionsJson()  and retrieve the needed object.
Might even think about putting that function in the Utility module in case other parts of the code eventually want to access that file.  But I don't think that's strictly necessary here.

[tkol2022]

Bug

Get-ApplicationsWithRiskyPermissions does not handle API permissions that belong to apps not listed in the "resources" section of RiskyPermissions.json.

I tested in Test Tenant 3 (E5) and it failed on this line:

I tracked down what was happening and the application being audited had an API permission assigned which belongs to an app that you don't have in the RiskyPermissions.json.

Potential fix

Check if the app id that the permission belongs to is in the "resources" section of RiskyPermissions.json and if not, you can skip it.

[tkol2022]

Variable name change recommendation

I recommend to rename the variable "Resource" in Get-ApplicationsWithRiskyPermissions. It is not clear which resource we are referring to. Maybe "ResourceAppDisplayName" or something like that because the variable indicates the name of the application that a specific permission belongs to. If you use the same name elsewhere to mean the same thing, then I would change that too.

[tkol2022]

Bug

In ExportAADProvider where the Get-FirstPartyRiskyApplications and Get-ThirdPartyRiskyServicePrincipals functions are called, I recommend adding parameter validation to ensure that the input parameters RiskyApps and RiskySPs are not $null.
If the preceding functions Get-ApplicationsWithRiskyPermissions or Get-ServicePrincipalsWithRiskyPermissions encounter an error then those parameters will probably be $null and then we probably don't want to call Get-FirstPartyRiskyApplications or Get-ThirdPartyRiskyServicePrincipals.
You should also make sure that the code handles situations where the RiskyApps and RiskySPs are "empty" because your Get-FirstPartyRiskyApplications and Get-ThirdPartyRiskyServicePrincipals functions do not accept empty input objects.

The reason I noticed this is because during testing, Get-ApplicationsWithRiskyPermissions failed due to error but the code proceeded to call the subsequent functions noted above which then generated more errors and error messages.

Your code does not accept empty input objects.

[tkol2022]

Terms

From what I have seen in multiple web sites, "first party" can refer to applications developed by Microsoft so we might need to use another term to describe apps that the organization running ScubaGear created and owns.

https://learn.microsoft.com/en-us/troubleshoot/entra/entra-id/governance/verify-first-party-apps-sign-in

[tkol2022]

Permissions to add

Mitchel, I recommend adding the permissions below to RiskyPermissions.json. Please review them and if you agree, go ahead and add them. For any additional permissions we can create a future issue to analyze all the Graph permissions and see if we want to augment or make any further changes.

• Application.Read.All (can get information on which apps or service principals can be used to gain access to the tenant)
• Organization.Read.All (can read licensing information which might help an adversary determine which security services are enabled during recon)
• RoleManagement.Read.Directory (can read which users have highly privileged roles)

More types of risks

In the future we might want to look at permissions prefixed with DeviceManagement*. There could be some risks that we want to flag there related to exposing user devices.

[tkol2022]

Function naming

I am wondering if we should rename the functions Get-FirstPartyRiskyApplications and Get-ThirdPartyRiskyServicePrincipals to more closely align with what they actually do and distinguish them from Get-ApplicationsWithRiskyPermissions and Get-ServicePrincipalsWithRiskyPermissions? Also the term "Get" can imply that a function is retrieving data from somewhere (database, graph, M365, etc.)

Here is what each seems to do:

• Get-FirstPartyRiskyApplications - This seems to be an aggregation function.
• Get-ThirdPartyRiskyServicePrincipals - This pulls objects from the input parameter RiskySPs that are developed by third parties. It's almost like a filter operation of sorts.

[tkol2022]

Test Results - Test Tenant 1 (G5)

• The code seems to create the JSON in ScubaResults for both applications (owned by the organization running ScubaGear) and third party developed service principals. I went through all the apps and service principals and it seems that the code pulls only the ones that have permissions flagged as high risk in the RiskyPermissions.json file, therefore it is behaving as expected.
• The code correctly reports on apps versus third party service principals in the first_party_risky_applications and third_party_risky_service_principals nodes inside ScubaResults.

The code flagged the following numbers of objects as being associated with risk in the tenant:
Applications (owned by the org): 10 objects
Third party service principals: 2 objects

Missing test data

We need some test data for the KeyCredentials, PasswordCredentials and FederatedCredentials fields for objects in the third_party_risky_service_principals node of ScubaResults because right now, the service principals that were flagged have empty data in those fields.

[tkol2022]

Test Results - Test Tenant 6 (GCC high) & Test Tenant 2 (G3)

The code fails with what appears to be the same problem as reported in an earlier comment. Identical error against both tenants.

[tkol2022]

Permissions to add

Mitchel, I recommend adding the permissions below to RiskyPermissions.json. Please review them and if you agree, go ahead and add them. For any additional permissions we can create a future issue to analyze all the Graph permissions and see if we want to augment or make any further changes.

• Application.Read.All (can get information on which apps or service principals can be used to gain access to the tenant)
• Organization.Read.All (can read licensing information which might help an adversary determine which security services are enabled during recon)
• RoleManagement.Read.Directory (can read which users have highly privileged roles)

More types of risks

In the future we might want to look at permissions prefixed with DeviceManagement*. There could be some risks that we want to flag there related to exposing user devices.

#1471 has been added for future improvements to RiskyPermissions.json

[tkol2022]

Performance Profiling

I noticed that the addition of the new code appears to add significant time to the execution of the AAD provider so I did some performance profiling. I tried out a couple of potential solutions and will follow-up by email with sample code and specifics. The cmdlet Get-MgBetaServicePrincipalAppRoleAssignment is invoked 720 times when running against the G5 test tenant and that is the root cause of the execution time because each one of those is a REST API call to Graph.

*execution times in screenshots below are in seconds.

Current code performance

Potential solution 1

Potential solution 2