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..3ff1f2206eae1cf62d7a2a8128c6c9f5b9940c30 100644
--- a/sandbox/linux/syscall_broker/broker_policy.cc
+++ b/sandbox/linux/syscall_broker/broker_policy.cc
@@ -19,26 +19,79 @@ namespace syscall_broker {
 
 namespace {
 
+// Validate a path to be evaulated by this policy.
+// Async signal safe - calls: strlen
+bool ValidatePath(const char* path) {
+  if (!path)
+    return false;
+
+  size_t len = strlen(path);
+  // No empty paths
+  if (len == 0)
+    return false;
+  // Paths must be absolute and not relative
+  if (path[0] != '/')
+    return false;
+  // No trailing / (but "/" is valid)
+  if (len > 1 && path[len - 1] == '/')
+    return false;
+  // No trailing /..
+  if (len >= 3 && path[len - 3] == '/' && path[len - 2] == '.' &&
+      path[len - 1] == '.')
+    return false;
+  // No /../ anywhere
+  for (size_t i = 0; i < len; i++) {
+    if (path[i] == '/' && (len - i) > 3) {
+      if (path[i + 1] == '.' && path[i + 2] == '.' && path[i + 3] == '/') {
+        return false;
+      }
+    }
+  }
+  return true;
+}
+
 // 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.
+// Async signal safe - No external calls
+bool CheckFlags(const BrokerPermission& perm, int flags) {
+  // First, check the access mode is valid.
   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) {
+  // Check if read is allowed
+  if (!perm.allow_read && (access_mode == O_RDONLY || access_mode == O_RDWR)) {
+    return false;
+  }
+
+  // Check if write is allowed
+  if (!perm.allow_write && (access_mode == O_WRONLY || access_mode == O_RDWR)) {
+    return false;
+  }
+
+  // Check if file creation is allowed.
+  if (!perm.allow_create && (flags & O_CREAT)) {
+    return false;
+  }
+
+  // If O_CREAT is present, ensure O_EXCL
+  if ((flags & O_CREAT) && !(flags & O_EXCL)) {
+    return false;
+  }
+
+  // If this file is to be unlinked, ensure its created.
+  if (perm.unlink && !(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)
+  if (flags & kCurrentProcessOpenFlagsMask) {
     return false;
+  }
 
   // Now check that all the flags are known to us.
   const int creation_and_status_flags = flags & ~O_ACCMODE;
@@ -53,40 +106,49 @@ bool IsAllowedOpenFlags(int 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;
+// Sets the filename.
+// Async signal safe if file_to_open == NULL
+void SetFileName(const BrokerPermission& perm,
+                 const char* requested_filename,
+                 const char** file_to_open) {
+  if (file_to_open) {
+    if (perm.recursive)
+      *file_to_open = requested_filename;
+    else
+      *file_to_open = perm.path.c_str();
   }
+}
 
-  // 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;
+bool ValidatePermission(const BrokerPermission& perm) {
+  // Don't allow unlinking on creation without create permission
+  if (perm.unlink && !perm.allow_create)
+    return false;
+  // Recursive paths must have a trailing slash
+  if (perm.recursive && perm.path.back() != '/')
+    return false;
+  // Whitelisted paths must be absolute.
+  if (perm.path.front() != '/')
+    return false;
+  return true;
 }
 
 }  // 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<BrokerPermission>& 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];
+  for (size_t i = 0; i < num_of_permissions_; i++) {
+    if (!ValidatePermission(permissions_array_[i])) {
+      // TODO: DIE HERE
+      // SANDBOX_DIE("Failed to Validate Broker permissions");
+    }
+  }
 }
 
 BrokerPolicy::~BrokerPolicy() {
@@ -107,34 +169,65 @@ bool BrokerPolicy::GetFileNameIfAllowedToAccess(
     const char* requested_filename,
     int requested_mode,
     const char** file_to_access) const {
+  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;
+  }
+  if (!ValidatePath(requested_filename))
+    return false;
+
   // 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)) {
     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;
-    }
-    default:
+
+  for (size_t i = 0; i < num_of_permissions_; i++) {
+    const char* path = permissions_array_[i].path.c_str();
+    if ((permissions_array_[i].recursive &&
+         strncmp(requested_filename, path, strlen(path)) == 0) ||
+        strcmp(requested_filename, path) == 0) {
+      switch (requested_mode) {
+        case F_OK:
+          if (permissions_array_[i].allow_read ||
+              permissions_array_[i].allow_write) {
+            SetFileName(permissions_array_[i], requested_filename,
+                        file_to_access);
+            return true;
+          }
+          break;
+        case R_OK:
+          if (permissions_array_[i].allow_read) {
+            SetFileName(permissions_array_[i], requested_filename,
+                        file_to_access);
+            return true;
+          }
+          break;
+        case W_OK:
+          if (permissions_array_[i].allow_write) {
+            SetFileName(permissions_array_[i], requested_filename,
+                        file_to_access);
+            return true;
+          }
+          break;
+        case R_OK | W_OK:
+          if (permissions_array_[i].allow_read &&
+              permissions_array_[i].allow_write) {
+            SetFileName(permissions_array_[i], requested_filename,
+                        file_to_access);
+            return true;
+          }
+          break;
+        default:
+          return false;
+      }
       return false;
+    }
   }
+  return false;
 }
 
 // Check if |requested_filename| can be opened with flags |requested_flags|.
@@ -147,27 +240,33 @@ 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;
+  if (!ValidatePath(requested_filename))
+    return false;
+  for (size_t i = 0; i < num_of_permissions_; i++) {
+    const char* path = permissions_array_[i].path.c_str();
+    if ((permissions_array_[i].recursive &&
+         strncmp(requested_filename, path, strlen(path)) == 0) ||
+        strcmp(requested_filename, path) == 0) {
+      if (CheckFlags(permissions_array_[i], requested_flags)) {
+        SetFileName(permissions_array_[i], requested_filename, file_to_open);
+        if (unlink_after_open)
+          *unlink_after_open = permissions_array_[i].unlink;
+        return true;
+      } else {
+        return false;
+      }
     }
-    default:
-      return false;
   }
+  return false;
 }
 
 }  // namespace syscall_broker