asterictnl-lvdw / M365SAT

Microsoft 365 Security Assessment Tool - A Easy-To-Use Microsoft 365 Security Assessment Tool
MIT License
35 stars 16 forks source link

Improve accuracy of Conditional Access Inspectors (e.g. CISMAz5223.ps1) #40

Open aquiros17 opened 1 month ago

aquiros17 commented 1 month ago

There are several checks that are not correctly configured. For example, with the control "CIS MAz 5.2.2.3 - No Conditional Access policies to block legacy authentication" (https://github.com/asterictnl-lvdw/M365SAT/blob/production/inspectors/M365/Azure/CIS30/E3/L1/CISMAz5223.ps1), the scripts check by conditional access policy name and it tries to find a conditional access with name like "Block legacy" (line 46 of the script).

We have several customers with conditional access policies that block legacy auth but the name of the conditional access policy does not contain "Block legacy", is multilanguage. I think that the script has to do an "intelligent" check not based on name, based on the configuration of the conditional access policies, like 365inspect.

Please, could you review this and other checks in order to do a best efford audit?

Thanks in advance,

Regards,

asterictnl-lvdw commented 1 month ago

Hi @aquiros17 ,

Thank you for your submission, as this is not a bug, but more of a request of change to improve the CISMAz5223.ps1 script I will take this in consideration and look if I can improve it in the next release.

Can you verify if this is the same case in CIS31?

Thank you.

~LvdW

aquiros17 commented 1 month ago

Hi @asterictnl-lvdw,

Where can I find the scripts of CIS31? I can't see..

asterictnl-lvdw commented 1 month ago

@aquiros17 ,

Please clone the alpha branch to test that out. I haven't released that to master yet. :)

~LvdW

asterictnl-lvdw commented 1 month ago

Hi @aquiros17 ,

I have scheduled this optimalisation for the upcoming bugfix release.

~LvdW

asterictnl-lvdw commented 1 month ago

Hello @aquiros17 ,

According to the Conditional Access Policy Optimal configuration I have written the following: $OptimalPolicy = Get-MgIdentityConditionalAccessPolicy | Where-Object { ($_.Conditions.Users.IncludeUsers -eq 'All') -and ($_.Conditions.Users.ExcludeUsers.Count -igt 1) -and ($_.Conditions.Applications.IncludeApplications -eq "All") -and ($_.Conditions.ClientAppTypes -contains "exchangeActiveSync" -and "other") -and $_.GrantControls.BuiltInControls -eq "block"}

The audit says that you can exclude one low risk account or directory role if you wish as a best-practice. Since users and directory roles have unique ID's I cannot validate onto that. You have to determine yourself what is low-risk as this is a relative.

I have pushed the change for CIS30 and CIS31 to Alpha and will be included in the next upcoming release v2.3.

~LvdW

asterictnl-lvdw commented 1 month ago

@aquiros17 ,

Please test on the master branch for me now if the Conditional Access Policy is now more accurate. Can you also check other inspector modules for me, please? Any feedback or improvements are very welcome <3

~LvdW

asterictnl-lvdw commented 3 weeks ago

Issue has been closed and fixed. If the problem has not been fixed, feel free to re-open the issue.

aquiros17 commented 2 weeks ago

Hi @asterictnl-lvdw, the problem has not been fixed. You have a lot of controls based on name of conditional access policies (for example, identity protection, mfa for admin roles, sign in frequency, etc.). For this reason, this controls are not accurated as expected, because the controls must be based on conditions and policies of conditional access, exchange, sharepoint, intune, defender, purview, etc. If the controls are based on name of conditional access policies, your report generates a lot of false positives.

The idea is great but I think that you need to redefine your code based on conditions and policies, like 365inspect ;)

asterictnl-lvdw commented 2 weeks ago

Hello @aquiros17 ,

Please let me know which ones they are and I will check upon them.

I want to know if 5223 is fixed or not. Because I have implemented the script above. This does not check upon name anymore but upon the settings and if one with the specific setting is existing within the tenant, if that is not the case then it indeed should fail as expected. Even that you have something in place that does not mean it is compliant.

I will look forward to the list so I can look into them.

Thank you.

~LvdW

asterictnl-lvdw commented 8 hours ago

@aquiros17 ,

Please provide me the list of faulty inspectors so I will change the code to be based on the configuration instead of naming.

Thank you.

~LvdW