diff options
author | jschuh@chromium.org <jschuh@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-13 00:47:49 +0000 |
---|---|---|
committer | jschuh@chromium.org <jschuh@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-13 00:47:49 +0000 |
commit | 39ee02eaa291b0ece3f5fe3bd40855911e114c3d (patch) | |
tree | 38cbb81f0efca6aac48be1281d815e694d910e8a /sandbox/src | |
parent | aa72d27d1e2a19fbb9f06cd728c4cc2ea1aa53fc (diff) | |
download | chromium_src-39ee02eaa291b0ece3f5fe3bd40855911e114c3d.zip chromium_src-39ee02eaa291b0ece3f5fe3bd40855911e114c3d.tar.gz chromium_src-39ee02eaa291b0ece3f5fe3bd40855911e114c3d.tar.bz2 |
Add sandbox support for associating peer processes
TEST=HandlePolicyTest.DuplicatePeerHandle
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131940
Review URL: https://chromiumcodereview.appspot.com/9960045
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132112 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sandbox/src')
-rw-r--r-- | sandbox/src/broker_services.cc | 92 | ||||
-rw-r--r-- | sandbox/src/broker_services.h | 29 | ||||
-rw-r--r-- | sandbox/src/handle_policy_test.cc | 22 | ||||
-rw-r--r-- | sandbox/src/sandbox.h | 8 |
4 files changed, 138 insertions, 13 deletions
diff --git a/sandbox/src/broker_services.cc b/sandbox/src/broker_services.cc index d361c2e..5ee5620 100644 --- a/sandbox/src/broker_services.cc +++ b/sandbox/src/broker_services.cc @@ -41,11 +41,33 @@ sandbox::ResultCode SpawnCleanup(sandbox::TargetProcess* target, DWORD error) { // executes TargetEventsThread(). enum { THREAD_CTRL_NONE, + THREAD_CTRL_REMOVE_PEER, THREAD_CTRL_QUIT, - THREAD_CTRL_LAST + THREAD_CTRL_LAST, }; -} +// Helper structure that allows the Broker to associate a job notification +// with a job object and with a policy. +struct JobTracker { + HANDLE job; + sandbox::PolicyBase* policy; + JobTracker(HANDLE cjob, sandbox::PolicyBase* cpolicy) + : job(cjob), policy(cpolicy) { + } +}; + +// Helper structure that allows the broker to track peer processes +struct PeerTracker { + HANDLE wait_object; + base::win::ScopedHandle process; + DWORD id; + HANDLE job_port; + PeerTracker(DWORD process_id, HANDLE broker_job_port) + : wait_object(NULL), id(process_id), job_port(broker_job_port) { + } +}; + +} // namespace namespace sandbox { @@ -85,6 +107,7 @@ BrokerServicesBase::~BrokerServicesBase() { // If there is no port Init() was never called successfully. if (!job_port_) return; + // Closing the port causes, that no more Job notifications are delivered to // the worker thread and also causes the thread to exit. This is what we // want to do since we are going to close all outstanding Jobs and notifying @@ -107,6 +130,18 @@ BrokerServicesBase::~BrokerServicesBase() { ::CloseHandle(job_thread_); delete thread_pool_; ::CloseHandle(no_targets_); + + // Cancel the wait events and delete remaining peer trackers. + for (PeerTrackerMap::iterator it = peer_map_.begin(); + it != peer_map_.end(); ++it) { + // Deregistration shouldn't fail, but we leak rather than crash if it does. + if (::UnregisterWaitEx(it->second->wait_object, INVALID_HANDLE_VALUE)) { + delete it->second; + } else { + NOTREACHED(); + } + } + // If job_port_ isn't NULL, assumes that the lock has been initialized. if (job_port_) ::DeleteCriticalSection(&lock_); @@ -209,9 +244,23 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) { } } + } else if (THREAD_CTRL_REMOVE_PEER == key) { + // Remove a process from our list of peers. + AutoLock lock(&broker->lock_); + PeerTrackerMap::iterator it = + broker->peer_map_.find(reinterpret_cast<DWORD>(ovl)); + // This shouldn't fail, but if it does leak the memory rather than crash. + if (::UnregisterWaitEx(it->second->wait_object, INVALID_HANDLE_VALUE)) { + delete it->second; + broker->peer_map_.erase(it); + } else { + NOTREACHED(); + } + } else if (THREAD_CTRL_QUIT == key) { // The broker object is being destroyed so the thread needs to exit. return 0; + } else { // We have not implemented more commands. NOTREACHED(); @@ -312,7 +361,44 @@ ResultCode BrokerServicesBase::WaitForAllTargets() { bool BrokerServicesBase::IsActiveTarget(DWORD process_id) { AutoLock lock(&lock_); - return child_process_ids_.find(process_id) != child_process_ids_.end(); + return child_process_ids_.find(process_id) != child_process_ids_.end() || + peer_map_.find(process_id) != peer_map_.end(); +} + +VOID CALLBACK BrokerServicesBase::RemovePeer(PVOID parameter, BOOLEAN) { + PeerTracker* peer = reinterpret_cast<PeerTracker*>(parameter); + // Don't check the return code because we this may fail (safely) at shutdown. + ::PostQueuedCompletionStatus(peer->job_port, 0, THREAD_CTRL_REMOVE_PEER, + reinterpret_cast<LPOVERLAPPED>(peer->id)); +} + +ResultCode BrokerServicesBase::AddTargetPeer(HANDLE peer_process) { + scoped_ptr<PeerTracker> peer(new PeerTracker(::GetProcessId(peer_process), + job_port_)); + if (!peer->id) + return SBOX_ERROR_GENERIC; + + if (!::DuplicateHandle(::GetCurrentProcess(), peer_process, + ::GetCurrentProcess(), peer->process.Receive(), + SYNCHRONIZE, FALSE, 0)) { + return SBOX_ERROR_GENERIC; + } + + AutoLock lock(&lock_); + if (!peer_map_.insert(std::make_pair(peer->id, peer.get())).second) + return SBOX_ERROR_BAD_PARAMS; + + if (!::RegisterWaitForSingleObject(&peer->wait_object, + peer->process, RemovePeer, + peer.get(), INFINITE, WT_EXECUTEONLYONCE | + WT_EXECUTEINWAITTHREAD)) { + peer_map_.erase(peer->id); + return SBOX_ERROR_GENERIC; + } + + // Leak the pointer since it will be cleaned up by the callback. + peer.release(); + return SBOX_ALL_OK; } } // namespace sandbox diff --git a/sandbox/src/broker_services.h b/sandbox/src/broker_services.h index 8e4cb54..98760d9 100644 --- a/sandbox/src/broker_services.h +++ b/sandbox/src/broker_services.h @@ -6,8 +6,10 @@ #define SANDBOX_SRC_BROKER_SERVICES_H__ #include <list> +#include <map> #include <set> #include "base/basictypes.h" +#include "base/win/scoped_handle.h" #include "sandbox/src/crosscall_server.h" #include "sandbox/src/job.h" #include "sandbox/src/sandbox.h" @@ -15,6 +17,13 @@ #include "sandbox/src/win2k_threadpool.h" #include "sandbox/src/win_utils.h" +namespace { + +struct JobTracker; +struct PeerTracker; + +} // namespace + namespace sandbox { class PolicyBase; @@ -45,6 +54,8 @@ class BrokerServicesBase : public BrokerServices, virtual ResultCode WaitForAllTargets(); + virtual ResultCode AddTargetPeer(HANDLE peer_process); + // Checks if the supplied process ID matches one of the broker's active // target processes // Returns: @@ -52,16 +63,6 @@ class BrokerServicesBase : public BrokerServices, bool IsActiveTarget(DWORD process_id); private: - // Helper structure that allows the Broker to associate a job notification - // with a job object and with a policy. - struct JobTracker { - HANDLE job; - PolicyBase* policy; - JobTracker(HANDLE cjob, PolicyBase* cpolicy) - : job(cjob), policy(cpolicy) { - } - }; - // Releases the Job and notifies the associated Policy object to its // resources as well. static void FreeResources(JobTracker* tracker); @@ -70,6 +71,9 @@ class BrokerServicesBase : public BrokerServices, // notifications and cleanup-related tasks. static DWORD WINAPI TargetEventsThread(PVOID param); + // Removes a target peer from the process list if it expires. + static VOID CALLBACK RemovePeer(PVOID parameter, BOOLEAN); + // The completion port used by the job objects to communicate events to // the worker thread. HANDLE job_port_; @@ -92,6 +96,11 @@ class BrokerServicesBase : public BrokerServices, typedef std::list<JobTracker*> JobTrackerList; JobTrackerList tracker_list_; + // Maps peer process IDs to the saved handle and wait event. + // Prevents peer callbacks from accessing the broker after destruction. + typedef std::map<DWORD, PeerTracker*> PeerTrackerMap; + PeerTrackerMap peer_map_; + // Provides a fast lookup to identify sandboxed processes. std::set<DWORD> child_process_ids_; diff --git a/sandbox/src/handle_policy_test.cc b/sandbox/src/handle_policy_test.cc index bccca67..bb08b86 100644 --- a/sandbox/src/handle_policy_test.cc +++ b/sandbox/src/handle_policy_test.cc @@ -65,5 +65,27 @@ TEST(HandlePolicyTest, DuplicateHandle) { EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(cmd_line.c_str())); } +// Tests that duplicating an object works only when the policy allows it. +TEST(HandlePolicyTest, DuplicatePeerHandle) { + TestRunner target; + TestRunner runner; + + // Kick off an asynchronous target process for testing. + target.SetAsynchronous(true); + target.SetUnsandboxed(true); + EXPECT_EQ(SBOX_TEST_SUCCEEDED, target.RunTest(L"Handle_WaitProcess 30000")); + + // First test that we fail to open the event. + std::wstring cmd_line = base::StringPrintf(L"Handle_DuplicateEvent %d", + target.process_id()); + EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(cmd_line.c_str())); + + // Now successfully open the event after adding a duplicate handle rule. + EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_HANDLES, + TargetPolicy::HANDLES_DUP_ANY, + L"Event")); + EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(cmd_line.c_str())); +} + } // namespace sandbox diff --git a/sandbox/src/sandbox.h b/sandbox/src/sandbox.h index 683bda7..18c05ce 100644 --- a/sandbox/src/sandbox.h +++ b/sandbox/src/sandbox.h @@ -84,6 +84,14 @@ class BrokerServices { // If the return is ERROR_GENERIC, you can call ::GetLastError() to get // more information. virtual ResultCode WaitForAllTargets() = 0; + + // Adds an unsandboxed process as a peer for policy decisions (e.g. + // HANDLES_DUP_ANY policy). + // Returns: + // ALL_OK if successful. All other return values imply failure. + // If the return is ERROR_GENERIC, you can call ::GetLastError() to get + // more information. + virtual ResultCode AddTargetPeer(HANDLE peer_process) = 0; }; // TargetServices models the current process from the perspective |