Fix potential Mach port right leak in base::MachPortBroker

An errant or malicious process could send Mach messages with extra
descriptors. While the message wouldn't be processed, it would cause the
rights to leak in the browser.

When handling a request, add additional checks for the msgh_id and
msgh_size. If the message does not pass muster, or it is from a process
that is not expected, ensure any rights contained within are destroyed.

Change-Id: If333372c72cb20d99713c78d5451c04a3321113f
Reviewed-on: https://chromium-review.googlesource.com/c/1305394
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603639}

base/mac/mach_port_broker.h

@@ -78,11 +78,13 @@ class BASE_EXPORT MachPortBroker : public base::PortProvider {
   // incoming message needs to be received.
   void HandleRequest();
 
-  // Updates the mapping for |pid| to include the given |mach_info|.  Does
+  // Updates the mapping for |pid| to include the given |mach_info|. Does
   // nothing if PlaceholderForPid() has not already been called for the given
   // |pid|. Callers MUST acquire the lock given by GetLock() before calling
-  // this method (and release the lock afterwards).
-  void FinalizePid(base::ProcessHandle pid, mach_port_t task_port);
+  // this method (and release the lock afterwards). Returns true if the port
+  // was accepeted for the PID, or false if it was rejected (e.g. due to an
+  // unknown sender).
+  bool FinalizePid(base::ProcessHandle pid, mach_port_t task_port);
 
   // Name used to identify a particular port broker.
   const std::string name_;

base/mac/mach_port_broker.mm

@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mach_logging.h"
+#include "base/mac/scoped_mach_msg_destroy.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 
@@ -17,6 +18,8 @@ namespace base {
 
 namespace {
 
+constexpr mach_msg_id_t kTaskPortMessageId = 'tskp';
+
 // Mach message structure used in the child as a sending message.
 struct MachPortBroker_ChildSendMsg {
   mach_msg_header_t header;
@@ -47,12 +50,12 @@ bool MachPortBroker::ChildSendTaskPortToParent(const std::string& name) {
 
   // Create the check in message. This will copy a send right on this process'
   // (the child's) task port and send it to the parent.
-  MachPortBroker_ChildSendMsg msg;
-  bzero(&msg, sizeof(msg));
+  MachPortBroker_ChildSendMsg msg{};
   msg.header.msgh_bits = MACH_MSGH_BITS_REMOTE(MACH_MSG_TYPE_COPY_SEND) |
                          MACH_MSGH_BITS_COMPLEX;
-  msg.header.msgh_remote_port = parent_port;
   msg.header.msgh_size = sizeof(msg);
+  msg.header.msgh_remote_port = parent_port;
+  msg.header.msgh_id = kTaskPortMessageId;
   msg.body.msgh_descriptor_count = 1;
   msg.child_task_port.name = mach_task_self();
   msg.child_task_port.disposition = MACH_MSG_TYPE_PORT_SEND;
@@ -130,8 +133,7 @@ void MachPortBroker::InvalidatePid(base::ProcessHandle pid) {
 }
 
 void MachPortBroker::HandleRequest() {
-  MachPortBroker_ParentRecvMsg msg;
-  bzero(&msg, sizeof(msg));
+  MachPortBroker_ParentRecvMsg msg{};
   msg.header.msgh_size = sizeof(msg);
   msg.header.msgh_local_port = server_port_.get();
 
@@ -151,6 +153,19 @@ void MachPortBroker::HandleRequest() {
     return;
   }
 
+  // Destroy any rights that this class does not take ownership of.
+  ScopedMachMsgDestroy scoped_msg(&msg.header);
+
+  // Validate that the received message is what is expected.
+  if ((msg.header.msgh_bits & MACH_MSGH_BITS_COMPLEX) == 0 ||
+      msg.header.msgh_id != kTaskPortMessageId ||
+      msg.header.msgh_size != sizeof(MachPortBroker_ChildSendMsg) ||
+      msg.child_task_port.disposition != MACH_MSG_TYPE_PORT_SEND ||
+      msg.child_task_port.type != MACH_MSG_PORT_DESCRIPTOR) {
+    LOG(ERROR) << "Received unexpected message";
+    return;
+  }
+
   // Use the kernel audit information to make sure this message is from
   // a task that this process spawned. The kernel audit token contains the
   // unspoofable pid of the task that sent the message.
@@ -160,12 +175,14 @@ void MachPortBroker::HandleRequest() {
   // Take the lock and update the broker information.
   {
     base::AutoLock lock(lock_);
-    FinalizePid(child_pid, child_task_port);
+    if (FinalizePid(child_pid, child_task_port)) {
+      scoped_msg.Disarm();
+    }
   }
   NotifyObservers(child_pid);
 }
 
-void MachPortBroker::FinalizePid(base::ProcessHandle pid,
+bool MachPortBroker::FinalizePid(base::ProcessHandle pid,
                                  mach_port_t task_port) {
   lock_.AssertAcquired();
 
@@ -173,12 +190,16 @@ void MachPortBroker::FinalizePid(base::ProcessHandle pid,
   if (it == mach_map_.end()) {
     // Do nothing for unknown pids.
     LOG(ERROR) << "Unknown process " << pid << " is sending Mach IPC messages!";
-    return;
+    return false;
+  }
+
+  if (it->second != MACH_PORT_NULL) {
+    NOTREACHED();
+    return false;
   }
 
-  DCHECK(it->second == MACH_PORT_NULL);
-  if (it->second == MACH_PORT_NULL)
-    it->second = task_port;
+  it->second = task_port;
+  return true;
 }
 
 }  // namespace base