Verify signed .crx extension installations

This is second try of:

http://codereview.chromium.org/115682

that was comitted in in 18189 and reverted.

BUG=12114
R=erikkay,wtc,aa

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

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

7 files changed, 336 insertions, 110 deletions

diff --git a/chrome/browser/extensions/extension_creator.cc b/chrome/browser/extensions/extension_creator.cc
index 8c38e17..9abd030 100755
--- a/chrome/browser/extensions/extension_creator.cc
+++ b/chrome/browser/extensions/extension_creator.cc
@@ -12,6 +12,7 @@
 #include "base/file_util.h"
 #include "base/scoped_handle.h"
 #include "base/string_util.h"
+#include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/zip.h"
 #include "net/base/base64.h"
@@ -20,8 +21,6 @@ namespace {
   const int kRSAKeySize = 1024;
 };
 
-const char ExtensionCreator::kExtensionHeaderMagic[] = "Cr24";
-
 bool ExtensionCreator::InitializeInput(
     const FilePath& extension_dir,
     const FilePath& private_key_path,
@@ -167,14 +166,14 @@ bool ExtensionCreator::WriteCRX(const FilePath& zip_path,
     return false;
   }
 
-  ExtensionCreator::ExtensionHeader header;
-  memcpy(&header.magic, ExtensionCreator::kExtensionHeaderMagic,
-         ExtensionCreator::kExtensionHeaderMagicSize);
-  header.version = kCurrentVersion;
+  ExtensionsService::ExtensionHeader header;
+  memcpy(&header.magic, ExtensionsService::kExtensionHeaderMagic,
+         ExtensionsService::kExtensionHeaderMagicSize);
+  header.version = ExtensionsService::kCurrentVersion;
   header.key_size = public_key.size();
   header.signature_size = signature.size();
 
-  fwrite(&header, sizeof(ExtensionCreator::ExtensionHeader), 1,
+  fwrite(&header, sizeof(ExtensionsService::ExtensionHeader), 1,
       crx_handle.get());
   fwrite(&public_key.front(), sizeof(uint8), public_key.size(),
       crx_handle.get());
diff --git a/chrome/browser/extensions/extension_creator.h b/chrome/browser/extensions/extension_creator.h
index febcecd..129e152 100755
--- a/chrome/browser/extensions/extension_creator.h
+++ b/chrome/browser/extensions/extension_creator.h
@@ -17,28 +17,6 @@
 // generated randomly (and optionally written to |output_private_key_path|.
 class ExtensionCreator {
  public:
-  // The size of the magic character sequence at the beginning of each crx file,
-  // in bytes. This should be a multiple of 4.
-  static const size_t kExtensionHeaderMagicSize = 4;
-
-  // The magic character sequence at the beginning of each crx file.
-  static const char kExtensionHeaderMagic[];
-
-  // The current version of the crx format.
-  static const uint32 kCurrentVersion = 2;
-
-  // This header is the first data at the beginning of an extension. Its
-  // contents are purposely 32-bit aligned so that it can just be slurped into
-  // a struct without manual parsing.
-  struct ExtensionHeader {
-    char magic[kExtensionHeaderMagicSize];
-    uint32 version;
-    size_t key_size;  // The size of the public key, in bytes.
-    size_t signature_size;  // The size of the signature, in bytes.
-    // An ASN.1-encoded PublicKeyInfo structure follows.
-    // The signature follows.
-  };
-
   ExtensionCreator() {}
 
   bool Run(const FilePath& extension_dir,
diff --git a/chrome/browser/extensions/extension_shelf_model_unittest.cc b/chrome/browser/extensions/extension_shelf_model_unittest.cc
index 230e2cc..82ce02e 100644
--- a/chrome/browser/extensions/extension_shelf_model_unittest.cc
+++ b/chrome/browser/extensions/extension_shelf_model_unittest.cc
@@ -15,7 +15,7 @@
 namespace {
 
 // The extension we're using as our test case.
-const char* kExtensionId = "00123456789abcdef0123456789abcdef0123456";
+const char* kExtensionId = "fc6f6ba6693faf6773c13701019f2e7a12f0febe";
 
 };  // namespace
 
diff --git a/chrome/browser/extensions/extension_uitest.cc b/chrome/browser/extensions/extension_uitest.cc
index 38a5bb6..12cac46 100644
--- a/chrome/browser/extensions/extension_uitest.cc
+++ b/chrome/browser/extensions/extension_uitest.cc
@@ -103,7 +103,7 @@ TEST_F(SimpleApiCallExtensionTest, RunTest) {
   namespace keys = extension_automation_constants;
 
   TestWithURL(GURL(
-      "chrome-extension://77774444789ABCDEF0123456789ABCDEF0123456/test.html"));
+      "chrome-extension://fc6f6ba6693faf6773c13701019f2e7a12f0febe/test.html"));
   AutomationProxyForExternalTab* proxy =
       static_cast<AutomationProxyForExternalTab*>(automation());
   ASSERT_GT(proxy->messages_received(), 0);
@@ -270,7 +270,7 @@ class RoundtripApiCallExtensionTest
 #if defined(OS_WIN)
 TEST_F(RoundtripApiCallExtensionTest, RunTest) {
   TestWithURL(GURL(
-      "chrome-extension://66664444789ABCDEF0123456789ABCDEF0123456/test.html"));
+      "chrome-extension://e5ead92b2c6795c1d2b92df9c5cb37de5582471a/test.html"));
   RoundtripAutomationProxy* proxy =
       static_cast<RoundtripAutomationProxy*>(automation());
 
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index bf79e78..6d99e84 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -6,6 +6,7 @@
 
 #include "app/l10n_util.h"
 #include "base/command_line.h"
+#include "base/crypto/signature_verifier.h"
 #include "base/file_util.h"
 #include "base/gfx/png_encoder.h"
 #include "base/scoped_handle.h"
@@ -20,6 +21,7 @@
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_thread.h"
+#include "chrome/browser/extensions/extension_creator.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/extensions/external_extension_provider.h"
@@ -38,6 +40,7 @@
 #include "chrome/common/url_constants.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
+#include "net/base/base64.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 #if defined(OS_WIN)
@@ -48,6 +51,8 @@
 
 // ExtensionsService.
 
+const char ExtensionsService::kExtensionHeaderMagic[] = "Cr24";
+
 const char* ExtensionsService::kInstallDirectoryName = "Extensions";
 const char* ExtensionsService::kCurrentVersionFileName = "Current Version";
 const char* ExtensionsServiceBackend::kTempExtensionName = "TEMP_INSTALL";
@@ -65,8 +70,17 @@ const wchar_t kState[] = L"state";
 // A temporary subdirectory where we unpack extensions.
 const char* kUnpackExtensionDir = "TEMP_UNPACK";
 
-// The version of the extension package that this code understands.
-const uint32 kExpectedVersion = 1;
+// Unpacking errors
+const char* kBadMagicNumberError = "Bad magic number";
+const char* kBadHeaderSizeError = "Excessively large key or signature";
+const char* kBadVersionNumberError = "Bad version number";
+const char* kInvalidExtensionHeaderError = "Invalid extension header";
+const char* kInvalidPublicKeyError = "Invalid public key";
+const char* kInvalidSignatureError = "Invalid signature";
+const char* kSignatureVerificationFailed = "Signature verification failed";
+const char* kSignatureVerificationInitFailed =
+    "Signature verification initialization failed. This is most likely "
+    "caused by a public key in the wrong format (should encode algorithm).";
 }
 
 // This class coordinates an extension unpack task which is run in a separate
@@ -77,11 +91,12 @@ class ExtensionsServiceBackend::UnpackerClient
  public:
   UnpackerClient(ExtensionsServiceBackend* backend,
                  const FilePath& extension_path,
+                 const std::string& public_key,
                  const std::string& expected_id,
                  bool from_external)
     : backend_(backend), extension_path_(extension_path),
-      expected_id_(expected_id), from_external_(from_external),
-      got_response_(false) {
+      public_key_(public_key), expected_id_(expected_id),
+      from_external_(from_external), got_response_(false) {
   }
 
   // Starts the unpack task.  We call back to the backend when the task is done,
@@ -146,6 +161,14 @@ class ExtensionsServiceBackend::UnpackerClient
   void OnUnpackExtensionSucceededImpl(
       const DictionaryValue& manifest,
       const ExtensionUnpacker::DecodedImages& images) {
+    // Add our public key into the parsed manifest. We want it to be saved so
+    // that we can later refer to it (eg for generating ids, validating
+    // signatures, etc).
+    // The const_cast is hacky, but seems like the right thing here, rather than
+    // making a full copy just to make this change.
+    const_cast<DictionaryValue*>(&manifest)->SetString(
+        Extension::kPublicKeyKey, public_key_);
+
     // The extension was unpacked to the temp dir inside our unpacking dir.
     FilePath extension_dir = temp_extension_path_.DirName().AppendASCII(
         ExtensionsServiceBackend::kTempExtensionName);
@@ -182,6 +205,9 @@ class ExtensionsServiceBackend::UnpackerClient
   // The path to the crx file that we're installing.
   FilePath extension_path_;
 
+  // The public key of the extension we're installing.
+  std::string public_key_;
+
   // The path to the copy of the crx file in the temporary directory where we're
   // unpacking it.
   FilePath temp_extension_path_;
@@ -355,7 +381,7 @@ void ExtensionsService::OnExtensionsLoaded(ExtensionList* new_extensions) {
 }
 
 void ExtensionsService::OnExtensionInstalled(Extension* extension,
-                                             bool update) {
+    Extension::InstallType install_type) {
   UpdateExtensionPref(ASCIIToWide(extension->id()), kState,
                       Value::CreateIntegerValue(Extension::ENABLED), false);
   UpdateExtensionPref(ASCIIToWide(extension->id()), kLocation,
@@ -385,7 +411,7 @@ void ExtensionsService::OnExternalExtensionInstalled(
                       Value::CreateIntegerValue(location), true);
 }
 
-void ExtensionsService::OnExtensionVersionReinstalled(const std::string& id) {
+void ExtensionsService::OnExtensionOverinstallAttempted(const std::string& id) {
   Extension* extension = GetExtensionByID(id);
   if (extension && extension->IsTheme()) {
     NotificationService::current()->Notify(
@@ -630,6 +656,21 @@ void ExtensionsServiceBackend::LoadSingleExtension(
   }
 }
 
+DictionaryValue* ExtensionsServiceBackend::ReadManifest(FilePath manifest_path,
+                                                        std::string* error) {
+  JSONFileValueSerializer serializer(manifest_path);
+  scoped_ptr<Value> root(serializer.Deserialize(error));
+  if (!root.get())
+    return NULL;
+
+  if (!root->IsType(Value::TYPE_DICTIONARY)) {
+    *error = Extension::kInvalidManifestError;
+    return NULL;
+  }
+
+  return static_cast<DictionaryValue*>(root.release());
+}
+
 Extension* ExtensionsServiceBackend::LoadExtension(
     const FilePath& extension_path,
     Extension::Location location,
@@ -641,22 +682,15 @@ Extension* ExtensionsServiceBackend::LoadExtension(
     return NULL;
   }
 
-  JSONFileValueSerializer serializer(manifest_path);
   std::string error;
-  scoped_ptr<Value> root(serializer.Deserialize(&error));
+  scoped_ptr<DictionaryValue> root(ReadManifest(manifest_path, &error));
   if (!root.get()) {
     ReportExtensionLoadError(extension_path, error);
     return NULL;
   }
 
-  if (!root->IsType(Value::TYPE_DICTIONARY)) {
-    ReportExtensionLoadError(extension_path, Extension::kInvalidManifestError);
-    return NULL;
-  }
-
   scoped_ptr<Extension> extension(new Extension(extension_path));
-  if (!extension->InitFromValue(*static_cast<DictionaryValue*>(root.get()),
-                                require_id, &error)) {
+  if (!extension->InitFromValue(*root.get(), require_id, &error)) {
     ReportExtensionLoadError(extension_path, error);
     return NULL;
   }
@@ -765,28 +799,36 @@ bool ExtensionsServiceBackend::ReadCurrentVersion(const FilePath& dir,
   return false;
 }
 
-bool ExtensionsServiceBackend::CheckCurrentVersion(
+Extension::InstallType ExtensionsServiceBackend::CompareToInstalledVersion(
+    const std::string& id,
     const std::string& new_version_str,
-    const std::string& current_version_str,
-    const FilePath& dest_dir) {
+    std::string *current_version_str) {
+  CHECK(current_version_str);
+  FilePath dir(install_directory_.AppendASCII(id.c_str()));
+  if (!ReadCurrentVersion(dir, current_version_str))
+    return Extension::NEW_INSTALL;
+
   scoped_ptr<Version> current_version(
-      Version::GetVersionFromString(current_version_str));
+    Version::GetVersionFromString(*current_version_str));
   scoped_ptr<Version> new_version(
-      Version::GetVersionFromString(new_version_str));
-  if (current_version->CompareTo(*new_version) >= 0) {
-    // Verify that the directory actually exists.  If it doesn't we'll return
-    // true so that the install code will repair the broken installation.
-    // TODO(erikkay): A further step would be to verify that the extension
-    // has actually loaded successfully.
-    FilePath version_dir = dest_dir.AppendASCII(current_version_str);
-    if (file_util::PathExists(version_dir)) {
-      std::string id = WideToASCII(dest_dir.BaseName().ToWStringHack());
-      StringToLowerASCII(&id);
-      ReportExtensionVersionReinstalled(id);
-      return false;
-    }
-  }
-  return true;
+    Version::GetVersionFromString(new_version_str));
+  int comp = new_version->CompareTo(*current_version);
+  if (comp > 0)
+    return Extension::UPGRADE;
+  else if (comp == 0)
+    return Extension::REINSTALL;
+  else
+    return Extension::DOWNGRADE;
+}
+
+bool ExtensionsServiceBackend::NeedsReinstall(const std::string& id,
+    const std::string& current_version) {
+  // Verify that the directory actually exists.
+  // TODO(erikkay): A further step would be to verify that the extension
+  // has actually loaded successfully.
+  FilePath dir(install_directory_.AppendASCII(id.c_str()));
+  FilePath version_dir(dir.AppendASCII(current_version));
+  return !file_util::PathExists(version_dir);
 }
 
 bool ExtensionsServiceBackend::InstallDirSafely(const FilePath& source_dir,
@@ -873,13 +915,107 @@ void ExtensionsServiceBackend::InstallExtension(
 }
 
 void ExtensionsServiceBackend::InstallOrUpdateExtension(
-    const FilePath& extension_path, const std::string& expected_id,
+    const FilePath& extension_path,
+    const std::string& expected_id,
     bool from_external) {
-  UnpackerClient* client =
-      new UnpackerClient(this, extension_path, expected_id, from_external);
+  std::string actual_public_key;
+  if (!ValidateSignature(extension_path, &actual_public_key))
+    return;  // Failures reported within ValidateSignature().
+
+  UnpackerClient* client = new UnpackerClient(
+      this, extension_path, actual_public_key, expected_id, from_external);
   client->Start();
 }
 
+bool ExtensionsServiceBackend::ValidateSignature(const FilePath& extension_path,
+                                                 std::string* key_out) {
+  ScopedStdioHandle file(file_util::OpenFile(extension_path, "rb"));
+  if (!file.get()) {
+    ReportExtensionInstallError(extension_path, "Could not open file.");
+    return NULL;
+  }
+
+  // Read and verify the header.
+  ExtensionsService::ExtensionHeader header;
+  size_t len;
+
+  // TODO(erikkay): Yuck.  I'm not a big fan of this kind of code, but it
+  // appears that we don't have any endian/alignment aware serialization
+  // code in the code base.  So for now, this assumes that we're running
+  // on a little endian machine with 4 byte alignment.
+  len = fread(&header, 1, sizeof(ExtensionsService::ExtensionHeader),
+      file.get());
+  if (len < sizeof(ExtensionsService::ExtensionHeader)) {
+    ReportExtensionInstallError(extension_path, kInvalidExtensionHeaderError);
+    return false;
+  }
+  if (strncmp(ExtensionsService::kExtensionHeaderMagic, header.magic,
+      sizeof(header.magic))) {
+    ReportExtensionInstallError(extension_path, kBadMagicNumberError);
+    return false;
+  }
+  if (header.version != ExtensionsService::kCurrentVersion) {
+    ReportExtensionInstallError(extension_path, kBadVersionNumberError);
+    return false;
+  }
+  if (header.key_size > ExtensionsService::kMaxPublicKeySize ||
+      header.signature_size > ExtensionsService::kMaxSignatureSize) {
+    ReportExtensionInstallError(extension_path, kBadHeaderSizeError);
+    return false;
+  }
+
+  std::vector<uint8> key;
+  key.resize(header.key_size);
+  len = fread(&key.front(), sizeof(uint8), header.key_size, file.get());
+  if (len < header.key_size) {
+    ReportExtensionInstallError(extension_path, kInvalidPublicKeyError);
+    return false;
+  }
+
+  std::vector<uint8> signature;
+  signature.resize(header.signature_size);
+  len = fread(&signature.front(), sizeof(uint8), header.signature_size,
+      file.get());
+  if (len < header.signature_size) {
+    ReportExtensionInstallError(extension_path, kInvalidSignatureError);
+    return false;
+  }
+
+  // Note: this structure is an ASN.1 which encodes the algorithm used
+  // with its parameters. This is defined in PKCS #1 v2.1 (RFC 3447).
+  // It is encoding: { OID sha1WithRSAEncryption      PARAMETERS NULL }
+  // TODO(aa): This needs to be factored away someplace common.
+  const uint8 signature_algorithm[15] = {
+    0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86,
+    0xf7, 0x0d, 0x01, 0x01, 0x05, 0x05, 0x00
+  };
+
+  base::SignatureVerifier verifier;
+  if (!verifier.VerifyInit(signature_algorithm,
+                           sizeof(signature_algorithm),
+                           &signature.front(),
+                           signature.size(),
+                           &key.front(),
+                           key.size())) {
+    ReportExtensionInstallError(extension_path,
+        kSignatureVerificationInitFailed);
+    return false;
+  }
+
+  unsigned char buf[1 << 12];
+  while ((len = fread(buf, 1, sizeof(buf), file.get())) > 0)
+    verifier.VerifyUpdate(buf, len);
+
+  if (!verifier.VerifyFinal()) {
+    ReportExtensionInstallError(extension_path, kSignatureVerificationFailed);
+    return false;
+  }
+
+  net::Base64Encode(std::string(reinterpret_cast<char*>(&key.front()),
+      key.size()), key_out);
+  return true;
+}
+
 void ExtensionsServiceBackend::OnExtensionUnpacked(
     const FilePath& extension_path,
     const FilePath& temp_extension_dir,
@@ -942,11 +1078,25 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
   FilePath dest_dir = install_directory_.AppendASCII(extension.id());
   std::string version = extension.VersionString();
   std::string current_version;
-  bool was_update = false;
-  if (ReadCurrentVersion(dest_dir, &current_version)) {
-    if (!CheckCurrentVersion(version, current_version, dest_dir))
+  Extension::InstallType install_type =
+      CompareToInstalledVersion(extension.id(), version, &current_version);
+
+  // Do not allow downgrade.
+  if (install_type == Extension::DOWNGRADE) {
+    ReportExtensionInstallError(extension_path,
+        "Error: Attempt to downgrade extension from more recent version.");
+    return;
+  }
+
+  if (install_type == Extension::REINSTALL) {
+    if (NeedsReinstall(extension.id(), current_version)) {
+      // Treat corrupted existing installation as new install case.
+      install_type = Extension::NEW_INSTALL;
+    } else {
+      // The client may use this as a signal (to switch themes, for instance).
+      ReportExtensionOverinstallAttempted(extension.id());
       return;
-    was_update = true;
+    }
   }
 
   // Write our parsed manifest back to disk, to ensure it doesn't contain an
@@ -1044,7 +1194,7 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
 
     frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(
         frontend_, &ExtensionsService::OnExtensionInstalled, extension,
-        was_update));
+        install_type));
 
     // Only one extension, but ReportExtensionsLoaded can handle multiple,
     // so we need to construct a list.
@@ -1073,10 +1223,10 @@ void ExtensionsServiceBackend::ReportExtensionInstallError(
   ExtensionErrorReporter::GetInstance()->ReportError(message, alert_on_error_);
 }
 
-void ExtensionsServiceBackend::ReportExtensionVersionReinstalled(
+void ExtensionsServiceBackend::ReportExtensionOverinstallAttempted(
     const std::string& id) {
   frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(
-      frontend_, &ExtensionsService::OnExtensionVersionReinstalled, id));
+      frontend_, &ExtensionsService::OnExtensionOverinstallAttempted, id));
 }
 
 bool ExtensionsServiceBackend::ShouldSkipInstallingExtension(
@@ -1224,9 +1374,14 @@ void ExtensionsServiceBackend::OnExternalExtensionFound(
 
 bool ExtensionsServiceBackend::ShouldInstall(const std::string& id,
                                              const Version* version) {
-  FilePath dir(install_directory_.AppendASCII(id.c_str()));
   std::string current_version;
-  if (ReadCurrentVersion(dir, &current_version))
-    return CheckCurrentVersion(version->GetString(), current_version, dir);
-  return true;
+  Extension::InstallType install_type =
+      CompareToInstalledVersion(id, version->GetString(), &current_version);
+
+  if (install_type == Extension::DOWNGRADE)
+    return false;
+
+  return (install_type == Extension::UPGRADE ||
+          install_type == Extension::NEW_INSTALL ||
+          NeedsReinstall(id, current_version));
 }
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index fb0feaf..7def6f1 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -34,10 +34,43 @@ class UserScriptMaster;
 
 typedef std::vector<Extension*> ExtensionList;
 
+
 // Manages installed and running Chromium extensions.
 class ExtensionsService
     : public base::RefCountedThreadSafe<ExtensionsService> {
  public:
+
+   // TODO(port): Move Crx package definitions to ExtentionCreator. They are
+   // currently here because ExtensionCreator is excluded on linux & mac.
+
+   // The size of the magic character sequence at the beginning of each crx
+   // file, in bytes. This should be a multiple of 4.
+   static const size_t kExtensionHeaderMagicSize = 4;
+
+   // The maximum size the crx parser will tolerate for a public key.
+   static const size_t kMaxPublicKeySize = 1 << 16;
+
+   // The maximum size the crx parser will tolerate for a signature.
+   static const size_t kMaxSignatureSize = 1 << 16;
+
+   // The magic character sequence at the beginning of each crx file.
+   static const char kExtensionHeaderMagic[];
+
+   // The current version of the crx format.
+   static const uint32 kCurrentVersion = 2;
+
+   // This header is the first data at the beginning of an extension. Its
+   // contents are purposely 32-bit aligned so that it can just be slurped into
+   // a struct without manual parsing.
+   struct ExtensionHeader {
+     char magic[kExtensionHeaderMagicSize];
+     uint32 version;
+     size_t key_size;  // The size of the public key, in bytes.
+     size_t signature_size;  // The size of the signature, in bytes.
+     // An ASN.1-encoded PublicKeyInfo structure follows.
+     // The signature follows.
+   };
+
   ExtensionsService(Profile* profile,
                     MessageLoop* frontend_loop,
                     MessageLoop* backend_loop);
@@ -117,14 +150,16 @@ class ExtensionsService
   void OnExtensionsLoaded(ExtensionList* extensions);
 
   // Called by the backend when an extensoin hsa been installed.
-  void OnExtensionInstalled(Extension* extension, bool is_update);
+  void OnExtensionInstalled(Extension* extension,
+      Extension::InstallType install_type);
 
   // Called by the backend when an external extension has been installed.
   void OnExternalExtensionInstalled(
       const std::string& id, Extension::Location location);
 
-  // Called by the backend when an extension has been reinstalled.
-  void OnExtensionVersionReinstalled(const std::string& id);
+  // Called by the backend when an attempt was made to reinstall the same
+  // version of an existing extension.
+  void OnExtensionOverinstallAttempted(const std::string& id);
 
   // The name of the directory inside the profile where extensions are
   // installed to.
@@ -221,6 +256,11 @@ class ExtensionsServiceBackend
   class UnpackerClient;
   friend class UnpackerClient;
 
+  // Utility function to read an extension manifest and return it as a
+  // DictionaryValue. If it fails, NULL is returned and |error| contains an
+  // appropriate message.
+  DictionaryValue* ReadManifest(FilePath manifest_path, std::string* error);
+
   // Load a single extension from |extension_path|, the top directory of
   // a specific extension where its manifest file lives.
   Extension* LoadExtension(const FilePath& extension_path,
@@ -241,6 +281,10 @@ class ExtensionsServiceBackend
                                 const std::string& expected_id,
                                 bool from_external);
 
+  // Validates the signature of the extension in |extension_path|. Returns true
+  // and the public key (in |key|) if the signature validates, false otherwise.
+  bool ValidateSignature(const FilePath& extension_path, std::string* key_out);
+
   // 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.
@@ -265,8 +309,9 @@ class ExtensionsServiceBackend
   void ReportExtensionInstallError(const FilePath& extension_path,
                                    const std::string& error);
 
-  // Notify the frontend that the extension had already been installed.
-  void ReportExtensionVersionReinstalled(const std::string& id);
+  // Notify the frontend that an attempt was made (but not carried out) to
+  // install the same version of an existing extension.
+  void ReportExtensionOverinstallAttempted(const std::string& id);
 
   // Checks a set of strings (containing id's to ignore) in order to determine
   // if the extension should be installed.
@@ -297,11 +342,14 @@ class ExtensionsServiceBackend
   // Reads the Current Version file from |dir| into |version_string|.
   bool ReadCurrentVersion(const FilePath& dir, std::string* version_string);
 
-  // Check that the version to be installed is greater than the current
-  // installed extension.
-  bool CheckCurrentVersion(const std::string& version,
-                           const std::string& current_version,
-                           const FilePath& dest_dir);
+  // Look for an existing installation of the extension |id| & return
+  // an InstallType that would result from installing |new_version_str|.
+  Extension::InstallType CompareToInstalledVersion(const std::string& id,
+      const std::string& new_version_str, std::string* current_version_str);
+
+  // Does an existing installed extension need to be reinstalled.
+  bool NeedsReinstall(const std::string& id,
+                      const std::string& current_version);
 
   // Install the extension dir by moving it from |source| to |dest| safely.
   bool InstallDirSafely(const FilePath& source,
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 1411d78..ab380fd 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -33,13 +33,13 @@ namespace {
 // Extension ids used during testing.
 const char* const all_zero = "0000000000000000000000000000000000000000";
 const char* const zero_n_one = "0000000000000000000000000000000000000001";
-const char* const good0 = "00123456789abcdef0123456789abcdef0123456";
-const char* const good1 = "10123456789abcdef0123456789abcdef0123456";
-const char* const good2 = "20123456789abcdef0123456789abcdef0123456";
-const char* const good_crx = "00123456789abcdef0123456789abcdef0123456";
-const char* const page_action = "8a5e4cb023c61b431e9b603a97c293429ce057c8";
-const char* const theme_crx = "f0123456789abcdef0123456789abcdef0126456";
-const char* const theme2_crx = "f0123456789adddef0123456789abcdef0126456";
+const char* const good0 = "fc6f6ba6693faf6773c13701019f2e7a12f0febe";
+const char* const good1 = "e5ead92b2c6795c1d2b92df9c5cb37de5582471a";
+const char* const good2 = "a37fed892f622823f4daaec4426a32fc7f6147dc";
+const char* const good_crx = "b3dd733cd71a98fa83f387455e12f5c5501c519e";
+const char* const page_action = "a4ca7d01469a010acb200568a0b8f4d9b3ac1f91";
+const char* const theme_crx = "80c45f5ae9e0f839d105c6a6d2461a036bc40a04";
+const char* const theme2_crx = "f9f6c52c01efdd5edd7c396b5f995a15fc7ad6d1";
 
 struct ExtensionsOrder {
   bool operator()(const Extension* a, const Extension* b) {
@@ -303,7 +303,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   }
   ASSERT_EQ(3u, loaded_.size());
 
-  EXPECT_EQ(std::string(good_crx), loaded_[0]->id());
+  EXPECT_EQ(std::string(good0), loaded_[0]->id());
   EXPECT_EQ(std::string("My extension 1"),
             loaded_[0]->name());
   EXPECT_EQ(std::string("The first extension that I made."),
@@ -313,8 +313,8 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   EXPECT_EQ(3u, service_->extensions()->size());
 
   ValidatePrefKeyCount(3);
-  ValidatePref(good_crx, L"state", Extension::ENABLED);
-  ValidatePref(good_crx, L"location", Extension::INTERNAL);
+  ValidatePref(good0, L"state", Extension::ENABLED);
+  ValidatePref(good0, L"location", Extension::INTERNAL);
   ValidatePref(good1, L"state", Extension::ENABLED);
   ValidatePref(good1, L"location", Extension::INTERNAL);
   ValidatePref(good2, L"state", Extension::ENABLED);
@@ -466,6 +466,10 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   ValidatePref(page_action, L"state", Extension::ENABLED);
   ValidatePref(page_action, L"location", Extension::INTERNAL);
 
+  // Bad signature. 
+  path = extensions_path.AppendASCII("bad_signature.crx");
+  InstallExtension(path, false);
+
   // 0-length extension file.
   path = extensions_path.AppendASCII("not_an_extension.crx");
   InstallExtension(path, false);
@@ -476,16 +480,6 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   InstallExtension(path, false);
   ValidatePrefKeyCount(pref_count);
 
-  // Poorly formed JSON.
-  path = extensions_path.AppendASCII("bad_json.crx");
-  InstallExtension(path, false);
-  ValidatePrefKeyCount(pref_count);
-
-  // Incorrect zip hash.
-  path = extensions_path.AppendASCII("bad_hash.crx");
-  InstallExtension(path, false);
-  ValidatePrefKeyCount(pref_count);
-
   // TODO(erikkay): add more tests for many of the failure cases.
   // TODO(erikkay): add tests for upgrade cases.
 }
@@ -627,6 +621,56 @@ TEST_F(ExtensionsServiceTest, Reinstall) {
   ValidatePref(good_crx, L"location", Extension::INTERNAL);
 }
 
+// Test upgrading a signed extension.
+TEST_F(ExtensionsServiceTest, UpgradeSignedGood) {
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+
+  FilePath path = extensions_path.AppendASCII("good.crx");
+  service_->InstallExtension(path);
+  loop_.RunAllPending();
+
+  ASSERT_TRUE(installed_);
+  ASSERT_EQ(1u, loaded_.size());
+  ASSERT_EQ(0u, GetErrors().size());
+
+  // Upgrade to version 2.0
+  path = extensions_path.AppendASCII("good2.crx");
+  service_->InstallExtension(path);
+  loop_.RunAllPending();
+
+  ASSERT_TRUE(installed_);
+  ASSERT_EQ(2u, loaded_.size());
+  ASSERT_EQ(0u, GetErrors().size());
+}
+
+// Test upgrading a signed extension with a bad signature.
+TEST_F(ExtensionsServiceTest, UpgradeSignedBad) {
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+
+  FilePath path = extensions_path.AppendASCII("good.crx");
+  service_->InstallExtension(path);
+  loop_.RunAllPending();
+
+  ASSERT_TRUE(installed_);
+  ASSERT_EQ(1u, loaded_.size());
+  ASSERT_EQ(0u, GetErrors().size());
+  installed_ = NULL;
+
+  // Try upgrading with a bad signature. This should fail during the unpack,
+  // because the key will not match the signature.
+  path = extensions_path.AppendASCII("good2_bad_signature.crx");
+  service_->InstallExtension(path);
+  loop_.RunAllPending();
+
+  ASSERT_FALSE(installed_);
+  ASSERT_EQ(1u, loaded_.size());
+  ASSERT_EQ(1u, GetErrors().size());
+}
+
 // Tests uninstalling normal extensions
 TEST_F(ExtensionsServiceTest, UninstallExtension) {
   FilePath extensions_path;
@@ -724,6 +768,8 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
 // Tests that we generate IDs when they are not specified in the manifest for
 // --load-extension.
 TEST_F(ExtensionsServiceTest, GenerateID) {
+  Extension::ResetGeneratedIdCounter();
+
   FilePath extensions_path;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");