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/basictypes.h"
+#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|.
+// See GetFileNameIfAllowedAccess() for an explaination of |file_to_open|.
+// async signal safe if |file_to_open| is NULL.
+// TODO(jln): assert signal safety.
+bool GetFileNameInWhitelist(const std::vector<std::string>& allowed_file_names,
+                            const std::string& requested_filename,
+                            const char** file_to_open) {
+  if (file_to_open && *file_to_open) {
+    // Make sure that callers never pass a non empty string. In case callers

[Jorge Lucangeli Obes, 2012/12/13 23:52:46]

nit: non-empty

[jln (very slow on Chromium), 2012/12/14 00:07:28]

Done.

+    // wrongly forget to check the return value and look at the string
+    // instead, this could catch bugs.
+    RAW_LOG(FATAL, "*file_to_open should be NULL");
+    return false;
+  }
+  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->c_str();
+    return true;
+  }
+  return false;
+}
+
+// We maintain a list of flags that have been reviewed for "sanity" and that
+// we're ok to allow in the broker.
+// i.e. here is where we wouldn't add O_RESET_FILE_SYSTEM.

[Jorge Lucangeli Obes, 2012/12/13 23:52:46]

nit: I.e. (i.e. with a capital 'I').

[jln (very slow on Chromium), 2012/12/14 00:07:28]

Done.

+bool IsWhiteListedOpenFlags(int flags) {
+  const int known_flags =

[Jorge Lucangeli Obes, 2012/12/13 23:52:46]

Do we need to keep this inside the function? I understand the desire to scope
everything as tight as possible, but we're recreating |unsupported_flags| on
every call.

The compiler might optimize that away though.

[jln (very slow on Chromium), 2012/12/14 00:07:28]

Indeed, I would be shocked if it wasn't optimized. That's why I use many temp
variables liberally to explicit every step.

+    O_RDONLY | O_WRONLY | O_RDWR |

[Markus (顧孟勤), 2012/12/14 00:40:34]

These do look like bit fields, don't they. It's deceptive though, they aren't

O_RDONLY is 0 (clearly not a bit field value)
O_WRONLY is 1, and
O_RDWR is 2

So, a caller could try passing in "3", which is undefined by the kernel, but
which would pass your checks.

You really need to treat the access mode differently from the file flags.

[jln (very slow on Chromium), 2012/12/14 01:11:29]

Done.

+    O_APPEND | O_ASYNC | O_CLOEXEC | O_CREAT | O_DIRECT |

[Markus (顧孟勤), 2012/12/14 00:40:34]

O_CLOEXEC doesn't make any sense here. It is lost when you send the file
descriptor over the socket.

Instead, you have to handle it on the receiving side by setting the
MSG_CMSG_CLOEXEC flag on recvmsg().

N.B.: You can't just use fcntl() to change the value after the fact. That
introduces a race condition. And F_CLOEXEC is used exactly, because you want to
avoid these type of race conditions.

[jln (very slow on Chromium), 2012/12/14 01:11:29]

Done.

+    O_DIRECTORY | O_EXCL | O_LARGEFILE | O_NOATIME | O_NOCTTY |
+    O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC;

[Markus (顧孟勤), 2012/12/14 00:40:34]

O_NONBLOCK and O_NDELAY are the same value. Don't list both of them, that's very
confusing.

If need be, add a comment:

O_NOFOLLOW | O_NONBLOCK /* aka O_NDELAY */ | O_SYNC | ...

[jln (very slow on Chromium), 2012/12/14 01:11:29]

Do you feel strongly about this ? I prefer to list everything and let the
compiler figure out which one are the same. I think it's more correct.

+
+  int unsuported_flags = ~known_flags;
+  bool has_unsupported_flags = flags & unsuported_flags;
+  return !has_unsupported_flags;
+}
+
+}  // namespace
+
+namespace sandbox {
+
+BrokerProcess::BrokerProcess(const std::vector<std::string>& allowed_r_files,
+                             const std::vector<std::string>& allowed_w_files,
+                             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_r_files_(allowed_r_files),
+      allowed_w_files_(allowed_w_files),
+      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];
+  // Use SOCK_SEQPACKET, because we need to preserve message boundaries
+  // but we also want to be notified (recvmsg should return and not block)
+  // when the connection has been broken (one of the processes died).
+  if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, socket_pair)) {
+    LOG(ERROR) << "Failed to create socketpair";
+    return false;
+  }
+
+  int child_pid = fork();
+  if (child_pid == -1) {
+     (void) HANDLE_EINTR(close(socket_pair[0]));

[Jorge Lucangeli Obes, 2012/12/13 23:52:46]

Is this the right alignment?

[jln (very slow on Chromium), 2012/12/14 00:07:28]

Good point!

+     (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[0]));
+    // 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[1], SHUT_RD);
+    ipc_socketpair_ = socket_pair[1];
+    is_child_ = false;
+    broker_pid_ = child_pid;
+    initialized_ = true;
+    return true;
+  } else {
+    // We are the broker.
+    (void) HANDLE_EINTR(close(socket_pair[1]));
+    // 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[0], SHUT_WR);
+    ipc_socketpair_ = socket_pair[0];
+    is_child_ = true;
+    // TODO(jln): activate a sandbox here.
+    initialized_ = true;
+    for (;;) {
+      HandleRequest();
+    }
+    _exit(1);
+  }
+  NOTREACHED();
+}
+
+// This function needs to be async signal safe.
+int BrokerProcess::Open(const char* pathname, int flags) const {
+  RAW_CHECK(initialized_);  // async signal safe CHECK().
+  if (!pathname)
+    return -EFAULT;
+  // 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 (!GetFileNameIfAllowedAccess(pathname, flags, NULL))
+      return -EPERM;
+  }
+
+  // Some flags will need particular care on the client side, which is
+  // not implemented for now.
+  if (flags & O_CLOEXEC || flags & O_NONBLOCK) {
+    return -EPERM;
+  }
+
+  Pickle write_pickle;
+  write_pickle.WriteInt(kCommandOpen);
+  write_pickle.WriteString(pathname);
+  write_pickle.WriteInt(flags);
+  RAW_CHECK(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) {
+      RAW_CHECK(returned_fd == -1);
+      return return_value;
+    } else {
+      // We have a real file descriptor to return.
+      RAW_CHECK(returned_fd >= 0);
+      return returned_fd;
+    }
+  } else {
+    RAW_LOG(ERROR, "Could not read pickle");
+    return -1;
+  }
+}
+
+// Handle a request on the IPC channel ipc_socketpair_.
+// 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 BrokerProcess::HandleRequest() const {
+
+  std::vector<int> fds;
+  char buf[kMaxMessageLength];
+  errno = 0;
+  const ssize_t msg_len = UnixDomainSocket::RecvMsg(ipc_socketpair_, buf,
+                                                    sizeof(buf), &fds);
+
+  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) {
+    PLOG(ERROR) << "Error reading message from the client";
+    return false;
+  }
+
+  const int temporary_ipc = fds.at(0);
+
+  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(temporary_ipc, pickle, iter);
+        (void) HANDLE_EINTR(close(temporary_ipc));
+        return r;
+      default:
+        NOTREACHED();
+        return false;
+    }
+  }
+
+  LOG(ERROR) << "Error parsing IPC request";
+  return false;
+}
+
+// Handle an open request contained in |read_pickle| and send the reply
+// on |reply_ipc|.
+bool BrokerProcess::HandleOpenRequest(int reply_ipc,
+                                      const Pickle& read_pickle,
+                                      PickleIterator iter) const {
+  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;
+
+  const char* file_to_open = NULL;
+  const bool file_is_in_whitelist =
+      GetFileNameIfAllowedAccess(requested_filename.c_str(),
+                                 flags, &file_to_open);
+
+  const bool safe_to_open_file =
+    IsWhiteListedOpenFlags(flags) && file_is_in_whitelist && file_to_open;
+
+  if (safe_to_open_file) {
+    int opened_fd = open(file_to_open, flags);
+    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(reply_ipc, 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;
+}
+
+// For paranoia, if |file_to_open| is not NULL, we will return the matching
+// string from the white list.
+// Async signal safe only if |file_to_open| is NULL.
+// Even if an attacker managed to fool the string comparison mechanism, we
+// would not open an attacker-controlled file name.
+// Return true if access should be allowed, false otherwise.
+bool BrokerProcess::GetFileNameIfAllowedAccess(const char* requested_filename,
+    int requested_flags, const char** file_to_open) const {
+  // O_RDONLY is the default and cannot be treated like a normal flag.
+  COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY_has_weird_value);

[Jorge Lucangeli Obes, 2012/12/13 23:52:46]

I know right? O_RDONLY is weird =P

[jln (very slow on Chromium), 2012/12/14 00:07:28]

The second part is the magic variable name that the macro will use as an array
with negative size. It's pretty much an error message.

I changed it to make it more clear.

+  if (requested_flags & O_WRONLY) {

[Markus (顧孟勤), 2012/12/14 00:40:34]

As I pointed out earlier, these are not flags and treating them as such as at
best confusing.

How about:

switch (requested_flags & O_ACCMODE) {
  case O_RDONLY: ...
  case O_WRONLY: ...
  case O_RDWR: ...
  default: There be dragons
}

[jln (very slow on Chromium), 2012/12/14 01:11:29]

Done.

+    return GetFileNameInWhitelist(allowed_w_files_, requested_filename,
+                                  file_to_open);
+  } else if (requested_flags & O_RDWR) {
+    bool allowed_for_read_and_write =
+        GetFileNameInWhitelist(allowed_r_files_, requested_filename, NULL) &&
+        GetFileNameInWhitelist(allowed_w_files_, requested_filename,
+                               file_to_open);
+    return allowed_for_read_and_write;
+  } else {
+    // O_RDONLY is 0 and the default.
+    return GetFileNameInWhitelist(allowed_r_files_, requested_filename,
+                                  file_to_open);
+  }
+}
+
+}  // namespace sandbox.