Skip to content

Commit

Permalink
sandbox: Add StatOnlyWithIntermediateDirs()
Browse files Browse the repository at this point in the history
Some use cases, such as some implementations of realpath(), require
calling stat() on all intermediate dirs of a path, but otherwise
do not need to open or read those directories.

Bug=b:112486795
Test=sandbox_linux_unittests passed

Change-Id: Iea0bad8be9e238a854de383ac6dc5124f462ead3
Reviewed-on: https://chromium-review.googlesource.com/1185376
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Commit-Queue: Drew Davenport <ddavenport@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585586}
  • Loading branch information
ddavenport-chromium authored and Commit Bot committed Aug 23, 2018
1 parent fbfc48c commit d2787ed
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 13 deletions.
13 changes: 9 additions & 4 deletions sandbox/linux/syscall_broker/broker_file_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ bool BrokerFilePermission::CheckStat(const char* requested_filename,
if (CheckAccessInternal(requested_filename, F_OK, file_to_access))
return true;

// Allow stat() on leading directories if have create permission.
if (!allow_create_)
// Allow stat() on leading directories if have create or stat() permission.
if (!(allow_create_ || allow_stat_with_intermediates_))
return false;

// NOTE: ValidatePath proved requested_length != 0;
Expand All @@ -214,7 +214,10 @@ bool BrokerFilePermission::CheckStat(const char* requested_filename,

// Special case for root: only one slash, otherwise must have a second
// slash in the right spot to avoid substring matches.
// |allow_stat_with_intermediates_| can match on the full path, and
// |allow_create_| only matches a leading directory.
if ((requested_length == 1 && requested_filename[0] == '/') ||
(allow_stat_with_intermediates_ && path_ == requested_filename) ||
(requested_length < path_.length() &&
memcmp(path_.c_str(), requested_filename, requested_length) == 0 &&
path_.c_str()[requested_length] == '/')) {
Expand All @@ -236,13 +239,15 @@ BrokerFilePermission::BrokerFilePermission(const std::string& path,
bool temporary_only,
bool allow_read,
bool allow_write,
bool allow_create)
bool allow_create,
bool allow_stat_with_intermediates)
: path_(path),
recursive_(recursive),
temporary_only_(temporary_only),
allow_read_(allow_read),
allow_write_(allow_write),
allow_create_(allow_create) {
allow_create_(allow_create),
allow_stat_with_intermediates_(allow_stat_with_intermediates) {
// Must have enough length for a '/'
CHECK(path_.length() > 0) << GetErrorMessageForTests();

Expand Down
26 changes: 17 additions & 9 deletions sandbox/linux/syscall_broker/broker_file_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,40 +25,45 @@ class SANDBOX_EXPORT BrokerFilePermission {
BrokerFilePermission& operator=(const BrokerFilePermission&) = default;

static BrokerFilePermission ReadOnly(const std::string& path) {
return BrokerFilePermission(path, false, false, true, false, false);
return BrokerFilePermission(path, false, false, true, false, false, false);
}

static BrokerFilePermission ReadOnlyRecursive(const std::string& path) {
return BrokerFilePermission(path, true, false, true, false, false);
return BrokerFilePermission(path, true, false, true, false, false, false);
}

static BrokerFilePermission WriteOnly(const std::string& path) {
return BrokerFilePermission(path, false, false, false, true, false);
return BrokerFilePermission(path, false, false, false, true, false, false);
}

static BrokerFilePermission ReadWrite(const std::string& path) {
return BrokerFilePermission(path, false, false, true, true, false);
return BrokerFilePermission(path, false, false, true, true, false, false);
}

static BrokerFilePermission ReadWriteCreate(const std::string& path) {
return BrokerFilePermission(path, false, false, true, true, true);
return BrokerFilePermission(path, false, false, true, true, true, false);
}

static BrokerFilePermission ReadWriteCreateRecursive(
const std::string& path) {
return BrokerFilePermission(path, true, false, true, true, true);
return BrokerFilePermission(path, true, false, true, true, true, false);
}

// Temporary files must always be newly created and do not confer rights to
// use pre-existing files of the same name.
static BrokerFilePermission ReadWriteCreateTemporary(
const std::string& path) {
return BrokerFilePermission(path, false, true, true, true, true);
return BrokerFilePermission(path, false, true, true, true, true, false);
}

static BrokerFilePermission ReadWriteCreateTemporaryRecursive(
const std::string& path) {
return BrokerFilePermission(path, true, true, true, true, true);
return BrokerFilePermission(path, true, true, true, true, true, false);
}

static BrokerFilePermission StatOnlyWithIntermediateDirs(
const std::string& path) {
return BrokerFilePermission(path, false, false, false, false, false, true);
}

// Returns true if |requested_filename| is allowed to be accessed
Expand Down Expand Up @@ -108,7 +113,8 @@ class SANDBOX_EXPORT BrokerFilePermission {
bool temporary_only,
bool allow_read,
bool allow_write,
bool allow_create);
bool allow_create,
bool allow_stat_with_intermediates);

// ValidatePath checks |path| and returns true if these conditions are met
// * Greater than 0 length
Expand Down Expand Up @@ -138,6 +144,8 @@ class SANDBOX_EXPORT BrokerFilePermission {
bool allow_read_;
bool allow_write_;
bool allow_create_;
// Allow stat() for |path| and all intermediate dirs.
bool allow_stat_with_intermediates_;
};

} // namespace syscall_broker
Expand Down
24 changes: 24 additions & 0 deletions sandbox/linux/syscall_broker/broker_file_permission_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,30 @@ TEST(BrokerFilePermission, ReadWriteCreateTemporaryRecursive) {
// expected.
}

TEST(BrokerFilePermission, StatOnlyWithIntermediateDirs) {
const char kPath[] = "/tmp/good/path";
const char kLeading1[] = "/";
const char kLeading2[] = "/tmp";
const char kLeading3[] = "/tmp/good/path";
const char kTrailing[] = "/tmp/good/path/bad";

BrokerFilePermission perm =
BrokerFilePermission::StatOnlyWithIntermediateDirs(kPath);
// No open or access permission.
ASSERT_FALSE(perm.CheckOpen(kPath, O_RDONLY, NULL, NULL));
ASSERT_FALSE(perm.CheckOpen(kPath, O_WRONLY, NULL, NULL));
ASSERT_FALSE(perm.CheckOpen(kPath, O_RDWR, NULL, NULL));
ASSERT_FALSE(perm.CheckAccess(kPath, R_OK, NULL));
ASSERT_FALSE(perm.CheckAccess(kPath, W_OK, NULL));

// Stat for all leading paths, but not trailing paths.
ASSERT_TRUE(perm.CheckStat(kPath, NULL));
ASSERT_TRUE(perm.CheckStat(kLeading1, NULL));
ASSERT_TRUE(perm.CheckStat(kLeading2, NULL));
ASSERT_TRUE(perm.CheckStat(kLeading3, NULL));
ASSERT_FALSE(perm.CheckStat(kTrailing, NULL));
}

TEST(BrokerFilePermission, ValidatePath) {
EXPECT_TRUE(BrokerFilePermissionTester::ValidatePath("/path"));
EXPECT_TRUE(BrokerFilePermissionTester::ValidatePath("/"));
Expand Down

0 comments on commit d2787ed

Please sign in to comment.