Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(229)

Unified Diff: sandbox/linux/syscall_broker/broker_policy.cc

Issue 761903003: Update from https://crrev.com/306655 (Closed) Base URL: git@github.com:domokit/mojo.git@master
Patch Set: Created 6 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « sandbox/linux/syscall_broker/broker_policy.h ('k') | sandbox/linux/syscall_broker/broker_process.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..d9f69e3b8111f27dcfdab1c86cac1d7af1bd11b6 100644
--- a/sandbox/linux/syscall_broker/broker_policy.cc
+++ b/sandbox/linux/syscall_broker/broker_policy.cc
@@ -17,76 +17,19 @@
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.
+ if (num_of_permissions_ > 0) {
+ permissions_array_ = &permissions_[0];
+ } else {
+ permissions_array_ = NULL;
+ }
}
BrokerPolicy::~BrokerPolicy() {
@@ -107,34 +50,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 +76,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
« no previous file with comments | « sandbox/linux/syscall_broker/broker_policy.h ('k') | sandbox/linux/syscall_broker/broker_process.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698