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

3 files changed, 199 insertions, 61 deletions

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 GetCurrentDirectory(FilePath* path);
 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 @@ bool RealPath(const FilePath& path, FilePath* real_path) {
 // 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 @@ bool VerifySpecificPathControlledByUser(const FilePath& path,
     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 CopyFile(const FilePath& from_path, const FilePath& to_path) {
 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 @@ bool VerifyPathControlledByUser(const FilePath& base,
   }
 
   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 @@ bool VerifyPathControlledByAdmin(const FilePath& path) {
   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 @@ class VerifyPathControlledByUserTest : public FileUtilTest {
     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 @@ class VerifyPathControlledByUserTest : public FileUtilTest {
   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 @@ TEST_F(VerifyPathControlledByUserTest, BadPaths) {
   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 @@ TEST_F(VerifyPathControlledByUserTest, Symlinks) {
       << "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 @@ TEST_F(VerifyPathControlledByUserTest, Symlinks) {
 
   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 @@ TEST_F(VerifyPathControlledByUserTest, OwnershipChecks) {
 
   // 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_gids_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          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_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_));
+
+  // 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 @@ TEST_F(VerifyPathControlledByUserTest, WriteBitChecks) {
 
   // 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)