1# HG changeset patch 2# User Bob Owen <bobowencode@gmail.com> 3# Date 1485985799 0 4# Wed Feb 01 21:49:59 2017 +0000 5# Node ID 8faee368c603dab03076d8900f01acfd776caaeb 6# Parent dba4611d335189b9a3314f5dc57935f554c8b945 7Reinstate sandbox::BrokerServices::AddTargetPeer r=aklotz 8 9This is basically a revert of chromium commit 996b42db5296bd3d11b3d7fde1a4602bbcefed2c. 10 11diff --git a/security/sandbox/chromium/sandbox/win/src/broker_services.cc b/security/sandbox/chromium/sandbox/win/src/broker_services.cc 12--- a/security/sandbox/chromium/sandbox/win/src/broker_services.cc 13+++ b/security/sandbox/chromium/sandbox/win/src/broker_services.cc 14@@ -41,16 +41,17 @@ sandbox::ResultCode SpawnCleanup(sandbox 15 delete target; 16 return sandbox::SBOX_ERROR_GENERIC; 17 } 18 19 // the different commands that you can send to the worker thread that 20 // executes TargetEventsThread(). 21 enum { 22 THREAD_CTRL_NONE, 23+ THREAD_CTRL_REMOVE_PEER, 24 THREAD_CTRL_QUIT, 25 THREAD_CTRL_LAST, 26 }; 27 28 // Helper structure that allows the Broker to associate a job notification 29 // with a job object and with a policy. 30 struct JobTracker { 31 JobTracker(base::win::ScopedHandle job, 32@@ -77,16 +78,37 @@ void JobTracker::FreeResources() { 33 HANDLE stale_job_handle = job.Get(); 34 job.Close(); 35 36 // In OnJobEmpty() we don't actually use the job handle directly. 37 policy->OnJobEmpty(stale_job_handle); 38 policy = nullptr; 39 } 40 } 41+ 42+// Helper structure that allows the broker to track peer processes 43+struct PeerTracker { 44+ PeerTracker(DWORD process_id, HANDLE broker_job_port) 45+ : wait_object(NULL), id(process_id), job_port(broker_job_port) { 46+ } 47+ 48+ HANDLE wait_object; 49+ base::win::ScopedHandle process; 50+ DWORD id; 51+ HANDLE job_port; 52+}; 53+ 54+void DeregisterPeerTracker(PeerTracker* peer) { 55+ // Deregistration shouldn't fail, but we leak rather than crash if it does. 56+ if (::UnregisterWaitEx(peer->wait_object, INVALID_HANDLE_VALUE)) { 57+ delete peer; 58+ } else { 59+ NOTREACHED(); 60+ } 61+} 62 63 } // namespace 64 65 namespace sandbox { 66 67 BrokerServicesBase::BrokerServicesBase() {} 68 69 // The broker uses a dedicated worker thread that services the job completion 70@@ -132,16 +154,22 @@ BrokerServicesBase::~BrokerServicesBase( 71 // Cannot clean broker services. 72 NOTREACHED(); 73 return; 74 } 75 76 tracker_list_.clear(); 77 thread_pool_.reset(); 78 79+ // Cancel the wait events and delete remaining peer trackers. 80+ for (PeerTrackerMap::iterator it = peer_map_.begin(); 81+ it != peer_map_.end(); ++it) { 82+ DeregisterPeerTracker(it->second); 83+ } 84+ 85 ::DeleteCriticalSection(&lock_); 86 } 87 88 scoped_refptr<TargetPolicy> BrokerServicesBase::CreatePolicy() { 89 // If you change the type of the object being created here you must also 90 // change the downcast to it in SpawnTarget(). 91 scoped_refptr<TargetPolicy> policy(new PolicyBase); 92 // PolicyBase starts with refcount 1. 93@@ -247,16 +275,23 @@ DWORD WINAPI BrokerServicesBase::TargetE 94 break; 95 } 96 97 default: { 98 NOTREACHED(); 99 break; 100 } 101 } 102+ } else if (THREAD_CTRL_REMOVE_PEER == key) { 103+ // Remove a process from our list of peers. 104+ AutoLock lock(&broker->lock_); 105+ PeerTrackerMap::iterator it = broker->peer_map_.find( 106+ static_cast<DWORD>(reinterpret_cast<uintptr_t>(ovl))); 107+ DeregisterPeerTracker(it->second); 108+ broker->peer_map_.erase(it); 109 } else if (THREAD_CTRL_QUIT == key) { 110 // The broker object is being destroyed so the thread needs to exit. 111 return 0; 112 } else { 113 // We have not implemented more commands. 114 NOTREACHED(); 115 } 116 } 117@@ -460,26 +495,71 @@ ResultCode BrokerServicesBase::SpawnTarg 118 // TODO(wfh): Find a way to make this have the correct lifetime. 119 policy_base->AddRef(); 120 121 // We have to signal the event once here because the completion port will 122 // never get a message that this target is being terminated thus we should 123 // not block WaitForAllTargets until we have at least one target with job. 124 if (child_process_ids_.empty()) 125 ::SetEvent(no_targets_.Get()); 126+ // We can not track the life time of such processes and it is responsibility 127+ // of the host application to make sure that spawned targets without jobs 128+ // are terminated when the main application don't need them anymore. 129+ // Sandbox policy engine needs to know that these processes are valid 130+ // targets for e.g. BrokerDuplicateHandle so track them as peer processes. 131+ AddTargetPeer(process_info.process_handle()); 132 } 133 134 *target_info = process_info.Take(); 135 return result; 136 } 137 138 139 ResultCode BrokerServicesBase::WaitForAllTargets() { 140 ::WaitForSingleObject(no_targets_.Get(), INFINITE); 141 return SBOX_ALL_OK; 142 } 143 144 bool BrokerServicesBase::IsActiveTarget(DWORD process_id) { 145 AutoLock lock(&lock_); 146- return child_process_ids_.find(process_id) != child_process_ids_.end(); 147+ return child_process_ids_.find(process_id) != child_process_ids_.end() || 148+ peer_map_.find(process_id) != peer_map_.end(); 149+} 150+ 151+VOID CALLBACK BrokerServicesBase::RemovePeer(PVOID parameter, BOOLEAN timeout) { 152+ PeerTracker* peer = reinterpret_cast<PeerTracker*>(parameter); 153+ // Don't check the return code because we this may fail (safely) at shutdown. 154+ ::PostQueuedCompletionStatus( 155+ peer->job_port, 0, THREAD_CTRL_REMOVE_PEER, 156+ reinterpret_cast<LPOVERLAPPED>(static_cast<uintptr_t>(peer->id))); 157+} 158+ 159+ResultCode BrokerServicesBase::AddTargetPeer(HANDLE peer_process) { 160+ std::unique_ptr<PeerTracker> peer( 161+ new PeerTracker(::GetProcessId(peer_process), job_port_.Get())); 162+ if (!peer->id) 163+ return SBOX_ERROR_GENERIC; 164+ 165+ HANDLE process_handle; 166+ if (!::DuplicateHandle(::GetCurrentProcess(), peer_process, 167+ ::GetCurrentProcess(), &process_handle, 168+ SYNCHRONIZE, FALSE, 0)) { 169+ return SBOX_ERROR_GENERIC; 170+ } 171+ peer->process.Set(process_handle); 172+ 173+ AutoLock lock(&lock_); 174+ if (!peer_map_.insert(std::make_pair(peer->id, peer.get())).second) 175+ return SBOX_ERROR_BAD_PARAMS; 176+ 177+ if (!::RegisterWaitForSingleObject( 178+ &peer->wait_object, peer->process.Get(), RemovePeer, peer.get(), 179+ INFINITE, WT_EXECUTEONLYONCE | WT_EXECUTEINWAITTHREAD)) { 180+ peer_map_.erase(peer->id); 181+ return SBOX_ERROR_GENERIC; 182+ } 183+ 184+ // Release the pointer since it will be cleaned up by the callback. 185+ ignore_result(peer.release()); 186+ return SBOX_ALL_OK; 187 } 188 189 } // namespace sandbox 190diff --git a/security/sandbox/chromium/sandbox/win/src/broker_services.h b/security/sandbox/chromium/sandbox/win/src/broker_services.h 191--- a/security/sandbox/chromium/sandbox/win/src/broker_services.h 192+++ b/security/sandbox/chromium/sandbox/win/src/broker_services.h 193@@ -19,16 +19,17 @@ 194 #include "sandbox/win/src/sandbox.h" 195 #include "sandbox/win/src/sharedmem_ipc_server.h" 196 #include "sandbox/win/src/win2k_threadpool.h" 197 #include "sandbox/win/src/win_utils.h" 198 199 namespace { 200 201 struct JobTracker; 202+struct PeerTracker; 203 204 } // namespace 205 206 namespace sandbox { 207 208 // BrokerServicesBase --------------------------------------------------------- 209 // Broker implementation version 0 210 // 211@@ -48,28 +49,35 @@ class BrokerServicesBase final : public 212 scoped_refptr<TargetPolicy> CreatePolicy() override; 213 ResultCode SpawnTarget(const wchar_t* exe_path, 214 const wchar_t* command_line, 215 scoped_refptr<TargetPolicy> policy, 216 ResultCode* last_warning, 217 DWORD* last_error, 218 PROCESS_INFORMATION* target) override; 219 ResultCode WaitForAllTargets() override; 220+ ResultCode AddTargetPeer(HANDLE peer_process) override; 221 222 // Checks if the supplied process ID matches one of the broker's active 223 // target processes 224 // Returns: 225 // true if there is an active target process for this ID, otherwise false. 226 bool IsActiveTarget(DWORD process_id); 227 228 private: 229+ typedef std::list<JobTracker*> JobTrackerList; 230+ typedef std::map<DWORD, PeerTracker*> PeerTrackerMap; 231+ 232 // The routine that the worker thread executes. It is in charge of 233 // notifications and cleanup-related tasks. 234 static DWORD WINAPI TargetEventsThread(PVOID param); 235 236+ // Removes a target peer from the process list if it expires. 237+ static VOID CALLBACK RemovePeer(PVOID parameter, BOOLEAN timeout); 238+ 239 // The completion port used by the job objects to communicate events to 240 // the worker thread. 241 base::win::ScopedHandle job_port_; 242 243 // Handle to a manual-reset event that is signaled when the total target 244 // process count reaches zero. 245 base::win::ScopedHandle no_targets_; 246 247@@ -81,16 +89,20 @@ class BrokerServicesBase final : public 248 CRITICAL_SECTION lock_; 249 250 // Provides a pool of threads that are used to wait on the IPC calls. 251 std::unique_ptr<ThreadProvider> thread_pool_; 252 253 // List of the trackers for closing and cleanup purposes. 254 std::list<std::unique_ptr<JobTracker>> tracker_list_; 255 256+ // Maps peer process IDs to the saved handle and wait event. 257+ // Prevents peer callbacks from accessing the broker after destruction. 258+ PeerTrackerMap peer_map_; 259+ 260 // Provides a fast lookup to identify sandboxed processes that belong to a 261 // job. Consult |jobless_process_handles_| for handles of processes without 262 // jobs. 263 std::set<DWORD> child_process_ids_; 264 265 DISALLOW_COPY_AND_ASSIGN(BrokerServicesBase); 266 }; 267 268diff --git a/security/sandbox/chromium/sandbox/win/src/sandbox.h b/security/sandbox/chromium/sandbox/win/src/sandbox.h 269--- a/security/sandbox/chromium/sandbox/win/src/sandbox.h 270+++ b/security/sandbox/chromium/sandbox/win/src/sandbox.h 271@@ -86,16 +86,24 @@ class BrokerServices { 272 PROCESS_INFORMATION* target) = 0; 273 274 // This call blocks (waits) for all the targets to terminate. 275 // Returns: 276 // ALL_OK if successful. All other return values imply failure. 277 // If the return is ERROR_GENERIC, you can call ::GetLastError() to get 278 // more information. 279 virtual ResultCode WaitForAllTargets() = 0; 280+ 281+ // Adds an unsandboxed process as a peer for policy decisions (e.g. 282+ // HANDLES_DUP_ANY policy). 283+ // Returns: 284+ // ALL_OK if successful. All other return values imply failure. 285+ // If the return is ERROR_GENERIC, you can call ::GetLastError() to get 286+ // more information. 287+ virtual ResultCode AddTargetPeer(HANDLE peer_process) = 0; 288 }; 289 290 // TargetServices models the current process from the perspective 291 // of a target process. To obtain a pointer to it use 292 // Sandbox::GetTargetServices(). Note that this call returns a non-null 293 // pointer only if this process is in fact a target. A process is a target 294 // only if the process was spawned by a call to BrokerServices::SpawnTarget(). 295 // 296