Revert 51231 - Loosen permission on extension temp dir when a flag is used.

Issue 35198 can not be reproduced locally.  To enable users to do experiments, three command line flags are added to chrome:

--issue35198-crxdir-browser: Have the browser process create the directory in which the extension will be unzipped.

--issue35198-logging: Enable log messages from directory creation in the utility process to be moved to the browser process.

--issue35198-permission: Use the most permissive file permissions possible on the extension unpack directory.

BUG=35198
TEST=manual

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

TBR=skerner@chromium.org
Review URL: http://codereview.chromium.org/2861039

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

12 files changed, 18 insertions, 143 deletions

diff --git a/base/file_util.h b/base/file_util.h
index f33e78b..48f431f 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -267,8 +267,7 @@ bool CreateTemporaryFileInDir(const FilePath& dir,
 // new directory does not have the same name as an existing directory.
 bool CreateTemporaryDirInDir(const FilePath& base_dir,
                              const FilePath::StringType& prefix,
-                             FilePath* new_dir,
-                             bool lossen_permissions);
+                             FilePath* new_dir);
 
 // Create a new directory under TempPath. If prefix is provided, the new
 // directory name is in the format of prefixyyyy.
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index e868a41..9d738da 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -428,15 +428,7 @@ static bool CreateTemporaryDirInDirImpl(const FilePath& base_dir,
 
 bool CreateTemporaryDirInDir(const FilePath& base_dir,
                              const FilePath::StringType& prefix,
-                             FilePath* new_dir,
-                             bool loosen_permissions) {
-  // To understand crbug/35198, the ability to call this
-  // this function on windows while giving loose permissions
-  // to the resulting directory has been temporarily added.
-  // It should not be posible to call this function with
-  // loosen_permissions == true on non-windows plarforms.
-  CHECK(!loosen_permissions);
-
+                             FilePath* new_dir) {
   FilePath::StringType mkdtemp_template = prefix;
   mkdtemp_template.append(FILE_PATH_LITERAL("XXXXXX"));
   return CreateTemporaryDirInDirImpl(base_dir, mkdtemp_template, new_dir);
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index b0a1a8e..39e5398 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -1569,8 +1569,7 @@ TEST_F(FileUtilTest, CreateNewTemporaryDirInDirTest) {
   ASSERT_TRUE(file_util::CreateTemporaryDirInDir(
                   test_dir_,
                   FILE_PATH_LITERAL("CreateNewTemporaryDirInDirTest"),
-                  &new_dir,
-                  false));
+                  &new_dir));
   EXPECT_TRUE(file_util::PathExists(new_dir));
   EXPECT_TRUE(test_dir_.IsParent(new_dir));
   EXPECT_TRUE(file_util::Delete(new_dir, false));
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index 74db90cd..8a15370 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -63,56 +63,6 @@ bool DevicePathToDriveLetterPath(const FilePath& device_path,
   return true;
 }
 
-// Build a security descriptor with the weakest possible file permissions.
-bool InitLooseSecurityDescriptor(SECURITY_ATTRIBUTES *sa,
-                                 SECURITY_DESCRIPTOR *sd) {
-  DWORD last_error;
-
-  if (!InitializeSecurityDescriptor(sd, SECURITY_DESCRIPTOR_REVISION)) {
-    last_error = GetLastError();
-    LOG(ERROR) << "InitializeSecurityDescriptor failed: GetLastError() = "
-               << last_error;
-    return false;
-  }
-
-  if (!SetSecurityDescriptorDacl(sd,
-                                 TRUE,  // bDaclPresent: Add one to |sd|.
-                                 NULL,  // pDacl: NULL means allow all access.
-                                 FALSE  // bDaclDefaulted: Not defaulted.
-                                 )) {
-    last_error = GetLastError();
-    LOG(ERROR) << "SetSecurityDescriptorDacl() failed: GetLastError() = "
-               << last_error;
-    return false;
-  }
-
-  if (!SetSecurityDescriptorGroup(sd,
-                                  NULL,  // pGroup: No no primary group.
-                                  FALSE  // bGroupDefaulted: Not defaulted.
-                                  )) {
-    last_error = GetLastError();
-    LOG(ERROR) << "SetSecurityDescriptorGroup() failed: GetLastError() = "
-               << last_error;
-    return false;
-  }
-
-  if (!SetSecurityDescriptorSacl(sd,
-                                 FALSE,  // bSaclPresent: No SACL.
-                                 NULL,
-                                 FALSE
-                                 )) {
-    last_error = GetLastError();
-    LOG(ERROR) << "SetSecurityDescriptorSacl() failed: GetLastError() = "
-               << last_error;
-    return false;
-  }
-
-  sa->nLength = sizeof(SECURITY_ATTRIBUTES);
-  sa->lpSecurityDescriptor = sd;
-  sa->bInheritHandle = TRUE;
-  return true;
-}
-
 }  // namespace
 
 std::wstring GetDirectoryFromPath(const std::wstring& path) {
@@ -600,19 +550,7 @@ bool CreateTemporaryFileInDir(const FilePath& dir,
 
 bool CreateTemporaryDirInDir(const FilePath& base_dir,
                              const FilePath::StringType& prefix,
-                             FilePath* new_dir,
-                             bool loosen_permissions) {
-  SECURITY_ATTRIBUTES sa;
-  SECURITY_DESCRIPTOR sd;
-
-  LPSECURITY_ATTRIBUTES directory_security_attributes = NULL;
-  if (loosen_permissions) {
-    if (InitLooseSecurityDescriptor(&sa, &sd))
-      directory_security_attributes = &sa;
-    else
-      LOG(ERROR) << "Failed to init security attributes, fall back to NULL.";
-  }
-
+                             FilePath* new_dir) {
   FilePath path_to_create;
   srand(static_cast<uint32>(time(NULL)));
 
@@ -627,8 +565,7 @@ bool CreateTemporaryDirInDir(const FilePath& base_dir,
     new_dir_name.append(IntToWString(rand() % kint16max));
 
     path_to_create = path_to_create.Append(new_dir_name);
-    if (::CreateDirectory(path_to_create.value().c_str(),
-                          directory_security_attributes))
+    if (::CreateDirectory(path_to_create.value().c_str(), NULL))
       break;
     count++;
   }
@@ -638,7 +575,6 @@ bool CreateTemporaryDirInDir(const FilePath& base_dir,
   }
 
   *new_dir = path_to_create;
-
   return true;
 }
 
@@ -648,10 +584,7 @@ bool CreateNewTempDirectory(const FilePath::StringType& prefix,
   if (!GetTempDir(&system_temp_dir))
     return false;
 
-  return CreateTemporaryDirInDir(system_temp_dir,
-                                 prefix,
-                                 new_temp_path,
-                                 false);
+  return CreateTemporaryDirInDir(system_temp_dir, prefix, new_temp_path);
 }
 
 bool CreateDirectory(const FilePath& full_path) {
diff --git a/base/scoped_temp_dir.cc b/base/scoped_temp_dir.cc
index c3fb00a..c8ed9c6 100644
--- a/base/scoped_temp_dir.cc
+++ b/base/scoped_temp_dir.cc
@@ -4,7 +4,6 @@
 
 #include "base/scoped_temp_dir.h"
 
-#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_util.h"
@@ -27,20 +26,16 @@ bool ScopedTempDir::CreateUniqueTempDir() {
   return true;
 }
 
-bool ScopedTempDir::CreateUniqueTempDirUnderPath(const FilePath& base_path,
-                                                 bool loose_permissions) {
+bool ScopedTempDir::CreateUniqueTempDirUnderPath(const FilePath& base_path) {
   // If |path| does not exist, create it.
-  if (!file_util::CreateDirectory(base_path)) {
-    LOG(ERROR) << "Failed to create base directory " << base_path.value();
+  if (!file_util::CreateDirectory(base_path))
     return false;
-  }
 
-  // Create a new, uniquely named directory under |base_path|.
+  // Create a new, uniquly named directory under |base_path|.
   if (!file_util::CreateTemporaryDirInDir(
           base_path,
           FILE_PATH_LITERAL("scoped_dir_"),
-          &path_,
-          loose_permissions)) {
+          &path_)) {
     return false;
   }
   return true;
diff --git a/base/scoped_temp_dir.h b/base/scoped_temp_dir.h
index a0708dc3..a5dca1e 100644
--- a/base/scoped_temp_dir.h
+++ b/base/scoped_temp_dir.h
@@ -26,8 +26,7 @@ class ScopedTempDir {
   bool CreateUniqueTempDir();
 
   // Creates a unique directory under a given path, and takes ownership of it.
-  bool CreateUniqueTempDirUnderPath(const FilePath& path,
-                                    bool loose_permissions);
+  bool CreateUniqueTempDirUnderPath(const FilePath& path);
 
   // Takes ownership of directory at |path|, creating it if necessary.
   // Don't call multiple times unless Take() has been called first.
diff --git a/base/scoped_temp_dir_unittest.cc b/base/scoped_temp_dir_unittest.cc
index e180119..4be0d07 100644
--- a/base/scoped_temp_dir_unittest.cc
+++ b/base/scoped_temp_dir_unittest.cc
@@ -65,7 +65,7 @@ TEST(ScopedTempDir, UniqueTempDirUnderPath) {
   FilePath test_path;
   {
     ScopedTempDir dir;
-    EXPECT_TRUE(dir.CreateUniqueTempDirUnderPath(base_path, false));
+    EXPECT_TRUE(dir.CreateUniqueTempDirUnderPath(base_path));
     test_path = dir.path();
     EXPECT_TRUE(file_util::DirectoryExists(test_path));
     EXPECT_TRUE(base_path.IsParent(test_path));
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
index c087649..eda5e3f 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
@@ -42,18 +42,8 @@ void SandboxedExtensionUnpacker::Start() {
   // file IO on.
   CHECK(ChromeThread::GetCurrentThreadIdentifier(&thread_identifier_));
 
-  // To understand crbug/35198, allow users who can reproduce the bug
-  // to loosen permissions on the scoped directory.
-  bool loosen_permissions = false;
-#if defined (OS_WIN)
-  loosen_permissions = CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kIssue35198Permission);
-  LOG(INFO) << "loosen_permissions = " << loosen_permissions;
-#endif
-
   // Create a temporary directory to work in.
-  if (!temp_dir_.CreateUniqueTempDirUnderPath(temp_path_,
-                                              loosen_permissions)) {
+  if (!temp_dir_.CreateUniqueTempDirUnderPath(temp_path_)) {
     ReportFailure("Could not create temporary directory.");
     return;
   }
@@ -62,15 +52,6 @@ void SandboxedExtensionUnpacker::Start() {
   extension_root_ = temp_dir_.path().AppendASCII(
       extension_filenames::kTempExtensionName);
 
-  // To understand crbug/35198, allow users who can reproduce the bug to
-  // create the unpack directory in the browser process.
-  bool crxdir_in_browser = CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kIssue35198CrxDirBrowser);
-  LOG(INFO) << "crxdir_in_browser = " << crxdir_in_browser;
-  if (crxdir_in_browser && !file_util::CreateDirectory(extension_root_)) {
-    LOG(ERROR) << "Failed to create directory " << extension_root_.value();
-  }
-
   // Extract the public key and validate the package.
   if (!ValidateSignature())
     return;  // ValidateSignature() already reported the error.
@@ -88,6 +69,9 @@ void SandboxedExtensionUnpacker::Start() {
   // the link will cause file system access outside the sandbox path.
   FilePath normalized_crx_path;
   if (!file_util::NormalizeFilePath(temp_crx_path, &normalized_crx_path)) {
+    // TODO(skerner): Remove this logging once crbug/13044 is fixed.
+    // This bug is starred by many users who have some kind of link.
+    // If NormalizeFilePath() fails we want to see it in the logs they send.
     LOG(ERROR) << "Could not get the normalized path of "
                << temp_crx_path.value();
     normalized_crx_path = temp_crx_path;
diff --git a/chrome/browser/utility_process_host.cc b/chrome/browser/utility_process_host.cc
index 6dbf3ba..be5397e 100644
--- a/chrome/browser/utility_process_host.cc
+++ b/chrome/browser/utility_process_host.cc
@@ -100,9 +100,6 @@ bool UtilityProcessHost::StartProcess(const FilePath& exposed_dir) {
     cmd_line->AppendSwitch(switches::kEnableExperimentalExtensionApis);
   }
 
-  if (browser_command_line.HasSwitch(switches::kIssue35198ExtraLogging))
-    cmd_line->AppendSwitch(switches::kIssue35198ExtraLogging);
-
 #if defined(OS_POSIX)
   // TODO(port): Sandbox this on Linux.  Also, zygote this to work with
   // Linux updating.
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index cb1ec25..e77bee5 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -528,13 +528,6 @@ const char kInternalNaCl[]                  = "internal-nacl";
 // Runs a trusted Pepper plugin inside the renderer process.
 const char kInternalPepper[]                = "internal-pepper";
 
-// The following flags allow users who can reproduce crbug/35198
-// to enable extra logging and behaviors.  They will be removed once
-// the issue is fixed.
-const char kIssue35198CrxDirBrowser[]       = "issue35198-crxdir-browser";
-const char kIssue35198ExtraLogging[]        = "issue35198-logging";
-const char kIssue35198Permission[]          = "issue35198-permission";
-
 // Specifies the flags passed to JS engine
 const char kJavaScriptFlags[]               = "js-flags";
 
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 8f25ebd..2f43e5c 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -160,9 +160,6 @@ extern const char kInstallerTestClean[];
 extern const char kInstallerTestForce[];
 extern const char kInternalNaCl[];
 extern const char kInternalPepper[];
-extern const char kIssue35198CrxDirBrowser[];
-extern const char kIssue35198ExtraLogging[];
-extern const char kIssue35198Permission[];
 extern const char kJavaScriptFlags[];
 extern const char kLoadExtension[];
 extern const char kLoadPlugin[];
diff --git a/chrome/common/extensions/extension_unpacker.cc b/chrome/common/extensions/extension_unpacker.cc
index 64ff857..599aa11 100644
--- a/chrome/common/extensions/extension_unpacker.cc
+++ b/chrome/common/extensions/extension_unpacker.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/common/extensions/extension_unpacker.h"
 
-#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/scoped_handle.h"
 #include "base/scoped_temp_dir.h"
@@ -12,7 +11,6 @@
 #include "base/thread.h"
 #include "base/values.h"
 #include "net/base/file_stream.h"
-#include "chrome/common/chrome_switches.h"
 #include "chrome/common/common_param_traits.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -144,23 +142,12 @@ bool ExtensionUnpacker::Run() {
     extension_path_.DirName().AppendASCII(filenames::kTempExtensionName);
 
 #if defined(OS_WIN)
-  // To understand crbug/35198, allow users who can reproduce the issue
-  // to enable extra logging while unpacking.
-  bool extra_logging = CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kIssue35198ExtraLogging);
-  LOG(INFO) << "Extra logging for issue 35198: " << extra_logging;
-
   std::ostringstream log_stream;
   std::string dir_string = WideToUTF8(temp_install_dir_.value());
   log_stream << kCouldNotCreateDirectoryError << dir_string << std::endl;
-
   if (!file_util::CreateDirectoryExtraLogging(temp_install_dir_, log_stream)) {
-    if (extra_logging) {
-      log_stream.flush();
-      SetError(log_stream.str());
-    } else {
-      SetError(kCouldNotCreateDirectoryError + dir_string);
-    }
+    log_stream.flush();
+    SetError(log_stream.str());
     return false;
   }
 #else