From 467e6e5abdf594359dc4faf213afe7ec436a23d8 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Tue, 28 Nov 2017 19:03:43 +0000 Subject: [PATCH] Rename BrokerFilePermission::unlink_ to temporary_only_ This better describes its function, and avoids confusion later on should we broker an unlink() call. In that case, the correct permission check would have nothing to do with the unlink_ member. Tidy while we're at it: Remove unused static method (apart from test). Convert bool functions to return bool expressions. General readability stuff. TBR=piman@chromium.org Change-Id: Id81b9d494a677f8c37865e5145158a03bdb18ae8 Reviewed-on: https://chromium-review.googlesource.com/786083 Reviewed-by: Antoine Labour Reviewed-by: Robert Sesek Reviewed-by: Tom Sepez Commit-Queue: Tom Sepez Cr-Commit-Position: refs/heads/master@{#519772} --- content/gpu/gpu_sandbox_hook_linux.cc | 2 +- .../syscall_broker/broker_file_permission.cc | 113 ++++++++---------- .../syscall_broker/broker_file_permission.h | 27 ++--- .../broker_file_permission_unittest.cc | 13 +- 4 files changed, 62 insertions(+), 93 deletions(-) diff --git a/content/gpu/gpu_sandbox_hook_linux.cc b/content/gpu/gpu_sandbox_hook_linux.cc index 2f20f8ab552785..e33ec47219ffc6 100644 --- a/content/gpu/gpu_sandbox_hook_linux.cc +++ b/content/gpu/gpu_sandbox_hook_linux.cc @@ -183,7 +183,7 @@ void AddStandardGpuWhiteList(std::vector* permissions) { // For shared memory. permissions->push_back( - BrokerFilePermission::ReadWriteCreateUnlinkRecursive(kDevShm)); + BrokerFilePermission::ReadWriteCreateTemporaryRecursive(kDevShm)); // For DRI cards. for (int i = 0; i <= 9; ++i) { diff --git a/sandbox/linux/syscall_broker/broker_file_permission.cc b/sandbox/linux/syscall_broker/broker_file_permission.cc index 39073444467070..3947145eaf6eb2 100644 --- a/sandbox/linux/syscall_broker/broker_file_permission.cc +++ b/sandbox/linux/syscall_broker/broker_file_permission.cc @@ -14,7 +14,6 @@ #include "sandbox/linux/syscall_broker/broker_common.h" namespace sandbox { - namespace syscall_broker { // Async signal safe @@ -52,25 +51,21 @@ bool BrokerFilePermission::ValidatePath(const char* path) { // methods are async signal safe in common standard libs. // TODO(leecam): remove dependency on std::string bool BrokerFilePermission::MatchPath(const char* requested_filename) const { - const char* path = path_.c_str(); - if ((recursive_ && strncmp(requested_filename, path, strlen(path)) == 0)) { - // Note: This prefix match will allow any path under the whitelisted - // path, for any number of directory levels. E.g. if the whitelisted - // path is /good/ then the following will be permitted by the policy. - // /good/file1 - // /good/folder/file2 - // /good/folder/folder2/file3 - // If an attacker could make 'folder' a symlink to ../../ they would have - // access to the entire filesystem. - // Whitelisting with multiple depths is useful, e.g /proc/ but - // the system needs to ensure symlinks can not be created! - // That said if an attacker can convert any of the absolute paths - // to a symlink they can control any file on the system also. - return true; - } else if (strcmp(requested_filename, path) == 0) { - return true; - } - return false; + // Note: This recursive match will allow any path under the whitelisted + // path, for any number of directory levels. E.g. if the whitelisted + // path is /good/ then the following will be permitted by the policy. + // /good/file1 + // /good/folder/file2 + // /good/folder/folder2/file3 + // If an attacker could make 'folder' a symlink to ../../ they would have + // access to the entire filesystem. + // Whitelisting with multiple depths is useful, e.g /proc/ but + // the system needs to ensure symlinks can not be created! + // That said if an attacker can convert any of the absolute paths + // to a symlink they can control any file on the system also. + return recursive_ + ? strncmp(requested_filename, path_.c_str(), path_.length()) == 0 + : strcmp(requested_filename, path_.c_str()) == 0; } // Async signal safe. @@ -82,45 +77,39 @@ bool BrokerFilePermission::CheckAccess(const char* requested_filename, const char** file_to_access) const { // First, check if |mode| is existence, ability to read or ability // to write. We do not support X_OK. - if (mode != F_OK && mode & ~(R_OK | W_OK)) { + if (mode != F_OK && mode & ~(R_OK | W_OK)) return false; - } if (!ValidatePath(requested_filename)) return false; - if (!MatchPath(requested_filename)) { + if (!MatchPath(requested_filename)) return false; - } + bool allowed = false; switch (mode) { case F_OK: - if (allow_read_ || allow_write_) - allowed = true; + allowed = allow_read_ || allow_write_; break; case R_OK: - if (allow_read_) - allowed = true; + allowed = allow_read_; break; case W_OK: - if (allow_write_) - allowed = true; + allowed = allow_write_; break; case R_OK | W_OK: - if (allow_read_ && allow_write_) - allowed = true; + allowed = allow_read_ && allow_write_; break; default: - return false; + break; } + if (!allowed) + return false; - if (allowed && file_to_access) { - if (!recursive_) - *file_to_access = path_.c_str(); - else - *file_to_access = requested_filename; - } - return allowed; + if (file_to_access) + *file_to_access = recursive_ ? requested_filename : path_.c_str(); + + return true; } // Async signal safe. @@ -134,9 +123,8 @@ bool BrokerFilePermission::CheckOpen(const char* requested_filename, if (!ValidatePath(requested_filename)) return false; - if (!MatchPath(requested_filename)) { + if (!MatchPath(requested_filename)) return false; - } // First, check the access mode is valid. const int access_mode = flags & O_ACCMODE; @@ -165,8 +153,8 @@ bool BrokerFilePermission::CheckOpen(const char* requested_filename, return false; } - // If this file is to be unlinked, ensure it's created. - if (unlink_ && !(flags & O_CREAT)) { + // If this file is to be temporary, ensure it's created. + if (temporary_only_ && !(flags & O_CREAT)) { return false; } @@ -178,7 +166,6 @@ bool BrokerFilePermission::CheckOpen(const char* requested_filename, // 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 | @@ -190,55 +177,49 @@ bool BrokerFilePermission::CheckOpen(const char* requested_filename, if (has_unknown_flags) return false; - if (file_to_open) { - if (!recursive_) - *file_to_open = path_.c_str(); - else - *file_to_open = requested_filename; - } + if (file_to_open) + *file_to_open = recursive_ ? requested_filename : path_.c_str(); + if (unlink_after_open) - *unlink_after_open = unlink_; + *unlink_after_open = temporary_only_; return true; } const char* BrokerFilePermission::GetErrorMessageForTests() { - static char kInvalidBrokerFileString[] = "Invalid BrokerFilePermission"; - return kInvalidBrokerFileString; + return "Invalid BrokerFilePermission"; } BrokerFilePermission::BrokerFilePermission(const std::string& path, bool recursive, - bool unlink, + bool temporary_only, bool allow_read, bool allow_write, bool allow_create) : path_(path), recursive_(recursive), - unlink_(unlink), + temporary_only_(temporary_only), allow_read_(allow_read), allow_write_(allow_write), allow_create_(allow_create) { - // Validate this permission and die if invalid! - // Must have enough length for a '/' CHECK(path_.length() > 0) << GetErrorMessageForTests(); + // Whitelisted paths must be absolute. CHECK(path_[0] == '/') << GetErrorMessageForTests(); - // Don't allow unlinking on creation without create permission - if (unlink_) { + // Don't allow temporary creation without create permission + if (temporary_only_) CHECK(allow_create) << GetErrorMessageForTests(); - } + + // Recursive paths must have a trailing slash, absolutes must not. const char last_char = *(path_.rbegin()); - // Recursive paths must have a trailing slash - if (recursive_) { + if (recursive_) CHECK(last_char == '/') << GetErrorMessageForTests(); - } else { + else CHECK(last_char != '/') << GetErrorMessageForTests(); - } } } // namespace syscall_broker -} // namespace sandbox \ No newline at end of file +} // namespace sandbox diff --git a/sandbox/linux/syscall_broker/broker_file_permission.h b/sandbox/linux/syscall_broker/broker_file_permission.h index 243114517acbd4..b39ac6ca1b9ef0 100644 --- a/sandbox/linux/syscall_broker/broker_file_permission.h +++ b/sandbox/linux/syscall_broker/broker_file_permission.h @@ -11,7 +11,6 @@ #include "sandbox/sandbox_export.h" namespace sandbox { - namespace syscall_broker { // BrokerFilePermission defines a path for whitelisting. @@ -50,11 +49,7 @@ class SANDBOX_EXPORT BrokerFilePermission { return BrokerFilePermission(path, true, false, true, true, true); } - static BrokerFilePermission ReadWriteCreateUnlink(const std::string& path) { - return BrokerFilePermission(path, false, true, true, true, true); - } - - static BrokerFilePermission ReadWriteCreateUnlinkRecursive( + static BrokerFilePermission ReadWriteCreateTemporaryRecursive( const std::string& path) { return BrokerFilePermission(path, true, true, true, true, true); } @@ -65,16 +60,17 @@ class SANDBOX_EXPORT BrokerFilePermission { // the |requested_filename| in the case of a recursive match, // or a pointer the matched path in the whitelist if an absolute // match. - // If not NULL |unlink_after_open| is set to point to true if the - // caller should unlink the path after opening. + // If not NULL, |unlink_after_open| is set to point to true if the + // caller is required to unlink the path after opening. // Async signal safe if |file_to_open| is NULL. bool CheckOpen(const char* requested_filename, int flags, const char** file_to_open, bool* unlink_after_open) const; + // Returns true if |requested_filename| is allowed to be accessed // by this permission as per access(2). - // If |file_to_open| is not NULL it is set to point to either + // If |file_to_open| is not NULL, it is set to point to either // the |requested_filename| in the case of a recursive match, // or a pointer to the matched path in the whitelist if an absolute // match. @@ -86,9 +82,11 @@ class SANDBOX_EXPORT BrokerFilePermission { private: friend class BrokerFilePermissionTester; + + // NOTE: Validates the permission and dies if invalid! BrokerFilePermission(const std::string& path, bool recursive, - bool unlink, + bool temporary_only, bool allow_read, bool allow_write, bool allow_create); @@ -107,18 +105,17 @@ class SANDBOX_EXPORT BrokerFilePermission { static const char* GetErrorMessageForTests(); // These are not const as std::vector requires copy-assignment and this class - // is stored in vectors. All methods are marked const so - // the compiler will still enforce no changes outside of the constructor. + // is stored in vectors. All methods are marked const so the compiler will + // still enforce no changes outside of the constructor. std::string path_; - bool recursive_; // Allow everything under this path. |path| must be a dir. - bool unlink_; // unlink after opening. + bool recursive_; // Allow everything under |path| (must be a dir). + bool temporary_only_; // File must be unlink'd after opening. bool allow_read_; bool allow_write_; bool allow_create_; }; } // namespace syscall_broker - } // namespace sandbox #endif // SANDBOX_LINUX_SYSCALL_BROKER_BROKER_FILE_PERMISSION_H_ diff --git a/sandbox/linux/syscall_broker/broker_file_permission_unittest.cc b/sandbox/linux/syscall_broker/broker_file_permission_unittest.cc index 83840779f9641c..dc56b4cd41747e 100644 --- a/sandbox/linux/syscall_broker/broker_file_permission_unittest.cc +++ b/sandbox/linux/syscall_broker/broker_file_permission_unittest.cc @@ -231,20 +231,11 @@ void CheckUnlink(BrokerFilePermission& perm, ASSERT_TRUE(unlink); } -TEST(BrokerFilePermission, ReadWriteCreateUnlink) { - const char kPath[] = "/tmp/good"; - BrokerFilePermission perm = - BrokerFilePermission::ReadWriteCreateUnlink(kPath); - CheckUnlink(perm, kPath, O_RDWR); - // Don't do anything here, so that ASSERT works in the subfunction as - // expected. -} - -TEST(BrokerFilePermission, ReadWriteCreateUnlinkRecursive) { +TEST(BrokerFilePermission, ReadWriteCreateTemporaryRecursive) { const char kPath[] = "/tmp/good/"; const char kPathFile[] = "/tmp/good/file"; BrokerFilePermission perm = - BrokerFilePermission::ReadWriteCreateUnlinkRecursive(kPath); + BrokerFilePermission::ReadWriteCreateTemporaryRecursive(kPath); CheckUnlink(perm, kPathFile, O_RDWR); // Don't do anything here, so that ASSERT works in the subfunction as // expected.