Add a sandbox policy for duplicating handles into the broker.

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

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

5 files changed, 71 insertions, 16 deletions

diff --git a/sandbox/src/handle_dispatcher.cc b/sandbox/src/handle_dispatcher.cc
index 921a42f..52ecb4a 100644
--- a/sandbox/src/handle_dispatcher.cc
+++ b/sandbox/src/handle_dispatcher.cc
@@ -70,8 +70,9 @@ bool HandleDispatcher::DuplicateHandleProxy(IPCInfo* ipc,
   }
   type_info->Name.Buffer[type_info->Name.Length / sizeof(wchar_t)] = L'\0';
 
-  CountedParameterSet<NameBased> params;
-  params[NameBased::NAME] = ParamPickerMake(type_info->Name.Buffer);
+  CountedParameterSet<HandleTarget> params;
+  params[HandleTarget::NAME] = ParamPickerMake(type_info->Name.Buffer);
+  params[HandleTarget::TARGET] = ParamPickerMake(target_process_id);
 
   EvalResult eval = policy_base_->EvalPolicy(IPC_DUPLICATEHANDLEPROXY_TAG,
                                              params.GetBase());
diff --git a/sandbox/src/handle_policy.cc b/sandbox/src/handle_policy.cc
index ef06e32..355dda8 100644
--- a/sandbox/src/handle_policy.cc
+++ b/sandbox/src/handle_policy.cc
@@ -19,12 +19,29 @@ namespace sandbox {
 bool HandlePolicy::GenerateRules(const wchar_t* type_name,
                                  TargetPolicy::Semantics semantics,
                                  LowLevelPolicy* policy) {
-  // We don't support any other semantics for handles yet.
-  if (TargetPolicy::HANDLES_DUP_ANY != semantics) {
-    return false;
-  }
   PolicyRule duplicate_rule(ASK_BROKER);
-  if (!duplicate_rule.AddStringMatch(IF, NameBased::NAME, type_name,
+
+  switch (semantics) {
+    case TargetPolicy::HANDLES_DUP_ANY: {
+      if (!duplicate_rule.AddNumberMatch(IF_NOT, HandleTarget::TARGET,
+                                         ::GetCurrentProcessId(), EQUAL)) {
+        return false;
+      }
+      break;
+    }
+
+    case TargetPolicy::HANDLES_DUP_BROKER: {
+      if (!duplicate_rule.AddNumberMatch(IF, HandleTarget::TARGET,
+                                         ::GetCurrentProcessId(), EQUAL)) {
+        return false;
+      }
+      break;
+    }
+
+    default:
+     return false;
+  }
+  if (!duplicate_rule.AddStringMatch(IF, HandleTarget::NAME, type_name,
                                      CASE_INSENSITIVE)) {
     return false;
   }
@@ -46,17 +63,23 @@ DWORD HandlePolicy::DuplicateHandleProxyAction(EvalResult eval_result,
     return ERROR_ACCESS_DENIED;
   }
 
-  // Make sure the target is one of our sandboxed children.
-  if (!BrokerServicesBase::GetInstance()->IsActiveTarget(target_process_id)) {
-    return ERROR_ACCESS_DENIED;
-  }
+  base::win::ScopedHandle remote_target_process;
+  if (target_process_id != ::GetCurrentProcessId()) {
+    // Sandboxed children are dynamic, so we check that manually.
+    if (!BrokerServicesBase::GetInstance()->IsActiveTarget(target_process_id)) {
+      return ERROR_ACCESS_DENIED;
+    }
 
-  base::win::ScopedHandle target_process(::OpenProcess(PROCESS_DUP_HANDLE,
-                                                       FALSE,
-                                                       target_process_id));
-  if (NULL == target_process)
-    return ::GetLastError();
+    remote_target_process.Set(::OpenProcess(PROCESS_DUP_HANDLE, FALSE,
+                                            target_process_id));
+    if (!remote_target_process.IsValid())
+      return ::GetLastError();
+  }
 
+  // If the policy didn't block us and we have no valid target, then the broker
+  // (this process) is the valid target.
+  HANDLE target_process = remote_target_process.IsValid() ?
+                          remote_target_process : ::GetCurrentProcess();
   DWORD result = ERROR_SUCCESS;
   if (!::DuplicateHandle(client_info.process, source_handle, target_process,
                          target_handle, desired_access, FALSE,
diff --git a/sandbox/src/handle_policy_test.cc b/sandbox/src/handle_policy_test.cc
index bb08b86..05eb39b 100644
--- a/sandbox/src/handle_policy_test.cc
+++ b/sandbox/src/handle_policy_test.cc
@@ -87,5 +87,28 @@ TEST(HandlePolicyTest, DuplicatePeerHandle) {
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(cmd_line.c_str()));
 }
 
+// Tests that duplicating an object works only when the policy allows it.
+TEST(HandlePolicyTest, DuplicateBrokerHandle) {
+  TestRunner runner;
+
+  // First test that we fail to open the event.
+  std::wstring cmd_line = base::StringPrintf(L"Handle_DuplicateEvent %d",
+                                             ::GetCurrentProcessId());
+  EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(cmd_line.c_str()));
+
+  // Add the peer rule and make sure we fail again.
+  EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_HANDLES,
+                             TargetPolicy::HANDLES_DUP_ANY,
+                             L"Event"));
+  EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(cmd_line.c_str()));
+
+
+  // Now successfully open the event after adding a broker handle rule.
+  EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_HANDLES,
+                             TargetPolicy::HANDLES_DUP_BROKER,
+                             L"Event"));
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(cmd_line.c_str()));
+}
+
 }  // namespace sandbox
 
diff --git a/sandbox/src/policy_params.h b/sandbox/src/policy_params.h
index e99e742..e1fb3fc 100644
--- a/sandbox/src/policy_params.h
+++ b/sandbox/src/policy_params.h
@@ -52,6 +52,13 @@ POLPARAMS_BEGIN(OpenKey)
   POLPARAM(ACCESS)
 POLPARAMS_END(OpenKey)
 
+// Policy parameter for name-based policies.
+POLPARAMS_BEGIN(HandleTarget)
+  POLPARAM(NAME)
+  POLPARAM(TARGET)
+POLPARAMS_END(HandleTarget)
+
+
 }  // namespace sandbox
 
 #endif  // SANDBOX_SRC_POLICY_PARAMS_H__
diff --git a/sandbox/src/sandbox_policy.h b/sandbox/src/sandbox_policy.h
index 1f561f5..ced8acb 100644
--- a/sandbox/src/sandbox_policy.h
+++ b/sandbox/src/sandbox_policy.h
@@ -142,6 +142,7 @@ class TargetPolicy {
                            // only.
     HANDLES_DUP_ANY,       // Allows duplicating handles opened with any
                            // access permissions.
+    HANDLES_DUP_BROKER,    // Allows duplicating handles to the broker process.
     NAMEDPIPES_ALLOW_ANY,  // Allows creation of a named pipe.
     PROCESS_MIN_EXEC,      // Allows to create a process with minimal rights
                            // over the resulting process and thread handles.