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

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

base/process/launch.h

@@ -128,6 +128,15 @@ struct BASE_EXPORT LaunchOptions {
   int ctrl_terminal_fd;
 #endif  // defined(OS_CHROMEOS)
 
+#if defined(OS_MACOSX)
+  // If this name is non-empty, the new child, after fork() but before exec(),
+  // will look up this server name in the bootstrap namespace. The resulting
+  // service port will be replaced as the bootstrap port in the child. Because
+  // the process's IPC space is cleared on exec(), any rights to the old
+  // bootstrap port will not be transferred to the new process.
+  std::string replacement_bootstrap_name;
+#endif
+
 #endif  // !defined(OS_WIN)
 };
 
@@ -250,6 +259,11 @@ BASE_EXPORT void RaiseProcessToHighPriority();
 // in the child after forking will restore the standard exception handler.
 // See http://crbug.com/20371/ for more details.
 void RestoreDefaultExceptionHandler();
+
+// Look up the bootstrap server named |replacement_bootstrap_name| via the
+// current |bootstrap_port|. Then replace the task's bootstrap port with the
+// received right.
+void ReplaceBootstrapPort(const std::string& replacement_bootstrap_name);
 #endif  // defined(OS_MACOSX)
 
 // Creates a LaunchOptions object suitable for launching processes in a test

base/process/launch_mac.cc

@@ -5,6 +5,9 @@
 #include "base/process/launch.h"
 
 #include <mach/mach.h>
+#include <servers/bootstrap.h>
+
+#include "base/logging.h"
 
 namespace base {
 
@@ -25,4 +28,21 @@ void RestoreDefaultExceptionHandler() {
                            EXCEPTION_DEFAULT, THREAD_STATE_NONE);
 }
 
+void ReplaceBootstrapPort(const std::string& new_bootstrap_name) {
+  // This function is called between fork() and exec(), so it should take care
+  // to run properly in that situation.
+
+  mach_port_t port = MACH_PORT_NULL;
+  kern_return_t kr = bootstrap_look_up(bootstrap_port,
+      new_bootstrap_name.c_str(), &port);
+  if (kr != KERN_SUCCESS) {
+    RAW_LOG(FATAL, "Failed to look up replacement bootstrap port.");
+  }
+
+  kr = task_set_bootstrap_port(mach_task_self(), port);
+  if (kr != KERN_SUCCESS) {
+    RAW_LOG(FATAL, "Failed to replace bootstrap port.");
+  }
+}
+
 }  // namespace base

base/process/launch_posix.cc

@@ -385,6 +385,8 @@ bool LaunchProcess(const std::vector<std::string>& argv,
 
 #if defined(OS_MACOSX)
     RestoreDefaultExceptionHandler();
+    if (!options.replacement_bootstrap_name.empty())
+      ReplaceBootstrapPort(options.replacement_bootstrap_name);
 #endif  // defined(OS_MACOSX)
 
     ResetChildSignalHandlersToDefaults();

content/browser/bootstrap_sandbox_mac.cc

@@ -40,7 +40,6 @@ class BootstrapSandboxPolicy : public BrowserChildProcessObserver {
   virtual ~BootstrapSandboxPolicy();
 
   void RegisterSandboxPolicies();
-  void RegisterNPAPIPolicy();
 
   scoped_ptr<sandbox::BootstrapSandbox> sandbox_;
 };
@@ -71,15 +70,6 @@ BootstrapSandboxPolicy::~BootstrapSandboxPolicy() {
 }
 
 void BootstrapSandboxPolicy::RegisterSandboxPolicies() {
-  RegisterNPAPIPolicy();
-}
-
-void BootstrapSandboxPolicy::RegisterNPAPIPolicy() {
-  sandbox::BootstrapSandboxPolicy policy;
-  policy.default_rule = sandbox::Rule(sandbox::POLICY_ALLOW);
-  policy.rules[kBootstrapPortNameForNPAPIPlugins] =
-      sandbox::Rule(sandbox_->real_bootstrap_port());
-  sandbox_->RegisterSandboxPolicy(SANDBOX_TYPE_NPAPI, policy);
 }
 
 }  // namespace

content/browser/child_process_launcher.cc

@@ -288,6 +288,8 @@ class ChildProcessLauncher::Context
       const int bootstrap_sandbox_policy = delegate->GetSandboxType();
       if (ShouldEnableBootstrapSandbox() &&
           bootstrap_sandbox_policy != SANDBOX_TYPE_INVALID) {
+        options.replacement_bootstrap_name =
+            GetBootstrapSandbox()->server_bootstrap_name();
         GetBootstrapSandbox()->PrepareToForkWithPolicy(
             bootstrap_sandbox_policy);
       }

content/browser/plugin_process_host.cc

@@ -96,13 +96,6 @@ class PluginSandboxedProcessLauncherDelegate
   virtual int GetIpcFd() OVERRIDE {
     return ipc_fd_;
   }
-
-#if defined(OS_MACOSX)
-  virtual SandboxType GetSandboxType() OVERRIDE {
-    return SANDBOX_TYPE_NPAPI;
-  }
-#endif  // OS_MACOSX
-
 #endif  // OS_WIN
 
  private:

content/common/sandbox_mac.mm

@@ -57,7 +57,6 @@ SandboxTypeToResourceIDMapping kDefaultSandboxTypeToResourceIDMapping[] = {
   { SANDBOX_TYPE_UTILITY,  IDR_UTILITY_SANDBOX_PROFILE },
   { SANDBOX_TYPE_GPU,      IDR_GPU_SANDBOX_PROFILE },
   { SANDBOX_TYPE_PPAPI,    IDR_PPAPI_SANDBOX_PROFILE },
-  { SANDBOX_TYPE_NPAPI,    -1 },
 };
 
 COMPILE_ASSERT(arraysize(kDefaultSandboxTypeToResourceIDMapping) == \

content/common/sandbox_mac_unittest_helper.mm

@@ -56,10 +56,6 @@ bool MacSandboxTest::RunTestInAllSandboxTypes(const char* test_name,
   for(int i = static_cast<int>(SANDBOX_TYPE_FIRST_TYPE);
       i < SANDBOX_TYPE_AFTER_LAST_TYPE;
       ++i) {
-    // This sandbox type is not enforced using a Seatbelt policy.
-    if (i == SANDBOX_TYPE_NPAPI)
-      continue;
-
     if (!RunTestInSandbox(static_cast<SandboxType>(i),
             test_name, test_data)) {
       LOG(ERROR) << "Sandboxed test (" << test_name << ")" <<

content/plugin/plugin_main_mac.mm

@@ -3,14 +3,11 @@
 // found in the LICENSE file.
 
 #import <AppKit/AppKit.h>
-#include <servers/bootstrap.h>
 
 #include "base/environment.h"
-#include "base/mac/mach_logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_util.h"
 #include "content/common/plugin_carbon_interpose_constants_mac.h"
-#include "content/common/sandbox_init_mac.h"
 #include "content/plugin/plugin_interpose_util_mac.h"
 #include "content/public/common/content_client.h"
 
@@ -49,27 +46,6 @@ void TrimInterposeEnvironment() {
 #endif
 
 void InitializeChromeApplication() {
-  // The bootstrap sandbox has taken over the bootstrap port. However, NPAPI
-  // plugins request servers with the BOOTSTRAP_PER_PID_SERVICE flag. This
-  // will fail, since the browser will be forwarding the message on behalf of
-  // the plugin, and the browser has already created these per-pid services
-  // for itself.
-  //
-  // Instead, request the real bootstrap port from the sandbox server, which
-  // can then be used by the plugin.
-  mach_port_t new_bootstrap_port = MACH_PORT_NULL;
-  kern_return_t kr = bootstrap_look_up(bootstrap_port,
-      kBootstrapPortNameForNPAPIPlugins, &new_bootstrap_port);
-  BOOTSTRAP_LOG_IF(ERROR, kr != KERN_SUCCESS, kr)
-      << "Failed to look up original bootstrap port.";
-  if (kr == KERN_SUCCESS) {
-    bootstrap_port = new_bootstrap_port;
-    kr = task_set_bootstrap_port(mach_task_self(), new_bootstrap_port);
-    MACH_LOG_IF(ERROR, kr != KERN_SUCCESS, kr)
-        << "Failed to reset TASK_BOOTSTRAP_PORT.";
-  }
-
-
   [NSApplication sharedApplication];
   mac_plugin_interposing::SetUpCocoaInterposing();
 }

content/public/common/sandbox_type_mac.h

@@ -34,10 +34,6 @@ enum SandboxType {
   // The PPAPI plugin process.
   SANDBOX_TYPE_PPAPI,
 
-  // The NPAPI plugin process. This does not use a Seatbelt/.sb sandbox policy,
-  // but it uses a bootstrap sandbox.
-  SANDBOX_TYPE_NPAPI,
-
   SANDBOX_TYPE_AFTER_LAST_TYPE,  // Placeholder to ease iteration.
 };
 

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(

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_;

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;

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;
   }
 

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,

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) |

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