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;
+
+// Some flags will need special treatment on the client side and are not
+// supported for now.
+int UnspportedFlagsMask() {

[Markus (顧孟勤), 2012/12/14 01:36:23]

s/UnspportedFlagsMask/UnsupportedFlagsMask/g

You might even consider calling them ClientSideFlagsMask, instead. Because
that's really what they are.

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

I renamed it to ForCurrentProcessFlagsMask.

+  return O_CLOEXEC | O_NONBLOCK;
+}
+
+// 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
+    // 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.
+bool IsAllowedOpenFlags(int flags) {
+  // First, check the access mode
+  const int access_mode = flags & O_ACCMODE;
+  if (access_mode != O_RDONLY && access_mode != O_WRONLY &&
+      access_mode != O_RDWR) {
+    return false;
+  }
+
+  // Some flags will need particular care on the client side, which is
+  // not implemented for now.
+  if (flags & UnspportedFlagsMask()) {
+    return false;
+  }
+
+  const int creation_status_flags = flags & ~O_ACCMODE;
+
+  const int known_flags =
+    O_APPEND | O_ASYNC | O_CLOEXEC | O_CREAT | O_DIRECT |
+    O_DIRECTORY | O_EXCL | O_LARGEFILE | O_NOATIME | O_NOCTTY |
+    O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC;

[Markus (顧孟勤), 2012/12/14 01:36:23]

I would really prefer if you took out the flags that we are not prepared to
handle.

The code is hard enough to read as is, including flags that we then immediately
remove again just makes the code harder to explain.

I am already a little confused by the fact that you sort the flag values
alphabetically. But I understand why you are doing that.

Personally, I would have preferred grouping the flags logically (including a
comment explaining why the flags in this group are safe). But that's not easy to
do either. So, your solution is more expedient.

+
+  const int unsuported_flags = ~known_flags;

[Markus (顧孟勤), 2012/12/14 01:36:23]

If you keep things like O_NONBLOCK and O_CLOEXEC in your list of known_flags,
then the above line is not technically correct.

Yes, these flags are "known", but they are not "supported" in the broker
process, as they are client-side only.

I maintain that it is a fatal bug if you ever see them in the broker process, as
they change the behavior of the broker.

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

Hopefully I've made this whole area more clear now :)

+  const bool has_unsupported_flags = creation_status_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]));
+    (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;
+  }
+
+  Pickle write_pickle;
+  write_pickle.WriteInt(kCommandOpen);
+  write_pickle.WriteString(pathname);
+  write_pickle.WriteInt(flags);

[Markus (顧孟勤), 2012/12/14 01:36:23]

This should read: write_pickle.WriteInt(flags & ~UnsupportedFlagsMask);

Don't send requests to the broker that the broker can't possibly handle. It
makes the code much harder to reason about.

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

I really want all the relevant checks in *one* function that I can call from
both the client and the server.

I've modified GetFileNameIfAllowedAcess() along those lines.

[Markus (顧孟勤), 2012/12/14 02:10:41]

I am not going to fight you more over this. But I think you are aiming for the
wrong goal. Client and broker are fundamentally different and need a different
set of checks.

On 2012/12/14 02:01:19, Julien Tinnes wrote:

+  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 =
+    IsAllowedOpenFlags(flags) && file_is_in_whitelist && file_to_open;
+
+  if (safe_to_open_file) {
+    int opened_fd = open(file_to_open, flags);

[Markus (顧孟勤), 2012/12/14 01:36:23]

You should probably do "open(file_to_open, flags | O_CLOEXEC)"

This is security in depth. You currently don't expect that your broker ever
execs something else. But should it decide to do so in the future, then there is
the possibility that you could leak file descriptors. That would result in
really subtle bugs in the client.

N.B. it doesn't affect the client one bit, whether you have O_CLOEXEC set or
not; the kernel overrides this flag when sending the descriptor.

[Markus (顧孟勤), 2012/12/14 01:36:23]

I am undecided on whether you should also set O_NONBLOCK in the call to open().
In principle, this is a good idea, because your broker is single-threaded and
causing it to dead-lock on open() would be very bad indeed.

In practice, I am not quite sure what the kernel does if you try to send a file
descriptor that has a pending open() operation. And I don't have any good idea
how to quickly test this. I don't have any file systems that block in open().

So, maybe, this could just be a TODO?

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

Done.

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

I think that we do want to block in this case. The typical use case for this is
the initialization of a third-party library. We need it for now for the GPU
process for some particular cards.
Most notably, it needs to open a device file, which I could imagine could block.

If we need to support multiple threads efficiently, we'll probably need to make
many changes.

+    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 {
+  switch (requested_flags & O_ACCMODE) {
+    case O_RDONLY:
+      return GetFileNameInWhitelist(allowed_r_files_, requested_filename,
+                                    file_to_open);
+    case O_WRONLY:
+      return GetFileNameInWhitelist(allowed_w_files_, requested_filename,
+                                    file_to_open);
+    case 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;
+    }
+    default:
+      return false;
+  }
+}
+
+}  // namespace sandbox.