Replace handles that the handle closer closes with dummy Events.

sandbox/win/src/handle_closer_agent.cc

Index: sandbox/win/src/handle_closer_agent.cc
diff --git a/sandbox/win/src/handle_closer_agent.cc b/sandbox/win/src/handle_closer_agent.cc
index 07c6a09854d2d5355a84f30f782ec57f1a04f3ab..26f0a73e03c87b6f005d62e655861837fa3df9ae 100644
--- a/sandbox/win/src/handle_closer_agent.cc
+++ b/sandbox/win/src/handle_closer_agent.cc
@@ -30,6 +30,44 @@ NTSTATUS QueryObjectTypeInformation(HANDLE handle,
   return status;
 }
 
+bool AttemptToStuffHandleSlot(HANDLE to_stuff, const base::string16& type) {

[cpu_(ooo_6.6-7.5), 2015/02/18 18:00:00]

to_stuff  -> closed_handle ?

[Will Harris, 2015/02/18 22:11:28]

Done.

+  // Only attempt to stuff Files and Events at the moment.
+  if (type != L"Event" &&
+      type != L"File") {
+    return true;
+  }
+
+  HANDLE dummy = ::CreateEvent(NULL, FALSE, FALSE, NULL);

[forshaw, 2015/02/18 10:50:10]

Can we just use one event object for all stuffed handles in a process? Each new
event does consume some pool in the kernel for the event object which while not
a massive burden especially on x64 there seems no reason to waste memory
unnecessarily. The memory footprint of a process handle entry is ultimately
lower than an event object. I can't see a security/compatibility risk of doing
this considering what we're already breaking.

[cpu_(ooo_6.6-7.5), 2015/02/18 18:00:00]

given that is single threaded we can indeed use a function static and make it
once.  The just dup dup dup

[Will Harris, 2015/02/18 22:11:29]

we can't create the template handle static in this function because then it will
be likely to be the same handle ID as the one we just closed, so instead I
create it in constructor, and then close it again after we're done with the
handle closer agent.

+
+  if (dummy == INVALID_HANDLE_VALUE)

[forshaw, 2015/02/18 10:50:10]

While unlikely to fail CreateEvent actually returns NULL on error not
INVALID_HANDLE_VALUE.

[Will Harris, 2015/02/18 22:11:29]

Done.

+    return false;
+
+  std::vector<HANDLE> to_close;
+  DWORD options = DUPLICATE_SAME_ACCESS;

[Sigurður Ásgeirsson, 2015/02/18 13:56:00]

looks pretty reasonable - you may want to document the assumptions this relies
on, namely what the handle reuse policy is, and the reliance on
single-threadedness for this to work reliably.

[Will Harris, 2015/02/18 22:11:29]

changed this to 0 access as per other comments.

+
+  while (reinterpret_cast<uintptr_t>(dummy) <
+         reinterpret_cast<uintptr_t>(to_stuff)) {
+    HANDLE dup_dummy;

[cpu_(ooo_6.6-7.5), 2015/02/18 18:00:00]

move dup_dummy below 51

[Will Harris, 2015/02/18 22:11:29]

Acknowledged.

+    to_close.push_back(dummy);
+
+    if (!::DuplicateHandle(::GetCurrentProcess(), dummy, ::GetCurrentProcess(),
+                           &dup_dummy, 0, false, options))
+      break;

[Sigurður Ásgeirsson, 2015/02/18 19:02:47]

come to think of it, another check you may want to add here is that you're
making forward progress - e.g. you expect dup_dummy > dummy.
That way you won't be caught infinitely looping, no matter what...

[Will Harris, 2015/02/18 22:11:28]

We can't do this because we don't know if the handle we are trying to close
might have been created before the handle closer initialized the dummy template
handle, and thus already be lower.

In experiments I've found that the first DuplicateHandle always dups into the
target handle ID.

+    dummy = dup_dummy;
+  }
+
+  if (dummy != to_stuff)
+    to_close.push_back(dummy);
+
+  for (auto h : to_close)
+    ::CloseHandle(h);
+
+  // We want to know when we're not able to stuff handles.
+  DCHECK(dummy == to_stuff);
+
+  return dummy == to_stuff;
+}
+
 }  // namespace
 
 namespace sandbox {
@@ -136,6 +174,8 @@ bool HandleCloserAgent::CloseHandles() {
         return false;
       if (!::CloseHandle(handle))
         return false;
+      // Attempt to stuff this handle with a new empty Event.

[cpu_(ooo_6.6-7.5), 2015/02/18 18:00:00]

"new dummy event"

[Will Harris, 2015/02/18 22:11:29]

Done.

+      AttemptToStuffHandleSlot(handle, result->first);
     }
   }