Correctly handle child processes of sandboxed target processes.

If the Job blocks child process creation then
JOB_OBJECT_MSG_ACTIVE_PROCESS_LIMIT followed by a
JOB_OBJECT_MSG_EXIT_PROCESS is sent to the job completion port of the
calling process.

This was causing a mismatch in the target_process count since it was not
being incremented. This CL correctly increments target_counter when
these phantom processes are blocked.

Since it's not possible to know the process id of a a process that has
been blocked by the Job process limit, this CL adds a second counter to
track these untracked processes and verifies when receving a
JOB_OBJECT_MSG_EXIT_PROCESS or JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS that
it is either a fully tracked process in broker->child_process_ids_ or
that it is an untracked child process of a target.

This also adds tests for the case when a child process and a child
process of a target crash, which tests the
JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS path which was previously untested.

BUG=584753
TEST=sbox_integration_tests

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

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

3 files changed, 89 insertions, 5 deletions

diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc
index f60c30b..26b2561 100644
--- a/sandbox/win/src/broker_services.cc
+++ b/sandbox/win/src/broker_services.cc
@@ -195,6 +195,7 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
   HANDLE no_targets = broker->no_targets_.Get();
 
   int target_counter = 0;
+  int untracked_target_counter = 0;
   ::ResetEvent(no_targets);
 
   while (true) {
@@ -226,6 +227,14 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
         }
 
         case JOB_OBJECT_MSG_NEW_PROCESS: {
+          DWORD handle = static_cast<DWORD>(reinterpret_cast<uintptr_t>(ovl));
+          {
+            AutoLock lock(&broker->lock_);
+            size_t count = broker->child_process_ids_.count(handle);
+            // Child process created from sandboxed process.
+            if (count == 0)
+              untracked_target_counter++;
+          }
           ++target_counter;
           if (1 == target_counter) {
             ::ResetEvent(no_targets);
@@ -235,11 +244,17 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
 
         case JOB_OBJECT_MSG_EXIT_PROCESS:
         case JOB_OBJECT_MSG_ABNORMAL_EXIT_PROCESS: {
+          size_t erase_result = 0;
           {
             AutoLock lock(&broker->lock_);
-            broker->child_process_ids_.erase(
+            erase_result = broker->child_process_ids_.erase(
                 static_cast<DWORD>(reinterpret_cast<uintptr_t>(ovl)));
           }
+          if (erase_result != 1U) {
+            // The process was untracked e.g. a child process of the target.
+            --untracked_target_counter;
+            DCHECK(untracked_target_counter >= 0);
+          }
           --target_counter;
           if (0 == target_counter)
             ::SetEvent(no_targets);
@@ -249,6 +264,10 @@ DWORD WINAPI BrokerServicesBase::TargetEventsThread(PVOID param) {
         }
 
         case JOB_OBJECT_MSG_ACTIVE_PROCESS_LIMIT: {
+          // A child process attempted and failed to create a child process.
+          // Windows does not reveal the process id.
+          untracked_target_counter++;
+          target_counter++;
           break;
         }
 
diff --git a/sandbox/win/src/process_mitigations_test.cc b/sandbox/win/src/process_mitigations_test.cc
index 6c82976..b7c608e 100644
--- a/sandbox/win/src/process_mitigations_test.cc
+++ b/sandbox/win/src/process_mitigations_test.cc
@@ -205,7 +205,8 @@ void TestWin10ImageLoadLowLabel(bool is_success_test) {
 namespace sandbox {
 
 // A shared helper test command that will attempt to CreateProcess
-// with a given command line.
+// with a given command line. The second parameter, if set to non-zero
+// will cause the child process to return exit code STATUS_ACCESS_VIOLATION.
 //
 // ***Make sure you've enabled basic process creation in the
 // test sandbox settings via:
@@ -213,15 +214,19 @@ namespace sandbox {
 // sandbox::TargetPolicy::SetTokenLevel(),
 // and TestRunner::SetDisableCsrss().
 SBOX_TESTS_COMMAND int TestChildProcess(int argc, wchar_t** argv) {
-  if (argc < 1)
+  if (argc < 2)
     return SBOX_TEST_INVALID_PARAMETER;
 
+  int desired_exit_code = _wtoi(argv[1]);
+  if (desired_exit_code)
+      desired_exit_code = STATUS_ACCESS_VIOLATION;
+
   std::wstring cmd = argv[0];
   base::LaunchOptions options = base::LaunchOptionsForTest();
   base::Process setup_proc = base::LaunchProcess(cmd.c_str(), options);
 
   if (setup_proc.IsValid()) {
-    setup_proc.Terminate(0, false);
+    setup_proc.Terminate(desired_exit_code, false);
     return SBOX_TEST_SUCCEEDED;
   }
   // Note: GetLastError from CreateProcess returns 5, "ERROR_ACCESS_DENIED".
@@ -672,6 +677,7 @@ TEST(ProcessMitigationsTest, CheckChildProcessSuccess) {
 
   std::wstring test_command = L"TestChildProcess ";
   test_command += cmd.value().c_str();
+  test_command += L" 0";
 
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(test_command.c_str()));
 }
@@ -695,9 +701,58 @@ TEST(ProcessMitigationsTest, CheckChildProcessFailure) {
 
   std::wstring test_command = L"TestChildProcess ";
   test_command += cmd.value().c_str();
+  test_command += L" 0";
 
   EXPECT_EQ(SBOX_TEST_FAILED, runner.RunTest(test_command.c_str()));
 }
 
-}  // namespace sandbox
+// This test validates that we can spawn a child process if
+// MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED mitigation is
+// not set. This also tests that a crashing child process is correctly handled
+// by the broker.
+TEST(ProcessMitigationsTest, CheckChildProcessSuccessAbnormalExit) {
+  TestRunner runner;
+  sandbox::TargetPolicy* policy = runner.GetPolicy();
+
+  // Set a policy that would normally allow for process creation.
+  policy->SetJobLevel(JOB_INTERACTIVE, 0);
+  policy->SetTokenLevel(USER_UNPROTECTED, USER_UNPROTECTED);
+  runner.SetDisableCsrss(false);
+
+  base::FilePath cmd;
+  EXPECT_TRUE(base::PathService::Get(base::DIR_SYSTEM, &cmd));
+  cmd = cmd.Append(L"calc.exe");
+
+  std::wstring test_command = L"TestChildProcess ";
+  test_command += cmd.value().c_str();
+  test_command += L" 1";
+
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(test_command.c_str()));
+}
 
+// This test validates that setting the
+// MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED mitigation prevents
+// the spawning of child processes.  This also tests that a crashing child
+// process is correctly handled by the broker.
+TEST(ProcessMitigationsTest, CheckChildProcessFailureAbnormalExit) {
+  TestRunner runner;
+  sandbox::TargetPolicy* policy = runner.GetPolicy();
+
+  // Now set the job level to be <= JOB_LIMITED_USER
+  // and ensure we can no longer create a child process.
+  policy->SetJobLevel(JOB_LIMITED_USER, 0);
+  policy->SetTokenLevel(USER_UNPROTECTED, USER_UNPROTECTED);
+  runner.SetDisableCsrss(false);
+
+  base::FilePath cmd;
+  EXPECT_TRUE(base::PathService::Get(base::DIR_SYSTEM, &cmd));
+  cmd = cmd.Append(L"calc.exe");
+
+  std::wstring test_command = L"TestChildProcess ";
+  test_command += cmd.value().c_str();
+  test_command += L" 1";
+
+  EXPECT_EQ(SBOX_TEST_FAILED, runner.RunTest(test_command.c_str()));
+}
+
+}  // namespace sandbox
diff --git a/sandbox/win/src/process_policy_test.cc b/sandbox/win/src/process_policy_test.cc
index 8062d46..28d5ac2 100644
--- a/sandbox/win/src/process_policy_test.cc
+++ b/sandbox/win/src/process_policy_test.cc
@@ -270,6 +270,10 @@ SBOX_TESTS_COMMAND int Process_OpenToken(int argc, wchar_t **argv) {
   return SBOX_TEST_FAILED;
 }
 
+SBOX_TESTS_COMMAND int Process_Crash(int argc, wchar_t **argv) {
+  __debugbreak();
+  return SBOX_TEST_FAILED;
+}
 // Generate a event name, used to test thread creation.
 std::wstring GenerateEventName(DWORD pid) {
   wchar_t buff[30] = {0};
@@ -409,6 +413,12 @@ TEST(ProcessPolicyTest, CreateProcessAW) {
             runner.RunTest(L"Process_RunApp6 findstr.exe"));
 }
 
+// Tests that the broker correctly handles a process crashing within the job.
+TEST(ProcessPolicyTest, CreateProcessCrashy) {
+  TestRunner runner;
+  EXPECT_EQ(STATUS_BREAKPOINT, runner.RunTest(L"Process_Crash"));
+}
+
 TEST(ProcessPolicyTest, OpenToken) {
   TestRunner runner;
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(L"Process_OpenToken"));