# HG changeset patch # User Bob Owen # Date 1485985799 0 # Wed Feb 01 21:49:59 2017 +0000 # Node ID 8faee368c603dab03076d8900f01acfd776caaeb # Parent dba4611d335189b9a3314f5dc57935f554c8b945 Reinstate sandbox::BrokerServices::AddTargetPeer r=aklotz This is basically a revert of chromium commit 996b42db5296bd3d11b3d7fde1a4602bbcefed2c. diff --git a/security/sandbox/chromium/sandbox/win/src/broker_services.cc b/security/sandbox/chromium/sandbox/win/src/broker_services.cc --- a/security/sandbox/chromium/sandbox/win/src/broker_services.cc +++ b/security/sandbox/chromium/sandbox/win/src/broker_services.cc @@ -41,16 +41,17 @@ sandbox::ResultCode SpawnCleanup(sandbox delete target; return sandbox::SBOX_ERROR_GENERIC; } // the different commands that you can send to the worker thread that // executes TargetEventsThread(). enum { THREAD_CTRL_NONE, + THREAD_CTRL_REMOVE_PEER, THREAD_CTRL_QUIT, THREAD_CTRL_LAST, }; // Helper structure that allows the Broker to associate a job notification // with a job object and with a policy. struct JobTracker { JobTracker(base::win::ScopedHandle job, @@ -77,16 +78,37 @@ void JobTracker::FreeResources() { HANDLE stale_job_handle = job.Get(); job.Close(); // In OnJobEmpty() we don't actually use the job handle directly. policy->OnJobEmpty(stale_job_handle); policy = nullptr; } } + +// Helper structure that allows the broker to track peer processes +struct PeerTracker { + PeerTracker(DWORD process_id, HANDLE broker_job_port) + : wait_object(NULL), id(process_id), job_port(broker_job_port) { + } + + HANDLE wait_object; + base::win::ScopedHandle process; + DWORD id; + HANDLE job_port; +}; + +void DeregisterPeerTracker(PeerTracker* peer) { + // Deregistration shouldn't fail, but we leak rather than crash if it does. + if (::UnregisterWaitEx(peer->wait_object, INVALID_HANDLE_VALUE)) { + delete peer; + } else { + NOTREACHED(); + } +} } // namespace namespace sandbox { BrokerServicesBase::BrokerServicesBase() {} // The broker uses a dedicated worker thread that services the job completion @@ -132,16 +154,22 @@ BrokerServicesBase::~BrokerServicesBase( // Cannot clean broker services. NOTREACHED(); return; } tracker_list_.clear(); thread_pool_.reset(); + // Cancel the wait events and delete remaining peer trackers. + for (PeerTrackerMap::iterator it = peer_map_.begin(); + it != peer_map_.end(); ++it) { + DeregisterPeerTracker(it->second); + } + ::DeleteCriticalSection(&lock_); } scoped_refptr BrokerServicesBase::CreatePolicy() { // If you change the type of the object being created here you must also // change the downcast to it in SpawnTarget(). scoped_refptr policy(new PolicyBase); // PolicyBase starts with refcount 1. @@ -247,16 +275,23 @@ DWORD WINAPI BrokerServicesBase::TargetE break; } default: { NOTREACHED(); break; } } + } 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( + static_cast(reinterpret_cast(ovl))); + DeregisterPeerTracker(it->second); + broker->peer_map_.erase(it); } 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(); } } @@ -460,26 +495,71 @@ ResultCode BrokerServicesBase::SpawnTarg // TODO(wfh): Find a way to make this have the correct lifetime. policy_base->AddRef(); // We have to signal the event once here because the completion port will // never get a message that this target is being terminated thus we should // not block WaitForAllTargets until we have at least one target with job. if (child_process_ids_.empty()) ::SetEvent(no_targets_.Get()); + // We can not track the life time of such processes and it is responsibility + // of the host application to make sure that spawned targets without jobs + // are terminated when the main application don't need them anymore. + // Sandbox policy engine needs to know that these processes are valid + // targets for e.g. BrokerDuplicateHandle so track them as peer processes. + AddTargetPeer(process_info.process_handle()); } *target_info = process_info.Take(); return result; } ResultCode BrokerServicesBase::WaitForAllTargets() { ::WaitForSingleObject(no_targets_.Get(), INFINITE); return SBOX_ALL_OK; } 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 timeout) { + PeerTracker* peer = reinterpret_cast(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(static_cast(peer->id))); +} + +ResultCode BrokerServicesBase::AddTargetPeer(HANDLE peer_process) { + std::unique_ptr peer( + new PeerTracker(::GetProcessId(peer_process), job_port_.Get())); + if (!peer->id) + return SBOX_ERROR_GENERIC; + + HANDLE process_handle; + if (!::DuplicateHandle(::GetCurrentProcess(), peer_process, + ::GetCurrentProcess(), &process_handle, + SYNCHRONIZE, FALSE, 0)) { + return SBOX_ERROR_GENERIC; + } + peer->process.Set(process_handle); + + 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.Get(), RemovePeer, peer.get(), + INFINITE, WT_EXECUTEONLYONCE | WT_EXECUTEINWAITTHREAD)) { + peer_map_.erase(peer->id); + return SBOX_ERROR_GENERIC; + } + + // Release the pointer since it will be cleaned up by the callback. + ignore_result(peer.release()); + return SBOX_ALL_OK; } } // namespace sandbox diff --git a/security/sandbox/chromium/sandbox/win/src/broker_services.h b/security/sandbox/chromium/sandbox/win/src/broker_services.h --- a/security/sandbox/chromium/sandbox/win/src/broker_services.h +++ b/security/sandbox/chromium/sandbox/win/src/broker_services.h @@ -19,16 +19,17 @@ #include "sandbox/win/src/sandbox.h" #include "sandbox/win/src/sharedmem_ipc_server.h" #include "sandbox/win/src/win2k_threadpool.h" #include "sandbox/win/src/win_utils.h" namespace { struct JobTracker; +struct PeerTracker; } // namespace namespace sandbox { // BrokerServicesBase --------------------------------------------------------- // Broker implementation version 0 // @@ -48,28 +49,35 @@ class BrokerServicesBase final : public scoped_refptr CreatePolicy() override; ResultCode SpawnTarget(const wchar_t* exe_path, const wchar_t* command_line, scoped_refptr policy, ResultCode* last_warning, DWORD* last_error, PROCESS_INFORMATION* target) override; ResultCode WaitForAllTargets() override; + ResultCode AddTargetPeer(HANDLE peer_process) override; // Checks if the supplied process ID matches one of the broker's active // target processes // Returns: // true if there is an active target process for this ID, otherwise false. bool IsActiveTarget(DWORD process_id); private: + typedef std::list JobTrackerList; + typedef std::map PeerTrackerMap; + // The routine that the worker thread executes. It is in charge of // 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 timeout); + // The completion port used by the job objects to communicate events to // the worker thread. base::win::ScopedHandle job_port_; // Handle to a manual-reset event that is signaled when the total target // process count reaches zero. base::win::ScopedHandle no_targets_; @@ -81,16 +89,20 @@ class BrokerServicesBase final : public CRITICAL_SECTION lock_; // Provides a pool of threads that are used to wait on the IPC calls. std::unique_ptr thread_pool_; // List of the trackers for closing and cleanup purposes. std::list> tracker_list_; + // Maps peer process IDs to the saved handle and wait event. + // Prevents peer callbacks from accessing the broker after destruction. + PeerTrackerMap peer_map_; + // Provides a fast lookup to identify sandboxed processes that belong to a // job. Consult |jobless_process_handles_| for handles of processes without // jobs. std::set child_process_ids_; DISALLOW_COPY_AND_ASSIGN(BrokerServicesBase); }; diff --git a/security/sandbox/chromium/sandbox/win/src/sandbox.h b/security/sandbox/chromium/sandbox/win/src/sandbox.h --- a/security/sandbox/chromium/sandbox/win/src/sandbox.h +++ b/security/sandbox/chromium/sandbox/win/src/sandbox.h @@ -86,16 +86,24 @@ class BrokerServices { PROCESS_INFORMATION* target) = 0; // This call blocks (waits) for all the targets to terminate. // 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 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 // of a target process. To obtain a pointer to it use // Sandbox::GetTargetServices(). Note that this call returns a non-null // pointer only if this process is in fact a target. A process is a target // only if the process was spawned by a call to BrokerServices::SpawnTarget(). //