Fix inherited handles conflicting with handle tracker

The inherited handles are stored in a ScopedHandle, so the handle tracker
marks them as non-closeable for the lifetime of that ScopedHandle in the
browser process. That non-closeable handle is inherited to the child, but
should be closeable there the child's is a new, valid handle.

We can't mark only the child copy as closeable, so for the duration of
target creation, mark the handle as closeable, and after process creation,
restore the handle protection state.

R=wfh@chromium.org
BUG=524267
TEST=HandleInheritanceTests.InheritByValue

Committed: https://crrev.com/bfca1f2f2e75e3d36a171ce9987e7a72bedfbd85
Cr-Commit-Position: refs/heads/master@{#346835}

Review URL: https://codereview.chromium.org/1318803005

Cr-Commit-Position: refs/heads/master@{#346955}

2 files changed, 64 insertions, 0 deletions

diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index 1c72ef5..1a4c2f3 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -498,6 +498,23 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   if (NULL == thread_pool_)
     thread_pool_ = new Win2kThreadPool();
 
+  // We need to temporarily mark all inherited handles as closeable. The handle
+  // tracker may have marked the handles we're passing to the child as
+  // non-closeable, but the child is getting new copies that it's allowed to
+  // close. We're about to mark these handles as closeable for this process
+  // (when we close them below in ClearSharedHandles()) but that will be too
+  // late -- there will already another copy in the child that's non-closeable.
+  // After launching we restore the non-closability of these handles. We don't
+  // have any way here to affect *only* the child's copy, as the process
+  // launching mechanism takes care of doing the duplication-with-the-same-value
+  // into the child.
+  std::vector<DWORD> inherited_handle_information(inherited_handle_list.size());
+  for (size_t i = 0; i < inherited_handle_list.size(); ++i) {
+    const HANDLE& inherited_handle = inherited_handle_list[i];
+    ::GetHandleInformation(inherited_handle, &inherited_handle_information[i]);
+    ::SetHandleInformation(inherited_handle, HANDLE_FLAG_PROTECT_FROM_CLOSE, 0);
+  }
+
   // Create the TargetProces object and spawn the target suspended. Note that
   // Brokerservices does not own the target object. It is owned by the Policy.
   base::win::ScopedProcessInformation process_info;
@@ -510,6 +527,13 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
                                     policy_base->GetLowBoxSid() ? true : false,
                                     startup_info, &process_info);
 
+  // Restore the previous handle protection values.
+  for (size_t i = 0; i < inherited_handle_list.size(); ++i) {
+    ::SetHandleInformation(inherited_handle_list[i],
+                           HANDLE_FLAG_PROTECT_FROM_CLOSE,
+                           inherited_handle_information[i]);
+  }
+
   policy_base->ClearSharedHandles();
 
   if (ERROR_SUCCESS != win_result) {
diff --git a/sandbox/win/src/handle_inheritance_test.cc b/sandbox/win/src/handle_inheritance_test.cc
index 37974e3..95a369a 100644
--- a/sandbox/win/src/handle_inheritance_test.cc
+++ b/sandbox/win/src/handle_inheritance_test.cc
@@ -6,6 +6,9 @@
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/memory/shared_memory.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "sandbox/win/tests/common/controller.h"
@@ -49,4 +52,41 @@ TEST(HandleInheritanceTests, TestStdoutInheritance) {
   }
 }
 
+SBOX_TESTS_COMMAND int
+HandleInheritanceTests_ValidInheritedHandle(int argc, wchar_t **argv) {
+  if (argc != 1)
+    return SBOX_TEST_FAILED_TO_RUN_TEST;
+  base::SharedMemoryHandle handle = nullptr;
+  base::StringToUint(argv[0], reinterpret_cast<unsigned int *>(&handle));
+
+  // This handle we inherited must be both valid and closeable.
+  DWORD flags = 0;
+  if (!GetHandleInformation(handle, &flags))
+    return SBOX_TEST_FAILED;
+  if ((flags & HANDLE_FLAG_PROTECT_FROM_CLOSE) != 0)
+    return SBOX_TEST_FAILED;
+
+  return SBOX_TEST_SUCCEEDED;
 }
+
+TEST(HandleInheritanceTests, InheritByValue) {
+  // Handle inheritance doesn't work on XP.
+  if (base::win::GetVersion() < base::win::VERSION_VISTA)
+    return;
+
+  base::SharedMemory test_shared_memory;
+  ASSERT_TRUE(test_shared_memory.CreateAnonymous(1000));
+  ASSERT_TRUE(test_shared_memory.Map(0));
+
+  TestRunner runner;
+  void *shared_handle =
+      runner.GetPolicy()->AddHandleToShare(test_shared_memory.handle());
+
+  std::string command_line =
+      "HandleInheritanceTests_ValidInheritedHandle " +
+      base::UintToString(reinterpret_cast<unsigned int>(shared_handle));
+  int result = runner.RunTest(base::UTF8ToUTF16(command_line).c_str());
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED, result);
+}
+
+}  // namespace sandbox