Initial implementation of the Mac Bootstrap Sandbox.

sandbox/mac/bootstrap_sandbox.cc

+// Copyright 2014 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/mac/bootstrap_sandbox.h"
+
+#include "base/logging.h"
+
+#include "sandbox/mac/launchd_interception_server.h"
+
+namespace sandbox {
+
+// static
+scoped_ptr<BootstrapSandbox> BootstrapSandbox::Create() {
+  scoped_ptr<BootstrapSandbox> sandbox(new BootstrapSandbox());
+  sandbox->server_.reset(new LaunchdInterceptionServer(sandbox.get()));
+
+  if (!sandbox->server_->Initialize()) {
+    sandbox.reset();
+  } else {
+    kern_return_t kr = task_set_special_port(mach_task_self(),
+        TASK_BOOTSTRAP_PORT, sandbox->server_->server_port());

[Mark Mentovai, 2014/05/06 20:51:50]

There’s a global extern mach_port_t named bootstrap_port that is the cached
bootstrap port, and it works in much the same way as the mach_task_self_ global.
Things use it to communicate with the bootstrap server without having to go
through task_get_special_port. Since you’re changing the task bootstrap port
here, you should strongly consider setting bootstrap_port to the same port.

[Robert Sesek, 2014/05/08 20:58:12]

Right, this is something that I considered but wasn't sure was necessary. Since
it's not necessary to change the bootstrap port for sandboxing purposes in the
parent, and it's only being done because of the concerns over concurrent access
to task_get_special_port(), it seems like additional overhead for callers trying
to talk to the bootstrap server via the extern global.

I'm happy to update the global, too, but it seems OK to leave it as-is.

[Mark Mentovai, 2014/05/09 20:11:19]

rsesek wrote:
OK, then you should mention in a comment that you’ve thought about it and aren’t
touching it, and that that you also don’t envision anything wrong with touching
it.

[Robert Sesek, 2014/05/09 22:04:03]

Done.

+    if (kr != KERN_SUCCESS)
+      sandbox.reset();
+  }
+
+  return sandbox.Pass();
+}
+
+BootstrapSandbox::~BootstrapSandbox() {
+  CHECK_EQ(KERN_SUCCESS, task_set_special_port(mach_task_self(),
+      TASK_BOOTSTRAP_PORT, real_bootstrap_port_));
+}
+
+void BootstrapSandbox::RegisterSandboxPolicy(
+    int sandbox_policy_id,
+    const BootstrapSandboxPolicy& policy) {
+  DCHECK(IsPolicyValid(policy));

[Mark Mentovai, 2014/05/06 20:51:50]

You’re CHECKy elsewhere, why’d you land on DCHECK for policy registration?

[Robert Sesek, 2014/05/08 20:58:12]

Done.

+  DCHECK_GT(sandbox_policy_id, 0);
+  base::AutoLock lock(lock_);
+  DCHECK(policies_.find(sandbox_policy_id) == policies_.end());
+  policies_.insert(std::make_pair(sandbox_policy_id, policy));
+}
+
+bool BootstrapSandbox::PrepareToForkWithPolicy(int sandbox_policy_id) {
+  base::AutoLock lock(lock_);
+
+  // Make sure the policy exists.
+  if (policies_.find(sandbox_policy_id) == policies_.end())
+    return false;

[Mark Mentovai, 2014/05/06 20:51:50]

Based on the implementation here, it appears that you don’t need
PrepareToForkWithPolicy at all, you can just pass the policy ID to FinishedFork.
This check is the only thing that can fail, but returning failure here isn’t
that much better than treating an unknown policy ID as “reject everything.” I
don’t see a massive advantage to checking for the policy before forking.

Taking the same lock twice for each fork in the parent process sucks.

[Robert Sesek, 2014/05/08 20:58:12]

Not really, because the child could start executing before the parent calls
FinishFork. How would you know what sandbox policy to apply to this new child?
I changed it to a CHECK. The point is to prevent developer error. I think a
"reject everything" approach for an unknown policy may seem reasonable, but it's
really just letting the programmer do the wrong thing and get away with it.
Per offline discussion, taking the lock twice sucks, but the other options are
not much better. Other options (for posterity):

  1. Acquire in Prepare() and Release in Finished(), but this means blocking
servicing of the bootstrap port for the duration of LaunchProcess
  2. Use atomicops to do this, but this makes PolicyForProcess() subtle and
tricky, to the extent of it not being worth it.
  3. Use a RWLock, but this still requires taking the lock multiple times if we
do not want to create the situation of (1). Also, no RWLock implementation
exists currently.

+
+  // Make sure that we aren't trying to fork() on two different threads
+  // simultaneously.
+  CHECK(!is_across_fork_) << "Cannot interleave PrepareToForkWithPolicy()";
+  is_across_fork_ = true;
+
+  // Store the policy for the process we're about to create.
+  effective_policy_id_ = sandbox_policy_id;
+
+  return true;
+}
+
+void BootstrapSandbox::FinishedFork(base::ProcessHandle handle) {
+  base::AutoLock lock(lock_);
+  CHECK(is_across_fork_) <<
+      "Must PrepareToForkWithPolicy() before FinishedFork()";
+  is_across_fork_ = false;
+
+  const auto& existing_process = sandboxed_processes_.find(handle);
+  CHECK(existing_process == sandboxed_processes_.end());
+  sandboxed_processes_.insert(std::make_pair(handle, effective_policy_id_));
+
+  VLOG(3) << "Bootstrap sandbox enforced for pid " << handle;
+
+  effective_policy_id_ = -1;
+}
+
+void BootstrapSandbox::ChildDied(base::ProcessHandle handle) {
+  base::AutoLock lock(lock_);
+  sandboxed_processes_.erase(handle);

[Mark Mentovai, 2014/05/06 20:51:50]

You were very CHECKy about things not being in the map when you were inserting
them. Do you want to be CHECKy that they’re in the map before erasing them? (You
can get an iterator, CHECK it, and then erase using the iterator to avoid doing
the same map lookup twice.)

[Robert Sesek, 2014/05/08 20:58:12]

Done.

+}
+
+const BootstrapSandboxPolicy* BootstrapSandbox::PolicyForProcess(
+    pid_t pid) const {
+  base::AutoLock lock(lock_);
+  const auto& process = sandboxed_processes_.find(pid);
+
+  int policy_id = -1;
+  // The new child could send bootstrap requests before the parent calls
+  // FinishFork().
+  if (process != sandboxed_processes_.end()) {
+    policy_id = process->second;
+  } else if (is_across_fork_) {
+    policy_id = effective_policy_id_;
+  }

[Mark Mentovai, 2014/05/06 20:51:50]

You can just say “else return NULL” here and not have to initialize policy_id to
-1.

[Robert Sesek, 2014/05/08 20:58:12]

This is because effective_policy_id_ could be -1.

+
+  if (policy_id == -1)
+    return NULL;
+
+  return &policies_.find(policy_id)->second;
+}
+
+BootstrapSandbox::BootstrapSandbox()
+    : real_bootstrap_port_(MACH_PORT_NULL),
+      is_across_fork_(false),
+      effective_policy_id_(-1) {
+  CHECK_EQ(KERN_SUCCESS, task_get_special_port(
+      mach_task_self(), TASK_BOOTSTRAP_PORT, &real_bootstrap_port_));

[Mark Mentovai, 2014/05/06 20:51:50]

When you task_get_special_port, you need to mach_port_deallocate the port you
get when you’re done with it. Here, that’d be in the destructor. ScopedMachPort?

[Robert Sesek, 2014/05/08 20:58:12]

Done.

+}
+
+}  // namespace sandbox