Update write checks for external extension file on mac.
BUG=100565
TEST=VerifyPathControlledByUserTest.*
Review URL: http://codereview.chromium.org/8318011
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@105923 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 7312f42a14b8d0717cd44bea53690eb27486e32d
diff --git a/base/file_util.h b/base/file_util.h
index a0d517d..b0c2459 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -20,6 +20,7 @@
#include <stdio.h>
+#include <set>
#include <stack>
#include <string>
#include <vector>
@@ -378,18 +379,21 @@
BASE_EXPORT bool SetCurrentDirectory(const FilePath& path);
#if defined(OS_POSIX)
-// Test that |path| can only be changed by a specific user and group.
+// Test that |path| can only be changed by a given user and members of
+// a given set of groups.
// Specifically, test that all parts of |path| under (and including) |base|:
// * Exist.
-// * Are owned by a specific user and group.
+// * Are owned by a specific user.
// * Are not writable by all users.
+// * Are owned by a memeber of a given set of groups, or are not writable by
+// their group.
// * Are not symbolic links.
// This is useful for checking that a config file is administrator-controlled.
// |base| must contain |path|.
BASE_EXPORT bool VerifyPathControlledByUser(const FilePath& base,
const FilePath& path,
uid_t owner_uid,
- gid_t group_gid);
+ const std::set<gid_t>& group_gids);
#endif // defined(OS_POSIX)
#if defined(OS_MACOSX)
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 323f5aa..bb9977e 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -38,6 +38,7 @@
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/singleton.h"
+#include "base/stl_util.h"
#include "base/string_util.h"
#include "base/stringprintf.h"
#include "base/sys_string_conversions.h"
@@ -91,7 +92,7 @@
// Helper for VerifyPathControlledByUser.
bool VerifySpecificPathControlledByUser(const FilePath& path,
uid_t owner_uid,
- gid_t group_gid) {
+ const std::set<gid_t>& group_gids) {
stat_wrapper_t stat_info;
if (CallLstat(path.value().c_str(), &stat_info) != 0) {
PLOG(ERROR) << "Failed to get information on path "
@@ -111,9 +112,10 @@
return false;
}
- if (stat_info.st_gid != group_gid) {
+ if ((stat_info.st_mode & S_IWGRP) &&
+ !ContainsKey(group_gids, stat_info.st_gid)) {
LOG(ERROR) << "Path " << path.value()
- << " is owned by the wrong group.";
+ << " is writable by an unprivileged group.";
return false;
}
@@ -990,7 +992,7 @@
bool VerifyPathControlledByUser(const FilePath& base,
const FilePath& path,
uid_t owner_uid,
- gid_t group_gid) {
+ const std::set<gid_t>& group_gids) {
if (base != path && !base.IsParent(path)) {
LOG(ERROR) << "|base| must be a subdirectory of |path|. base = \""
<< base.value() << "\", path = \"" << path.value() << "\"";
@@ -1014,12 +1016,13 @@
}
FilePath current_path = base;
- if (!VerifySpecificPathControlledByUser(current_path, owner_uid, group_gid))
+ if (!VerifySpecificPathControlledByUser(current_path, owner_uid, group_gids))
return false;
for (; ip != path_components.end(); ++ip) {
current_path = current_path.Append(*ip);
- if (!VerifySpecificPathControlledByUser(current_path, owner_uid, group_gid))
+ if (!VerifySpecificPathControlledByUser(
+ current_path, owner_uid, group_gids))
return false;
}
return true;
@@ -1031,20 +1034,28 @@
const FilePath kFileSystemRoot("/");
// The name of the administrator group on mac os.
- const char kAdminGroupName[] = "admin";
+ const char* const kAdminGroupNames[] = {
+ "admin",
+ "wheel"
+ };
// Reading the groups database may touch the file system.
base::ThreadRestrictions::AssertIOAllowed();
- struct group *group_record = getgrnam(kAdminGroupName);
- if (!group_record) {
- PLOG(ERROR) << "Could not get the group ID of group \""
- << kAdminGroupName << "\".";
- return false;
+ std::set<gid_t> allowed_group_ids;
+ for (int i = 0, ie = arraysize(kAdminGroupNames); i < ie; ++i) {
+ struct group *group_record = getgrnam(kAdminGroupNames[i]);
+ if (!group_record) {
+ PLOG(ERROR) << "Could not get the group ID of group \""
+ << kAdminGroupNames[i] << "\".";
+ continue;
+ }
+
+ allowed_group_ids.insert(group_record->gr_gid);
}
return VerifyPathControlledByUser(
- kFileSystemRoot, path, kRootUid, group_record->gr_gid);
+ kFileSystemRoot, path, kRootUid, allowed_group_ids);
}
#endif // defined(OS_MACOSX)
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index b76938e..8ac1076 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -1867,7 +1867,9 @@
struct stat stat_buf;
ASSERT_EQ(0, stat(base_dir_.value().c_str(), &stat_buf));
uid_ = stat_buf.st_uid;
- gid_ = stat_buf.st_gid;
+ ok_gids_.insert(stat_buf.st_gid);
+ bad_gids_.insert(stat_buf.st_gid + 1);
+
ASSERT_EQ(uid_, getuid()); // This process should be the owner.
// To ensure that umask settings do not cause the initial state
@@ -1892,7 +1894,9 @@
FilePath sub_dir_;
FilePath text_file_;
uid_t uid_;
- gid_t gid_;
+
+ std::set<gid_t> ok_gids_;
+ std::set<gid_t> bad_gids_;
};
TEST_F(VerifyPathControlledByUserTest, BadPaths) {
@@ -1900,23 +1904,25 @@
FilePath does_not_exist = base_dir_.AppendASCII("does")
.AppendASCII("not")
.AppendASCII("exist");
-
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, does_not_exist, uid_, gid_));
+ base_dir_, does_not_exist, uid_, ok_gids_));
// |base| not a subpath of |path|.
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(sub_dir_, base_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, base_dir_, uid_, ok_gids_));
// An empty base path will fail to be a prefix for any path.
FilePath empty;
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(empty, base_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ empty, base_dir_, uid_, ok_gids_));
// Finding that a bad call fails proves nothing unless a good call succeeds.
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
}
TEST_F(VerifyPathControlledByUserTest, Symlinks) {
@@ -1928,9 +1934,11 @@
<< "Failed to create symlink.";
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, file_link, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, file_link, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(file_link, file_link, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ file_link, file_link, uid_, ok_gids_));
// Symlink from one directory to another within the path.
FilePath link_to_sub_dir = base_dir_.AppendASCII("link_to_sub_dir");
@@ -1942,25 +1950,22 @@
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, file_path_with_link, uid_, gid_));
+ base_dir_, file_path_with_link, uid_, ok_gids_));
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- link_to_sub_dir, file_path_with_link, uid_, gid_));
+ link_to_sub_dir, file_path_with_link, uid_, ok_gids_));
// Symlinks in parents of base path are allowed.
EXPECT_TRUE(
file_util::VerifyPathControlledByUser(
- file_path_with_link, file_path_with_link, uid_, gid_));
+ file_path_with_link, file_path_with_link, uid_, ok_gids_));
}
TEST_F(VerifyPathControlledByUserTest, OwnershipChecks) {
// Get a uid that is not the uid of files we create.
uid_t bad_uid = uid_ + 1;
- // Get a gid that is not ours.
- gid_t bad_gid = gid_ + 1;
-
// Make all files and directories non-world-writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(base_dir_, 0u, S_IWOTH));
@@ -1971,33 +1976,130 @@
// We control these paths.
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Another user does not control these paths.
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, sub_dir_, bad_uid, gid_));
+ base_dir_, sub_dir_, bad_uid, ok_gids_));
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, text_file_, bad_uid, gid_));
+ base_dir_, text_file_, bad_uid, ok_gids_));
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- sub_dir_, text_file_, bad_uid, gid_));
+ sub_dir_, text_file_, bad_uid, ok_gids_));
// Another group does not control the paths.
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, sub_dir_, uid_, bad_gid));
+ base_dir_, sub_dir_, uid_, bad_gids_));
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- base_dir_, text_file_, uid_, bad_gid));
+ base_dir_, text_file_, uid_, bad_gids_));
EXPECT_FALSE(
file_util::VerifyPathControlledByUser(
- sub_dir_, text_file_, uid_, bad_gid));
+ sub_dir_, text_file_, uid_, bad_gids_));
+}
+
+TEST_F(VerifyPathControlledByUserTest, GroupWriteTest) {
+ // Make all files and directories writable only by their owner.
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(base_dir_, 0u, S_IWOTH|S_IWGRP));
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(sub_dir_, 0u, S_IWOTH|S_IWGRP));
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(text_file_, 0u, S_IWOTH|S_IWGRP));
+
+ // Any group is okay because the path is not group-writable.
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
+
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, bad_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, bad_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, bad_gids_));
+
+ // No group is okay, because we don't check the group
+ // if no group can write.
+ std::set<gid_t> no_gids; // Empty set of gids.
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, no_gids));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, no_gids));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, no_gids));
+
+
+ // Make all files and directories writable by their group.
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(base_dir_, S_IWGRP, 0u));
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(sub_dir_, S_IWGRP, 0u));
+ ASSERT_NO_FATAL_FAILURE(
+ ChangePosixFilePermissions(text_file_, S_IWGRP, 0u));
+
+ // Now |ok_gids_| works, but |bad_gids_| fails.
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
+
+ EXPECT_FALSE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, bad_gids_));
+ EXPECT_FALSE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, bad_gids_));
+ EXPECT_FALSE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, bad_gids_));
+
+ // Because any group in the group set is allowed,
+ // the union of good and bad gids passes.
+
+ std::set<gid_t> multiple_gids;
+ std::set_union(
+ ok_gids_.begin(), ok_gids_.end(),
+ bad_gids_.begin(), bad_gids_.end(),
+ std::inserter(multiple_gids, multiple_gids.begin()));
+
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, multiple_gids));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, multiple_gids));
+ EXPECT_TRUE(
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, multiple_gids));
+
}
TEST_F(VerifyPathControlledByUserTest, WriteBitChecks) {
@@ -2011,72 +2113,93 @@
// Initialy, we control all parts of the path.
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Make base_dir_ world-writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(base_dir_, S_IWOTH, 0u));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Make sub_dir_ world writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(sub_dir_, S_IWOTH, 0u));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Make text_file_ world writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(text_file_, S_IWOTH, 0u));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Make sub_dir_ non-world writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(sub_dir_, 0u, S_IWOTH));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Make base_dir_ non-world-writable.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(base_dir_, 0u, S_IWOTH));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_FALSE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
// Back to the initial state: Nothing is writable, so every path
// should pass.
ASSERT_NO_FATAL_FAILURE(
ChangePosixFilePermissions(text_file_, 0u, S_IWOTH));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, sub_dir_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ base_dir_, text_file_, uid_, ok_gids_));
EXPECT_TRUE(
- file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+ file_util::VerifyPathControlledByUser(
+ sub_dir_, text_file_, uid_, ok_gids_));
}
#endif // defined(OS_POSIX)