CreateThread interception, to use CreateRemoteThread

sandbox/win/src/process_thread_policy.cc

Index: sandbox/win/src/process_thread_policy.cc
diff --git a/sandbox/win/src/process_thread_policy.cc b/sandbox/win/src/process_thread_policy.cc
index b4976c0bbebeaefc8cfb9cdf57c9533d2c3c0b96..e78608744fe8bd098bccf52ce29311611d754747 100644
--- a/sandbox/win/src/process_thread_policy.cc
+++ b/sandbox/win/src/process_thread_policy.cc
@@ -99,6 +99,9 @@ bool ProcessPolicy::GenerateRules(const wchar_t* name,
   if (!policy->AddRule(IPC_CREATEPROCESSW_TAG, process.get())) {
     return false;
   }
+  if (!policy->AddRule(IPC_CREATETHREAD_TAG, process.get())) {
+    return false;
+  }

[liamjm (20p), 2016/02/02 20:43:45]

cpu@: Can you comment on the suitability of adding create thread to this policy?

   return true;
 }
 
@@ -238,4 +241,31 @@ DWORD ProcessPolicy::CreateProcessWAction(EvalResult eval_result,
   return ERROR_SUCCESS;
 }
 
+DWORD ProcessPolicy::CreateThreadAction(

[forshaw, 2016/02/02 11:11:49]

Is this only going to be used in test code or will it end up in Chrome when ALPC
is eventually dropped? My concern is that many AV have a bit of a thing for
CreateRemoteThread, or at least used to. You might find you get spurious
failures in production code, or even in theory AV blocking Chrome for malicious
behavior. I don't have the context of why this is being implemented, so can't
comment if there's a better way.

[liamjm (20p), 2016/02/02 20:43:45]

This is the intended mechanism for CreateThread inside the renderer to work when
ALPC is dropped. Features are only enabled in tests at the moment.
Fair point re AV detection. 

+    EvalResult eval_result,
+    const ClientInfo& client_info,
+    const SIZE_T stack_size,
+    const LPTHREAD_START_ROUTINE start_address,
+    const LPVOID parameter,
+    const DWORD creation_flags,
+    LPDWORD thread_id,
+    HANDLE* handle) {
+  // The only action supported is ASK_BROKER which means create the process.

[forshaw, 2016/02/02 11:11:49]

nit: You mean thread not process?

[liamjm (20p), 2016/02/02 20:43:45]

Done.

+  if (GIVE_ALLACCESS != eval_result && GIVE_READONLY != eval_result) {
+    return ERROR_ACCESS_DENIED;
+  }
+  HANDLE local_handle =
+      CreateRemoteThread(client_info.process, nullptr, stack_size,
+                         start_address, parameter, creation_flags, thread_id);
+  if (!local_handle) {
+    return ::GetLastError();
+  }
+  if (!::DuplicateHandle(::GetCurrentProcess(), local_handle,
+                         client_info.process, handle, 0, FALSE,

[Will Harris, 2016/02/02 05:45:33]

should this 0 be THREAD_ALL_ACCESS if eval_result is GIVE_ALLACCESS?

[forshaw, 2016/02/02 11:11:49]

CreateRemoteThread should return a handle with THREAD_ALL_ACCESS rights, so I
think DUPLICATE_SAME_ACCESS makes the most sense. Especially as you cannot
change the rights to add additional ones as that will fail the DACL check (which
is based on the target process' token rather than the callers).

That said is there supposed to be a distinction between GIVE_ALLACCESS and
GIVE_READONLY?

[liamjm (20p), 2016/02/02 20:43:45]

If DUPLCIATE_SAME_ACCESS is specified below, then this arg is ignored.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251(v=vs.85).aspx

There is no difference between GIVE_ALLACCESSS and GIVE_READONLY.
I've added comments to the policies (PROCESS_MIN_EXEC and PROCESS_ALL_EXEC) in
sandbox_policy.h.

+                         DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {

[Will Harris, 2016/02/02 05:45:33]

I'm not sure this should be DUPLICATE_SAME_ACCESS

+    return ERROR_ACCESS_DENIED;
+  }
+  return ERROR_SUCCESS;
+}
+
 }  // namespace sandbox