sandbox: Extend BrokerPolicy to support file creation

sandbox/linux/syscall_broker/broker_policy.cc

Index: sandbox/linux/syscall_broker/broker_policy.cc
diff --git a/sandbox/linux/syscall_broker/broker_policy.cc b/sandbox/linux/syscall_broker/broker_policy.cc
index ee9a658319d992a585a911c8d030382716b14a85..0d67fc7d40538e09a6c8efa0e23d031fa40def75 100644
--- a/sandbox/linux/syscall_broker/broker_policy.cc
+++ b/sandbox/linux/syscall_broker/broker_policy.cc
@@ -17,76 +17,15 @@
 namespace sandbox {
 namespace syscall_broker {
 
-namespace {
-
-// 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;
-  }
-
-  // We only support a 2-parameters open, so we forbid O_CREAT.
-  if (flags & O_CREAT) {
-    return false;
-  }
-
-  // Some flags affect the behavior of the current process. We don't support
-  // them and don't allow them for now.
-  if (flags & kCurrentProcessOpenFlagsMask)
-    return false;
-
-  // Now check that all the flags are known to us.
-  const int creation_and_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;
-
-  const int unknown_flags = ~known_flags;
-  const bool has_unknown_flags = creation_and_status_flags & unknown_flags;
-  return !has_unknown_flags;
-}
-
-// Needs to be 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 char* 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;
-  }
-
-  // Look for |requested_filename| in |allowed_file_names|.
-  // We don't use ::find() because it takes a std::string and
-  // the conversion allocates memory.
-  for (const auto& allowed_file_name : allowed_file_names) {
-    if (strcmp(requested_filename, allowed_file_name.c_str()) == 0) {
-      if (file_to_open)
-        *file_to_open = allowed_file_name.c_str();
-      return true;
-    }
-  }
-  return false;
-}
-
-}  // namespace
-
 BrokerPolicy::BrokerPolicy(int denied_errno,
-                           const std::vector<std::string>& allowed_r_files,
-                           const std::vector<std::string>& allowed_w_files)
+                           const std::vector<BrokerFilePermission>& permissions)
     : denied_errno_(denied_errno),
-      allowed_r_files_(allowed_r_files),
-      allowed_w_files_(allowed_w_files) {
+      permissions_(permissions),
+      num_of_permissions_(permissions.size()) {
+  // The spec guarantees vectors store their elements contiguously
+  // so set up a pointer to array of element so it can be used
+  // in async signal safe code instead of vector operations.
+  permissions_array_ = &permissions_[0];

[jln (very slow on Chromium), 2014/11/24 20:12:17]

Nit: You should be able to have that in the initializer list as well. In which
case you could mark the pointer itself as const too.

[leecam, 2014/11/25 02:06:49]

The pointer is const. This should actually only be set when permissions_array_ >
0. So I've added that check and I'd like to leave it like this for now...

[jln (very slow on Chromium), 2014/11/26 01:02:31]

The pointer points to a const object, but it is not const itself.

It's fine if you want the check though.

 }
 
 BrokerPolicy::~BrokerPolicy() {
@@ -107,34 +46,20 @@ bool BrokerPolicy::GetFileNameIfAllowedToAccess(
     const char* requested_filename,
     int requested_mode,
     const char** file_to_access) const {
-  // First, check if |requested_mode| is existence, ability to read or ability
-  // to write. We do not support X_OK.
-  if (requested_mode != F_OK && requested_mode & ~(R_OK | W_OK)) {
+  if (file_to_access && *file_to_access) {
+    // 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_access should be NULL");
     return false;
   }
-  switch (requested_mode) {
-    case F_OK:
-      // We allow to check for file existence if we can either read or write.
-      return GetFileNameInWhitelist(
-                 allowed_r_files_, requested_filename, file_to_access) ||
-             GetFileNameInWhitelist(
-                 allowed_w_files_, requested_filename, file_to_access);
-    case R_OK:
-      return GetFileNameInWhitelist(
-          allowed_r_files_, requested_filename, file_to_access);
-    case W_OK:
-      return GetFileNameInWhitelist(
-          allowed_w_files_, requested_filename, file_to_access);
-    case R_OK | W_OK: {
-      bool allowed_for_read_and_write =
-          GetFileNameInWhitelist(allowed_r_files_, requested_filename, NULL) &&
-          GetFileNameInWhitelist(
-              allowed_w_files_, requested_filename, file_to_access);
-      return allowed_for_read_and_write;
+  for (size_t i = 0; i < num_of_permissions_; i++) {
+    if (permissions_array_[i].CheckAccess(requested_filename, requested_mode,
+                                          file_to_access)) {
+      return true;
     }
-    default:
-      return false;
   }
+  return false;
 }
 
 // Check if |requested_filename| can be opened with flags |requested_flags|.
@@ -147,27 +72,22 @@ bool BrokerPolicy::GetFileNameIfAllowedToAccess(
 // Async signal safe if and only if |file_to_open| is NULL.
 bool BrokerPolicy::GetFileNameIfAllowedToOpen(const char* requested_filename,
                                               int requested_flags,
-                                              const char** file_to_open) const {
-  if (!IsAllowedOpenFlags(requested_flags)) {
+                                              const char** file_to_open,
+                                              bool* unlink_after_open) const {
+  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;
   }
-  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;
+  for (size_t i = 0; i < num_of_permissions_; i++) {
+    if (permissions_array_[i].CheckOpen(requested_filename, requested_flags,
+                                        file_to_open, unlink_after_open)) {
+      return true;
     }
-    default:
-      return false;
   }
+  return false;
 }
 
 }  // namespace syscall_broker