Add a parameter to the sandbox policy to allow sandboxed process to run outside of a job and wire it to a cmd line flag.

This is needed for running chrome in Citrix or RemoteApp (Terminal Services) environments.
These envoronments both start the main process inside a job spawned by rdpinit.exe
(at least in the RemoteApp case) and the process are not allowed to escape it
therefore when the job assignment is attempted it failes with ERROR_PERMISSION_DENIED.
This is not a problem in Windows 8/Server 2012 because these allow nested jobs so we
should only respect this flag for versions older than that.

BUG=79091
TEST=Start Chrome as a published app with --allow-no-job and observe it spawning renderer processes properly.


Review URL: https://chromiumcodereview.appspot.com/10908171

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@160133 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 211 insertions, 26 deletions

diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index 0425845..65a7b2b 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -379,13 +379,24 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
   // 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();
-  scoped_ptr<JobTracker> tracker(new JobTracker(job.Take(), policy_base));
-  if (!AssociateCompletionPort(tracker->job, job_port_, tracker.get()))
-    return SpawnCleanup(target, 0);
-  // Save the tracker because in cleanup we might need to force closing
-  // the Jobs.
-  tracker_list_.push_back(tracker.release());
-  child_process_ids_.insert(process_info.process_id());
+  if (job.IsValid()) {
+    scoped_ptr<JobTracker> tracker(new JobTracker(job.Take(), policy_base));
+    if (!AssociateCompletionPort(tracker->job, job_port_, tracker.get()))
+      return SpawnCleanup(target, 0);
+    // Save the tracker because in cleanup we might need to force closing
+    // the Jobs.
+    tracker_list_.push_back(tracker.release());
+    child_process_ids_.insert(process_info.process_id());
+  } else {
+    // 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_);
+    // 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.
+  }
 
   *target_info = process_info.Take();
   return SBOX_ALL_OK;
diff --git a/sandbox/win/src/broker_services.h b/sandbox/win/src/broker_services.h
index 1f2e3fa..11c10e0 100644
--- a/sandbox/win/src/broker_services.h
+++ b/sandbox/win/src/broker_services.h
@@ -101,7 +101,8 @@ class BrokerServicesBase : public BrokerServices,
   typedef std::map<DWORD, PeerTracker*> PeerTrackerMap;
   PeerTrackerMap peer_map_;
 
-  // Provides a fast lookup to identify sandboxed processes.
+  // Provides a fast lookup to identify sandboxed processes that belong to a
+  // job. Consult |jobless_process_handles_| for handles of pocess without job.
   std::set<DWORD> child_process_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(BrokerServicesBase);
diff --git a/sandbox/win/src/job_unittest.cc b/sandbox/win/src/job_unittest.cc
index 8d84b78..597c86c 100644
--- a/sandbox/win/src/job_unittest.cc
+++ b/sandbox/win/src/job_unittest.cc
@@ -145,9 +145,13 @@ TEST(JobTest, SecurityLevel) {
   Job job5;
   ASSERT_EQ(ERROR_SUCCESS, job5.Init(JOB_UNPROTECTED, L"job5", 0));
 
+  // JOB_NONE means we run without a job object so Init should fail.
   Job job6;
-  ASSERT_EQ(ERROR_BAD_ARGUMENTS, job6.Init(
-      static_cast<JobLevel>(JOB_UNPROTECTED+1), L"job6", 0));
+  ASSERT_EQ(ERROR_BAD_ARGUMENTS, job6.Init(JOB_NONE, L"job6", 0));
+
+  Job job7;
+  ASSERT_EQ(ERROR_BAD_ARGUMENTS, job7.Init(
+      static_cast<JobLevel>(JOB_NONE+1), L"job7", 0));
 }
 
 // Tests the method "AssignProcessToJob".
diff --git a/sandbox/win/src/process_policy_test.cc b/sandbox/win/src/process_policy_test.cc
index 313cee6..937cbc8d 100644
--- a/sandbox/win/src/process_policy_test.cc
+++ b/sandbox/win/src/process_policy_test.cc
@@ -357,4 +357,28 @@ TEST(ProcessPolicyTest, TestGetProcessTokenMaxAccess) {
             runner.RunTest(L"Process_GetChildProcessToken findstr.exe"));
 }
 
+TEST(ProcessPolicyTest, TestGetProcessTokenMinAccessNoJob) {
+  TestRunner runner(JOB_NONE, USER_RESTRICTED_SAME_ACCESS, USER_LOCKDOWN);
+  string16 exe_path = MakeFullPathToSystem32(L"findstr.exe");
+  ASSERT_TRUE(!exe_path.empty());
+  EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_PROCESS,
+                             TargetPolicy::PROCESS_MIN_EXEC,
+                             exe_path.c_str()));
+
+  EXPECT_EQ(SBOX_TEST_DENIED,
+            runner.RunTest(L"Process_GetChildProcessToken findstr.exe"));
+}
+
+TEST(ProcessPolicyTest, TestGetProcessTokenMaxAccessNoJob) {
+  TestRunner runner(JOB_NONE, USER_INTERACTIVE, USER_INTERACTIVE);
+  string16 exe_path = MakeFullPathToSystem32(L"findstr.exe");
+  ASSERT_TRUE(!exe_path.empty());
+  EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_PROCESS,
+                             TargetPolicy::PROCESS_ALL_EXEC,
+                             exe_path.c_str()));
+
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"Process_GetChildProcessToken findstr.exe"));
+}
+
 }  // namespace sandbox
diff --git a/sandbox/win/src/sandbox_policy_base.cc b/sandbox/win/src/sandbox_policy_base.cc
index f942ff5a..10ac642 100644
--- a/sandbox/win/src/sandbox_policy_base.cc
+++ b/sandbox/win/src/sandbox_policy_base.cc
@@ -426,13 +426,17 @@ bool PolicyBase::SetupService(InterceptionManager* manager, int service) {
 }
 
 ResultCode PolicyBase::MakeJobObject(HANDLE* job) {
-  // Create the windows job object.
-  Job job_obj;
-  DWORD result = job_obj.Init(job_level_, NULL, ui_exceptions_);
-  if (ERROR_SUCCESS != result) {
-    return SBOX_ERROR_GENERIC;
+  if (job_level_ != JOB_NONE) {
+    // Create the windows job object.
+    Job job_obj;
+    DWORD result = job_obj.Init(job_level_, NULL, ui_exceptions_);
+    if (ERROR_SUCCESS != result) {
+      return SBOX_ERROR_GENERIC;
+    }
+    *job = job_obj.Detach();
+  } else {
+    *job = NULL;
   }
-  *job = job_obj.Detach();
   return SBOX_ALL_OK;
 }
 
diff --git a/sandbox/win/src/security_level.h b/sandbox/win/src/security_level.h
index 010185a..7ed1713 100644
--- a/sandbox/win/src/security_level.h
+++ b/sandbox/win/src/security_level.h
@@ -84,6 +84,9 @@ enum TokenLevel {
 //  JobLevel        |General                            |Quota               |
 //                  |restrictions                       |restrictions        |
 // -----------------|---------------------------------- |--------------------|
+// JOB_NONE         | No job is assigned to the         | None               |
+//                  | sandboxed process.                |                    |
+// -----------------|---------------------------------- |--------------------|
 // JOB_UNPROTECTED  | None                              | *Kill on Job close.|
 // -----------------|---------------------------------- |--------------------|
 // JOB_INTERACTIVE  | *Forbid system-wide changes using |                    |
@@ -119,7 +122,8 @@ enum JobLevel {
   JOB_RESTRICTED,
   JOB_LIMITED_USER,
   JOB_INTERACTIVE,
-  JOB_UNPROTECTED
+  JOB_UNPROTECTED,
+  JOB_NONE
 };
 
 // These flags correspond to various process-level mitigations (eg. ASLR and
diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc
index 4eea180..5de6ee0 100644
--- a/sandbox/win/src/target_process.cc
+++ b/sandbox/win/src/target_process.cc
@@ -124,7 +124,7 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
   if (startup_info.has_extended_startup_info())
     flags |= EXTENDED_STARTUPINFO_PRESENT;
 
-  if (base::win::GetVersion() < base::win::VERSION_WIN8) {
+  if (job_ && base::win::GetVersion() < base::win::VERSION_WIN8) {
     // Windows 8 implements nested jobs, but for older systems we need to
     // break out of any job we're in to enforce our restrictions.
     flags |= CREATE_BREAKAWAY_FROM_JOB;
@@ -149,13 +149,13 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
 
   DWORD win_result = ERROR_SUCCESS;
 
-  // Assign the suspended target to the windows job object.
-  if (!::AssignProcessToJobObject(job_, process_info.process_handle())) {
-    win_result = ::GetLastError();
-    // It might be a security breach if we let the target run outside the job
-    // so kill it before it causes damage.
-    ::TerminateProcess(process_info.process_handle(), 0);
-    return win_result;
+  if (job_) {
+    // Assign the suspended target to the windows job object.
+    if (!::AssignProcessToJobObject(job_, process_info.process_handle())) {
+      win_result = ::GetLastError();
+      ::TerminateProcess(process_info.process_handle(), 0);
+      return win_result;
+    }
   }
 
   if (initial_token_.IsValid()) {
@@ -165,6 +165,8 @@ DWORD TargetProcess::Create(const wchar_t* exe_path,
     HANDLE temp_thread = process_info.thread_handle();
     if (!::SetThreadToken(&temp_thread, initial_token_)) {
       win_result = ::GetLastError();
+      // It might be a security breach if we let the target run outside the job
+      // so kill it before it causes damage.
       ::TerminateProcess(process_info.process_handle(), 0);
       return win_result;
     }
diff --git a/sandbox/win/tests/common/controller.cc b/sandbox/win/tests/common/controller.cc
index 921ce12..1de2092 100644
--- a/sandbox/win/tests/common/controller.cc
+++ b/sandbox/win/tests/common/controller.cc
@@ -108,6 +108,7 @@ void TestRunner::Init(JobLevel job_level, TokenLevel startup_token,
   timeout_ = kDefaultTimeout;
   state_ = AFTER_REVERT;
   is_async_= false;
+  kill_on_destruction_ = true;
   target_process_id_ = 0;
 
   broker_ = GetBroker();
@@ -129,7 +130,7 @@ TargetPolicy* TestRunner::GetPolicy() {
 }
 
 TestRunner::~TestRunner() {
-  if (target_process_)
+  if (target_process_ && kill_on_destruction_)
     ::TerminateProcess(target_process_, 0);
 
   if (policy_)
diff --git a/sandbox/win/tests/common/controller.h b/sandbox/win/tests/common/controller.h
index 3d42878..6c143e5 100644
--- a/sandbox/win/tests/common/controller.h
+++ b/sandbox/win/tests/common/controller.h
@@ -107,6 +107,10 @@ class TestRunner {
   // Sets the desired state for the test to run.
   void SetTestState(SboxTestsState desired_state);
 
+  // Sets a flag whether the process should be killed when the TestRunner is
+  // destroyed.
+  void SetKillOnDestruction(bool value) { kill_on_destruction_ = value; }
+
   // Returns the pointers to the policy object. It can be used to modify
   // the policy manually.
   TargetPolicy* GetPolicy();
@@ -135,6 +139,7 @@ class TestRunner {
   bool is_init_;
   bool is_async_;
   bool no_sandbox_;
+  bool kill_on_destruction_;
   base::win::ScopedHandle target_process_;
   DWORD target_process_id_;
 };
diff --git a/sandbox/win/tests/integration_tests/integration_tests_test.cc b/sandbox/win/tests/integration_tests/integration_tests_test.cc
index 1f54c64..e1930c1 100644
--- a/sandbox/win/tests/integration_tests/integration_tests_test.cc
+++ b/sandbox/win/tests/integration_tests/integration_tests_test.cc
@@ -43,6 +43,17 @@ SBOX_TESTS_COMMAND int IntegrationTestsTest_state2(int argc, wchar_t **argv) {
   return state;
 }
 
+// Blocks the process for argv[0] milliseconds simulating stuck child.
+SBOX_TESTS_COMMAND int IntegrationTestsTest_stuck(int argc, wchar_t **argv) {
+  int timeout = 500;
+  if (argc > 0) {
+    timeout = _wtoi(argv[0]);
+  }
+
+  ::Sleep(timeout);
+  return 1;
+}
+
 // Returns the number of arguments
 SBOX_TESTS_COMMAND int IntegrationTestsTest_args(int argc, wchar_t **argv) {
   for (int i = 0; i < argc; i++) {
@@ -91,4 +102,122 @@ TEST(IntegrationTestsTest, ForwardsArguments) {
                               L"fourth"));
 }
 
+TEST(IntegrationTestsTest, StuckChild) {
+  TestRunner runner;
+  runner.SetTimeout(2000);
+  runner.SetAsynchronous(true);
+  runner.SetKillOnDestruction(false);
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 100"));
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  // In this case the process must have finished.
+  DWORD exit_code;
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(1, exit_code);
+}
+
+TEST(IntegrationTestsTest, StuckChildNoJob) {
+  TestRunner runner(JOB_NONE, USER_RESTRICTED_SAME_ACCESS, USER_LOCKDOWN);
+  runner.SetTimeout(2000);
+  runner.SetAsynchronous(true);
+  runner.SetKillOnDestruction(false);
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 2000"));
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  // In this case the processes are not tracked by the broker and should be
+  // still active.
+  DWORD exit_code;
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(STILL_ACTIVE, exit_code);
+  // Terminate the test process now.
+  ::TerminateProcess(runner.process(), 0);
+}
+
+TEST(IntegrationTestsTest, TwoStuckChildrenSecondOneHasNoJob) {
+  TestRunner runner;
+  runner.SetTimeout(2000);
+  runner.SetAsynchronous(true);
+  runner.SetKillOnDestruction(false);
+  TestRunner runner2(JOB_NONE, USER_RESTRICTED_SAME_ACCESS, USER_LOCKDOWN);
+  runner2.SetTimeout(2000);
+  runner2.SetAsynchronous(true);
+  runner2.SetKillOnDestruction(false);
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 100"));
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner2.RunTest(L"IntegrationTestsTest_stuck 2000"));
+  // Actually both runners share the same singleton broker.
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  // In this case the processes are not tracked by the broker and should be
+  // still active.
+  DWORD exit_code;
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(1, exit_code);
+  ASSERT_TRUE(::GetExitCodeProcess(runner2.process(), &exit_code));
+  ASSERT_EQ(STILL_ACTIVE, exit_code);
+  // Terminate the test process now.
+  ::TerminateProcess(runner2.process(), 0);
+}
+
+TEST(IntegrationTestsTest, TwoStuckChildrenFirstOneHasNoJob) {
+  TestRunner runner;
+  runner.SetTimeout(2000);
+  runner.SetAsynchronous(true);
+  runner.SetKillOnDestruction(false);
+  TestRunner runner2(JOB_NONE, USER_RESTRICTED_SAME_ACCESS, USER_LOCKDOWN);
+  runner2.SetTimeout(2000);
+  runner2.SetAsynchronous(true);
+  runner2.SetKillOnDestruction(false);
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner2.RunTest(L"IntegrationTestsTest_stuck 2000"));
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 100"));
+  // Actually both runners share the same singleton broker.
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  // In this case the processes are not tracked by the broker and should be
+  // still active.
+  DWORD exit_code;
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(1, exit_code);
+  ASSERT_TRUE(::GetExitCodeProcess(runner2.process(), &exit_code));
+  ASSERT_EQ(STILL_ACTIVE, exit_code);
+  // Terminate the test process now.
+  ::TerminateProcess(runner2.process(), 0);
+}
+
+TEST(IntegrationTestsTest, MultipleStuckChildrenSequential) {
+  TestRunner runner;
+  runner.SetTimeout(2000);
+  runner.SetAsynchronous(true);
+  runner.SetKillOnDestruction(false);
+  TestRunner runner2(JOB_NONE, USER_RESTRICTED_SAME_ACCESS, USER_LOCKDOWN);
+  runner2.SetTimeout(2000);
+  runner2.SetAsynchronous(true);
+  runner2.SetKillOnDestruction(false);
+
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 100"));
+  // Actually both runners share the same singleton broker.
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner2.RunTest(L"IntegrationTestsTest_stuck 2000"));
+  // Actually both runners share the same singleton broker.
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+
+  DWORD exit_code;
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(1, exit_code);
+  ASSERT_TRUE(::GetExitCodeProcess(runner2.process(), &exit_code));
+  ASSERT_EQ(STILL_ACTIVE, exit_code);
+  // Terminate the test process now.
+  ::TerminateProcess(runner2.process(), 0);
+
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED,
+            runner.RunTest(L"IntegrationTestsTest_stuck 100"));
+  // Actually both runners share the same singleton broker.
+  ASSERT_EQ(SBOX_ALL_OK, runner.broker()->WaitForAllTargets());
+  ASSERT_TRUE(::GetExitCodeProcess(runner.process(), &exit_code));
+  ASSERT_EQ(1, exit_code);
+}
+
 }  // namespace sandbox