diff options
author | mseaborn@chromium.org <mseaborn@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-03 09:15:02 +0000 |
---|---|---|
committer | mseaborn@chromium.org <mseaborn@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-02-03 09:15:02 +0000 |
commit | 7d80efdebd6fdea4d9bd5eb445afe952f2b48598 (patch) | |
tree | 955d2fd28a27c9a138c760db3bbeafadf4acd988 /sandbox/win | |
parent | 1d88aea97c70078531760e3063908082b137faa8 (diff) | |
download | chromium_src-7d80efdebd6fdea4d9bd5eb445afe952f2b48598.zip chromium_src-7d80efdebd6fdea4d9bd5eb445afe952f2b48598.tar.gz chromium_src-7d80efdebd6fdea4d9bd5eb445afe952f2b48598.tar.bz2 |
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
Diffstat (limited to 'sandbox/win')
-rw-r--r-- | sandbox/win/sandbox_win.gypi | 1 | ||||
-rw-r--r-- | sandbox/win/src/broker_services.cc | 33 | ||||
-rw-r--r-- | sandbox/win/src/handle_inheritance_test.cc | 54 | ||||
-rw-r--r-- | sandbox/win/src/sandbox_policy.h | 8 | ||||
-rw-r--r-- | sandbox/win/src/sandbox_policy_base.cc | 38 | ||||
-rw-r--r-- | sandbox/win/src/sandbox_policy_base.h | 7 | ||||
-rw-r--r-- | sandbox/win/src/target_process.cc | 3 | ||||
-rw-r--r-- | sandbox/win/src/target_process.h | 1 |
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); |