Alter the design of the bootstrap sandbox to only take over the bootstrap port of children when necessary.

Rather than replacing the bootstrap port outright in the browser process, this
change merely registers the sandboxed bootstrap port with launchd. When a
sandboxed child is being launched with base::LaunchProcess(), a new
LaunchOptions can specify a bootstrap name to look up and use as a replacement
bootstrap port.

The bootstrap port in the new child is replaced after fork() but before exec().
The kernel clears the IPC space during both of these system calls, so no other
references to the original bootstrap port will exist after replacing the port
with the sandboxed one and exec()ing.

This change also partially reverts r276026, which introduced a permissive
policy for NPAPI plugins. Since those plugins are no longer affected by the
bootstrap sandbox, it can be removed.

BUG=367863,383513,383517,383791,386330
R=jam@chromium.org, mark@chromium.org

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

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

7 files changed, 66 insertions, 48 deletions

diff --git a/sandbox/mac/bootstrap_sandbox.cc b/sandbox/mac/bootstrap_sandbox.cc
index 6407c68..a90f570 100644
--- a/sandbox/mac/bootstrap_sandbox.cc
+++ b/sandbox/mac/bootstrap_sandbox.cc
@@ -4,9 +4,13 @@
 
 #include "sandbox/mac/bootstrap_sandbox.h"
 
+#include <servers/bootstrap.h>
+#include <unistd.h>
+
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #include "base/mac/mach_logging.h"
-
+#include "base/strings/stringprintf.h"
 #include "sandbox/mac/launchd_interception_server.h"
 
 namespace sandbox {
@@ -19,41 +23,28 @@ scoped_ptr<BootstrapSandbox> BootstrapSandbox::Create() {
   scoped_ptr<BootstrapSandbox> sandbox(new BootstrapSandbox());
   sandbox->server_.reset(new LaunchdInterceptionServer(sandbox.get()));
 
-  if (!sandbox->server_->Initialize())
-    return null.Pass();
-
-  mach_port_t port = sandbox->server_->server_port();
-  kern_return_t kr = mach_port_insert_right(mach_task_self(), port, port,
-      MACH_MSG_TYPE_MAKE_SEND);
+  // Check in with launchd to get the receive right for the server that is
+  // published in the bootstrap namespace.
+  mach_port_t port = MACH_PORT_NULL;
+  kern_return_t kr = bootstrap_check_in(bootstrap_port,
+      sandbox->server_bootstrap_name().c_str(), &port);
   if (kr != KERN_SUCCESS) {
-    MACH_LOG(ERROR, kr) << "Failed to insert send right on bootstrap port.";
+    BOOTSTRAP_LOG(ERROR, kr)
+        << "Failed to bootstrap_check_in the sandbox server.";
     return null.Pass();
   }
-  base::mac::ScopedMachSendRight scoped_right(port);
-
-  // Note that the extern global bootstrap_port (in bootstrap.h) will not
-  // be changed here. The parent only has its bootstrap port replaced
-  // permanently because changing it repeatedly in a multi-threaded program
-  // could lead to unsafe access patterns. In a single-threaded program,
-  // the port would be restored after fork(). See the design document for
-  // a larger discussion.
-  //
-  // By not changing the global bootstrap_port, users of the bootstrap port
-  // in the parent can potentially skip an unnecessary indirection through
-  // the sandbox server.
-  kr = task_set_special_port(mach_task_self(), TASK_BOOTSTRAP_PORT, port);
-  if (kr != KERN_SUCCESS) {
-    MACH_LOG(ERROR, kr) << "Failed to set new bootstrap port.";
+  base::mac::ScopedMachReceiveRight scoped_port(port);
+
+  // Start the sandbox server.
+  if (sandbox->server_->Initialize(scoped_port.get()))
+    ignore_result(scoped_port.release());  // Transferred to server_.
+  else
     return null.Pass();
-  }
 
   return sandbox.Pass();
 }
 
 BootstrapSandbox::~BootstrapSandbox() {
-  kern_return_t kr = task_set_special_port(mach_task_self(),
-      TASK_BOOTSTRAP_PORT, real_bootstrap_port_);
-  MACH_CHECK(kr == KERN_SUCCESS, kr);
 }
 
 void BootstrapSandbox::RegisterSandboxPolicy(
@@ -69,6 +60,7 @@ void BootstrapSandbox::RegisterSandboxPolicy(
 void BootstrapSandbox::PrepareToForkWithPolicy(int sandbox_policy_id) {
   base::AutoLock lock(lock_);
 
+  // Verify that this is a real policy.
   CHECK(policies_.find(sandbox_policy_id) != policies_.end());
   CHECK_EQ(kNotAPolicy, effective_policy_id_)
       << "Cannot nest calls to PrepareToForkWithPolicy()";
@@ -89,6 +81,7 @@ void BootstrapSandbox::FinishedFork(base::ProcessHandle handle) {
   CHECK_NE(kNotAPolicy, effective_policy_id_)
       << "Must PrepareToForkWithPolicy() before FinishedFork()";
 
+  // Apply the policy to the new process.
   if (handle != base::kNullProcessHandle) {
     const auto& existing_process = sandboxed_processes_.find(handle);
     CHECK(existing_process == sandboxed_processes_.end());
@@ -125,7 +118,10 @@ const BootstrapSandboxPolicy* BootstrapSandbox::PolicyForProcess(
 }
 
 BootstrapSandbox::BootstrapSandbox()
-    : real_bootstrap_port_(MACH_PORT_NULL),
+    : server_bootstrap_name_(
+          base::StringPrintf("%s.sandbox.%d", base::mac::BaseBundleID(),
+              getpid())),
+      real_bootstrap_port_(MACH_PORT_NULL),
       effective_policy_id_(kNotAPolicy) {
   mach_port_t port = MACH_PORT_NULL;
   kern_return_t kr = task_get_special_port(
diff --git a/sandbox/mac/bootstrap_sandbox.h b/sandbox/mac/bootstrap_sandbox.h
index 53fc54f..dff7814 100644
--- a/sandbox/mac/bootstrap_sandbox.h
+++ b/sandbox/mac/bootstrap_sandbox.h
@@ -26,9 +26,10 @@ class LaunchdInterceptionServer;
 // process creates an instance of this class and registers policies that it
 // can enforce on its children.
 //
-// With this sandbox, the bootstrap port of the parent process is replaced, so
-// that child processes is taken over by the sandbox. Bootstrap messages from
-// the parent are forwarded to launchd. Requests from the child that would
+// With this sandbox, the parent process must replace the bootstrap port prior
+// to the sandboxed target's execution. This should be done by setting the
+// base::LaunchOptions.replacement_bootstrap_name to the
+// server_bootstrap_name() of this class. Requests from the child that would
 // normally go to launchd are filtered based on the specified per-process
 // policies. If a request is permitted by the policy, it is forwarded on to
 // launchd for servicing. If it is not, then the sandbox will reply with a
@@ -77,6 +78,7 @@ class SANDBOX_EXPORT BootstrapSandbox {
   // with the |pid|, this returns NULL.
   const BootstrapSandboxPolicy* PolicyForProcess(pid_t pid) const;
 
+  std::string server_bootstrap_name() const { return server_bootstrap_name_; }
   mach_port_t real_bootstrap_port() const { return real_bootstrap_port_; }
 
  private:
@@ -86,6 +88,10 @@ class SANDBOX_EXPORT BootstrapSandbox {
   // requests.
   scoped_ptr<LaunchdInterceptionServer> server_;
 
+  // The name in the system bootstrap server by which the |server_|'s port
+  // is known.
+  const std::string server_bootstrap_name_;
+
   // The original bootstrap port of the process, which is connected to the
   // real launchd server.
   base::mac::ScopedMachSendRight real_bootstrap_port_;
diff --git a/sandbox/mac/bootstrap_sandbox_unittest.mm b/sandbox/mac/bootstrap_sandbox_unittest.mm
index f9068d6..85f627f 100644
--- a/sandbox/mac/bootstrap_sandbox_unittest.mm
+++ b/sandbox/mac/bootstrap_sandbox_unittest.mm
@@ -96,7 +96,9 @@ class BootstrapSandboxTest : public base::MultiProcessTest {
                           const char* child_name,
                           base::ProcessHandle* out_pid) {
     sandbox_->PrepareToForkWithPolicy(policy_id);
-    base::ProcessHandle pid = SpawnChild(child_name);
+    base::LaunchOptions options;
+    options.replacement_bootstrap_name = sandbox_->server_bootstrap_name();
+    base::ProcessHandle pid = SpawnChildWithOptions(child_name, options);
     ASSERT_GT(pid, 0);
     sandbox_->FinishedFork(pid);
     int code = 0;
diff --git a/sandbox/mac/launchd_interception_server.cc b/sandbox/mac/launchd_interception_server.cc
index 70fd33e..c3d6eaa 100644
--- a/sandbox/mac/launchd_interception_server.cc
+++ b/sandbox/mac/launchd_interception_server.cc
@@ -27,7 +27,7 @@ LaunchdInterceptionServer::LaunchdInterceptionServer(
 LaunchdInterceptionServer::~LaunchdInterceptionServer() {
 }
 
-bool LaunchdInterceptionServer::Initialize() {
+bool LaunchdInterceptionServer::Initialize(mach_port_t server_receive_right) {
   mach_port_t task = mach_task_self();
   kern_return_t kr;
 
@@ -46,7 +46,8 @@ bool LaunchdInterceptionServer::Initialize() {
   }
   sandbox_send_port_.reset(sandbox_port_);
 
-  message_server_.reset(new MachMessageServer(this, kBufferSize));
+  message_server_.reset(
+      new MachMessageServer(this, server_receive_right, kBufferSize));
   return message_server_->Initialize();
 }
 
@@ -59,9 +60,9 @@ void LaunchdInterceptionServer::DemuxMessage(mach_msg_header_t* request,
       sandbox_->PolicyForProcess(sender_pid);
   if (policy == NULL) {
     // No sandbox policy is in place for the sender of this message, which
-    // means it is from the sandbox host process or an unsandboxed child.
-    VLOG(3) << "Message from pid " << sender_pid << " forwarded to launchd";
-    ForwardMessage(request);
+    // means it came from the unknown. Reject it.
+    VLOG(3) << "Message from unknown pid " << sender_pid << " rejected.";
+    message_server_->RejectMessage(request, MIG_REMOTE_ERROR);
     return;
   }
 
diff --git a/sandbox/mac/launchd_interception_server.h b/sandbox/mac/launchd_interception_server.h
index 2e80fec..ec25be1 100644
--- a/sandbox/mac/launchd_interception_server.h
+++ b/sandbox/mac/launchd_interception_server.h
@@ -28,8 +28,10 @@ class LaunchdInterceptionServer : public MessageDemuxer {
   explicit LaunchdInterceptionServer(const BootstrapSandbox* sandbox);
   virtual ~LaunchdInterceptionServer();
 
-  // Initializes the class and starts running the message server.
-  bool Initialize();
+  // Initializes the class and starts running the message server. If the
+  // |server_receive_right| is non-NULL, this class will take ownership of
+  // the receive right and intercept messages sent to that port.
+  bool Initialize(mach_port_t server_receive_right);
 
   // MessageDemuxer:
   virtual void DemuxMessage(mach_msg_header_t* request,
diff --git a/sandbox/mac/mach_message_server.cc b/sandbox/mac/mach_message_server.cc
index 9a10121..555ee0f 100644
--- a/sandbox/mac/mach_message_server.cc
+++ b/sandbox/mac/mach_message_server.cc
@@ -17,9 +17,10 @@ namespace sandbox {
 
 MachMessageServer::MachMessageServer(
     MessageDemuxer* demuxer,
+    mach_port_t server_receive_right,
     mach_msg_size_t buffer_size)
     : demuxer_(demuxer),
-      server_port_(MACH_PORT_NULL),
+      server_port_(server_receive_right),
       server_queue_(NULL),
       server_source_(NULL),
       buffer_size_(
@@ -39,14 +40,17 @@ bool MachMessageServer::Initialize() {
   mach_port_t task = mach_task_self();
   kern_return_t kr;
 
-  // Allocate a port for use as a new server port.
-  mach_port_t port;
-  if ((kr = mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE, &port)) !=
-          KERN_SUCCESS) {
-    MACH_LOG(ERROR, kr) << "Failed to allocate new server port.";
-    return false;
+  // Allocate a port for use as a new server port if one was not passed to the
+  // constructor.
+  if (!server_port_.is_valid()) {
+    mach_port_t port;
+    if ((kr = mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE, &port)) !=
+            KERN_SUCCESS) {
+      MACH_LOG(ERROR, kr) << "Failed to allocate new server port.";
+      return false;
+    }
+    server_port_.reset(port);
   }
-  server_port_.reset(port);
 
   // Allocate the message request and reply buffers.
   const int kMachMsgMemoryFlags = VM_MAKE_TAG(VM_MEMORY_MACH_MSG) |
diff --git a/sandbox/mac/mach_message_server.h b/sandbox/mac/mach_message_server.h
index f37b4fc..5c05c39 100644
--- a/sandbox/mac/mach_message_server.h
+++ b/sandbox/mac/mach_message_server.h
@@ -33,7 +33,14 @@ class MessageDemuxer {
 // different port, or reply to the message with a MIG error.
 class MachMessageServer {
  public:
-  MachMessageServer(MessageDemuxer* demuxer, mach_msg_size_t buffer_size);
+  // Creates a new Mach message server that will send messages to |demuxer|
+  // for handling. If the |server_receive_right| is non-NULL, this class will
+  // take ownership of the port and it will be used to receive messages.
+  // Otherwise the server will create a new receive right.
+  // The maximum size of messages is specified by |buffer_size|.
+  MachMessageServer(MessageDemuxer* demuxer,
+                    mach_port_t server_receive_right,
+                    mach_msg_size_t buffer_size);
   ~MachMessageServer();
 
   // Initializes the class and starts running the message server. If this