Linux sandbox: add a new low-level broker process mechanism.

sandbox/linux/services/broker_process.cc

+#include "sandbox/linux/services/broker_process.h"
+
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/pickle.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/posix/unix_domain_socket.h"
+
+namespace {
+
+static const int kCommandOpen = 'O';
+static const size_t kMaxMessageLength = 4096;
+
+// Check whether |requested_filename| is in |allowed_file_names|.
+// For paranoia, if file_to_open is not NULL, we will return the matching
+// string from the white list.
+// Even if an attacker managed to fool the string comparison mechanism, we
+// would not open an attacker-controlled file name.
+bool GetFileNameInWhitelist(const std::vector<std::string>& allowed_file_names,
+                            const std::string& requested_filename,
+                            std::string* file_to_open) {

[Markus (顧孟勤), 2012/12/13 09:48:52]

Do you want to set "file_to_open" to NULL before you do anything else?

[jln (very slow on Chromium), 2012/12/13 19:27:17]

Good point. I'll test that it's empty.

+  std::vector<std::string>::const_iterator it;
+  it = std::find(allowed_file_names.begin(), allowed_file_names.end(),
+                 requested_filename);
+  if (it < allowed_file_names.end()) {  // requested_filename was found?
+    if (file_to_open)
+      *file_to_open = *it;
+    return true;
+  }
+  return false;
+}
+
+bool HandleOpenRequest(int fd,
+                       const std::vector<std::string>& allowed_file_names,
+                       const Pickle& read_pickle, PickleIterator iter) {
+  std::string requested_filename;
+  int flags = 0;
+  if (!read_pickle.ReadString(&iter, &requested_filename) ||
+      !read_pickle.ReadInt(&iter, &flags)) {
+    return -1;
+  }
+
+  Pickle write_pickle;
+  std::vector<int> opened_files;
+  std::string file_to_open;
+  bool file_is_in_whitelist = GetFileNameInWhitelist(allowed_file_names,
+                                                     requested_filename,
+                                                     &file_to_open);
+  if (file_is_in_whitelist &&  // requested_filename is in the whitelist?
+      flags == O_RDONLY) {
+    int opened_fd = open(file_to_open.c_str(), O_RDONLY);
+    if (opened_fd < 0) {
+      write_pickle.WriteInt(-errno);
+    } else {
+      // Success.
+      opened_files.push_back(opened_fd);
+      write_pickle.WriteInt(0);
+    }
+  } else {
+    write_pickle.WriteInt(-EPERM);
+  }
+
+  CHECK_LE(write_pickle.size(), kMaxMessageLength);
+  ssize_t sent = UnixDomainSocket::SendMsg(fd, write_pickle.data(),
+                                           write_pickle.size(), opened_files);
+
+  // Close anything we have opened in this process.
+  for (std::vector<int>::iterator it = opened_files.begin();
+       it < opened_files.end(); ++it) {
+    (void) HANDLE_EINTR(close(*it));
+  }
+
+  if (sent <= 0) {
+    LOG(ERROR) << "Could not send IPC reply";
+    return false;
+  }
+  return true;
+}
+
+// Handle a request on the IPC channel FD.
+// A request should have a file descriptor attached on which we will reply and
+// that we will then close.
+// A request should start with an int that will be used as the command type.
+bool HandleRequest(int fd,
+                   const std::vector<std::string>& allowed_file_names) {
+
+  std::vector<int> fds;
+  char buf[kMaxMessageLength];
+  errno = 0;
+  const ssize_t msg_len = UnixDomainSocket::RecvMsg(fd, buf,
+                                                    sizeof(buf), &fds);

[Markus (顧孟勤), 2012/12/13 09:48:52]

What does RecvMsg() do for truncated messages? They can certainly confuse things
quite a bit and we probably will completely fail to parse subsequent data
correctly, when that happens.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

It does check msg_flags for MSG_TRUNC and MSG_CTRUNK and returns -1 in that
case.

+
+  if (msg_len == 0 || (msg_len == -1 && errno == ECONNRESET)) {
+    // EOF from our parent, or our parent died, we should die.
+    _exit(0);
+  }
+
+  // The parent should send exactly one file descriptor, on which we
+  // will write the reply.
+  if (msg_len < 0 || fds.size() != 1 || fds.at(0) < 0) {

[Markus (顧孟勤), 2012/12/13 09:48:52]

Instead of saying fds.at(0) all over the place, I would probably assign this
value to a suitably named local variable.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

Done.

+    PLOG(ERROR) << "Error reading message from the client";
+    return false;
+  }
+
+  Pickle pickle(buf, msg_len);
+  PickleIterator iter(pickle);
+  int command_type;
+  if (pickle.ReadInt(&iter, &command_type)) {
+    bool r = false;
+    // Go through all the possible IPC messages.
+    switch (command_type) {
+      case kCommandOpen:
+        // We reply on the file descriptor sent to us via the IPC channel.
+        r = HandleOpenRequest(fds.at(0), allowed_file_names,
+                              pickle, iter);
+        // Close the temporary reply channel.
+        (void) HANDLE_EINTR(close(fds.at(0)));
+        return r;
+      default:
+        NOTREACHED();
+        return false;
+    }
+  }
+
+  LOG(ERROR) << "Error parsing IPC request";
+  return false;
+}
+
+}  // namespace
+
+namespace sandbox {
+
+BrokerProcess::BrokerProcess(const std::vector<std::string>& allowed_file_names,
+                             bool fast_check_in_client,
+                             bool quiet_failures_for_tests)
+    : initialized_(false),
+      is_child_(false),
+      fast_check_in_client_(fast_check_in_client),
+      quiet_failures_for_tests_(quiet_failures_for_tests),
+      broker_pid_(-1),
+      allowed_file_names_(allowed_file_names),
+      ipc_socketpair_(-1) {
+}
+
+BrokerProcess::~BrokerProcess() {
+  if (initialized_ && ipc_socketpair_ != -1) {
+    void (HANDLE_EINTR(close(ipc_socketpair_)));
+  }
+}
+
+bool BrokerProcess::Init(void* sandbox_callback) {
+  CHECK(!initialized_);
+  CHECK_EQ(sandbox_callback, (void*) NULL) <<
+      "sandbox_callback is not implemented";
+  int socket_pair[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_pair)) {

[Markus (顧孟勤), 2012/12/13 09:48:52]

You need to know about package boundaries don't you? That probably means you
want to use SOCK_DGRAM.

This currently works, because the kernel cannot coalesce messages that contain
ancillary data. But that sounds really fragile. You shouldn't rely on this very
subtle feature.

I know you currently have no intention to do so, but if you ever added a new IPC
that didn't require ancillary data (e.g. because it is a "void" function), your
code would break in a rather hard-to-diagnose fashion -- and only when under
load from multiple threads.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

Very good point, thanks! SOCK_DGRAM wouldn't work, because I really want
recvmsg() to return an error when the connection has died.
I'll use SOCK_SEQPACKET instead.

+    LOG(ERROR) << "Failed to create socketpair";
+    return false;
+  }
+
+  int child_pid = fork();
+  if (child_pid == -1) {
+     (void) HANDLE_EINTR(close(socket_pair[0]));
+     (void) HANDLE_EINTR(close(socket_pair[1]));
+      return false;
+  }
+  if (child_pid) {
+    // We are the parent and we have just forked our broker process.
+    (void) HANDLE_EINTR(close(socket_pair[1]));
+    // We should only be able to write to the IPC channel. We'll always send
+    // a new file descriptor to receive the reply on.
+    shutdown(socket_pair[0], SHUT_RD);

[Markus (顧孟勤), 2012/12/13 09:48:52]

While it is somewhat arbitrary, your choice of indices is confusing. It would be
little more obvious if you followed the convention that pipe() uses. Index "0"
is used for reading, index "1" is used for writing.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

Done.

+    ipc_socketpair_ = socket_pair[0];
+    initialized_ = true;

[Markus (顧孟勤), 2012/12/13 09:48:52]

Why do we set initialized_ to true here? We later fall through to code that does
that again.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

Done.

+    is_child_ = false;
+    broker_pid_ = child_pid;
+
+  } else {
+    // We are the broker.
+    (void) HANDLE_EINTR(close(socket_pair[0]));
+    // We should only be able to read from this IPC channel. We will send our
+    // replies on a new file descriptor attached to the requests.
+    shutdown(socket_pair[1], SHUT_WR);
+    ipc_socketpair_ = socket_pair[1];
+    initialized_ = true;
+    is_child_ = true;
+    // TODO(jln): activate a sandbox here.
+    for (;;) {
+      HandleRequest(ipc_socketpair_, allowed_file_names_);
+    }
+  }
+
+  initialized_ = true;
+  return true;
+}
+
+// This function needs to be async signal safe.
+int BrokerProcess::Open(const char* pathname, int flags) const {
+  RAW_CHECK(initialized_);  // async signal safe CHECK().
+  // There is no point in forwarding a request that we know will be denied.
+  // Of course, the real security check needs to be on the other side of the
+  // IPC.
+  if (fast_check_in_client_) {
+    if(!GetFileNameInWhitelist(allowed_file_names_, pathname, NULL) ||
+       flags != O_RDONLY) {

[Markus (顧孟勤), 2012/12/13 09:48:52]

Can you add some comments that the handling of flags is overly simplified.

For example, it would be perfectly legitimate and safe for somebody to set the
O_NONBLOCK or O_CLOEXEC flag in addition to O_RDONLY.

But these are flags that must be handled in the caller and cannot be handled by
the broker process. So, we have to mask them prior to passing the flags to the
broker.

And then we later have to do the right thing to make the flags effective.

See SOCK_NONBLOCK and SOCK_CLOEXEC for how to do that.

[jln (very slow on Chromium), 2012/12/13 19:27:17]

I'll send a next iteration soon that also supports O_RDWR and calls fcntl
properly.

+      return -EPERM;
+    }
+  }
+
+  Pickle write_pickle;
+  write_pickle.WriteInt(kCommandOpen);
+  write_pickle.WriteString(pathname);
+  write_pickle.WriteInt(flags);
+  CHECK_LE(write_pickle.size(), kMaxMessageLength);
+
+  int returned_fd = -1;
+  uint8_t reply_buf[kMaxMessageLength];
+  // Send a request (in write_pickle) as well that will include a new
+  // temporary socketpair (created internally by SendRecvMsg()).
+  // Then read the reply on this new socketpair in reply_buf and put an
+  // eventual attached file descriptor in |returned_fd|.
+  // TODO(jln): this API needs some rewriting and documentation.
+  ssize_t msg_len = UnixDomainSocket::SendRecvMsg(ipc_socketpair_,
+                                                  reply_buf,
+                                                  sizeof(reply_buf),
+                                                  &returned_fd,
+                                                  write_pickle);
+  if (msg_len <= 0) {
+    if (!quiet_failures_for_tests_)
+      RAW_LOG(ERROR, "Could not make request to broker process");
+    return -ENOMEM;
+  }
+
+  Pickle read_pickle(reinterpret_cast<char*>(reply_buf), msg_len);
+  PickleIterator iter(read_pickle);
+  int return_value = -1;
+  // Now deserialize the return value and eventually return the file
+  // descriptor.
+  if (read_pickle.ReadInt(&iter, &return_value)) {
+    if (return_value < 0) {
+      CHECK_EQ(returned_fd, -1);
+      return return_value;
+    } else {
+      // We have a real file descriptor to return.
+      CHECK_GE(returned_fd, 0);
+      return returned_fd;
+    }
+  } else {
+    RAW_LOG(ERROR, "Could not read pickle");
+    return -1;
+  }
+}
+
+}  // namespace sandbox.