Rework target process creation to minimize creation routes

Changes the creation strategy for the target process to minimize the
differences between the "normal" process creation and the appcontainer
process creation. The hope is this minimization might remedy the failure
to initialize the process when appcontainer is being used on win8+

BUG=467920

Review URL: https://codereview.chromium.org/1263603002

Cr-Commit-Position: refs/heads/master@{#353752}

6 files changed, 62 insertions, 93 deletions

diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index 9579daa..fbebc83 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -113,18 +113,6 @@ void DeregisterPeerTracker(PeerTracker* peer) {
 
 namespace sandbox {
 
-// TODO(rvargas): Replace this structure with a std::pair of ScopedHandles.
-struct BrokerServicesBase::TokenPair {
-  TokenPair(base::win::ScopedHandle initial_token,
-         base::win::ScopedHandle lockdown_token)
-      : initial(initial_token.Pass()),
-        lockdown(lockdown_token.Pass()) {
-  }
-
-  base::win::ScopedHandle initial;
-  base::win::ScopedHandle lockdown;
-};
-
 BrokerServicesBase::BrokerServicesBase() : thread_pool_(NULL) {
 }
 
@@ -326,9 +314,11 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   // with the soon to be created target process.
   base::win::ScopedHandle initial_token;
   base::win::ScopedHandle lockdown_token;
+  base::win::ScopedHandle lowbox_token;
   ResultCode result = SBOX_ALL_OK;
 
-  result = policy_base->MakeTokens(&initial_token, &lockdown_token);
+  result =
+      policy_base->MakeTokens(&initial_token, &lockdown_token, &lowbox_token);
   if (SBOX_ALL_OK != result)
     return result;
 
@@ -445,13 +435,11 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   // Create the TargetProces object and spawn the target suspended. Note that
   // Brokerservices does not own the target object. It is owned by the Policy.
   base::win::ScopedProcessInformation process_info;
-  TargetProcess* target = new TargetProcess(initial_token.Pass(),
-                                            lockdown_token.Pass(),
-                                            job.Get(),
-                                            thread_pool_);
+  TargetProcess* target =
+      new TargetProcess(initial_token.Pass(), lockdown_token.Pass(),
+                        lowbox_token.Pass(), job.Get(), thread_pool_);
 
   DWORD win_result = target->Create(exe_path, command_line, inherit_handles,
-                                    policy_base->GetLowBoxSid() ? true : false,
                                     startup_info, &process_info);
 
   // Restore the previous handle protection values.
diff --git a/sandbox/win/src/broker_services.h b/sandbox/win/src/broker_services.h
index 5fd6ca4..b16d5fc 100644
--- a/sandbox/win/src/broker_services.h
+++ b/sandbox/win/src/broker_services.h
@@ -64,10 +64,8 @@ class BrokerServicesBase final : public BrokerServices,
   bool IsActiveTarget(DWORD process_id);
 
  private:
-  struct TokenPair;
   typedef std::list<JobTracker*> JobTrackerList;
   typedef std::map<DWORD, PeerTracker*> PeerTrackerMap;
-  typedef std::map<uint32_t, TokenPair*> TokenCacheMap;
 
   // The routine that the worker thread executes. It is in charge of
   // notifications and cleanup-related tasks.
diff --git a/sandbox/win/src/sandbox_policy_base.cc b/sandbox/win/src/sandbox_policy_base.cc
index 9c3bf0c..17f52c5 100644
--- a/sandbox/win/src/sandbox_policy_base.cc
+++ b/sandbox/win/src/sandbox_policy_base.cc
@@ -542,7 +542,8 @@ ResultCode PolicyBase::MakeJobObject(base::win::ScopedHandle* job) {
 }
 
 ResultCode PolicyBase::MakeTokens(base::win::ScopedHandle* initial,
-                                  base::win::ScopedHandle* lockdown) {
+                                  base::win::ScopedHandle* lockdown,
+                                  base::win::ScopedHandle* lowbox) {
   if (appcontainer_list_.get() && appcontainer_list_->HasAppContainer() &&
       lowbox_sid_) {
     return SBOX_ERROR_BAD_PARAMS;
@@ -613,7 +614,7 @@ ResultCode PolicyBase::MakeTokens(base::win::ScopedHandle* initial,
       return SBOX_ERROR_GENERIC;
 
     DCHECK(token_lowbox);
-    lockdown->Set(token_lowbox);
+    lowbox->Set(token_lowbox);
   }
 
   // Create the 'better' token. We use this token as the one that the main
diff --git a/sandbox/win/src/sandbox_policy_base.h b/sandbox/win/src/sandbox_policy_base.h
index 19686ee..a461bd7 100644
--- a/sandbox/win/src/sandbox_policy_base.h
+++ b/sandbox/win/src/sandbox_policy_base.h
@@ -82,9 +82,11 @@ class PolicyBase : public Dispatcher, public TargetPolicy {
   ResultCode MakeJobObject(base::win::ScopedHandle* job);
 
   // Creates the two tokens with the levels specified in a previous call to
-  // SetTokenLevel().
+  // SetTokenLevel(). Also creates a lowbox token if specified based on the
+  // lowbox SID.
   ResultCode MakeTokens(base::win::ScopedHandle* initial,
-                        base::win::ScopedHandle* lockdown);
+                        base::win::ScopedHandle* lockdown,
+                        base::win::ScopedHandle* lowbox);
 
   const AppContainerAttributes* GetAppContainer() const;
 
diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc
index 416713f..69dce20 100644
--- a/sandbox/win/src/target_process.cc
+++ b/sandbox/win/src/target_process.cc
@@ -65,19 +65,20 @@ void* GetBaseAddress(const wchar_t* exe_name, void* entry_point) {
   return base;
 }
 
-
 TargetProcess::TargetProcess(base::win::ScopedHandle initial_token,
                              base::win::ScopedHandle lockdown_token,
-                             HANDLE job, ThreadProvider* thread_pool)
-  // This object owns everything initialized here except thread_pool and
-  // the job_ handle. The Job handle is closed by BrokerServices and results
-  // eventually in a call to our dtor.
+                             base::win::ScopedHandle lowbox_token,
+                             HANDLE job,
+                             ThreadProvider* thread_pool)
+    // This object owns everything initialized here except thread_pool and
+    // the job_ handle. The Job handle is closed by BrokerServices and results
+    // eventually in a call to our dtor.
     : lockdown_token_(lockdown_token.Pass()),
       initial_token_(initial_token.Pass()),
+      lowbox_token_(lowbox_token.Pass()),
       job_(job),
       thread_pool_(thread_pool),
-      base_address_(NULL) {
-}
+      base_address_(NULL) {}
 
 TargetProcess::~TargetProcess() {
   DWORD exit_code = 0;
@@ -116,12 +117,11 @@ TargetProcess::~TargetProcess() {
 DWORD TargetProcess::Create(const wchar_t* exe_path,
                             const wchar_t* command_line,
                             bool inherit_handles,
-                            bool set_lockdown_token_after_create,
                             const base::win::StartupInformation& startup_info,
                             base::win::ScopedProcessInformation* target_info) {
-  if (set_lockdown_token_after_create &&
+  if (lowbox_token_.IsValid() &&
       base::win::GetVersion() < base::win::VERSION_WIN8) {
-    // We don't allow set_lockdown_token_after_create below Windows 8.
+    // We don't allow lowbox_token below Windows 8.
     return ERROR_INVALID_PARAMETER;
   }
 
@@ -143,38 +143,16 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     flags |= CREATE_BREAKAWAY_FROM_JOB;
   }
 
-  base::win::ScopedHandle scoped_lockdown_token(lockdown_token_.Take());
   PROCESS_INFORMATION temp_process_info = {};
-  if (set_lockdown_token_after_create) {
-    // First create process with a default token and then replace it later,
-    // after setting primary thread token. This is required for setting
-    // an AppContainer token along with an impersonation token.
-    if (!::CreateProcess(exe_path,
-                         cmd_line.get(),
-                         NULL,   // No security attribute.
-                         NULL,   // No thread attribute.
-                         inherit_handles,
-                         flags,
-                         NULL,   // Use the environment of the caller.
-                         NULL,   // Use current directory of the caller.
-                         startup_info.startup_info(),
-                         &temp_process_info)) {
-      return ::GetLastError();
-    }
-  } else {
-    if (!::CreateProcessAsUserW(scoped_lockdown_token.Get(),
-                                exe_path,
-                                cmd_line.get(),
-                                NULL,   // No security attribute.
-                                NULL,   // No thread attribute.
-                                inherit_handles,
-                                flags,
-                                NULL,   // Use the environment of the caller.
-                                NULL,   // Use current directory of the caller.
-                                startup_info.startup_info(),
-                                &temp_process_info)) {
-      return ::GetLastError();
-    }
+  if (!::CreateProcessAsUserW(lockdown_token_.Get(), exe_path, cmd_line.get(),
+                              NULL,  // No security attribute.
+                              NULL,  // No thread attribute.
+                              inherit_handles, flags,
+                              NULL,  // Use the environment of the caller.
+                              NULL,  // Use current directory of the caller.
+                              startup_info.startup_info(),
+                              &temp_process_info)) {
+    return ::GetLastError();
   }
   base::win::ScopedProcessInformation process_info(temp_process_info);
 
@@ -204,26 +182,6 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     initial_token_.Close();
   }
 
-  if (set_lockdown_token_after_create) {
-    PROCESS_ACCESS_TOKEN process_access_token;
-    process_access_token.thread = process_info.thread_handle();
-    process_access_token.token = scoped_lockdown_token.Get();
-
-    NtSetInformationProcess SetInformationProcess = NULL;
-    ResolveNTFunctionPtr("NtSetInformationProcess", &SetInformationProcess);
-
-    NTSTATUS status = SetInformationProcess(
-        process_info.process_handle(),
-        static_cast<PROCESS_INFORMATION_CLASS>(NtProcessInformationAccessToken),
-        &process_access_token,
-        sizeof(process_access_token));
-    if (!NT_SUCCESS(status)) {
-      win_result = ::GetLastError();
-      ::TerminateProcess(process_info.process_handle(), 0);  // exit code
-      return win_result;
-    }
-  }
-
   CONTEXT context;
   context.ContextFlags = CONTEXT_ALL;
   if (!::GetThreadContext(process_info.thread_handle(), &context)) {
@@ -248,6 +206,25 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     return win_result;
   }
 
+  if (lowbox_token_.IsValid()) {
+    PROCESS_ACCESS_TOKEN process_access_token;
+    process_access_token.thread = process_info.thread_handle();
+    process_access_token.token = lowbox_token_.Get();
+
+    NtSetInformationProcess SetInformationProcess = NULL;
+    ResolveNTFunctionPtr("NtSetInformationProcess", &SetInformationProcess);
+
+    NTSTATUS status = SetInformationProcess(
+        process_info.process_handle(),
+        static_cast<PROCESS_INFORMATION_CLASS>(NtProcessInformationAccessToken),
+        &process_access_token, sizeof(process_access_token));
+    if (!NT_SUCCESS(status)) {
+      win_result = ERROR_INVALID_TOKEN;
+      ::TerminateProcess(process_info.process_handle(), 0);  // exit code
+      return win_result;
+    }
+  }
+
   base_address_ = GetBaseAddress(exe_path, entry_point);
   sandbox_process_info_.Set(process_info.Take());
   return win_result;
@@ -374,9 +351,9 @@ void TargetProcess::Terminate() {
 }
 
 TargetProcess* MakeTestTargetProcess(HANDLE process, HMODULE base_address) {
-  TargetProcess* target = new TargetProcess(base::win::ScopedHandle(),
-                                            base::win::ScopedHandle(),
-                                            NULL, NULL);
+  TargetProcess* target =
+      new TargetProcess(base::win::ScopedHandle(), base::win::ScopedHandle(),
+                        base::win::ScopedHandle(), NULL, NULL);
   PROCESS_INFORMATION process_info = {};
   process_info.hProcess = process;
   target->sandbox_process_info_.Set(process_info);
diff --git a/sandbox/win/src/target_process.h b/sandbox/win/src/target_process.h
index e0200db..4cd5819 100644
--- a/sandbox/win/src/target_process.h
+++ b/sandbox/win/src/target_process.h
@@ -32,10 +32,13 @@ class ThreadProvider;
 // class are owned by the Policy used to create them.
 class TargetProcess {
  public:
-  // The constructor takes ownership of |initial_token| and |lockdown_token|.
+  // The constructor takes ownership of |initial_token|, |lockdown_token|
+  // and |lowbox_token|.
   TargetProcess(base::win::ScopedHandle initial_token,
                 base::win::ScopedHandle lockdown_token,
-                HANDLE job, ThreadProvider* thread_pool);
+                base::win::ScopedHandle lowbox_token,
+                HANDLE job,
+                ThreadProvider* thread_pool);
   ~TargetProcess();
 
   // TODO(cpu): Currently there does not seem to be a reason to implement
@@ -46,12 +49,9 @@ class TargetProcess {
   void Release() {}
 
   // Creates the new target process. The process is created suspended.
-  // When |set_lockdown_token_after_create| is set, the lockdown token
-  // is replaced after the process is created
   DWORD Create(const wchar_t* exe_path,
                const wchar_t* command_line,
                bool inherit_handles,
-               bool set_lockdown_token_after_create,
                const base::win::StartupInformation& startup_info,
                base::win::ScopedProcessInformation* target_info);
 
@@ -103,6 +103,9 @@ class TargetProcess {
   // The token associated with the process. It provides the core of the
   // sbox security.
   base::win::ScopedHandle lockdown_token_;
+  // The lowbox token associated with the process. This token is set after the
+  // process creation.
+  base::win::ScopedHandle lowbox_token_;
   // The token given to the initial thread so that the target process can
   // start. It has more powers than the lockdown_token.
   base::win::ScopedHandle initial_token_;