Convert process mitigations to delayed form.

Due to a bug in CreateProcess when enabling an AppContainer profile and
setting process mitigations at the same time CreateProcess will fail
with ERROR_INVALID_PARAMETER. This is due to CreateProcess internally
enabling some mitigations such as force image relocation. To try and
preserve the set mitigations we convert all possible to delayed
mitigations instead. This CL also fixes a typo in a member variable name.

Bug: 760977
Change-Id: I45f65c5ba5bab83270fcf113fbc6fbae66caa7a0
Reviewed-on: https://chromium-review.googlesource.com/902047Reviewed-by: Penny MacNeil <pennymac@chromium.org>
Commit-Queue: James Forshaw <forshaw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534754}

sandbox/win/src/app_container_test.cc

@@ -210,6 +210,8 @@ TEST_F(AppContainerProfileTest, CheckIncompatibleOptions) {
   EXPECT_EQ(SBOX_ERROR_BAD_PARAMS,
             policy_->SetIntegrityLevel(INTEGRITY_LEVEL_UNTRUSTED));
   EXPECT_EQ(SBOX_ERROR_BAD_PARAMS, policy_->SetLowBox(kAppContainerSid));
+  EXPECT_EQ(SBOX_ERROR_BAD_PARAMS,
+            policy_->SetProcessMitigations(MITIGATION_HEAP_TERMINATE));
 }
 
 TEST_F(AppContainerProfileTest, NoCapabilities) {

sandbox/win/src/process_mitigations.cc

@@ -431,20 +431,22 @@ bool ApplyProcessMitigationsToSuspendedProcess(HANDLE process,
   return true;
 }
 
+MitigationFlags GetAllowedPostStartupProcessMitigations() {
+  return MITIGATION_HEAP_TERMINATE | MITIGATION_DEP |
+         MITIGATION_DEP_NO_ATL_THUNK | MITIGATION_RELOCATE_IMAGE |
+         MITIGATION_RELOCATE_IMAGE_REQUIRED | MITIGATION_BOTTOM_UP_ASLR |
+         MITIGATION_STRICT_HANDLE_CHECKS | MITIGATION_EXTENSION_POINT_DISABLE |
+         MITIGATION_DLL_SEARCH_ORDER | MITIGATION_HARDEN_TOKEN_IL_POLICY |
+         MITIGATION_WIN32K_DISABLE | MITIGATION_DYNAMIC_CODE_DISABLE |
+         MITIGATION_DYNAMIC_CODE_DISABLE_WITH_OPT_OUT |
+         MITIGATION_FORCE_MS_SIGNED_BINS | MITIGATION_NONSYSTEM_FONT_DISABLE |
+         MITIGATION_IMAGE_LOAD_NO_REMOTE | MITIGATION_IMAGE_LOAD_NO_LOW_LABEL |
+         MITIGATION_IMAGE_LOAD_PREFER_SYS32;
+}
+
 bool CanSetProcessMitigationsPostStartup(MitigationFlags flags) {
   // All of these mitigations can be enabled after startup.
-  return !(
-      flags &
-      ~(MITIGATION_HEAP_TERMINATE | MITIGATION_DEP |
-        MITIGATION_DEP_NO_ATL_THUNK | MITIGATION_RELOCATE_IMAGE |
-        MITIGATION_RELOCATE_IMAGE_REQUIRED | MITIGATION_BOTTOM_UP_ASLR |
-        MITIGATION_STRICT_HANDLE_CHECKS | MITIGATION_EXTENSION_POINT_DISABLE |
-        MITIGATION_DLL_SEARCH_ORDER | MITIGATION_HARDEN_TOKEN_IL_POLICY |
-        MITIGATION_WIN32K_DISABLE | MITIGATION_DYNAMIC_CODE_DISABLE |
-        MITIGATION_DYNAMIC_CODE_DISABLE_WITH_OPT_OUT |
-        MITIGATION_FORCE_MS_SIGNED_BINS | MITIGATION_NONSYSTEM_FONT_DISABLE |
-        MITIGATION_IMAGE_LOAD_NO_REMOTE | MITIGATION_IMAGE_LOAD_NO_LOW_LABEL |
-        MITIGATION_IMAGE_LOAD_PREFER_SYS32));
+  return !(flags & ~GetAllowedPostStartupProcessMitigations());
 }
 
 bool CanSetProcessMitigationsPreStartup(MitigationFlags flags) {

sandbox/win/src/process_mitigations.h

@@ -38,6 +38,9 @@ void ConvertProcessMitigationsToPolicy(MitigationFlags flags,
 bool ApplyProcessMitigationsToSuspendedProcess(HANDLE process,
                                                MitigationFlags flags);
 
+// Returns the list of process mitigations which can be enabled post startup.
+MitigationFlags GetAllowedPostStartupProcessMitigations();
+
 // Returns true if all the supplied flags can be set after a process starts.
 bool CanSetProcessMitigationsPostStartup(MitigationFlags flags);
 

sandbox/win/src/sandbox_policy_base.cc

@@ -267,7 +267,7 @@ void PolicyBase::DestroyAlternateDesktop() {
 }
 
 ResultCode PolicyBase::SetIntegrityLevel(IntegrityLevel integrity_level) {
-  if (_app_container_profile)
+  if (app_container_profile_)
     return SBOX_ERROR_BAD_PARAMS;
   integrity_level_ = integrity_level;
   return SBOX_ALL_OK;
@@ -288,7 +288,7 @@ ResultCode PolicyBase::SetLowBox(const wchar_t* sid) {
     return SBOX_ERROR_UNSUPPORTED;
 
   DCHECK(sid);
-  if (lowbox_sid_ || _app_container_profile)
+  if (lowbox_sid_ || app_container_profile_)
     return SBOX_ERROR_BAD_PARAMS;
 
   if (!ConvertStringSidToSid(sid, &lowbox_sid_))
@@ -298,7 +298,7 @@ ResultCode PolicyBase::SetLowBox(const wchar_t* sid) {
 }
 
 ResultCode PolicyBase::SetProcessMitigations(MitigationFlags flags) {
-  if (!CanSetProcessMitigationsPreStartup(flags))
+  if (app_container_profile_ || !CanSetProcessMitigationsPreStartup(flags))
     return SBOX_ERROR_BAD_PARAMS;
   mitigations_ = flags;
   return SBOX_ALL_OK;
@@ -603,16 +603,23 @@ ResultCode PolicyBase::SetAppContainerProfile(AppContainerProfile* profile) {
     return SBOX_ERROR_UNSUPPORTED;
 
   DCHECK(profile);
-  if (lowbox_sid_ || _app_container_profile ||
+  if (lowbox_sid_ || app_container_profile_ ||
       integrity_level_ != INTEGRITY_LEVEL_LAST)
     return SBOX_ERROR_BAD_PARAMS;
 
-  _app_container_profile = profile;
+  app_container_profile_ = profile;
+  // A bug exists in CreateProcess where enabling an AppContainer profile and
+  // passing a set of mitigation flags will generate ERROR_INVALID_PARAMETER.
+  // Apply best efforts here and convert set mitigations to delayed mitigations.
+  delayed_mitigations_ =
+      mitigations_ & GetAllowedPostStartupProcessMitigations();
+  DCHECK(delayed_mitigations_ == (mitigations_ & ~MITIGATION_SEHOP));
+  mitigations_ = 0;
   return SBOX_ALL_OK;
 }
 
 scoped_refptr<AppContainerProfile> PolicyBase::GetAppContainerProfile() {
-  return _app_container_profile;
+  return app_container_profile_;
 }
 
 ResultCode PolicyBase::SetupAllInterceptions(TargetProcess* target) {

sandbox/win/src/sandbox_policy_base.h

@@ -174,7 +174,7 @@ class PolicyBase final : public TargetPolicy {
   base::HandlesToInheritVector handles_to_share_;
   bool enable_opm_redirection_;
 
-  scoped_refptr<AppContainerProfile> _app_container_profile;
+  scoped_refptr<AppContainerProfile> app_container_profile_;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyBase);
 };