Use ScopedProcessInformation and other RAII types in sandbox.

sandbox/src/broker_services.cc

 // Copyright (c) 2006-2009 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 "sandbox/src/broker_services.h"
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/threading/platform_thread.h"
+#include "base/win/scoped_handle.h"
+#include "base/win/scoped_process_information.h"
 #include "sandbox/src/sandbox_policy_base.h"
 #include "sandbox/src/sandbox.h"
 #include "sandbox/src/target_process.h"
 #include "sandbox/src/win2k_threadpool.h"
 #include "sandbox/src/win_utils.h"
 
 namespace {
 
 // Utility function to associate a completion port to a job object.
 bool AssociateCompletionPort(HANDLE job, HANDLE port, void* key) {
@@ skipping 191 matching lines @@
       NOTREACHED();
     }
   }
 
   NOTREACHED();
   return 0;
 }
 
 // SpawnTarget does all the interesting sandbox setup and creates the target
 // process inside the sandbox.
-ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
-                                           const wchar_t* command_line,
-                                           TargetPolicy* policy,
-                                           PROCESS_INFORMATION* target_info) {
+ResultCode BrokerServicesBase::SpawnTarget(
+    const wchar_t* exe_path,
+    const wchar_t* command_line,
+    TargetPolicy* policy,
+    base::win::ScopedProcessInformation* target_info) {
   if (!exe_path)
     return SBOX_ERROR_BAD_PARAMS;
 
   if (!policy)
     return SBOX_ERROR_BAD_PARAMS;
 
   // Even though the resources touched by SpawnTarget can be accessed in
   // multiple threads, the method itself cannot be called from more than
   // 1 thread. This is to protect the global variables used while setting up
   // the child process.
   static DWORD thread_id = ::GetCurrentThreadId();
   DCHECK(thread_id == ::GetCurrentThreadId());
 
   AutoLock lock(&lock_);
 
   // This downcast is safe as long as we control CreatePolicy()
   PolicyBase* policy_base = static_cast<PolicyBase*>(policy);
 
   // Construct the tokens and the job object that we are going to associate
   // with the soon to be created target process.
-  HANDLE lockdown_token = NULL;
-  HANDLE initial_token = NULL;
-  DWORD win_result = policy_base->MakeTokens(&initial_token, &lockdown_token);
+  base::win::ScopedHandle lockdown_token;
+  base::win::ScopedHandle initial_token;
+  DWORD win_result = policy_base->MakeTokens(initial_token.Receive(),
+                                             lockdown_token.Receive());
   if (ERROR_SUCCESS != win_result)
     return SBOX_ERROR_GENERIC;
 
-  HANDLE job = NULL;
-  win_result = policy_base->MakeJobObject(&job);
+  base::win::ScopedHandle job;
+  win_result = policy_base->MakeJobObject(job.Receive());
   if (ERROR_SUCCESS != win_result)

[erikwright (departed), 2012/03/30 16:29:31]

Previously lockdown_token and initial_token could leak here (and in the next
early return).

     return SBOX_ERROR_GENERIC;
 
   if (ERROR_ALREADY_EXISTS == ::GetLastError())
     return SBOX_ERROR_GENERIC;
 
   // Construct the thread pool here in case it is expensive.
   // The thread pool is shared by all the targets
   if (NULL == thread_pool_)
     thread_pool_ = new Win2kThreadPool();
 
   // Create the TargetProces object and spawn the target suspended. Note that
   // Brokerservices does not own the target object. It is owned by the Policy.
-  PROCESS_INFORMATION process_info = {0};
-  TargetProcess* target = new TargetProcess(initial_token, lockdown_token,
-                                            job, thread_pool_);
+  base::win::ScopedProcessInformation process_info;
+  TargetProcess* target = new TargetProcess(initial_token.Take(),
+                                            lockdown_token.Take(),
+                                            job,
+                                            thread_pool_);
 
   std::wstring desktop = policy_base->GetAlternateDesktop();
 
   win_result = target->Create(exe_path, command_line,
                               desktop.empty() ? NULL : desktop.c_str(),
                               &process_info);
   if (ERROR_SUCCESS != win_result)

[erikwright (departed), 2012/03/30 16:29:31]

Previously job could be leaked here and in the following early return.

     return SpawnCleanup(target, win_result);
 
-  if ((INVALID_HANDLE_VALUE == process_info.hProcess) ||

[erikwright (departed), 2012/03/30 16:29:31]

This check was redundant. It only protects you if somehow TargetProcess::Create
returns ERROR_SUCCESS from one of its failure modes. If one wishes to protect
against this type of bug, I suggest instead converting TargetProcess::Create to
return a boolean, and potentially take a DWORD* for returning platform error
codes.

-      (INVALID_HANDLE_VALUE == process_info.hThread))
-    return SpawnCleanup(target, win_result);
-
   // Now the policy is the owner of the target.
   if (!policy_base->AddTarget(target)) {
     return SpawnCleanup(target, 0);
   }
 
   // We are going to keep a pointer to the policy because we'll call it when
   // the job object generates notifications using the completion port.
   policy_base->AddRef();
-  JobTracker* tracker = new JobTracker(job, policy_base);
-  if (!AssociateCompletionPort(job, job_port_, tracker))
+  scoped_ptr<JobTracker> tracker(new JobTracker(job.Take(), policy_base));
+  if (!AssociateCompletionPort(tracker->job, job_port_, tracker.get()))
     return SpawnCleanup(target, 0);

[erikwright (departed), 2012/03/30 16:29:31]

tracker was previously leaked here.

   // Save the tracker because in cleanup we might need to force closing
   // the Jobs.
-  tracker_list_.push_back(tracker);
+  tracker_list_.push_back(tracker.release());
 
-  // We return the caller a duplicate of the process handle so they
-  // can close it at will.
-  HANDLE dup_process_handle = NULL;
-  if (!::DuplicateHandle(::GetCurrentProcess(), process_info.hProcess,
-                         ::GetCurrentProcess(), &dup_process_handle,
-                         0, FALSE, DUPLICATE_SAME_ACCESS))

[erikwright (departed), 2012/03/30 16:29:31]

The responsibility for duplicating is now in TargetProcess::Create

-    return SpawnCleanup(target, 0);
+  target_info->Swap(&process_info);
 
-  *target_info = process_info;
-  target_info->hProcess = dup_process_handle;
   return SBOX_ALL_OK;
 }
 
 
 ResultCode BrokerServicesBase::WaitForAllTargets() {
   ::WaitForSingleObject(no_targets_, INFINITE);
   return SBOX_ALL_OK;
 }
 
 }  // namespace sandbox