Skip to content

Add missing risky app permissions and GCC High GUIDs to RiskyAppPermissions.json#2179

Open
skirkpatrickMSFT wants to merge 22 commits into
mainfrom
2004-review-riskypermissionsjson-to-identify-missing-permissions
Open

Add missing risky app permissions and GCC High GUIDs to RiskyAppPermissions.json#2179
skirkpatrickMSFT wants to merge 22 commits into
mainfrom
2004-review-riskypermissionsjson-to-identify-missing-permissions

Conversation

@skirkpatrickMSFT

@skirkpatrickMSFT skirkpatrickMSFT commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR makes three categories of improvements to ScubaGear's risky app permission detection:

1. File rename and relocation
Renamed Modules/Permissions/RiskyPermissions.json to schemas/RiskyAppPermissions.json to better reflect its purpose (it covers app permissions, not just risky permissions broadly) and to co-locate it with other ScubaGear schema files. Updated all references across the codebase: path resolution in AADRiskyPermissionsHelper.psm1, error messages in AADHybridExchangeHelper.psm1, test context strings, GitHub URLs in baselines/aad.md and ScubaBaselines.json, and comments in AADConfig.rego and docs/misc/tooloutputschema.md.

2. Performance improvements to AADRiskyPermissionsHelper.psm1

  • Added module-scoped caching to Get-RiskyPermissionsJson so the JSON file is loaded once and reused across calls
  • Added Build-PermissionLookup and Get-PermissionLookup to create an O(1) hashtable lookup structure keyed by ResourceDisplayName > RoleType > GUID
  • Replaced O(n) -contains checks in Format-Permission with ContainsKey() hashtable lookups
  • Added optional RiskyPermissionsJson dependency injection parameter to Get-ApplicationsWithRiskyPermissions, Get-ServicePrincipalsWithRiskyPermissions, Get-ServicePrincipalsWithRiskyDelegatedPermissionClassifications, and Get-ExchangeHybridIds
  • Updated ExportAADProvider.psm1 to load the JSON once and pass it to all downstream calls

3. Expanded permission coverage and GCC High support in RiskyAppPermissions.json

New resource sections (4):

  • Power BI Service - 2 Application + 28 Delegated permissions
  • Dataverse - 1 Delegated permission (user_impersonation, Critical)
  • PowerApps Service - 1 Delegated permission (User, Medium)
  • Microsoft Flow Service - 6 Delegated permissions

New Microsoft Graph permissions (43):

  • Teams-related: Chat., Channel., OnlineMeetings., TeamworkDevice., Presence., TeamsActivity., TeamsTab., TeamsAppInstallation., Teamwork.Migrate.All, TeamSettings., TeamMember.
  • Defender/security: SecurityAlert., SecurityIncident., SecurityIdentitiesSensors., ThreatHunting., ThreatIntelligence., ThreatSubmission., ThreatSubmissionPolicy., CustomDetection., eDiscovery., AttackSimulation.

GCC High GUID additions:

  • 31 Microsoft Graph Application permissions have different GUIDs in GCCH - all added as additional entries
  • 8 Microsoft Graph Delegated permissions have different GUIDs in GCCH - all added
  • 3 EXO Application permissions (IMAP/POP/SMTP .AccessAsApp) have different GUIDs in GCCH - all added

Risk level corrections (4):

  • Capacity.ReadWrite.All (Power BI): Medium to High
  • Content.Create (Power BI): Medium to High
  • PowerApps User: High to Medium
  • Flows.Write.Plans: High to Medium

Motivation and context

AADRiskyPermissionsHelper.psm1 performs GUID-based lookups against RiskyAppPermissions.json to identify risky permissions granted to service principals in a tenant. Before these changes:

  1. The JSON file lived in Modules/Permissions/ with no clear connection to the schemas/ directory where other schema files are kept.
  2. The JSON was reloaded from disk on every call - an unnecessary performance cost when checking many service principals.
  3. Permission lookups used O(n) array searches rather than O(1) hashtable lookups.
  4. The file covered only Microsoft Graph, EXO, SPO, and Office 365 Management APIs - missing Teams, Defender, Power BI, Dataverse, PowerApps, and Flow entirely.
  5. Only commercial-environment GUIDs were present - in GCC High tenants, many Microsoft Graph and EXO permission GUIDs differ from commercial, causing detections to silently fail.

Since commercial and GCCH GUIDs never collide, adding GCCH GUIDs as additional entries in the same Application/Delegated sections required no changes to the lookup logic.

Permissions and services confirmed absent in GCCH (no GCCH entries needed): BookingsAppointment.ReadWrite.All, AuthenticationContext.ReadWrite.All, RoleManagement.ReadWrite.Exchange, Calendars.ReadBasic.All, AttackSimulation.ReadWrite.All, Policy.ReadWrite.SecurityDefaults, ThreatSubmission.*, ThreatSubmissionPolicy.ReadWrite.All, EXO Organization.ReadWrite.All, PowerApps Service, Microsoft Flow Service, and 7 Power BI Delegated permissions.

Testing

  • All 46 AAD provider unit tests, 31 HybridExchange unit tests, and 337 Rego tests pass after the rename and code changes.
  • All 251 AAD provider unit tests pass after the performance refactor with no regressions.
  • All new and GCCH GUIDs were verified by querying live Microsoft Graph endpoints (graph.microsoft.com for commercial, graph.microsoft.us for GCCH) using Invoke-MgGraphRequest.
  • Commercial verification: queried the relevant service principal for each product and confirmed every commercial GUID in the JSON matches a real appRole or oauth2PermissionScope on that SP.
  • GCCH verification: connected to the USGov environment (graph.microsoft.us) and compared all JSON GUIDs against GCCH service principals to identify which differed, which were absent, and which new GUIDs to add.
  • Verified the 3 new GCCH EXO GUIDs (28b54159, de49976f, ce81e10f) do not exist in any commercial service principal, confirming no collisions.
  • JSON validated as parseable (ConvertFrom-Json) after each set of changes.

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.
  • Resolved all merge conflicts on branch.
  • Squash all commits into one PR level commit using the Squash and merge button.

Post-merge checklist

  • Feature branch deleted after merge to clean up repository.
  • Close issues resolved by this PR if the closing keywords did not activate.
  • Verified that all checks pass on parent branch after merge.

- Add module-scoped cache to Get-RiskyPermissionsJson (load once, reuse)
- Add Build-PermissionLookup/Get-PermissionLookup for O(1) permission checks
- Replace -contains (O(n)) with hashtable ContainsKey() in Format-Permission
- Add optional RiskyPermissionsJson param to Get-ApplicationsWithRiskyPermissions,
  Get-ServicePrincipalsWithRiskyPermissions, and
  Get-ServicePrincipalsWithRiskyDelegatedPermissionClassifications
- Add optional RiskyPermissionsJson param to Get-ExchangeHybridIds
- Load JSON once in ExportAADProvider and pass to all downstream calls
- All 251 AAD provider unit tests pass with no regressions
…json

- git mv Modules/Permissions/RiskyPermissions.json -> schemas/RiskyAppPermissions.json
- Update path resolution in AADRiskyPermissionsHelper (Parent.Parent.Parent + schemas/)
- Update error messages in AADHybridExchangeHelper to reference new filename
- Update all test assertions and context strings
- Update GitHub URLs in baselines/aad.md, ScubaBaselines.json
- Update comments in AADConfig.rego, tooloutputschema.md
- All 46 AAD provider tests, 31 HybridExchange tests, 337 Rego tests pass
- Add 33 Graph Application permissions (11 Critical, 9 High, 10 Medium, 3 Low)
- Add 4 EXO Application permissions (Exchange.ManageAsApp/V2, Organization.ReadWrite.All, Mailbox.Migration)
- Add 13 EXO Delegated permissions (Exchange.Manage, Mail.*.All, protocol access, etc.)
- Add 1 SPO Application permission (Sites.Manage.All)
- Add 4 SPO Delegated permissions (ProjectWebApp.FullControl, Migration, etc.)
- Fix EXO Delegated typo: Calendar.Read -> Calendars.Read (GUID 5b9be81f)
- All 278 GUIDs verified against live Graph API with 0 errors
…form

- Add Teams application permissions (Calls, TeamSettings, TeamsTab,
  TeamsAppInstallation, Teamwork.Migrate.All, TeamsActivity, Presence)
- Add Defender/security application permissions (SecurityAlert,
  ThreatHunting, ThreatIndicators, ThreatIntelligence, ThreatSubmission,
  CustomDetection, SecurityIdentitiesSensors)
- Add corresponding delegated permissions for Teams and Defender
- Add Power BI Service, Dataverse, PowerApps Service, and Microsoft Flow
  Service as new resource sections with verified GUIDs from tenant
- Correct risk levels for delegated permissions after review
Many Microsoft Graph, EXO, and other service permission GUIDs differ
between commercial and GCC High environments. Since AADRiskyPermissionsHelper
performs GUID-based lookups, GCCH tenants would miss risky permission
detections without the GCCH-specific GUIDs.

Added GCCH GUIDs for:
- 31 Microsoft Graph Application permissions (different GUIDs in GCCH)
- 8 Microsoft Graph Delegated permissions (different GUIDs in GCCH)
- 3 EXO Application permissions: IMAP.AccessAsApp, POP.AccessAsApp, SMTP.SendAsApp

Permissions absent in GCCH (no GCCH entry needed):
- BookingsAppointment.ReadWrite.All, AuthenticationContext.ReadWrite.All,
  RoleManagement.ReadWrite.Exchange (App), Calendars.ReadBasic.All,
  AttackSimulation.ReadWrite.All, Policy.ReadWrite.SecurityDefaults,
  ThreatSubmission.ReadWrite.All, ThreatSubmission.Read.All,
  ThreatSubmissionPolicy.ReadWrite.All (App and/or Del),
  EXO Organization.ReadWrite.All

Services not present in GCCH (no GCCH entries needed):
- PowerApps Service (475226c6-020e-4fb2-8a90-7a972cbfc1d4)
- Microsoft Flow Service (7df0a125-d3be-4c96-aa54-591f83ff541c)

Power BI Delegated permissions absent in GCCH (7 permissions):
Item.ReadWrite.All, Connection.ReadWrite.All, Pipeline.ReadWrite.All,
Pipeline.Deploy, Item.Read.All, Connection.Read.All, Pipeline.Read.All
@tkol2022

tkol2022 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

This comment is regarding public reported issue #1718 and is unrelated to the scope of this PR. So this is just FYI. No action requested.

I ran this branch against aad so that I could compare with main and I don't see any performance difference related to the two slowest functions reported in that public issue.

main:
image

this PR branch:

image

@skirkpatrickMSFT

Copy link
Copy Markdown
Collaborator Author

@tkol2022 I resolved the Warning you were getting and I think I will have a few changes to help with the slowness as well.I am testing those and they are not on this branch yet.

@skirkpatrickMSFT

Copy link
Copy Markdown
Collaborator Author

@tkol2022 Try this recent update. I am showing about a 72% faster run for Get-ApplicationsWithRiskyPermissions, and 24ish% with Get-ServicePrincipalsWithRiskyPermissions.

For both Get-ApplicationsWithRiskyPermissions and Get-ServicePrincipalsWithRiskyPermissions, the main change was replacing repeated linear scans with cached hash lookups. I now load and parse RiskyAppPermissions.json once per module session, build a permission lookup table once, and build AppId/name maps once. That means permission matching no longer repeatedly walks the JSON object tree for every permission on every app or service principal.

I also added a per-resource permission-type cache. Before, resolving whether a permission ID was an application role or delegated scope meant repeatedly scanning appRoles and oauth2PermissionScopes for the same resource service principal. Now Get-PermissionTypeDetails builds a cached lookup once per resource app and reuses it for every subsequent role ID resolution.

The biggest service-principal improvement was batching Graph calls. Get-ServicePrincipalsWithRiskyPermissions used to be much more expensive because app role assignments were effectively handled one object at a time. I introduced Invoke-GraphBatchRequestsWithRetry, which sends service principal assignment lookups in Graph batch requests, retries only transient failures like 429 and 5xx, honors Retry-After, and uses bounded backoff. That sharply reduces request overhead and made the service-principal path the main source of the speedup.

I also reduced redundant resource permission fetches. Get-ResourcePermissions now reuses the shared ResourcePermissionCache, so once a resource app’s roles and scopes are fetched, both cmdlets can reuse that data instead of querying Graph again.

@FollyBeachGurl FollyBeachGurl requested review from tkol2022 and removed request for tkol2022 June 11, 2026 17:31
@ahuynhECS ahuynhECS requested review from tkol2022 and removed request for tkol2022 June 11, 2026 17:57
@skirkpatrickMSFT

Copy link
Copy Markdown
Collaborator Author

This comment is regarding public reported issue #1718 and is unrelated to the scope of this PR. So this is just FYI. No action requested.

I ran this branch against aad so that I could compare with main and I don't see any performance difference related to the two slowest functions reported in that public issue.

main: image

this PR branch:

image

Updated

@DickTracyII DickTracyII left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ran through manual tests against aad, IMO looks like it working like it should.

I do think there should be a pester test that tests against the new gcch GUID's, incase they get removed or changed

Comment thread PowerShell/ScubaGear/schemas/RiskyAppPermissions.json
- Remove internal helper exports from AADRiskyPermissionsHelper export surface

- Keep Get-RiskyAppPermissionsJson exported for cross-module dependency and document intent

- Remove unused RiskyAppPermissionsJson parameter from Get-ExchangeHybridIds

- Add dedicated unit tests for Get-RiskyAppPermissionsJson, New-PermissionLookup, and Get-PermissionLookup

- Add cloud mapping regression tests for GCC/GCCHigh GUID coverage

- Add GCCHigh cloud metadata tags for reviewed GUID entries in RiskyAppPermissions.json
- Add mock RiskyAppPermissionsJson with Microsoft Graph and Office 365 Exchange Online permissions
- Add mocks for Get-PermissionLookup and New-RiskyAppResourceLookup to properly build lookup structures
- Pass RiskyAppPermissionsJson parameter to all function calls in tests to ensure mock data is used
- This fixes the regression caused by removing Get-PermissionLookup from module exports
- Add PSReviewUnusedParameter suppress attributes for script block parameters
- Replace Write-Host with Write-Output for host-independent output
- Replace empty catch block with Write-Verbose to comply with PSAvoidUsingEmptyCatchBlock rule
- Remove trailing whitespace from 5 files
- Add proper help comment block to Get-ExeHash function in Support.psm1
- Fix unused parameter warning in Benchmark-AADRiskyPermissionsHelper.ps1 by explicitly referencing parameters with null assignment
Convert MockRiskyAppPermissionsJson to PSCustomObject so PSObject.Properties iteration works in mocked lookup builders.
Run the existing Selenium driver update script before invoking product tests so Start-SeChrome has a matching chromedriver on CI.
The functional test rerun started before the driver update change and passed, so remove the action.yaml change to keep the test harness unchanged.
@skirkpatrickMSFT

Copy link
Copy Markdown
Collaborator Author

All requested updates made. Tests pass except for Tenant7_G5 in functional tests due to the configuration for the 429 errors that currently reside in that tenant.

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.

Review RiskyPermissions.json to identify missing permissions

4 participants