Skip to content

Commit

Permalink
Rename BrokerFilePermission::unlink_ to temporary_only_
Browse files Browse the repository at this point in the history 
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 <piman@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519772}
  • Loading branch information
@tsepez
tsepez authored and Commit Bot committed Nov 28, 2017
1 parent 2cf1979 commit 467e6e5
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 93 deletions.
2 changes: 1 addition & 1 deletion content/gpu/gpu_sandbox_hook_linux.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ void AddStandardGpuWhiteList(std::vector<BrokerFilePermission>* permissions) {

// For shared memory.
permissions->push_back(
BrokerFilePermission::ReadWriteCreateUnlinkRecursive(kDevShm));
BrokerFilePermission::ReadWriteCreateTemporaryRecursive(kDevShm));

// For DRI cards.
for (int i = 0; i <= 9; ++i) {
Expand Down
113 changes: 47 additions & 66 deletions sandbox/linux/syscall_broker/broker_file_permission.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "sandbox/linux/syscall_broker/broker_common.h"

namespace sandbox {

namespace syscall_broker {

// Async signal safe
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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 |
Expand All @@ -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
} // namespace sandbox
27 changes: 12 additions & 15 deletions sandbox/linux/syscall_broker/broker_file_permission.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "sandbox/sandbox_export.h"

namespace sandbox {

namespace syscall_broker {

// BrokerFilePermission defines a path for whitelisting.
Expand Down Expand Up @@ -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);
}
Expand All @@ -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.
Expand All @@ -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);
Expand All @@ -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_
13 changes: 2 additions & 11 deletions sandbox/linux/syscall_broker/broker_file_permission_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 467e6e5

Please sign in to comment.