Enable DEP earlier on Vista and below

sandbox/win/src/process_mitigations.cc

Index: sandbox/win/src/process_mitigations.cc
===================================================================
--- sandbox/win/src/process_mitigations.cc	(revision 157852)
+++ sandbox/win/src/process_mitigations.cc	(working copy)
@@ -6,7 +6,9 @@
 
 #include "base/win/windows_version.h"
 #include "sandbox/win/src/nt_internals.h"
+#include "sandbox/win/src/sandbox_types.h"
 #include "sandbox/win/src/sandbox_utils.h"
+#include "sandbox/win/src/target_process.h"
 #include "sandbox/win/src/win_utils.h"
 
 namespace {
@@ -22,6 +24,11 @@
 typedef BOOL (WINAPI *SetDefaultDllDirectoriesFunction)(
     DWORD DirectoryFlags);
 
+void CALLBACK ApplyMitigationsCallback(ULONG_PTR flags) {
+  if (!sandbox::ApplyProcessMitigationsToCurrentProcess(flags))
+    ::TerminateProcess(::GetCurrentProcess(), sandbox::SBOX_FATAL_MITIGATION);
+}
+
 }  // namespace
 
 namespace sandbox {
@@ -237,42 +244,24 @@
 }
 
 MitigationFlags FilterPostStartupProcessMitigations(MitigationFlags flags) {
-  // Anything prior to XP SP2.
-  if (!IsXPSP2OrLater())
-    return 0;
-
   base::win::Version version = base::win::GetVersion();
 
-  // Windows XP SP2+.
   if (version < base::win::VERSION_VISTA) {
-    return flags & (MITIGATION_DEP |
-                    MITIGATION_DEP_NO_ATL_THUNK);
+    return 0;
 
-  // Windows Vista
-  if (version < base::win::VERSION_WIN7) {
-    return flags & (MITIGATION_DEP |
-                    MITIGATION_DEP_NO_ATL_THUNK |
-                    MITIGATION_BOTTOM_UP_ASLR |

[rvargas (doing something else), 2012/10/03 02:42:37]

Are you losing these?

[jschuh, 2012/10/03 03:00:06]

No. The DEP flags don't need to be passed due to this patch, and the bottom-up
ASLR flags were a mistake.

-                    MITIGATION_DLL_SEARCH_ORDER |
-                    MITIGATION_HEAP_TERMINATE);
-  }
-
-  // Windows 7 and Vista.
   } else if (version < base::win::VERSION_WIN8) {
-    return flags & (MITIGATION_BOTTOM_UP_ASLR |
-                    MITIGATION_DLL_SEARCH_ORDER |
+    return flags & (MITIGATION_DLL_SEARCH_ORDER |
                     MITIGATION_HEAP_TERMINATE);
   }
 
   // Windows 8 and above.
-  return flags & (MITIGATION_BOTTOM_UP_ASLR |
-                  MITIGATION_DLL_SEARCH_ORDER);
+  return flags & (MITIGATION_DLL_SEARCH_ORDER);
 }
 
-bool ApplyProcessMitigationsToSuspendedProcess(HANDLE process,
-                                               MitigationFlags flags) {
-// This is a hack to fake a weak bottom-up ASLR on 32-bit Windows.
+bool ApplyProcessMitigationsToSuspendedTarget(TargetProcess* target,
+                                              MitigationFlags flags) {
 #if !defined(_WIN64)
+  // This is a hack to fake a weak bottom-up ASLR on 32-bit Windows.
   if (flags & MITIGATION_BOTTOM_UP_ASLR) {
     unsigned int limit;
     rand_s(&limit);
@@ -280,6 +269,7 @@
     const size_t kMask64k = 0xFFFF;
     // Random range (512k-16.5mb) in 64k steps.
     const char* end = ptr + ((((limit % 16384) + 512) * 1024) & ~kMask64k);
+    HANDLE process = target->Process();
     while (ptr < end) {
       MEMORY_BASIC_INFORMATION memory_info;
       if (!::VirtualQueryEx(process, ptr, &memory_info, sizeof(memory_info)))
@@ -291,6 +281,16 @@
       ptr += size;
     }
   }
+
+  // Since the process is suspended, we can schedule an APC to set the DEP
+  // policy immediately after then loader finishes.

[rvargas (doing something else), 2012/10/03 02:42:37]

nit: the loader

[jschuh, 2012/10/03 03:00:06]

Done.

+  ULONG_PTR dep_flags = flags & (MITIGATION_DEP | MITIGATION_DEP_NO_ATL_THUNK);
+  if (dep_flags && base::win::GetVersion() < base::win::VERSION_WIN7) {
+    if (!::QueueUserAPC(ApplyMitigationsCallback, target->MainThread(),

[rvargas (doing something else), 2012/10/03 02:42:37]

I'm not fully convinced that this is a good idea, given that the process has not
started yet, and it is going to wake up through an APC. Yes, queuing another APC
_should_ not interfere with anything, but still... there's no guarantee that the
kernel won't use two APCs by itself (and that this one won't be in the middle).

On the other hand, what are we really gaining? We are applying DEP early on in
the process lifetime in any case, before any untrusted code can run... is your
concern our code, or the initialization code of all DLLs that we depend on?

[jschuh, 2012/10/03 03:00:06]

That's pretty much it. I want to do it before we explicitly load any DLLs. And
after lockdown it doesn't really matter, because we won't be loading anything
anyway.

[rvargas (doing something else), 2012/10/03 22:23:57]

But the bulk of the benefit is by protecting pages created after initialization,
right? I mean, the code executed at dll init should be minimal.

[rvargas (doing something else), 2012/10/11 23:04:44]

btw, this breaks with the SANDBOX_EXPORTS mode, but to be fair I wouldn't be
surprised if that's broken already.

+                        static_cast<ULONG_PTR>(dep_flags))) {
+      return false;
+    }
+  }
 #endif
 
   return true;