Have the browser process rewrite manifest.json and theme/page action images

that the extension unpacker process parsed.

BUG=11680
Review URL: http://codereview.chromium.org/115595

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

5 files changed, 123 insertions, 42 deletions

diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 04ccd13..e62f537 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/extensions_service.h"
 
 #include "base/file_util.h"
+#include "base/gfx/png_encoder.h"
 #include "base/scoped_handle.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_util.h"
@@ -29,6 +30,7 @@
 #include "chrome/common/pref_service.h"
 #include "chrome/common/unzip.h"
 #include "chrome/common/url_constants.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 #if defined(OS_WIN)
 #include "base/registry.h"
@@ -129,35 +131,42 @@ class ExtensionsServiceBackend::UnpackerClient
       // Cheesy... but if we don't have a ResourceDispatcherHost, assume we're
       // in a unit test and run the unpacker directly in-process.
       ExtensionUnpacker unpacker(temp_extension_path_);
-      bool success = unpacker.Run();
-      OnUnpackExtensionReply(success, unpacker.error_message());
+      if (unpacker.Run()) {
+        OnUnpackExtensionSucceeded(*unpacker.parsed_manifest(),
+                                   unpacker.decoded_images());
+      } else {
+        OnUnpackExtensionFailed(unpacker.error_message());
+      }
     }
   }
 
  private:
   // UtilityProcessHost::Client
   virtual void OnProcessCrashed() {
-    OnUnpackExtensionReply(false, "Chrome crashed while trying to install");
+    OnUnpackExtensionFailed("Chrome crashed while trying to install");
   }
 
-  virtual void OnUnpackExtensionReply(bool success,
-                                      const std::string& error_message) {
-    if (success) {
-      // The extension was unpacked to the temp dir inside our unpacking dir.
-      FilePath extension_dir = temp_extension_path_.DirName().AppendASCII(
-          ExtensionsServiceBackend::kTempExtensionName);
-      backend_->OnExtensionUnpacked(extension_path_, extension_dir,
-                                    expected_id_, from_external_);
-    } else {
-      backend_->ReportExtensionInstallError(extension_path_, error_message);
-    }
+  virtual void OnUnpackExtensionSucceeded(
+      const DictionaryValue& manifest,
+      const std::vector< Tuple2<SkBitmap, FilePath> >& images) {
+    // The extension was unpacked to the temp dir inside our unpacking dir.
+    FilePath extension_dir = temp_extension_path_.DirName().AppendASCII(
+        ExtensionsServiceBackend::kTempExtensionName);
+    backend_->OnExtensionUnpacked(extension_path_, extension_dir,
+                                  expected_id_, from_external_,
+                                  manifest, images);
+    Cleanup();
+  }
+
+  virtual void OnUnpackExtensionFailed(const std::string& error_message) {
+    backend_->ReportExtensionInstallError(extension_path_, error_message);
     Cleanup();
-    Release();  // balanced in Run()
   }
 
   // Cleans up our temp directory.
   void Cleanup() {
     file_util::Delete(temp_extension_path_.DirName(), true);
+    Release();  // balanced in Run()
   }
 
   // Starts the utility process that unpacks our extension.
@@ -812,19 +821,12 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
     const FilePath& extension_path,
     const FilePath& temp_extension_dir,
     const std::string expected_id,
-    bool from_external) {
-  // TODO(mpcomplete): the utility process should pass up a parsed manifest that
-  // we rewrite in the browser.
-  // Bug http://code.google.com/p/chromium/issues/detail?id=11680
-  scoped_ptr<DictionaryValue> manifest(ReadManifest(extension_path));
-  if (!manifest.get()) {
-    // ReadManifest has already reported the extension error.
-    return;
-  }
-
+    bool from_external,
+    const DictionaryValue& manifest,
+    const std::vector< Tuple2<SkBitmap, FilePath> >& images) {
   Extension extension;
   std::string error;
-  if (!extension.InitFromValue(*manifest,
+  if (!extension.InitFromValue(manifest,
                                true,  // require ID
                                &error)) {
     ReportExtensionInstallError(extension_path, "Invalid extension manifest.");
@@ -849,16 +851,61 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
     was_update = true;
   }
 
+  // Write our parsed manifest back to disk, to ensure it doesn't contain an
+  // exploitable bug that can be used to compromise the browser.
+  std::string manifest_json;
+  JSONStringValueSerializer serializer(&manifest_json);
+  serializer.set_pretty_print(true);
+  if (!serializer.Serialize(manifest)) {
+    ReportExtensionInstallError(extension_path,
+                                "Error serializing manifest.json.");
+    return;
+  }
+
+  FilePath manifest_path =
+      temp_extension_dir.AppendASCII(Extension::kManifestFilename);
+  if (!file_util::WriteFile(manifest_path,
+                            manifest_json.data(), manifest_json.size())) {
+    ReportExtensionInstallError(extension_path, "Error saving manifest.json.");
+    return;
+  }
+
+  // Write our parsed images back to disk as well.
+  for (size_t i = 0; i < images.size(); ++i) {
+    const SkBitmap& image = images[i].a;
+    FilePath path = temp_extension_dir.Append(images[i].b);
+
+    std::vector<unsigned char> image_data;
+    // TODO(mpcomplete): It's lame that we're encoding all images as PNG, even
+    // though they may originally be .jpg, etc.  Figure something out.
+    // http://code.google.com/p/chromium/issues/detail?id=12459
+    if (!PNGEncoder::EncodeBGRASkBitmap(image, false, &image_data)) {
+      ReportExtensionInstallError(extension_path,
+                                  "Error re-encoding theme image.");
+      return;
+    }
+
+    // Note: we're overwriting existing files that the utility process wrote,
+    // so we can be sure the directory exists.
+    const char* image_data_ptr = reinterpret_cast<const char*>(&image_data[0]);
+    if (!file_util::WriteFile(path, image_data_ptr, image_data.size())) {
+      ReportExtensionInstallError(extension_path, "Error saving theme image.");
+      return;
+    }
+  }
+
   // <profile>/Extensions/<dir_name>/<version>
   FilePath version_dir = dest_dir.AppendASCII(version);
+
+  // If anything fails after this, we want to delete the extension dir.
+  ScopedTempDir scoped_version_dir;
+  scoped_version_dir.Set(version_dir);
+
   if (!InstallDirSafely(temp_extension_dir, version_dir))
     return;
 
-  if (!SetCurrentVersion(dest_dir, version)) {
-    if (!file_util::Delete(version_dir, true))
-      LOG(WARNING) << "Can't remove " << dest_dir.value();
+  if (!SetCurrentVersion(dest_dir, version))
     return;
-  }
 
   // To mark that this extension was installed from an external source, create a
   // zero-length file.  At load time, this is used to indicate that the
@@ -872,7 +919,7 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
   // Load the extension immediately and then report installation success. We
   // don't load extensions for external installs because external installation
   // occurs before the normal startup so we just let startup pick them up. We
-  // don't notify installation because there is no UI or external install so
+  // don't notify installation because there is no UI for external install so
   // there is nobody to notify.
   if (!from_external) {
     Extension* extension = LoadExtension(version_dir, true);  // require id
@@ -890,6 +937,8 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
     // Hand off ownership of the loaded extensions to the frontend.
     ReportExtensionsLoaded(extensions.release());
   }
+
+  scoped_version_dir.Take();
 }
 
 void ExtensionsServiceBackend::ReportExtensionInstallError(
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index 47abcbd..56bebee 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -22,6 +22,7 @@ class GURL;
 class PrefService;
 class Profile;
 class ResourceDispatcherHost;
+class SkBitmap;
 class SiteInstance;
 class UserScriptMaster;
 typedef std::vector<Extension*> ExtensionList;
@@ -171,10 +172,15 @@ class ExtensionsServiceBackend
   // Finish installing an extension after it has been unpacked to
   // |temp_extension_dir| by our utility process.  If |expected_id| is not
   // empty, it's verified against the extension's manifest before installation.
-  void OnExtensionUnpacked(const FilePath& extension_path,
-                           const FilePath& temp_extension_dir,
-                           const std::string expected_id,
-                           bool from_external);
+  // |manifest| and |images| are parsed information from the extension that
+  // we want to write to disk in the browser process.
+  void OnExtensionUnpacked(
+      const FilePath& extension_path,
+      const FilePath& temp_extension_dir,
+      const std::string expected_id,
+      bool from_external,
+      const DictionaryValue& manifest,
+      const std::vector< Tuple2<SkBitmap, FilePath> >& images);
 
   // Notify the frontend that there was an error loading an extension.
   void ReportExtensionLoadError(const FilePath& extension_path,
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 8471fbe..7b0a88f 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -77,6 +77,7 @@ class ExtensionsServiceTest
     profile_.reset(new TestingProfile());
     service_ = new ExtensionsService(profile_.get(), &loop_, &loop_,
                                      registry_path_);
+    total_successes_ = 0;
   }
 
   static void SetUpTestCase() {
@@ -126,10 +127,13 @@ class ExtensionsServiceTest
     loop_.RunAllPending();
     std::vector<std::string> errors = GetErrors();
     if (should_succeed) {
+      ++total_successes_;
+
       EXPECT_TRUE(installed_) << path.value();
       EXPECT_EQ(1u, loaded_.size()) << path.value();
       EXPECT_EQ(0u, errors.size()) << path.value();
-      EXPECT_EQ(1u, service_->extensions()->size()) << path.value();
+      EXPECT_EQ(total_successes_, service_->extensions()->size()) <<
+          path.value();
       EXPECT_TRUE(service_->GetExtensionByID(loaded_[0]->id())) << path.value();
       for (std::vector<std::string>::iterator err = errors.begin();
         err != errors.end(); ++err) {
@@ -148,6 +152,7 @@ class ExtensionsServiceTest
  protected:
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<ExtensionsService> service_;
+  size_t total_successes_;
   MessageLoop loop_;
   std::vector<Extension*> loaded_;
   std::string unloaded_id_;
@@ -303,6 +308,14 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   TestInstallExtension(path, true);
   // TODO(erikkay): verify the contents of the installed extension.
 
+  // An extension with theme images.
+  path = extensions_path.AppendASCII("theme.crx");
+  TestInstallExtension(path, true);
+
+  // An extension with page actions.
+  path = extensions_path.AppendASCII("page_action.crx");
+  TestInstallExtension(path, true);
+
   // 0-length extension file.
   path = extensions_path.AppendASCII("not_an_extension.crx");
   TestInstallExtension(path, false);
@@ -369,6 +382,7 @@ TEST_F(ExtensionsServiceTest, UninstallExtension) {
 
   // Uninstall it.
   service_->UninstallExtension(extension_id);
+  total_successes_ = 0;
 
   // We should get an unload notification.
   ASSERT_TRUE(unloaded_id_.length());
diff --git a/chrome/browser/utility_process_host.cc b/chrome/browser/utility_process_host.cc
index 20b34d5..4c3d83b 100644
--- a/chrome/browser/utility_process_host.cc
+++ b/chrome/browser/utility_process_host.cc
@@ -81,7 +81,9 @@ void UtilityProcessHost::OnChannelError() {
 void UtilityProcessHost::Client::OnMessageReceived(
     const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(UtilityProcessHost, message)
-    IPC_MESSAGE_HANDLER(UtilityHostMsg_UnpackExtension_Reply,
-                        Client::OnUnpackExtensionReply)
+    IPC_MESSAGE_HANDLER(UtilityHostMsg_UnpackExtension_Succeeded,
+                        Client::OnUnpackExtensionSucceeded)
+    IPC_MESSAGE_HANDLER(UtilityHostMsg_UnpackExtension_Failed,
+                        Client::OnUnpackExtensionFailed)
   IPC_END_MESSAGE_MAP_EX()
 }
diff --git a/chrome/browser/utility_process_host.h b/chrome/browser/utility_process_host.h
index 8cb281e..0efaa56 100644
--- a/chrome/browser/utility_process_host.h
+++ b/chrome/browser/utility_process_host.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UTILITY_PROCESS_HOST_H_
 
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
@@ -14,7 +15,9 @@
 #include "chrome/common/ipc_channel.h"
 
 class CommandLine;
+class DictionaryValue;
 class MessageLoop;
+class SkBitmap;
 
 // This class acts as the browser-side host to a utility child process.  A
 // utility process is a short-lived sandboxed process that is created to run
@@ -32,10 +35,17 @@ class UtilityProcessHost : public ChildProcessHost {
     // Called when the process has crashed.
     virtual void OnProcessCrashed() {}
 
-    // Called when the process sends a reply to an UnpackExtension message.
-    // If success if false, error_message contains a description of the problem.
-    virtual void OnUnpackExtensionReply(bool success,
-                                        const std::string& error_message) {}
+    // Called when the extension has unpacked successfully.  |manifest| is the
+    // parsed manifest.json file.  |images| contains a list of decoded images
+    // and the associated paths where those images live on disk.
+    virtual void OnUnpackExtensionSucceeded(
+        const DictionaryValue& manifest,
+        const std::vector< Tuple2<SkBitmap, FilePath> >& images) {}
+
+    // Called when an error occurred while unpacking the extension.
+    // |error_message| contains a description of the problem.
+    virtual void OnUnpackExtensionFailed(const std::string& error_message) {}
+
    private:
     friend class UtilityProcessHost;
     void OnMessageReceived(const IPC::Message& message);