Perform detailed code review of AAD Rego policies that check conditional access

[tkol2022]

💡 Summary

Recently we identified that a couple of the AAD Rego rulesets dealing with AAD conditional access, do not check that the tenant policies were configured exactly as specified in the baseline document instructions. The purpose of this issue is to review the code for all the Rego rulesets that examine conditional access to ensure they are coded consistent with each of the configuration characteristics described the baseline implementation instructions.

Example policy 3.7

This describes a problem that was found with AAD Rego policy 3.7. In the baseline document instructions the following three characteristics of the Access controls > Grant > Grant Access configuration of the conditional access policy are described. Before it was fixed, in some cases 3.7 could have been configured incorrectly in the tenant but still pass the ScubaGear check.

• Require device to be marked as compliant
• Require Microsoft Entra hybrid joined device
• Require one of the selected controls

The original Rego code examined if the user ticked the box Require device to be marked as compliant or if they ticked the box Require Microsoft Entra hybrid joined device but nothing else. It did not check if the box Require one of the selected controls was checked and it did not check if the user checked any irrelevant boxes such as Example terms of us (which would fundamentally change the way the access policy is applied). In this context, Example terms of us should not be checked and the code should validate that.

Because of the original code flaw, the non compliant configuration in the screenshot below would Pass in ScubaGear.

Implementation notes

• Perform a review of the Rego policies related to conditional access as describe above. Helper rulesets are within the scope of this review.

• Review the pull request linked here for a reference of how the code flaw described above for 3.7 was corrected.

• For any Rego policies identified as needed an update, examine the respective unit tests and functional tests to determine if any new tests (or updates to existing tests) are needed to exercise additional logic paths.

• Create new coding issues to rectify any problems found

[dagarwal-mitre]

Conditional Access Policies which need to be updated

General notes for the updates needed are to use PolicyConditionsMatch more for consistency and prevent code repetition.

MS.AAD.1.1v1

Similar to 3.7, currently 1.1 checks if Exchange ActiveSync clients and Other clients are clicked, but doesn't check to ensure that those two are the only boxes checked.

MS.AAD.2.3v1

For consistency update line 141 from PolicyConditionsMatch(CAPolicy) to PolicyConditionsMatch(CAPolicy) == true

MS.AAD.3.1v1

Check if we can use PolicyConditionsMatch rather than the 3 lines (183-185) used together
Replace

"All" in CAPolicy.Conditions.Users.IncludeUsers
"All" in CAPolicy.Conditions.Applications.IncludeApplications
CAPolicy.State == "enabled" 

with PolicyConditionsMatch(CAPolicy) == true

MS.AAD.3.2v1

For consistency update line 221 from PolicyConditionsMatch(CAPolicy) to PolicyConditionsMatch(CAPolicy) == true

MS.AAD.3.6v1

Missing check for Target resources > Cloud apps > All cloud apps
To fix this try using PolicyConditionsMatch(CAPolicy) == true

MS.AAD.3.8v1

Use 3.7 as a model for this one as they are very similar

1. Check if PolicyConditionsMatch can be used to reduce code repetition
2. After checking compliantDevice and domainJoinedDevice, check to ensure no other built in controls are selected
3. Check for For multiple controls > Require one of the selected controls by checking GrantControls.Operator is set to "OR"

[dagarwal-mitre]

@tkol2022 @Sloane4 @mitchelbaker-cisa
Please take a look at my comment, about the code review for AAD Conditional access policies, and let me know if there is anything I can clarify or missed, and should update.

[tkol2022]

Nice job! Here are my comments.

• Go ahead and create a new issue scoped to performing the code updates related to your findings from this review. We will prioritize it in a future stand-up and place it into an appropriate release.
• For policy 3.6 I didn't understand your comment. Rephrase it so that it is more clear what you are recommending the developer to do.
• Instead of putting line numbers, put snippets of Rego code that contain the changes you are recommending. This is because the line numbers could change from now until someone works on the future issue to update the code.

[dagarwal-mitre]

Nice job! Here are my comments.

• Go ahead and create a new issue scoped to performing the code updates related to your findings from this review. We will prioritize it in a future stand-up and place it into an appropriate release.
• For policy 3.6 I didn't understand your comment. Rephrase it so that it is more clear what you are recommending the developer to do.
• Instead of putting line numbers, put snippets of Rego code that contain the changes you are recommending. This is because the line numbers could change from now until someone works on the future issue to update the code.

Created issue #1323 and updated the comment with your suggestions, if there isn't anything else I will be closing this issue

[tkol2022]

Thanks for the final updates.