Fix non-signal-safe calls in BrokerClient

The current BrokerClient is not signal-safe because of its use of base::Pickle
which uses new and delete, and also its use of std::vector, which is not
technically signal safe either. These are used in messaging IPCs with the
BrokerHost when a syscall is trapped, thus BrokerClient must be made signal
safe. This is discussed in https://crbug.com/255063.

This CL creates a new BrokerSimpleMessage class to handle simple IPC messaging
with strings and ints only. It uses fixed sized messages to simplify logic and
allow it to allocate all its memory on the stack. It creates send, receive, and
a synchronous send-and-receive method. It also adds a basic set of unit tests
to sanity check behavior.

Bug: 255063
Change-Id: I8077a515921b62969a1b8b173d903f2a118ed186
Reviewed-on: https://chromium-review.googlesource.com/553400
Commit-Queue: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558415}

sandbox/linux/BUILD.gn

@@ -99,6 +99,7 @@ source_set("sandbox_linux_unittests_sources") {
     "services/yama_unittests.cc",
     "syscall_broker/broker_file_permission_unittest.cc",
     "syscall_broker/broker_process_unittest.cc",
+    "syscall_broker/broker_simple_message_unittest.cc",
     "tests/main.cc",
     "tests/scoped_temporary_file.cc",
     "tests/scoped_temporary_file.h",
@@ -371,6 +372,8 @@ component("sandbox_services") {
     "syscall_broker/broker_permission_list.h",
     "syscall_broker/broker_process.cc",
     "syscall_broker/broker_process.h",
+    "syscall_broker/broker_simple_message.cc",
+    "syscall_broker/broker_simple_message.h",
   ]
 
   defines = [ "SANDBOX_IMPLEMENTATION" ]
@@ -420,6 +423,8 @@ component("sandbox_services") {
       "syscall_broker/broker_permission_list.h",
       "syscall_broker/broker_process.cc",
       "syscall_broker/broker_process.h",
+      "syscall_broker/broker_simple_message.cc",
+      "syscall_broker/broker_simple_message.h",
     ]
   } else if (!is_android) {
     sources += [

sandbox/linux/syscall_broker/broker_channel.h

@@ -7,6 +7,7 @@
 
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
+#include "sandbox/sandbox_export.h"
 
 namespace sandbox {
 
@@ -15,7 +16,7 @@ namespace syscall_broker {
 // A small class to create a pipe-like communication channel. It is based on a
 // SOCK_SEQPACKET unix socket, which is connection-based and guaranteed to
 // preserve message boundaries.
-class BrokerChannel {
+class SANDBOX_EXPORT BrokerChannel {
  public:
   typedef base::ScopedFD EndPoint;
   static void CreatePair(EndPoint* reader, EndPoint* writer);

sandbox/linux/syscall_broker/broker_client.h

@@ -10,7 +10,6 @@
 #include <unistd.h>
 
 #include "base/macros.h"
-#include "base/pickle.h"
 #include "sandbox/linux/syscall_broker/broker_channel.h"
 #include "sandbox/linux/syscall_broker/broker_command.h"
 
@@ -33,12 +32,10 @@ class BrokerClient {
   // and save an IPC round trip.
   // |ipc_channel| needs to be a suitable SOCK_SEQPACKET unix socket.
   // |fast_check_in_client| should be set to true and
-  // |quiet_failures_for_tests| to false unless you are writing tests.
   BrokerClient(const BrokerPermissionList& policy,
                BrokerChannel::EndPoint ipc_channel,
                const BrokerCommandSet& allowed_command_set,
-               bool fast_check_in_client,
-               bool quiet_failures_for_tests);
+               bool fast_check_in_client);
   ~BrokerClient();
 
   // Get the file descriptor used for IPC. This is used for tests.
@@ -93,20 +90,12 @@ class BrokerClient {
                         void* result_ptr,
                         size_t expected_result_size) const;
 
-  ssize_t SendRecvRequest(const base::Pickle& request_pickle,
-                          int recvmsg_flags,
-                          uint8_t* reply_buf,
-                          size_t reply_buf_size,
-                          int* returned_fd) const;
-
   const BrokerPermissionList& broker_permission_list_;
   const BrokerChannel::EndPoint ipc_channel_;
   const BrokerCommandSet allowed_command_set_;
   const bool fast_check_in_client_;  // Whether to forward a request that we
                                      // know will be denied to the broker. (Used
                                      // for tests).
-  const bool quiet_failures_for_tests_;  // Disable certain error message when
-                                         // testing for failures.
 
   DISALLOW_COPY_AND_ASSIGN(BrokerClient);
 };

sandbox/linux/syscall_broker/broker_command.h

@@ -17,8 +17,6 @@ namespace syscall_broker {
 
 class BrokerPermissionList;
 
-constexpr size_t kMaxMessageLength = 4096;
-
 // Some flags are local to the current process and cannot be sent over a Unix
 // socket. They need special treatment from the client.
 // O_CLOEXEC is tricky because in theory another thread could call execve()

sandbox/linux/syscall_broker/broker_process.cc

@@ -78,7 +78,7 @@ bool BrokerProcess::Init(
     broker_pid_ = child_pid;
     broker_client_ = std::make_unique<BrokerClient>(
         broker_permission_list_, std::move(ipc_writer), allowed_command_set_,
-        fast_check_in_client_, quiet_failures_for_tests_);
+        fast_check_in_client_);
     initialized_ = true;
     return true;
   }

sandbox/linux/syscall_broker/broker_simple_message.cc

+// Copyright 2018 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/linux/syscall_broker/broker_simple_message.h"
+
+#include <errno.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include "base/files/scoped_file.h"
+#include "base/logging.h"
+#include "base/numerics/safe_math.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/posix/unix_domain_socket.h"
+#include "base/process/process_handle.h"
+#include "build/build_config.h"
+
+namespace sandbox {
+
+namespace syscall_broker {
+
+BrokerSimpleMessage::BrokerSimpleMessage()
+    : read_only_(false),
+      write_only_(false),
+      broken_(false),
+      length_(0),
+      read_next_(message_),
+      write_next_(message_) {}
+
+ssize_t BrokerSimpleMessage::SendRecvMsgWithFlags(int fd,
+                                                  int recvmsg_flags,
+                                                  int* result_fd,
+                                                  BrokerSimpleMessage* reply) {
+  RAW_CHECK(reply);
+
+  // This socketpair is only used for the IPC and is cleaned up before
+  // returning.
+  base::ScopedFD recv_sock;
+  base::ScopedFD send_sock;
+  if (!base::CreateSocketPair(&recv_sock, &send_sock))
+    return -1;
+
+  if (!SendMsg(fd, send_sock.get()))
+    return -1;
+
+  // Close the sending end of the socket right away so that if our peer closes
+  // it before sending a response (e.g., from exiting), RecvMsgWithFlags() will
+  // return EOF instead of hanging.
+  send_sock.reset();
+
+  base::ScopedFD recv_fd;
+  const ssize_t reply_len =
+      reply->RecvMsgWithFlags(recv_sock.get(), recvmsg_flags, &recv_fd);
+  recv_sock.reset();
+  if (reply_len == -1)
+    return -1;
+
+  if (result_fd)
+    *result_fd = (recv_fd == -1) ? -1 : recv_fd.release();
+
+  return reply_len;
+}
+
+bool BrokerSimpleMessage::SendMsg(int fd, int send_fd) {
+  if (broken_)
+    return false;
+
+  struct msghdr msg = {};
+  const void* buf = reinterpret_cast<const void*>(message_);
+  struct iovec iov = {const_cast<void*>(buf), length_};
+  msg.msg_iov = &iov;
+  msg.msg_iovlen = 1;
+
+  const unsigned control_len = CMSG_SPACE(sizeof(int));
+  char control_buffer[control_len];
+  if (send_fd >= 0) {
+    struct cmsghdr* cmsg;
+    msg.msg_control = control_buffer;
+    msg.msg_controllen = control_len;
+    cmsg = CMSG_FIRSTHDR(&msg);
+    cmsg->cmsg_level = SOL_SOCKET;
+    cmsg->cmsg_type = SCM_RIGHTS;
+    cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+    memcpy(CMSG_DATA(cmsg), &send_fd, sizeof(int));
+    msg.msg_controllen = cmsg->cmsg_len;
+  }
+
+  // Avoid a SIGPIPE if the other end breaks the connection.
+  // Due to a bug in the Linux kernel (net/unix/af_unix.c) MSG_NOSIGNAL isn't
+  // regarded for SOCK_SEQPACKET in the AF_UNIX domain, but it is mandated by
+  // POSIX.
+  const int flags = MSG_NOSIGNAL;
+  const ssize_t r = HANDLE_EINTR(sendmsg(fd, &msg, flags));
+  return static_cast<ssize_t>(length_) == r;
+}
+
+ssize_t BrokerSimpleMessage::RecvMsgWithFlags(int fd,
+                                              int flags,
+                                              base::ScopedFD* return_fd) {
+  // The message must be fresh and unused.
+  RAW_CHECK(!read_only_ && !write_only_);
+  read_only_ = true;  // The message should not be written to again.
+  struct msghdr msg = {};
+  struct iovec iov = {message_, kMaxMessageLength};
+  msg.msg_iov = &iov;
+  msg.msg_iovlen = 1;
+
+#if defined(OS_NACL_NONSFI)
+  const size_t kControlBufferSize =
+      CMSG_SPACE(sizeof(int) * base::UnixDomainSocket::kMaxFileDescriptors);
+#else
+  const size_t kControlBufferSize =
+      CMSG_SPACE(sizeof(int) * base::UnixDomainSocket::kMaxFileDescriptors) +
+      // The PNaCl toolchain for Non-SFI binary build does not support ucred.
+      CMSG_SPACE(sizeof(struct ucred));
+#endif  // defined(OS_NACL_NONSFI)
+
+  char control_buffer[kControlBufferSize];
+  msg.msg_control = control_buffer;
+  msg.msg_controllen = sizeof(control_buffer);
+
+  const ssize_t r = HANDLE_EINTR(recvmsg(fd, &msg, flags));
+  if (r == -1)
+    return -1;
+
+  int* wire_fds = NULL;
+  size_t wire_fds_len = 0;
+  base::ProcessId pid = -1;
+
+  if (msg.msg_controllen > 0) {
+    struct cmsghdr* cmsg;
+    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+      const size_t payload_len = cmsg->cmsg_len - CMSG_LEN(0);
+      if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) {
+        DCHECK_EQ(payload_len % sizeof(int), 0u);
+        DCHECK_EQ(wire_fds, static_cast<void*>(nullptr));
+        wire_fds = reinterpret_cast<int*>(CMSG_DATA(cmsg));
+        wire_fds_len = payload_len / sizeof(int);
+      }
+#if !defined(OS_NACL_NONSFI)
+      // The PNaCl toolchain for Non-SFI binary build does not support
+      // SCM_CREDENTIALS.
+      if (cmsg->cmsg_level == SOL_SOCKET &&
+          cmsg->cmsg_type == SCM_CREDENTIALS) {
+        DCHECK_EQ(payload_len, sizeof(struct ucred));
+        DCHECK_EQ(pid, -1);
+        pid = reinterpret_cast<struct ucred*>(CMSG_DATA(cmsg))->pid;
+      }
+#endif
+    }
+  }
+
+  if (msg.msg_flags & MSG_TRUNC || msg.msg_flags & MSG_CTRUNC) {
+    for (size_t i = 0; i < wire_fds_len; ++i) {
+      close(wire_fds[i]);
+    }
+    errno = EMSGSIZE;
+    return -1;
+  }
+
+  if (wire_fds) {
+    if (wire_fds_len > 1) {
+      // Only one FD is accepted by this receive.
+      for (unsigned i = 0; i < wire_fds_len; ++i) {
+        close(wire_fds[i]);
+      }
+      errno = EMSGSIZE;
+      NOTREACHED();
+      return -1;
+    }
+
+    *return_fd = base::ScopedFD(wire_fds[0]);
+  }
+
+  // At this point, |r| is guaranteed to be >= 0.
+  length_ = static_cast<size_t>(r);
+  return r;
+}
+
+bool BrokerSimpleMessage::AddStringToMessage(const char* string) {
+  // strlen() + 1 to always include the '\0' terminating character.
+  return AddDataToMessage(string, strlen(string) + 1);
+}
+
+bool BrokerSimpleMessage::AddDataToMessage(const char* data, size_t length) {
+  if (read_only_ || broken_)
+    return false;
+
+  write_only_ = true;  // Message should only be written to going forward.
+
+  base::CheckedNumeric<size_t> safe_length(length);
+  safe_length += length_;
+  safe_length += sizeof(EntryType);
+  safe_length += sizeof(length);
+
+  if (safe_length.ValueOrDie() > kMaxMessageLength) {
+    broken_ = true;
+    return false;
+  }
+
+  EntryType type = EntryType::DATA;
+
+  // Write the type to the message
+  memcpy(write_next_, &type, sizeof(EntryType));
+  write_next_ += sizeof(EntryType);
+  // Write the length of the buffer to the message
+  memcpy(write_next_, &length, sizeof(length));
+  write_next_ += sizeof(length);
+  // Write the data in the buffer to the message
+  memcpy(write_next_, data, length);
+  write_next_ += length;
+  length_ = write_next_ - message_;
+
+  return true;
+}
+
+bool BrokerSimpleMessage::AddIntToMessage(int data) {
+  if (read_only_ || broken_)
+    return false;
+
+  write_only_ = true;  // Message should only be written to going forward.
+
+  base::CheckedNumeric<size_t> safe_length(length_);
+  safe_length += sizeof(int);
+  safe_length += sizeof(EntryType);
+
+  if (!safe_length.IsValid() || safe_length.ValueOrDie() > kMaxMessageLength) {
+    broken_ = true;
+    return false;
+  }
+
+  EntryType type = EntryType::INT;
+
+  memcpy(write_next_, &type, sizeof(EntryType));
+  write_next_ += sizeof(EntryType);
+  memcpy(write_next_, &data, sizeof(int));
+  write_next_ += sizeof(int);
+  length_ = write_next_ - message_;
+
+  return true;
+}
+
+bool BrokerSimpleMessage::ReadString(const char** data) {
+  size_t str_len;
+  bool result = ReadData(data, &str_len);
+  return result && (*data)[str_len - 1] == '\0';
+}
+
+bool BrokerSimpleMessage::ReadData(const char** data, size_t* length) {
+  if (write_only_ || broken_)
+    return false;
+
+  read_only_ = true;  // Message should not be written to.
+  if (read_next_ > (message_ + length_)) {
+    broken_ = true;
+    return false;
+  }
+
+  if (!ValidateType(EntryType::DATA)) {
+    broken_ = true;
+    return false;
+  }
+
+  // Get the length of the data buffer from the message.
+  if ((read_next_ + sizeof(size_t)) > (message_ + length_)) {
+    broken_ = true;
+    return false;
+  }
+  memcpy(length, read_next_, sizeof(size_t));
+  read_next_ = read_next_ + sizeof(size_t);
+
+  // Get the raw data buffer from the message.
+  if ((read_next_ + *length) > (message_ + length_)) {
+    broken_ = true;
+    return false;
+  }
+  *data = reinterpret_cast<char*>(read_next_);
+  read_next_ = read_next_ + *length;
+  return true;
+}
+
+bool BrokerSimpleMessage::ReadInt(int* result) {
+  if (write_only_ || broken_)
+    return false;
+
+  read_only_ = true;  // Message should not be written to.
+  if (read_next_ > (message_ + length_)) {
+    broken_ = true;
+    return false;
+  }
+
+  if (!ValidateType(EntryType::INT)) {
+    broken_ = true;
+    return false;
+  }
+
+  if ((read_next_ + sizeof(int)) > (message_ + length_)) {
+    broken_ = true;
+    return false;
+  }
+  memcpy(result, read_next_, sizeof(int));
+  read_next_ = read_next_ + sizeof(int);
+  return true;
+}
+
+bool BrokerSimpleMessage::ValidateType(EntryType expected_type) {
+  if ((read_next_ + sizeof(EntryType)) > (message_ + length_))
+    return false;
+
+  EntryType type;
+  memcpy(&type, read_next_, sizeof(EntryType));
+  if (type != expected_type)
+    return false;
+
+  read_next_ = read_next_ + sizeof(EntryType);
+  return true;
+}
+
+}  // namespace syscall_broker
+
+}  // namespace sandbox

sandbox/linux/syscall_broker/broker_simple_message.h

+// Copyright 2018 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.
+
+#ifndef SANDBOX_LINUX_SYSCALL_BROKER_BROKER_SIMPLE_MESSAGE_H_
+#define SANDBOX_LINUX_SYSCALL_BROKER_BROKER_SIMPLE_MESSAGE_H_
+
+#include <stdint.h>
+#include <sys/types.h>
+
+#include "base/files/scoped_file.h"
+#include "sandbox/sandbox_export.h"
+
+namespace sandbox {
+namespace syscall_broker {
+
+// This class is meant to provide a very simple messaging mechanism that is
+// signal-safe for the broker to utilize. This addresses many of the issues
+// outlined in https://crbug.com/255063. In short, the use of the standard
+// base::UnixDomainSockets is not possible because it uses base::Pickle and
+// std::vector, which are not signal-safe.
+//
+// In implementation, much of the code for sending and receiving is taken from
+// base::UnixDomainSockets and re-used below. Thus, ultimately, it might be
+// worthwhile making a first-class base-supported signal-safe set of mechanisms
+// that reduces the code duplication.
+class SANDBOX_EXPORT BrokerSimpleMessage {
+ public:
+  BrokerSimpleMessage();
+
+  // Signal-safe
+  ssize_t SendRecvMsgWithFlags(int fd,
+                               int recvmsg_flags,
+                               int* send_fd,
+                               BrokerSimpleMessage* reply);
+
+  // Use sendmsg to write the given msg and the file descriptor |send_fd|.
+  // Returns true if successful. Signal-safe.
+  bool SendMsg(int fd, int send_fd);
+
+  // Similar to RecvMsg, but allows to specify |flags| for recvmsg(2).
+  // Guaranteed to return either 1 or 0 fds. Signal-safe.
+  ssize_t RecvMsgWithFlags(int fd, int flags, base::ScopedFD* return_fd);
+
+  // Adds a NUL-terminated C-style string to the message as a raw buffer.
+  // Returns true if the internal message buffer has room for the data, and the
+  // data is successfully appended.
+  bool AddStringToMessage(const char* string);
+
+  // Adds a raw data buffer to the message. If the raw data is actually a
+  // string, be sure to have length explicitly include the '\0' terminating
+  // character. Returns true if the internal message buffer has room for the
+  // data, and the data is successfully appended.
+  bool AddDataToMessage(const char* buffer, size_t length);
+
+  // Adds an int to the message. Returns true if the internal message buffer has
+  // room for the int and the int is successfully added.
+  bool AddIntToMessage(int int_to_add);
+
+  // This returns a pointer to the next available data buffer in |data|. The
+  // pointer is owned by |this| class. The resulting buffer is a string and
+  // terminated with a '\0' character.
+  bool ReadString(const char** string);
+
+  // This returns a pointer to the next available data buffer in the message
+  // in |data|, and the length of the buffer in |length|. The buffer is owned by
+  // |this| class.
+  bool ReadData(const char** data, size_t* length);
+
+  // This reads the next available int from the message and stores it in
+  // |result|.
+  bool ReadInt(int* result);
+
+  // The maximum length of a message in the fixed size buffer.
+  static constexpr size_t kMaxMessageLength = 4096;
+
+ private:
+  friend class BrokerSimpleMessageTestHelper;
+
+  enum class EntryType { DATA = 0xBDBDBD80, INT = 0xBDBDBD81 };
+
+  // Returns whether or not the next available entry matches the expected entry
+  // type.
+  bool ValidateType(EntryType expected_type);
+
+  // Set to true once a message is read from, it may never be written to.
+  bool read_only_;
+  // Set to true once a message is written to, it may never be read from.
+  bool write_only_;
+  // Set when an operation fails, so that all subsequed operations fail,
+  // including any attempt to send the broken message.
+  bool broken_;
+  // The current length of the contents in the |message_| buffer.
+  size_t length_;
+  // The pointer to the next location in the |message_| buffer to read from.
+  uint8_t* read_next_;
+  // The pointer to the next location in the |message_| buffer to write from.
+  uint8_t* write_next_;
+  // The statically allocated buffer of size |kMaxMessageLength|.
+  uint8_t message_[kMaxMessageLength];
+};
+
+}  // namespace syscall_broker
+}  // namespace sandbox
+
+#endif  // SANDBOX_LINUX_SYSCALL_BROKER_BROKER_SIMPLE_MESSAGE_H_