Windows: Allow subprocesses to inherit non-console stdout/stderr

Before this change, the renderer and other subprocesses never inherit
stdout and stderr when they are pipe handles.  Stdout/stderr will be
pipe handles when chrome.exe/browser_tests.exe is running under
Buildbot or under Cygwin's default terminal, mintty.

We fix this by specifying PROC_THREAD_ATTRIBUTE_HANDLE_LIST in the
arguments to CreateProcess().

The fix only applies on Windows >=Vista.

Although it's probably safe for stdout/stderr to be inherited when it
is a pipe handle or file handle, we put this behind the flag
"--enable-logging".  (This flag already makes stderr work when
chrome.exe/browser_tests.exe is running under a Windows console -- a
case which is not handled by the code path we're adding here because a
Windows console is not an inheritable kernel handle.)

Note that this relies on the fix committed in http://crrev.com/178656.

BUG=171836
TEST=manually add logging to renderer process and check that it appears
  when running chrome.exe or browser_tests.exe


Review URL: https://chromiumcodereview.appspot.com/12033045

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@180303 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 142 insertions, 3 deletions

diff --git a/sandbox/win/sandbox_win.gypi b/sandbox/win/sandbox_win.gypi
index 620a0c0..abdc29c 100644
--- a/sandbox/win/sandbox_win.gypi
+++ b/sandbox/win/sandbox_win.gypi
@@ -209,6 +209,7 @@
       'sources': [
         'src/app_container_test.cc',
         'src/file_policy_test.cc',
+        'src/handle_inheritance_test.cc',
         'src/handle_policy_test.cc',
         'tests/integration_tests/integration_tests_test.cc',
         'src/handle_closer_test.cc',
diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index f547a5b..921eb4f 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -322,6 +322,7 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
         const_cast<wchar_t*>(desktop.c_str());
   }
 
+  bool inherit_handles = false;
   if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
     int attribute_count = 0;
     const AppContainerAttributes* app_container =
@@ -336,6 +337,18 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
     if (mitigations)
       ++attribute_count;
 
+    HANDLE stdout_handle = policy_base->GetStdoutHandle();
+    HANDLE stderr_handle = policy_base->GetStderrHandle();
+    HANDLE inherit_handle_list[2];
+    int inherit_handle_count = 0;
+    if (stdout_handle != INVALID_HANDLE_VALUE)
+      inherit_handle_list[inherit_handle_count++] = stdout_handle;
+    // Handles in the list must be unique.
+    if (stderr_handle != stdout_handle && stderr_handle != INVALID_HANDLE_VALUE)
+      inherit_handle_list[inherit_handle_count++] = stderr_handle;
+    if (inherit_handle_count)
+      ++attribute_count;
+
     if (!startup_info.InitializeProcThreadAttributeList(attribute_count))
       return SBOX_ERROR_PROC_THREAD_ATTRIBUTES;
 
@@ -352,6 +365,22 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
         return SBOX_ERROR_PROC_THREAD_ATTRIBUTES;
       }
     }
+
+    if (inherit_handle_count) {
+      if (!startup_info.UpdateProcThreadAttribute(
+              PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
+              inherit_handle_list,
+              sizeof(inherit_handle_list[0]) * inherit_handle_count)) {
+        return SBOX_ERROR_PROC_THREAD_ATTRIBUTES;
+      }
+      startup_info.startup_info()->dwFlags |= STARTF_USESTDHANDLES;
+      startup_info.startup_info()->hStdInput = INVALID_HANDLE_VALUE;
+      startup_info.startup_info()->hStdOutput = stdout_handle;
+      startup_info.startup_info()->hStdError = stderr_handle;
+      // Allowing inheritance of handles is only secure now that we
+      // have limited which handles will be inherited.
+      inherit_handles = true;
+    }
   }
 
   // Construct the thread pool here in case it is expensive.
@@ -367,8 +396,8 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
                                             job,
                                             thread_pool_);
 
-  DWORD win_result = target->Create(exe_path, command_line, startup_info,
-                                    &process_info);
+  DWORD win_result = target->Create(exe_path, command_line, inherit_handles,
+                                    startup_info, &process_info);
   if (ERROR_SUCCESS != win_result)
     return SpawnCleanup(target, win_result);
 
diff --git a/sandbox/win/src/handle_inheritance_test.cc b/sandbox/win/src/handle_inheritance_test.cc
new file mode 100644
index 0000000..cd9cae7
--- /dev/null
+++ b/sandbox/win/src/handle_inheritance_test.cc
@@ -0,0 +1,54 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stdio.h>
+
+#include "base/file_util.h"
+#include "base/win/windows_version.h"
+#include "sandbox/win/tests/common/controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sandbox {
+
+SBOX_TESTS_COMMAND int HandleInheritanceTests_PrintToStdout(int argc,
+                                                            wchar_t** argv) {
+  printf("Example output to stdout\n");
+  return SBOX_TEST_SUCCEEDED;
+}
+
+TEST(HandleInheritanceTests, TestStdoutInheritance) {
+  wchar_t temp_directory[MAX_PATH];
+  wchar_t temp_file_name[MAX_PATH];
+  ASSERT_NE(::GetTempPath(MAX_PATH, temp_directory), 0u);
+  ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
+
+  SECURITY_ATTRIBUTES attrs = {};
+  attrs.nLength = sizeof(attrs);
+  attrs.lpSecurityDescriptor = NULL;
+  attrs.bInheritHandle = TRUE;
+  HANDLE file_handle = CreateFile(
+      temp_file_name, GENERIC_WRITE,
+      FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+      &attrs, OPEN_EXISTING, 0, NULL);
+  EXPECT_NE(file_handle, INVALID_HANDLE_VALUE);
+
+  TestRunner runner;
+  EXPECT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetStdoutHandle(file_handle));
+  int result = runner.RunTest(L"HandleInheritanceTests_PrintToStdout");
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, result);
+  EXPECT_TRUE(::CloseHandle(file_handle));
+
+  std::string data;
+  EXPECT_TRUE(file_util::ReadFileToString(FilePath(temp_file_name), &data));
+  // Redirection uses a feature that was added in Windows Vista.
+  if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
+    EXPECT_EQ("Example output to stdout\r\n", data);
+  } else {
+    EXPECT_EQ("", data);
+  }
+
+  EXPECT_TRUE(::DeleteFile(temp_file_name));
+}
+
+}
diff --git a/sandbox/win/src/sandbox_policy.h b/sandbox/win/src/sandbox_policy.h
index f0fc2bc..733356a 100644
--- a/sandbox/win/src/sandbox_policy.h
+++ b/sandbox/win/src/sandbox_policy.h
@@ -187,6 +187,14 @@ class TargetPolicy {
   // refuse to perform the interception.
   virtual void SetStrictInterceptions() = 0;
 
+  // Set the handles the target process should inherit for stdout and
+  // stderr.  The handles the caller passes must remain valid for the
+  // lifetime of the policy object.  This only has an effect on
+  // Windows Vista and later versions.  These methods accept pipe and
+  // file handles, but not console handles.
+  virtual ResultCode SetStdoutHandle(HANDLE handle) = 0;
+  virtual ResultCode SetStderrHandle(HANDLE handle) = 0;
+
   // Adds a policy rule effective for processes spawned using this policy.
   // subsystem: One of the above enumerated windows subsystems.
   // semantics: One of the above enumerated FileSemantics.
diff --git a/sandbox/win/src/sandbox_policy_base.cc b/sandbox/win/src/sandbox_policy_base.cc
index 10ac642..aaff0a1 100644
--- a/sandbox/win/src/sandbox_policy_base.cc
+++ b/sandbox/win/src/sandbox_policy_base.cc
@@ -33,6 +33,7 @@
 #include "sandbox/win/src/window.h"
 
 namespace {
+
 // The standard windows size for one memory page.
 const size_t kOneMemPage = 4096;
 // The IPC and Policy shared memory sizes.
@@ -49,6 +50,19 @@ sandbox::PolicyGlobal* MakeBrokerPolicyMemory() {
   policy->data_size = kTotalPolicySz - sizeof(sandbox::PolicyGlobal);
   return policy;
 }
+
+bool IsInheritableHandle(HANDLE handle) {
+  if (!handle)
+    return false;
+  if (handle == INVALID_HANDLE_VALUE)
+    return false;
+  // File handles (FILE_TYPE_DISK) and pipe handles are known to be
+  // inheritable.  Console handles (FILE_TYPE_CHAR) are not
+  // inheritable via PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
+  DWORD handle_type = GetFileType(handle);
+  return handle_type == FILE_TYPE_DISK || handle_type == FILE_TYPE_PIPE;
+}
+
 }
 
 namespace sandbox {
@@ -70,6 +84,8 @@ PolicyBase::PolicyBase()
       use_alternate_winstation_(false),
       file_system_init_(false),
       relaxed_interceptions_(true),
+      stdout_handle_(INVALID_HANDLE_VALUE),
+      stderr_handle_(INVALID_HANDLE_VALUE),
       integrity_level_(INTEGRITY_LEVEL_LAST),
       delayed_integrity_level_(INTEGRITY_LEVEL_LAST),
       mitigations_(0),
@@ -308,6 +324,20 @@ void PolicyBase::SetStrictInterceptions() {
   relaxed_interceptions_ = false;
 }
 
+ResultCode PolicyBase::SetStdoutHandle(HANDLE handle) {
+  if (!IsInheritableHandle(handle))
+    return SBOX_ERROR_BAD_PARAMS;
+  stdout_handle_ = handle;
+  return SBOX_ALL_OK;
+}
+
+ResultCode PolicyBase::SetStderrHandle(HANDLE handle) {
+  if (!IsInheritableHandle(handle))
+    return SBOX_ERROR_BAD_PARAMS;
+  stderr_handle_ = handle;
+  return SBOX_ALL_OK;
+}
+
 ResultCode PolicyBase::AddRule(SubSystem subsystem, Semantics semantics,
                                const wchar_t* pattern) {
   if (NULL == policy_) {
@@ -567,6 +597,14 @@ EvalResult PolicyBase::EvalPolicy(int service,
   return DENY_ACCESS;
 }
 
+HANDLE PolicyBase::GetStdoutHandle() {
+  return stdout_handle_;
+}
+
+HANDLE PolicyBase::GetStderrHandle() {
+  return stderr_handle_;
+}
+
 // We service IPC_PING_TAG message which is a way to test a round trip of the
 // IPC subsystem. We receive a integer cookie and we are expected to return the
 // cookie times two (or three) and the current tick count.
diff --git a/sandbox/win/src/sandbox_policy_base.h b/sandbox/win/src/sandbox_policy_base.h
index efac6a0..0bb906e 100644
--- a/sandbox/win/src/sandbox_policy_base.h
+++ b/sandbox/win/src/sandbox_policy_base.h
@@ -58,6 +58,8 @@ class PolicyBase : public Dispatcher, public TargetPolicy {
       MitigationFlags flags) OVERRIDE;
   virtual MitigationFlags GetDelayedProcessMitigations() OVERRIDE;
   virtual void SetStrictInterceptions() OVERRIDE;
+  virtual ResultCode SetStdoutHandle(HANDLE handle) OVERRIDE;
+  virtual ResultCode SetStderrHandle(HANDLE handle) OVERRIDE;
   virtual ResultCode AddRule(SubSystem subsystem, Semantics semantics,
                              const wchar_t* pattern) OVERRIDE;
   virtual ResultCode AddDllToUnload(const wchar_t* dll_name);
@@ -90,6 +92,9 @@ class PolicyBase : public Dispatcher, public TargetPolicy {
 
   EvalResult EvalPolicy(int service, CountedParameterSetBase* params);
 
+  HANDLE GetStdoutHandle();
+  HANDLE GetStderrHandle();
+
  private:
   ~PolicyBase();
 
@@ -123,6 +128,8 @@ class PolicyBase : public Dispatcher, public TargetPolicy {
   // Helps the file system policy initialization.
   bool file_system_init_;
   bool relaxed_interceptions_;
+  HANDLE stdout_handle_;
+  HANDLE stderr_handle_;
   IntegrityLevel integrity_level_;
   IntegrityLevel delayed_integrity_level_;
   MitigationFlags mitigations_;
diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc
index 2bc1fec..9300cce 100644
--- a/sandbox/win/src/target_process.cc
+++ b/sandbox/win/src/target_process.cc
@@ -110,6 +110,7 @@ TargetProcess::~TargetProcess() {
 // object.
 DWORD TargetProcess::Create(const wchar_t* exe_path,
                             const wchar_t* command_line,
+                            bool inherit_handles,
                             const base::win::StartupInformation& startup_info,
                             base::win::ScopedProcessInformation* target_info) {
   exe_name_.reset(_wcsdup(exe_path));
@@ -137,7 +138,7 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
                               cmd_line.get(),
                               NULL,   // No security attribute.
                               NULL,   // No thread attribute.
-                              FALSE,  // Do not inherit handles.
+                              inherit_handles,
                               flags,
                               NULL,   // Use the environment of the caller.
                               NULL,   // Use current directory of the caller.
diff --git a/sandbox/win/src/target_process.h b/sandbox/win/src/target_process.h
index be4cd2a..0b72d98 100644
--- a/sandbox/win/src/target_process.h
+++ b/sandbox/win/src/target_process.h
@@ -47,6 +47,7 @@ class TargetProcess {
   // Creates the new target process. The process is created suspended.
   DWORD Create(const wchar_t* exe_path,
                const wchar_t* command_line,
+               bool inherit_handles,
                const base::win::StartupInformation& startup_info,
                base::win::ScopedProcessInformation* target_info);