Extensions: Fix UAF in ExternalInstallError.

Currently, ExternalInstallError::RemoveError() calls
RemoveExternalInstallError() on ExternalInstallManager (its owner) which deletes
the ExternalInstallError instance. This also causes the deletion of the
extension id, which is passed as a reference. This is further accessed in
ExternalInstallManager::RemoveExternalInstallError leading to a UAF. This CL
fixes the issue by using a copy of the passed string reference in
RemoveExternalInstallError. A regression test is also added which fails on the
current master.

BUG=739142

Change-Id: I26c57a19a7d88e2a11eb17d3c45c371e95de700c
Reviewed-on: https://chromium-review.googlesource.com/572763Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487266}

chrome/browser/extensions/extension_service_unittest.cc

@@ -6354,6 +6354,35 @@ TEST_F(ExtensionServiceTest, MultipleExternalInstallErrors) {
   EXPECT_FALSE(HasExternalInstallErrors(service_));
 }
 
+// Regression test for crbug.com/739142. Verifies that no UAF occurs when
+// ExternalInstallError needs to be deleted asynchronously.
+TEST_F(ExtensionServiceTest, InstallPromptAborted) {
+  FeatureSwitch::ScopedOverride prompt(
+      FeatureSwitch::prompt_for_external_extensions(), true);
+  InitializeEmptyExtensionService();
+
+  MockExternalProvider* reg_provider =
+      AddMockExternalProvider(Manifest::EXTERNAL_REGISTRY);
+
+  reg_provider->UpdateOrAddExtension(good_crx, "1.0.0.0",
+                                     data_dir().AppendASCII("good.crx"));
+  WaitForExternalExtensionInstalled();
+  EXPECT_EQ(
+      1u, service()->external_install_manager()->GetErrorsForTesting().size());
+  EXPECT_FALSE(service()->IsExtensionEnabled(good_crx));
+  EXPECT_TRUE(GetError(good_crx));
+
+  // Abort the extension install prompt. This should cause the
+  // ExternalInstallError to be deleted asynchronously.
+  GetError(good_crx)->OnInstallPromptDone(
+      ExtensionInstallPrompt::Result::ABORTED);
+  EXPECT_TRUE(GetError(good_crx));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(GetError(good_crx));
+
+  EXPECT_FALSE(HasExternalInstallErrors(service_));
+}
+
 TEST_F(ExtensionServiceTest, MultipleExternalInstallBubbleErrors) {
   FeatureSwitch::ScopedOverride prompt(
       FeatureSwitch::prompt_for_external_extensions(), true);

chrome/browser/extensions/external_install_manager.cc

@@ -114,10 +114,15 @@ void ExternalInstallManager::RemoveExternalInstallError(
     const std::string& extension_id) {
   auto iter = errors_.find(extension_id);
   if (iter != errors_.end()) {
+    // The |extension_id| may be owned by the ExternalInstallError, which is
+    // deleted subsequently. To avoid any UAFs, make a safe copy of
+    // |extension_id| now.
+    std::string extension_id_copy = extension_id;
+
     if (iter->second.get() == currently_visible_install_alert_)
       currently_visible_install_alert_ = nullptr;
     errors_.erase(iter);
-    unacknowledged_ids_.erase(extension_id);
+    unacknowledged_ids_.erase(extension_id_copy);
     UpdateExternalExtensionAlert();
   }
 }