Add external extensions json source in proper mac location.

The old path will be deprecated once developers have migrated.

BUG=67203
TEST=FileUtilTest.IsPathControledByAdmin

Review URL: http://codereview.chromium.org/7718021

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@102274 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 547 insertions, 39 deletions

diff --git a/base/file_util.h b/base/file_util.h
index 133854b..2cc8caa 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -377,6 +377,32 @@ BASE_EXPORT bool GetCurrentDirectory(FilePath* path);
 // Sets the current working directory for the process.

 BASE_EXPORT bool SetCurrentDirectory(const FilePath& path);

 

+#if defined(OS_POSIX)

+// Test that |path| can only be changed by a specific user and group.

+// Specifically, test that all parts of |path| under (and including) |base|:

+// * Exist.

+// * Are owned by a specific user and group.

+// * Are not writable by all users.

+// * 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);

+#endif  // defined(OS_POSIX)

+

+#if defined(OS_MACOSX)

+// Is |path| writable only by a user with administrator privileges?

+// This function uses Mac OS conventions.  The super user is assumed to have

+// uid 0, and the administrator group is assumed to be named "admin".

+// Testing that |path|, and every parent directory including the root of

+// the filesystem, are owned by the superuser, controlled by the group

+// "admin", are not writable by all users, and contain no symbolic links.

+// Will return false if |path| does not exist.

+BASE_EXPORT bool VerifyPathControlledByAdmin(const FilePath& path);

+#endif  // defined(OS_MACOSX)

+

 // A class to handle auto-closing of FILE*'s.

 class ScopedFILEClose {

  public:

diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 9904584..323f5aa 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <fnmatch.h>
+#include <grp.h>
 #include <libgen.h>
 #include <limits.h>
 #include <stdio.h>
@@ -52,19 +53,6 @@ namespace file_util {
 
 namespace {
 
-// Helper for NormalizeFilePath(), defined below.
-bool RealPath(const FilePath& path, FilePath* real_path) {
-  base::ThreadRestrictions::AssertIOAllowed();  // For realpath().
-  FilePath::CharType buf[PATH_MAX];
-  if (!realpath(path.value().c_str(), buf))
-    return false;
-
-  *real_path = FilePath(buf);
-  return true;
-}
-
-}  // namespace
-
 #if defined(OS_OPENBSD) || defined(OS_FREEBSD) || \
     (defined(OS_MACOSX) && \
      MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_5)
@@ -73,14 +61,73 @@ static int CallStat(const char *path, stat_wrapper_t *sb) {
   base::ThreadRestrictions::AssertIOAllowed();
   return stat(path, sb);
 }
+static int CallLstat(const char *path, stat_wrapper_t *sb) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  return lstat(path, sb);
+}
 #else
 typedef struct stat64 stat_wrapper_t;
 static int CallStat(const char *path, stat_wrapper_t *sb) {
   base::ThreadRestrictions::AssertIOAllowed();
   return stat64(path, sb);
 }
+static int CallLstat(const char *path, stat_wrapper_t *sb) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  return lstat64(path, sb);
+}
 #endif
 
+// Helper for NormalizeFilePath(), defined below.
+bool RealPath(const FilePath& path, FilePath* real_path) {
+  base::ThreadRestrictions::AssertIOAllowed();  // For realpath().
+  FilePath::CharType buf[PATH_MAX];
+  if (!realpath(path.value().c_str(), buf))
+    return false;
+
+  *real_path = FilePath(buf);
+  return true;
+}
+
+// Helper for VerifyPathControlledByUser.
+bool VerifySpecificPathControlledByUser(const FilePath& path,
+                                        uid_t owner_uid,
+                                        gid_t group_gid) {
+  stat_wrapper_t stat_info;
+  if (CallLstat(path.value().c_str(), &stat_info) != 0) {
+    PLOG(ERROR) << "Failed to get information on path "
+                << path.value();
+    return false;
+  }
+
+  if (S_ISLNK(stat_info.st_mode)) {
+    LOG(ERROR) << "Path " << path.value()
+               << " is a symbolic link.";
+    return false;
+  }
+
+  if (stat_info.st_uid != owner_uid) {
+    LOG(ERROR) << "Path " << path.value()
+               << " is owned by the wrong user.";
+    return false;
+  }
+
+  if (stat_info.st_gid != group_gid) {
+    LOG(ERROR) << "Path " << path.value()
+               << " is owned by the wrong group.";
+    return false;
+  }
+
+  if (stat_info.st_mode & S_IWOTH) {
+    LOG(ERROR) << "Path " << path.value()
+               << " is writable by any user.";
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
 static std::string TempFileName() {
 #if defined(OS_MACOSX)
   return StringPrintf(".%s.XXXXXX", base::mac::BaseBundleID());
@@ -940,4 +987,65 @@ bool CopyFile(const FilePath& from_path, const FilePath& to_path) {
 }
 #endif  // defined(OS_MACOSX)
 
+bool VerifyPathControlledByUser(const FilePath& base,
+                                const FilePath& path,
+                                uid_t owner_uid,
+                                gid_t group_gid) {
+  if (base != path && !base.IsParent(path)) {
+     LOG(ERROR) << "|base| must be a subdirectory of |path|.  base = \""
+                << base.value() << "\", path = \"" << path.value() << "\"";
+     return false;
+  }
+
+  std::vector<FilePath::StringType> base_components;
+  std::vector<FilePath::StringType> path_components;
+
+  base.GetComponents(&base_components);
+  path.GetComponents(&path_components);
+
+  std::vector<FilePath::StringType>::const_iterator ib, ip;
+  for (ib = base_components.begin(), ip = path_components.begin();
+       ib != base_components.end(); ++ib, ++ip) {
+    // |base| must be a subpath of |path|, so all components should match.
+    // If these CHECKs fail, look at the test that base is a parent of
+    // path at the top of this function.
+    CHECK(ip != path_components.end());
+    CHECK(*ip == *ib);
+  }
+
+  FilePath current_path = base;
+  if (!VerifySpecificPathControlledByUser(current_path, owner_uid, group_gid))
+    return false;
+
+  for (; ip != path_components.end(); ++ip) {
+    current_path = current_path.Append(*ip);
+    if (!VerifySpecificPathControlledByUser(current_path, owner_uid, group_gid))
+      return false;
+  }
+  return true;
+}
+
+#if defined(OS_MACOSX)
+bool VerifyPathControlledByAdmin(const FilePath& path) {
+  const unsigned kRootUid = 0;
+  const FilePath kFileSystemRoot("/");
+
+  // The name of the administrator group on mac os.
+  const char kAdminGroupName[] = "admin";
+
+  // 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;
+  }
+
+  return VerifyPathControlledByUser(
+      kFileSystemRoot, path, kRootUid, group_record->gr_gid);
+}
+#endif  // defined(OS_MACOSX)
+
 }  // namespace file_util
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index 7b12fb8c..6ee4779 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -110,6 +110,27 @@ bool DeleteReparsePoint(HANDLE source) {
 }
 #endif
 
+#if defined(OS_POSIX)
+// Provide a simple way to change the permissions bits on |path| in tests.
+// ASSERT failures will return, but not stop the test.  Caller should wrap
+// calls to this function in ASSERT_NO_FATAL_FAILURE().
+void ChangePosixFilePermissions(const FilePath& path,
+                                mode_t mode_bits_to_set,
+                                mode_t mode_bits_to_clear) {
+  ASSERT_FALSE(mode_bits_to_set & mode_bits_to_clear)
+      << "Can't set and clear the same bits.";
+
+  struct stat stat_buf;
+  ASSERT_EQ(0, stat(path.value().c_str(), &stat_buf));
+
+  mode_t updated_mode_bits = stat_buf.st_mode;
+  updated_mode_bits |= mode_bits_to_set;
+  updated_mode_bits &= ~mode_bits_to_clear;
+
+  ASSERT_EQ(0, chmod(path.value().c_str(), updated_mode_bits));
+}
+#endif  // defined(OS_POSIX)
+
 const wchar_t bogus_content[] = L"I'm cannon fodder.";
 
 const file_util::FileEnumerator::FileType FILES_AND_DIRECTORIES =
@@ -1813,4 +1834,248 @@ TEST_F(FileUtilTest, IsDirectoryEmpty) {
   EXPECT_FALSE(file_util::IsDirectoryEmpty(empty_dir));
 }
 
+#if defined(OS_POSIX)
+
+// Testing VerifyPathControlledByAdmin() is hard, because there is no
+// way a test can make a file owned by root, or change file paths
+// at the root of the file system.  VerifyPathControlledByAdmin()
+// is implemented as a call to VerifyPathControlledByUser, which gives
+// us the ability to test with paths under the test's temp directory,
+// using a user id we control.
+// Pull tests of VerifyPathControlledByUserTest() into a separate test class
+// with a common SetUp() method.
+class VerifyPathControlledByUserTest : public FileUtilTest {
+ protected:
+  virtual void SetUp() {
+    FileUtilTest::SetUp();
+
+    // Create a basic structure used by each test.
+    // base_dir_
+    //  |-> sub_dir_
+    //       |-> text_file_
+
+    base_dir_ = temp_dir_.path().AppendASCII("base_dir");
+    ASSERT_TRUE(file_util::CreateDirectory(base_dir_));
+
+    sub_dir_ = base_dir_.AppendASCII("sub_dir");
+    ASSERT_TRUE(file_util::CreateDirectory(sub_dir_));
+
+    text_file_ = sub_dir_.AppendASCII("file.txt");
+    CreateTextFile(text_file_, L"This text file has some text in it.");
+
+    // Our user and group id.
+    uid_ = getuid();
+    gid_ = getgid();
+
+    // To ensure that umask settings do not cause the initial state
+    // of permissions to be different from what we expect, explicitly
+    // set permissions on the directories we create.
+    // Make all files and directories non-world-writable.
+    mode_t enabled_permissions =
+        S_IRWXU | // User can read, write, traverse
+        S_IRWXG;  // Group can read, write, traverse
+    mode_t disabled_permissions =
+        S_IRWXO;  // Other users can't read, write, traverse.
+
+    ASSERT_NO_FATAL_FAILURE(
+        ChangePosixFilePermissions(
+            base_dir_, enabled_permissions, disabled_permissions));
+    ASSERT_NO_FATAL_FAILURE(
+        ChangePosixFilePermissions(
+            sub_dir_, enabled_permissions, disabled_permissions));
+  }
+
+  FilePath base_dir_;
+  FilePath sub_dir_;
+  FilePath text_file_;
+  uid_t uid_;
+  gid_t gid_;
+};
+
+TEST_F(VerifyPathControlledByUserTest, BadPaths) {
+  // File does not exist.
+  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| not a subpath of |path|.
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(sub_dir_, base_dir_, uid_, gid_));
+
+  // 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_));
+
+  // Finding that a bad call fails proves nothing unless a good call succeeds.
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+}
+
+TEST_F(VerifyPathControlledByUserTest, Symlinks) {
+  // Symlinks in the path should cause failure.
+
+  // Symlink to the file at the end of the path.
+  FilePath file_link =  base_dir_.AppendASCII("file_link");
+  ASSERT_TRUE(file_util::CreateSymbolicLink(text_file_, file_link))
+      << "Failed to create symlink.";
+
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, file_link, uid_, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(file_link, file_link, uid_, gid_));
+
+  // Symlink from one directory to another within the path.
+  FilePath link_to_sub_dir =  base_dir_.AppendASCII("link_to_sub_dir");
+  ASSERT_TRUE(file_util::CreateSymbolicLink(sub_dir_, link_to_sub_dir))
+    << "Failed to create symlink.";
+
+  FilePath file_path_with_link = link_to_sub_dir.AppendASCII("file.txt");
+  ASSERT_TRUE(file_util::PathExists(file_path_with_link));
+
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          base_dir_, file_path_with_link, uid_, gid_));
+
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          link_to_sub_dir, file_path_with_link, uid_, gid_));
+
+  // Symlinks in parents of base path are allowed.
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(
+          file_path_with_link, file_path_with_link, uid_, gid_));
+}
+
+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));
+  ASSERT_NO_FATAL_FAILURE(
+      ChangePosixFilePermissions(sub_dir_, 0u, S_IWOTH));
+  ASSERT_NO_FATAL_FAILURE(
+      ChangePosixFilePermissions(text_file_, 0u, S_IWOTH));
+
+  // We control these paths.
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // Another user does not control these paths.
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          base_dir_, sub_dir_, bad_uid, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          base_dir_, text_file_, bad_uid, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          sub_dir_, text_file_, bad_uid, gid_));
+
+  // Another group does not control the paths.
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          base_dir_, sub_dir_, uid_, bad_gid));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          base_dir_, text_file_, uid_, bad_gid));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(
+          sub_dir_, text_file_, uid_, bad_gid));
+}
+
+TEST_F(VerifyPathControlledByUserTest, WriteBitChecks) {
+  // Make all files and directories non-world-writable.
+  ASSERT_NO_FATAL_FAILURE(
+      ChangePosixFilePermissions(base_dir_, 0u, S_IWOTH));
+  ASSERT_NO_FATAL_FAILURE(
+      ChangePosixFilePermissions(sub_dir_, 0u, S_IWOTH));
+  ASSERT_NO_FATAL_FAILURE(
+      ChangePosixFilePermissions(text_file_, 0u, S_IWOTH));
+
+  // Initialy, we control all parts of the path.
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, sub_dir_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+   // 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_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // 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_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // 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_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // 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_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // 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_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_FALSE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+
+  // 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_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(base_dir_, text_file_, uid_, gid_));
+  EXPECT_TRUE(
+      file_util::VerifyPathControlledByUser(sub_dir_, text_file_, uid_, gid_));
+}
+
+#endif  // defined(OS_POSIX)
+
 }  // namespace
diff --git a/chrome/browser/extensions/external_extension_provider_impl.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
index f288af0..2a61c58 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -222,14 +222,38 @@ void ExternalExtensionProviderImpl::CreateExternalProviders(
     VisitorInterface* service,
     Profile* profile,
     ProviderCollection* provider_list) {
+
+  // On Mac OS, items in /Library/... should be written by the superuser.
+  // Check that all components of the path are writable by root only.
+  ExternalPrefExtensionLoader::Options options;
+#if defined(OS_MACOSX)
+  options = ExternalPrefExtensionLoader::ENSURE_PATH_CONTROLLED_BY_ADMIN;
+#else
+  options = ExternalPrefExtensionLoader::NONE;
+#endif
+
+  provider_list->push_back(
+      linked_ptr<ExternalExtensionProviderInterface>(
+          new ExternalExtensionProviderImpl(
+              service,
+              new ExternalPrefExtensionLoader(
+                  chrome::DIR_EXTERNAL_EXTENSIONS, options),
+              Extension::EXTERNAL_PREF,
+              Extension::EXTERNAL_PREF_DOWNLOAD)));
+
+#if defined(OS_MACOSX)
+  // Support old path to external extensions file as we migrate to the
+  // new one.  See crbug/67203.
   provider_list->push_back(
       linked_ptr<ExternalExtensionProviderInterface>(
           new ExternalExtensionProviderImpl(
               service,
               new ExternalPrefExtensionLoader(
-                  chrome::DIR_EXTERNAL_EXTENSIONS),
+                  chrome::DIR_DEPRICATED_EXTERNAL_EXTENSIONS,
+                  ExternalPrefExtensionLoader::NONE),
               Extension::EXTERNAL_PREF,
               Extension::EXTERNAL_PREF_DOWNLOAD)));
+#endif
 
 #if defined(OS_CHROMEOS)
   // Chrome OS specific source for OEM customization.
diff --git a/chrome/browser/extensions/external_pref_extension_loader.cc b/chrome/browser/extensions/external_pref_extension_loader.cc
index 8d90ee7..29739d0 100644
--- a/chrome/browser/extensions/external_pref_extension_loader.cc
+++ b/chrome/browser/extensions/external_pref_extension_loader.cc
@@ -7,7 +7,9 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/path_service.h"
+#include "chrome/common/chrome_paths.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/json_value_serializer.h"
 
@@ -34,8 +36,10 @@ DictionaryValue* ExtractPrefs(const FilePath& path,
 
 }  // namespace
 
-ExternalPrefExtensionLoader::ExternalPrefExtensionLoader(int base_path_key)
-    : base_path_key_(base_path_key) {
+ExternalPrefExtensionLoader::ExternalPrefExtensionLoader(int base_path_key,
+                                                         Options options)
+    : base_path_key_(base_path_key),
+      options_(options){
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -55,32 +59,72 @@ void ExternalPrefExtensionLoader::StartLoading() {
           &ExternalPrefExtensionLoader::LoadOnFileThread));
 }
 
-void ExternalPrefExtensionLoader::LoadOnFileThread() {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
+DictionaryValue* ExternalPrefExtensionLoader::ReadJsonPrefsFile() {
   // TODO(skerner): Some values of base_path_key_ will cause
   // PathService::Get() to return false, because the path does
   // not exist.  Find and fix the build/install scripts so that
   // this can become a CHECK().  Known examples include chrome
   // OS developer builds and linux install packages.
   // Tracked as crbug.com/70402 .
+  if (!PathService::Get(base_path_key_, &base_path_)) {
+    return NULL;
+  }
+
+  FilePath json_file = base_path_.Append(
+      FILE_PATH_LITERAL("external_extensions.json"));
 
-  scoped_ptr<DictionaryValue> prefs;
-  if (PathService::Get(base_path_key_, &base_path_)) {
-    FilePath json_file;
-    json_file =
-        base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
+  if (!file_util::PathExists(json_file)) {
+    // This is not an error.  The file does not exist by default.
+    return NULL;
+  }
 
-    if (file_util::PathExists(json_file)) {
-      JSONFileValueSerializer serializer(json_file);
-      prefs.reset(ExtractPrefs(json_file, &serializer));
+  if (IsOptionSet(ENSURE_PATH_CONTROLLED_BY_ADMIN)) {
+#if defined(OS_MACOSX)
+    if (!file_util::VerifyPathControlledByAdmin(json_file)) {
+      LOG(ERROR) << "Can not read external extensions source.  The file "
+                 << json_file.value() << " and every directory in its path, "
+                 << "must be owned by root, have group \"admin\", and not be "
+                 << "writable by all users. These restrictions prevent "
+                 << "unprivleged users from making chrome install extensions "
+                 << "on other users' accounts.";
+      return NULL;
     }
+#else
+    // The only platform that uses this check is Mac OS.  If you add one,
+    // you need to implement file_util::VerifyPathControlledByAdmin() for
+    // that platform.
+    NOTREACHED();
+#endif  // defined(OS_MACOSX)
   }
 
-  if (!prefs.get())
-    prefs.reset(new DictionaryValue());
+  JSONFileValueSerializer serializer(json_file);
+  return ExtractPrefs(json_file, &serializer);
+}
+
+void ExternalPrefExtensionLoader::LoadOnFileThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  prefs_.reset(prefs.release());
+  prefs_.reset(ReadJsonPrefsFile());
+  if (!prefs_.get())
+    prefs_.reset(new DictionaryValue());
+
+  // We want to deprecate the external extensions file inside the app
+  // bundle on mac os.  Use a histogram to see how many extensions
+  // are installed using the deprecated path, and how many are installed
+  // from the supported path.  We can use this data to measure the
+  // effectiveness of asking developers to use the new path, or any
+  // automatic migration methods we implement.
+#if defined(OS_MACOSX)
+  // The deprecated path only exists on mac for now.
+  if (base_path_key_ == chrome::DIR_DEPRICATED_EXTERNAL_EXTENSIONS) {
+    UMA_HISTOGRAM_COUNTS_100("Extensions.DepricatedExternalJsonCount",
+                             prefs_->size());
+  }
+#endif  // defined(OS_MACOSX)
+  if (base_path_key_ == chrome::DIR_EXTERNAL_EXTENSIONS) {
+    UMA_HISTOGRAM_COUNTS_100("Extensions.ExternalJsonCount",
+                             prefs_->size());
+  }
 
   // If we have any records to process, then we must have
   // read the .json file.  If we read the .json file, then
diff --git a/chrome/browser/extensions/external_pref_extension_loader.h b/chrome/browser/extensions/external_pref_extension_loader.h
index 8473635..4d48755 100644
--- a/chrome/browser/extensions/external_pref_extension_loader.h
+++ b/chrome/browser/extensions/external_pref_extension_loader.h
@@ -20,24 +20,38 @@
 // thread and they are expecting public method calls from the UI thread.
 class ExternalPrefExtensionLoader : public ExternalExtensionLoader {
  public:
+  enum Options {
+    NONE = 0,
+
+    // Ensure that only root can force an external install by checking
+    // that all components of the path to external extensions files are
+    // owned by root and not writable by any non-root user.
+    ENSURE_PATH_CONTROLLED_BY_ADMIN = 1 << 0
+  };
+
   // |base_path_key| is the directory containing the external_extensions.json
   // file.  Relative file paths to extension files are resolved relative
   // to this path.
-  explicit ExternalPrefExtensionLoader(int base_path_key);
+  explicit ExternalPrefExtensionLoader(int base_path_key, Options options);
 
   virtual const FilePath GetBaseCrxFilePath() OVERRIDE;
 
  protected:
   virtual void StartLoading() OVERRIDE;
+  bool IsOptionSet(Options option) {
+    return (options_ & option) != 0;
+  }
 
  private:
   friend class base::RefCountedThreadSafe<ExternalExtensionLoader>;
 
   virtual ~ExternalPrefExtensionLoader() {}
 
+  DictionaryValue* ReadJsonPrefsFile();
   void LoadOnFileThread();
 
   int base_path_key_;
+  Options options_;
   FilePath base_path_;
 
   DISALLOW_COPY_AND_ASSIGN(ExternalPrefExtensionLoader);
diff --git a/chrome/common/chrome_paths.cc b/chrome/common/chrome_paths.cc
index db1f2b5..3b3260f 100644
--- a/chrome/common/chrome_paths.cc
+++ b/chrome/common/chrome_paths.cc
@@ -334,16 +334,12 @@ bool PathProvider(int key, FilePath* result) {
 #endif
     case chrome::DIR_EXTERNAL_EXTENSIONS:
 #if defined(OS_MACOSX)
-      if (!PathService::Get(base::DIR_EXE, &cur))
+      if (!chrome::GetGlobalApplicationSupportDirectory(&cur))
         return false;
 
-      // On Mac, built-in extensions are in Contents/Extensions, a sibling of
-      // the App dir. If there are none, it may not exist.
-      // TODO(skerner): Reading external extensions from a file inside the
-      // app budle causes several problems.  Change this path to be outside
-      // the app bundle.  crbug/67203
-      cur = cur.DirName();
-      cur = cur.Append(FILE_PATH_LITERAL("Extensions"));
+      cur = cur.Append(FILE_PATH_LITERAL("Google"))
+               .Append(FILE_PATH_LITERAL("Chrome"))
+               .Append(FILE_PATH_LITERAL("External Extensions"));
       create_dir = false;
 #else
       if (!PathService::Get(base::DIR_MODULE, &cur))
@@ -353,6 +349,22 @@ bool PathProvider(int key, FilePath* result) {
       create_dir = true;
 #endif
       break;
+
+#if defined(OS_MACOSX)
+    case DIR_DEPRICATED_EXTERNAL_EXTENSIONS:
+      // TODO(skerner): Reading external extensions from a file inside the
+      // app budle causes several problems.  Once users have a chance to
+      // migrate, remove this path.  crbug/67203
+      if (!PathService::Get(base::DIR_EXE, &cur))
+        return false;
+
+      cur = cur.DirName();
+      cur = cur.Append(FILE_PATH_LITERAL("Extensions"));
+      create_dir = false;
+
+      break;
+#endif
+
     case chrome::DIR_DEFAULT_APPS:
 #if defined(OS_MACOSX)
       cur = base::mac::MainAppBundlePath();
@@ -363,6 +375,7 @@ bool PathProvider(int key, FilePath* result) {
       cur = cur.Append(FILE_PATH_LITERAL("default_apps"));
 #endif
       break;
+
     default:
       return false;
   }
diff --git a/chrome/common/chrome_paths.h b/chrome/common/chrome_paths.h
index 78ca6b1..2462bf6 100644
--- a/chrome/common/chrome_paths.h
+++ b/chrome/common/chrome_paths.h
@@ -57,6 +57,13 @@ enum {
 #endif
 
   DIR_EXTERNAL_EXTENSIONS,      // Directory where installer places .crx files.
+
+#if defined(OS_MACOSX)
+  DIR_DEPRICATED_EXTERNAL_EXTENSIONS,  // Former home of external extensions.
+                                       // We read from the old path for now,
+                                       // to give users time to migrate.
+#endif
+
   DIR_DEFAULT_APPS,             // Directory where installer places .crx files
                                 // to be installed when chrome is first run.
   FILE_RESOURCE_MODULE,         // Full path and filename of the module that
diff --git a/chrome/common/chrome_paths_internal.h b/chrome/common/chrome_paths_internal.h
index baf5777..23c3ec4 100644
--- a/chrome/common/chrome_paths_internal.h
+++ b/chrome/common/chrome_paths_internal.h
@@ -76,6 +76,9 @@ FilePath GetFrameworkBundlePath();
 // Get the local library directory.
 bool GetLocalLibraryDirectory(FilePath* result);
 
+// Get the global Application Support directory (under /Library/).
+bool GetGlobalApplicationSupportDirectory(FilePath* result);
+
 // Returns the NSBundle for the outer browser application, even when running
 // inside the helper. In unbundled applications, such as tests, returns nil.
 NSBundle* OuterAppBundle();
diff --git a/chrome/common/chrome_paths_mac.mm b/chrome/common/chrome_paths_mac.mm
index 7331d78..9df7cd5 100644
--- a/chrome/common/chrome_paths_mac.mm
+++ b/chrome/common/chrome_paths_mac.mm
@@ -105,6 +105,10 @@ bool GetUserDocumentsDirectory(FilePath* result) {
   return base::mac::GetUserDirectory(NSDocumentDirectory, result);
 }
 
+bool GetGlobalApplicationSupportDirectory(FilePath* result) {
+  return base::mac::GetLocalDirectory(NSApplicationSupportDirectory, result);
+}
+
 void GetUserCacheDirectory(const FilePath& profile_dir, FilePath* result) {
   // If the profile directory is under ~/Library/Application Support,
   // use a suitable cache directory under ~/Library/Caches.  For