Do not double-unref send rights when using POLICY_SUBSTITUE_PORT.

By destroying the reply message, the already-copied-out right will be unrefed
again, leading to an over-release of send rights.

This also requires that Rule(POLICY_SUBSTITUTE_PORT) users provide a send right
that can be duplicated with MACH_MSG_TYPE_COPY_SEND, rather than using MAKE_SEND.

BUG=367863
R=mark@chromium.org

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

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

sandbox/mac/bootstrap_sandbox_unittest.mm

@@ -225,11 +225,27 @@ struct SubstitutePortAckRecv : public SubstitutePortAckSend {
 const char kSubstituteAck[] = "Hello, this is doge!";
 
 TEST_F(BootstrapSandboxTest, PolicySubstitutePort) {
+  mach_port_t task = mach_task_self();
+
   mach_port_t port;
-  ASSERT_EQ(KERN_SUCCESS, mach_port_allocate(mach_task_self(),
-      MACH_PORT_RIGHT_RECEIVE, &port));
+  ASSERT_EQ(KERN_SUCCESS, mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE,
+      &port));
   base::mac::ScopedMachReceiveRight scoped_port(port);
 
+  mach_port_urefs_t send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(0u, send_rights);
+
+  ASSERT_EQ(KERN_SUCCESS, mach_port_insert_right(task, port, port,
+      MACH_MSG_TYPE_MAKE_SEND));
+  base::mac::ScopedMachSendRight scoped_port_send(port);
+
+  send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(1u, send_rights);
+
   BootstrapSandboxPolicy policy(BaselinePolicy());
   policy[kTestServer] = Rule(port);
   sandbox_->RegisterSandboxPolicy(1, policy);
@@ -245,6 +261,11 @@ TEST_F(BootstrapSandboxTest, PolicySubstitutePort) {
       TestTimeouts::tiny_timeout().InMilliseconds(), MACH_PORT_NULL);
   EXPECT_EQ(KERN_SUCCESS, kr);
 
+  send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(1u, send_rights);
+
   EXPECT_EQ(0, strncmp(kSubstituteAck, msg.buf, sizeof(msg.buf)));
 }
 

sandbox/mac/launchd_interception_server.cc

@@ -76,6 +76,12 @@ bool LaunchdInterceptionServer::Initialize() {
     return false;
   }
   sandbox_port_.reset(port);
+  if ((kr = mach_port_insert_right(task, sandbox_port_, sandbox_port_,
+          MACH_MSG_TYPE_MAKE_SEND) != KERN_SUCCESS)) {
+    MACH_LOG(ERROR, kr) << "Failed to allocate dummy sandbox port send right.";
+    return false;
+  }
+  sandbox_send_port_.reset(sandbox_port_);
 
   // Set up the dispatch queue to service the bootstrap port.
   // TODO(rsesek): Specify DISPATCH_QUEUE_SERIAL, in the 10.7 SDK. NULL means
@@ -216,16 +222,13 @@ void LaunchdInterceptionServer::HandleLookUp(mach_msg_header_t* request,
     else
       result_port = rule.substitute_port;
 
-    // Grant an additional send right on the result_port so that it can be
-    // sent to the sandboxed child process.
-    kern_return_t kr = mach_port_insert_right(mach_task_self(),
-        result_port, result_port, MACH_MSG_TYPE_MAKE_SEND);
-    if (kr != KERN_SUCCESS) {
-      MACH_LOG(ERROR, kr) << "Unable to insert right on result_port.";
-    }
-
     compat_shim_.look_up2_fill_reply(reply, result_port);
-    SendReply(reply);
+    // If the message was sent successfully, clear the result_port out of the
+    // message so that it is not destroyed at the end of ReceiveMessage. The
+    // above-inserted right has been moved out of the process, and destroying
+    // the message will unref yet another right.
+    if (SendReply(reply))
+      compat_shim_.look_up2_fill_reply(reply, MACH_PORT_NULL);
   } else {
     NOTREACHED();
   }
@@ -246,12 +249,12 @@ void LaunchdInterceptionServer::HandleSwapInteger(mach_msg_header_t* request,
   }
 }
 
-void LaunchdInterceptionServer::SendReply(mach_msg_header_t* reply) {
+bool LaunchdInterceptionServer::SendReply(mach_msg_header_t* reply) {
   kern_return_t kr = mach_msg(reply, MACH_SEND_MSG, reply->msgh_size, 0,
       MACH_PORT_NULL, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
-  if (kr != KERN_SUCCESS) {
-    MACH_LOG(ERROR, kr) << "Unable to send intercepted reply message.";
-  }
+  MACH_LOG_IF(ERROR, kr != KERN_SUCCESS, kr)
+      << "Unable to send intercepted reply message.";
+  return kr == KERN_SUCCESS;
 }
 
 void LaunchdInterceptionServer::ForwardMessage(mach_msg_header_t* request,

sandbox/mac/launchd_interception_server.h

@@ -54,8 +54,8 @@ class LaunchdInterceptionServer {
                          mach_msg_header_t* reply,
                          pid_t sender_pid);
 
-  // Sends a reply message.
-  void SendReply(mach_msg_header_t* reply);
+  // Sends a reply message. Returns true if the message was sent successfully.
+  bool SendReply(mach_msg_header_t* reply);
 
   // Forwards the original |request| on to real bootstrap server for handling.
   void ForwardMessage(mach_msg_header_t* request, mach_msg_header_t* reply);
@@ -88,6 +88,9 @@ class LaunchdInterceptionServer {
   // The Mach port handed out in reply to denied look up requests. All denied
   // requests share the same port, though nothing reads messages from it.
   base::mac::ScopedMachReceiveRight sandbox_port_;
+  // The send right for the above |sandbox_port_|, used with
+  // MACH_MSG_TYPE_COPY_SEND when handing out references to the dummy port.
+  base::mac::ScopedMachSendRight sandbox_send_port_;
 
   // The compatibility shim that handles differences in message header IDs and
   // request/reply structures between different OS X versions.

sandbox/mac/os_compatibility.cc

@@ -87,7 +87,7 @@ void LaunchdLookUp2FillReply(mach_msg_header_t* header, mach_port_t port) {
       MACH_MSGH_BITS_COMPLEX;
   reply->msgh_body.msgh_descriptor_count = 1;
   reply->service_port.name = port;
-  reply->service_port.disposition = MACH_MSG_TYPE_PORT_SEND;
+  reply->service_port.disposition = MACH_MSG_TYPE_COPY_SEND;
   reply->service_port.type = MACH_MSG_PORT_DESCRIPTOR;
 }
 

sandbox/mac/policy.h

@@ -42,7 +42,8 @@ struct SANDBOX_EXPORT Rule {
   PolicyDecision result;
 
   // The Rule does not take ownership of this port, but additional send rights
-  // will be allocated to it before it is sent to a client.
+  // will be allocated to it before it is sent to a client. This name must
+  // denote a send right that can duplicated with MACH_MSG_TYPE_COPY_SEND.
   mach_port_t substitute_port;
 };