Bug 1820535: Refactor CIG sandbox policy set up. r=handyman

This moves the configuration into a separate function to simplify the main
policy settings functions and ensure that mitigations and policy rules are set
in the correct order.

Differential Revision: https://phabricator.services.mozilla.com/D171759

security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp

@@ -82,6 +82,8 @@ static sandbox::ResultCode AddWin32kLockdownPolicy(
   }
 
   sandbox::MitigationFlags flags = aPolicy->GetProcessMitigations();
+  MOZ_ASSERT(flags,
+             "Mitigations should be set before AddWin32kLockdownPolicy.");
   MOZ_ASSERT(!(flags & sandbox::MITIGATION_WIN32K_DISABLE),
              "Check not enabling twice.  Should not happen.");
 
@@ -466,12 +468,13 @@ static const Maybe<Vector<const wchar_t*>>& GetPrespawnCigExceptionModules() {
 #endif
 }
 
-static sandbox::ResultCode InitSignedPolicyRulesToBypassCig(
+static sandbox::ResultCode AllowProxyLoadFromBinDir(
-    sandbox::TargetPolicy* aPolicy,
+    sandbox::TargetPolicy* aPolicy) {
-    const Vector<const wchar_t*>& aExceptionModules) {
+  // Allow modules in the directory containing the executable such as
+  // mozglue.dll, nss3.dll, etc.
   static UniquePtr<nsString> sInstallDir;
   if (!sInstallDir) {
-    // Since this function is called before sBinDir is initialized,
+    // Since this function can be called before sBinDir is initialized,
     // we cache the install path by ourselves.
     UniquePtr<wchar_t[]> appDirStr;
     if (GetInstallDirectory(appDirStr)) {
@@ -495,21 +498,53 @@ static sandbox::ResultCode InitSignedPolicyRulesToBypassCig(
       return sandbox::SBOX_ERROR_GENERIC;
     }
   }
+  return aPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_SIGNED_BINARY,
+                          sandbox::TargetPolicy::SIGNED_ALLOW_LOAD,
+                          sInstallDir->get());
+}
 
-  // Allow modules in the directory containing the executable such as
+static sandbox::ResultCode AddCigToPolicy(
-  // mozglue.dll, nss3.dll, etc.
+    sandbox::TargetPolicy* aPolicy, bool aAlwaysProxyBinDirLoading = false) {
-  auto result = aPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_SIGNED_BINARY,
+  const Maybe<Vector<const wchar_t*>>& exceptionModules =
-                                 sandbox::TargetPolicy::SIGNED_ALLOW_LOAD,
+      GetPrespawnCigExceptionModules();
-                                 sInstallDir->get());
+  if (exceptionModules.isNothing()) {
+    sandbox::MitigationFlags delayedMitigations =
+        aPolicy->GetDelayedProcessMitigations();
+    MOZ_ASSERT(delayedMitigations,
+               "Delayed mitigations should be set before AddCigToPolicy.");
+    MOZ_ASSERT(!(delayedMitigations & sandbox::MITIGATION_FORCE_MS_SIGNED_BINS),
+               "AddCigToPolicy should not be called twice.");
+
+    delayedMitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
+    sandbox::ResultCode result =
+        aPolicy->SetDelayedProcessMitigations(delayedMitigations);
+    if (result != sandbox::SBOX_ALL_OK) {
+      return result;
+    }
+
+    if (aAlwaysProxyBinDirLoading) {
+      result = AllowProxyLoadFromBinDir(aPolicy);
+    }
+    return result;
+  }
+
+  sandbox::MitigationFlags mitigations = aPolicy->GetProcessMitigations();
+  MOZ_ASSERT(mitigations, "Mitigations should be set before AddCigToPolicy.");
+  MOZ_ASSERT(!(mitigations & sandbox::MITIGATION_FORCE_MS_SIGNED_BINS),
+             "AddCigToPolicy should not be called twice.");
+
+  mitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
+  sandbox::ResultCode result = aPolicy->SetProcessMitigations(mitigations);
   if (result != sandbox::SBOX_ALL_OK) {
     return result;
   }
 
-  if (aExceptionModules.empty()) {
+  result = AllowProxyLoadFromBinDir(aPolicy);
-    return sandbox::SBOX_ALL_OK;
+  if (result != sandbox::SBOX_ALL_OK) {
+    return result;
   }
 
-  for (const wchar_t* path : aExceptionModules) {
+  for (const wchar_t* path : exceptionModules.ref()) {
     result = aPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_SIGNED_BINARY,
                               sandbox::TargetPolicy::SIGNED_ALLOW_LOAD, path);
     if (result != sandbox::SBOX_ALL_OK) {
@@ -1060,12 +1095,6 @@ bool SandboxBroker::SetSecurityLevelForRDDProcess() {
       sandbox::MITIGATION_DEP_NO_ATL_THUNK | sandbox::MITIGATION_DEP |
       sandbox::MITIGATION_IMAGE_LOAD_PREFER_SYS32;
 
-  const Maybe<Vector<const wchar_t*>>& exceptionModules =
-      GetPrespawnCigExceptionModules();
-  if (exceptionModules.isSome()) {
-    mitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-  }
-
   if (StaticPrefs::security_sandbox_rdd_shadow_stack_enabled()) {
     mitigations |= sandbox::MITIGATION_CET_COMPAT_MODE;
   }
@@ -1073,13 +1102,6 @@ bool SandboxBroker::SetSecurityLevelForRDDProcess() {
   result = mPolicy->SetProcessMitigations(mitigations);
   SANDBOX_ENSURE_SUCCESS(result, "Invalid flags for SetProcessMitigations.");
 
-  if (exceptionModules.isSome()) {
-    // This needs to be called after MITIGATION_FORCE_MS_SIGNED_BINS is set
-    // because of DCHECK in PolicyBase::AddRuleInternal.
-    result = InitSignedPolicyRulesToBypassCig(mPolicy, exceptionModules.ref());
-    SANDBOX_ENSURE_SUCCESS(result, "Failed to initialize signed policy rules.");
-  }
-
   mitigations = sandbox::MITIGATION_STRICT_HANDLE_CHECKS |
                 sandbox::MITIGATION_DLL_SEARCH_ORDER;
 
@@ -1088,14 +1110,13 @@ bool SandboxBroker::SetSecurityLevelForRDDProcess() {
     mitigations |= DynamicCodeFlagForSystemMediaLibraries();
   }
 
-  if (exceptionModules.isNothing()) {
-    mitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-  }
-
   result = mPolicy->SetDelayedProcessMitigations(mitigations);
   SANDBOX_ENSURE_SUCCESS(result,
                          "Invalid flags for SetDelayedProcessMitigations.");
 
+  result = AddCigToPolicy(mPolicy);
+  SANDBOX_ENSURE_SUCCESS(result, "Failed to initialize signed policy rules.");
+
   // Add the policy for the client side of a pipe. It is just a file
   // in the \pipe\ namespace. We restrict it to pipes that start with
   // "chrome." so the sandboxed process cannot connect to system services.
@@ -1178,12 +1199,6 @@ bool SandboxBroker::SetSecurityLevelForSocketProcess() {
       sandbox::MITIGATION_DEP_NO_ATL_THUNK | sandbox::MITIGATION_DEP |
       sandbox::MITIGATION_IMAGE_LOAD_PREFER_SYS32;
 
-  const Maybe<Vector<const wchar_t*>>& exceptionModules =
-      GetPrespawnCigExceptionModules();
-  if (exceptionModules.isSome()) {
-    mitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-  }
-
   if (StaticPrefs::security_sandbox_socket_shadow_stack_enabled()) {
     mitigations |= sandbox::MITIGATION_CET_COMPAT_MODE;
   }
@@ -1191,13 +1206,6 @@ bool SandboxBroker::SetSecurityLevelForSocketProcess() {
   result = mPolicy->SetProcessMitigations(mitigations);
   SANDBOX_ENSURE_SUCCESS(result, "Invalid flags for SetProcessMitigations.");
 
-  if (exceptionModules.isSome()) {
-    // This needs to be called after MITIGATION_FORCE_MS_SIGNED_BINS is set
-    // because of DCHECK in PolicyBase::AddRuleInternal.
-    result = InitSignedPolicyRulesToBypassCig(mPolicy, exceptionModules.ref());
-    SANDBOX_ENSURE_SUCCESS(result, "Failed to initialize signed policy rules.");
-  }
-
   if (StaticPrefs::security_sandbox_socket_win32k_disable()) {
     result = AddWin32kLockdownPolicy(mPolicy, false);
     SANDBOX_ENSURE_SUCCESS(result, "Failed to add the win32k lockdown policy");
@@ -1207,14 +1215,13 @@ bool SandboxBroker::SetSecurityLevelForSocketProcess() {
                 sandbox::MITIGATION_DLL_SEARCH_ORDER |
                 sandbox::MITIGATION_DYNAMIC_CODE_DISABLE;
 
-  if (exceptionModules.isNothing()) {
-    mitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-  }
-
   result = mPolicy->SetDelayedProcessMitigations(mitigations);
   SANDBOX_ENSURE_SUCCESS(result,
                          "Invalid flags for SetDelayedProcessMitigations.");
 
+  result = AddCigToPolicy(mPolicy);
+  SANDBOX_ENSURE_SUCCESS(result, "Failed to initialize signed policy rules.");
+
   // Add the policy for the client side of a pipe. It is just a file
   // in the \pipe\ namespace. We restrict it to pipes that start with
   // "chrome." so the sandboxed process cannot connect to system services.
@@ -1382,53 +1389,26 @@ bool BuildUtilitySandbox(sandbox::TargetPolicy* policy,
   policy->SetLockdownDefaultDacl();
   policy->AddRestrictingRandomSid();
 
-  sandbox::MitigationFlags initialMitigations = us.mInitialMitigations;
+  result = policy->SetProcessMitigations(us.mInitialMitigations);
-  sandbox::MitigationFlags delayedMitigations = us.mDelayedMitigations;
-
-  if (us.mUseCig) {
-    const Maybe<Vector<const wchar_t*>>& exceptionModules =
-        GetPrespawnCigExceptionModules();
-    if (exceptionModules.isSome()) {
-      initialMitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-    } else {
-      delayedMitigations |= sandbox::MITIGATION_FORCE_MS_SIGNED_BINS;
-    }
-  }
-
-  result = policy->SetProcessMitigations(initialMitigations);
   SANDBOX_ENSURE_SUCCESS(result, "Invalid flags for SetProcessMitigations.");
 
+  result = policy->SetDelayedProcessMitigations(us.mDelayedMitigations);
+  SANDBOX_ENSURE_SUCCESS(result,
+                         "Invalid flags for SetDelayedProcessMitigations.");
+
   // Win32k lockdown might not work on earlier versions
   // Bug 1719212, 1769992
-  if (IsWin10FallCreatorsUpdateOrLater() && us.mUseWin32kLockdown) {
+  if (us.mUseWin32kLockdown && IsWin10FallCreatorsUpdateOrLater()) {
     result = AddWin32kLockdownPolicy(policy, false);
     SANDBOX_ENSURE_SUCCESS(result, "Failed to add the win32k lockdown policy");
   }
 
   if (us.mUseCig) {
-    const Maybe<Vector<const wchar_t*>>& exceptionModules =
+    bool alwaysProxyBinDirLoading = mozilla::HasPackageIdentity();
-        GetPrespawnCigExceptionModules();
+    result = AddCigToPolicy(policy, alwaysProxyBinDirLoading);
-    if (exceptionModules.isSome()) {
+    SANDBOX_ENSURE_SUCCESS(result, "Failed to initialize signed policy rules.");
-      // This needs to be called after MITIGATION_FORCE_MS_SIGNED_BINS is set
-      // because of DCHECK in PolicyBase::AddRuleInternal.
-      result = InitSignedPolicyRulesToBypassCig(policy, exceptionModules.ref());
-      SANDBOX_ENSURE_SUCCESS(result,
-                             "Failed to initialize signed policy rules.");
-    }
-
-    // Running audio decoder somehow fails on MSIX packages unless we do that
-    if (mozilla::HasPackageIdentity() && exceptionModules.isNothing()) {
-      const Vector<const wchar_t*> emptyVector;
-      result = InitSignedPolicyRulesToBypassCig(policy, emptyVector);
-      SANDBOX_ENSURE_SUCCESS(result,
-                             "Failed to initialize signed policy rules.");
-    }
   }
 
-  result = policy->SetDelayedProcessMitigations(delayedMitigations);
-  SANDBOX_ENSURE_SUCCESS(result,
-                         "Invalid flags for SetDelayedProcessMitigations.");
-
   // Add the policy for the client side of a pipe. It is just a file
   // in the \pipe\ namespace. We restrict it to pipes that start with
   // "chrome." so the sandboxed process cannot connect to system services.