Restructure sandbox code to reduce dependencies pulled in by intercept code.

sandbox/win/src/sandbox_nt_util.cc

Index: sandbox/win/src/sandbox_nt_util.cc
diff --git a/sandbox/win/src/sandbox_nt_util.cc b/sandbox/win/src/sandbox_nt_util.cc
index 71314611283e85057964617b191b3514f18b964e..7abecbe646299002f186ff36093b74d6a1fa7d6e 100644
--- a/sandbox/win/src/sandbox_nt_util.cc
+++ b/sandbox/win/src/sandbox_nt_util.cc
@@ -107,56 +107,6 @@ namespace sandbox {
 // Handle for our private heap.
 void* g_heap = NULL;
 
-SANDBOX_INTERCEPT HANDLE g_shared_section;
-SANDBOX_INTERCEPT size_t g_shared_IPC_size = 0;
-SANDBOX_INTERCEPT size_t g_shared_policy_size = 0;
-
-void* volatile g_shared_policy_memory = NULL;
-void* volatile g_shared_IPC_memory = NULL;
-
-// Both the IPC and the policy share a single region of memory in which the IPC
-// memory is first and the policy memory is last.
-bool MapGlobalMemory() {

[rvargas (doing something else), 2013/11/26 20:42:12]

I don't see anything in this block of code that should cause a dependency from
base. What am I missing?

This should be usable directly from nt code (without kernel32) because it is
called from nt interceptions. If that is not the case, it has to be fixed :(

[robertshield, 2013/11/27 20:28:01]

The code in just this block doesn't actually pull in base (that was another
bit), but it does pull in some user32 symbols (__imp__GetDesktopWindow,
__imp__CloseWindowStation) etc. 

This happens because of the g_shared_section vars above which is defined in
another compilation unit (sandbox.obj) which itself has user32 dependencies. 

Found that a nice fix to this is to have the globals that are shared between
compilation units be defined in their own cu that itself incurs no extra
dependencies, so added sandbox_globals.cc. Let me know what you think.

-  if (NULL == g_shared_IPC_memory) {
-    void* memory = NULL;
-    SIZE_T size = 0;
-    // Map the entire shared section from the start.
-    NTSTATUS ret = g_nt.MapViewOfSection(g_shared_section, NtCurrentProcess,
-                                         &memory, 0, 0, NULL, &size, ViewUnmap,
-                                         0, PAGE_READWRITE);
-
-    if (!NT_SUCCESS(ret) || NULL == memory) {
-      NOTREACHED_NT();
-      return false;
-    }
-
-    if (NULL != _InterlockedCompareExchangePointer(&g_shared_IPC_memory,
-                                                   memory, NULL)) {
-        // Somebody beat us to the memory setup.
-        ret = g_nt.UnmapViewOfSection(NtCurrentProcess, memory);
-        VERIFY_SUCCESS(ret);
-    }
-    DCHECK_NT(g_shared_IPC_size > 0);
-    g_shared_policy_memory = reinterpret_cast<char*>(g_shared_IPC_memory)
-                             + g_shared_IPC_size;
-  }
-  DCHECK_NT(g_shared_policy_memory);
-  DCHECK_NT(g_shared_policy_size > 0);
-  return true;
-}
-
-void* GetGlobalIPCMemory() {
-  if (!MapGlobalMemory())
-    return NULL;
-  return g_shared_IPC_memory;
-}
-
-void* GetGlobalPolicyMemory() {
-  if (!MapGlobalMemory())
-    return NULL;
-  return g_shared_policy_memory;
-}
-
 bool InitHeap() {
   if (!g_heap) {
     // Create a new heap using default values for everything.
@@ -205,74 +155,6 @@ bool ValidParameter(void* buffer, size_t size, RequiredAccess intent) {
   return true;
 }
 
-NTSTATUS CopyData(void* destination, const void* source, size_t bytes) {
-  NTSTATUS ret = STATUS_SUCCESS;
-  __try {
-    if (SandboxFactory::GetTargetServices()->GetState()->InitCalled()) {

[rvargas (doing something else), 2013/11/26 20:42:12]

Same thing here... and ExtractModuleName... is it the issue that it calls
GetTargetServices? We should be able to provide that without a real
implementation of the rest of the sandbox in case that it matters, but all
external dependencies boil down to variables in memory that are supposed to be
set up by the parent process.

[robertshield, 2013/11/27 20:28:01]

Yes, the GetTargetServices() call is the problem. That causes sandbox.obj to be
preserved and that carries with it a bunch of depdendencies. I've split this
code out into sandbox_nt_util_base.cc which solves this a bit more nicely than
the previous patch.

[rvargas (doing something else), 2013/11/27 23:53:50]

Carlos may have an opinion here, but I'd say we could either have a cached
pointer somewhere (instead of going into TargetServicesBase::GetInstance()
everytime), or split target_services into methods that depend on kerenl/user and
methods that don't.

[robertshield, 2013/11/29 01:21:26]

I believe this is now solved by your observation that we can use ntdll's memcpy.

-      memcpy(destination, source, bytes);
-    } else {
-      const char* from = reinterpret_cast<const char*>(source);
-      char* to = reinterpret_cast<char*>(destination);
-      for (size_t i = 0; i < bytes; i++) {
-        to[i] = from[i];
-      }
-    }
-  } __except(EXCEPTION_EXECUTE_HANDLER) {
-    ret = GetExceptionCode();
-  }
-  return ret;
-}
-
-// Hacky code... replace with AllocAndCopyObjectAttributes.
-NTSTATUS AllocAndCopyName(const OBJECT_ATTRIBUTES* in_object,
-                          wchar_t** out_name, uint32* attributes,
-                          HANDLE* root) {
-  if (!InitHeap())
-    return STATUS_NO_MEMORY;
-
-  DCHECK_NT(out_name);
-  *out_name = NULL;
-  NTSTATUS ret = STATUS_UNSUCCESSFUL;
-  __try {
-    do {
-      if (in_object->RootDirectory != static_cast<HANDLE>(0) && !root)
-        break;
-      if (NULL == in_object->ObjectName)
-        break;
-      if (NULL == in_object->ObjectName->Buffer)
-        break;
-
-      size_t size = in_object->ObjectName->Length + sizeof(wchar_t);
-      *out_name = new(NT_ALLOC) wchar_t[size/sizeof(wchar_t)];
-      if (NULL == *out_name)
-        break;
-
-      ret = CopyData(*out_name, in_object->ObjectName->Buffer,
-                     size - sizeof(wchar_t));
-      if (!NT_SUCCESS(ret))
-        break;
-
-      (*out_name)[size / sizeof(wchar_t) - 1] = L'\0';
-
-      if (attributes)
-        *attributes = in_object->Attributes;
-
-      if (root)
-        *root = in_object->RootDirectory;
-      ret = STATUS_SUCCESS;
-    } while (false);
-  } __except(EXCEPTION_EXECUTE_HANDLER) {
-    ret = GetExceptionCode();
-  }
-
-  if (!NT_SUCCESS(ret) && *out_name) {
-    operator delete(*out_name, NT_ALLOC);
-    *out_name = NULL;
-  }
-
-  return ret;
-}
-
 NTSTATUS GetProcessId(HANDLE process, ULONG *process_id) {
   PROCESS_BASIC_INFORMATION proc_info;
   ULONG bytes_returned;
@@ -431,55 +313,6 @@ UNICODE_STRING* GetBackingFilePath(PVOID address) {
   }
 }
 
-UNICODE_STRING* ExtractModuleName(const UNICODE_STRING* module_path) {
-  if ((!module_path) || (!module_path->Buffer))
-    return NULL;
-
-  wchar_t* sep = NULL;
-  int start_pos = module_path->Length / sizeof(wchar_t) - 1;
-  int ix = start_pos;
-
-  for (; ix >= 0; --ix) {
-    if (module_path->Buffer[ix] == L'\\') {
-      sep = &module_path->Buffer[ix];
-      break;
-    }
-  }
-
-  // Ends with path separator. Not a valid module name.
-  if ((ix == start_pos) && sep)
-    return NULL;
-
-  // No path separator found. Use the entire name.
-  if (!sep) {
-    sep = &module_path->Buffer[-1];
-  }
-
-  // Add one to the size so we can null terminate the string.
-  size_t size_bytes = (start_pos - ix + 1) * sizeof(wchar_t);
-
-  // Based on the code above, size_bytes should always be small enough
-  // to make the static_cast below safe.
-  DCHECK_NT(kuint16max > size_bytes);
-  char* str_buffer = new(NT_ALLOC) char[size_bytes + sizeof(UNICODE_STRING)];
-  if (!str_buffer)
-    return NULL;
-
-  UNICODE_STRING* out_string = reinterpret_cast<UNICODE_STRING*>(str_buffer);
-  out_string->Buffer = reinterpret_cast<wchar_t*>(&out_string[1]);
-  out_string->Length = static_cast<USHORT>(size_bytes - sizeof(wchar_t));
-  out_string->MaximumLength = static_cast<USHORT>(size_bytes);
-
-  NTSTATUS ret = CopyData(out_string->Buffer, &sep[1], out_string->Length);
-  if (!NT_SUCCESS(ret)) {
-    operator delete(out_string, NT_ALLOC);
-    return NULL;
-  }
-
-  out_string->Buffer[out_string->Length / sizeof(wchar_t)] = L'\0';
-  return out_string;
-}
-
 NTSTATUS AutoProtectMemory::ChangeProtection(void* address, size_t bytes,
                                              ULONG protect) {
   DCHECK_NT(!changed_);
@@ -546,6 +379,26 @@ bool IsSupportedRenameCall(FILE_RENAME_INFORMATION* file_info, DWORD length,
   return true;
 }
 
+bool WriteProtectedChildMemory(HANDLE child_process, void* address,
+                               const void* buffer, size_t length) {
+  // First, remove the protections.
+  DWORD old_protection;
+  if (!::VirtualProtectEx(child_process, address, length,

[rvargas (doing something else), 2013/11/26 20:42:12]

This, on the other hand, is not part of the nt layer (depends on kernel32) so it
cannot be here.

[robertshield, 2013/11/27 20:28:01]

Moved it back.

+                          PAGE_WRITECOPY, &old_protection))
+    return false;
+
+  SIZE_T written;
+  bool ok = ::WriteProcessMemory(child_process, address, buffer, length,
+                                 &written) && (length == written);
+
+  // Always attempt to restore the original protection.
+  if (!::VirtualProtectEx(child_process, address, length,
+                          old_protection, &old_protection))
+    return false;
+
+  return ok;
+}
+
 }  // namespace sandbox
 
 void* operator new(size_t size, sandbox::AllocationType type,