[Extensions] Support withholding hosts during extension installation

This change adds logic to the extension installation flow that allows
for host permissions to be withheld by default if certain creation
flags are supplied.

Bug: 984069
Change-Id: I53aa71814b34bd19dc30a9b11b1dc5955bec1106
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1759408
Commit-Queue: Tim Judkins <tjudkins@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705346}

chrome/browser/extensions/scripting_permissions_modifier.cc

@@ -35,10 +35,10 @@ bool CanWithholdFromExtension(const Extension& extension) {
                                                       extension.location());
 }
 
-// Iterates over |requested_permissions| and adds any permissions that should
-// be granted to |granted_permissions_out|. These include any non-host
+// Iterates over |requested_permissions| and returns a permission set of any
+// permissions that should be  granted. These include any non-host
 // permissions or host permissions that are present in
-// |runtime_granted_permissions|. |granted_permissions_out| may contain new
+// |runtime_granted_permissions|. The returned permission set may contain new
 // patterns not found in either |requested_permissions| or
 // |runtime_granted_permissions| in the case of overlapping host permissions
 // (such as *://*.google.com/* and https://*/*, which would intersect with
@@ -363,7 +363,16 @@ ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
   if (ShouldConsiderExtension(extension)) {
     base::Optional<bool> pref_value =
         extension_prefs.GetShouldWithholdPermissions(extension.id());
-    should_withhold = pref_value.has_value() && pref_value.value() == true;
+    if (pref_value.has_value()) {
+      should_withhold = pref_value.value();
+    } else {
+      should_withhold =
+          extension.creation_flags() & Extension::WITHHOLD_PERMISSIONS;
+    }
+  } else {
+    // The withhold creation flag should never have been set in cases where
+    // withholding isn't allowed.
+    DCHECK(!(extension.creation_flags() & Extension::WITHHOLD_PERMISSIONS));
   }
 
   should_withhold &= !permissions.effective_hosts().is_empty();
@@ -376,6 +385,11 @@ ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
   // permissions API.
   std::unique_ptr<const PermissionSet> runtime_granted_permissions =
       GetRuntimePermissionsFromPrefs(extension, extension_prefs);
+  // If there were no runtime granted permissions found in the prefs, default to
+  // a new empty set.
+  if (!runtime_granted_permissions) {
+    runtime_granted_permissions = std::make_unique<PermissionSet>();
+  }
   return PartitionHostPermissions(permissions, *runtime_granted_permissions);
 }
 

chrome/browser/extensions/scripting_permissions_modifier.h

@@ -99,8 +99,8 @@ class ScriptingPermissionsModifier {
   void RemoveAllGrantedHostPermissions();
 
   // Takes in a set of permissions and withholds any permissions that should not
-  // be granted for the given |extension|, populating |granted_permissions_out|
-  // with the set of all permissions that can be granted.
+  // be granted for the given |extension|, returning a permission set with all
+  // of the permissions that can be granted.
   // Note: we pass in |permissions| explicitly here, as this is used during
   // permission initialization, where the active permissions on the extension
   // may not be the permissions to compare against.

chrome/browser/extensions/scripting_permissions_modifier_unittest.cc

@@ -7,7 +7,7 @@
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/chrome_test_extension_loader.h"
 #include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/browser/extensions/extension_service_test_with_install.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/permissions_test_util.h"
 #include "chrome/browser/extensions/permissions_updater.h"
@@ -61,7 +61,30 @@ void InitializeExtensionPermissions(Profile* profile,
   updater.GrantActivePermissions(&extension);
 }
 
-using ScriptingPermissionsModifierUnitTest = ExtensionServiceTestBase;
+void CheckActiveHostPermissions(
+    const Extension& extension,
+    const std::vector<std::string>& explicit_hosts,
+    const std::vector<std::string>& scriptable_hosts) {
+  EXPECT_THAT(GetExplicitPatternsAsStrings(extension),
+              testing::UnorderedElementsAreArray(explicit_hosts));
+  EXPECT_THAT(GetScriptablePatternsAsStrings(extension),
+              testing::UnorderedElementsAreArray(scriptable_hosts));
+}
+
+void CheckWithheldHostPermissions(
+    const Extension& extension,
+    const std::vector<std::string>& explicit_hosts,
+    const std::vector<std::string>& scriptable_hosts) {
+  const PermissionsData* permissions_data = extension.permissions_data();
+  EXPECT_THAT(GetPatternsAsStrings(
+                  permissions_data->withheld_permissions().explicit_hosts()),
+              testing::UnorderedElementsAreArray(explicit_hosts));
+  EXPECT_THAT(GetPatternsAsStrings(
+                  permissions_data->withheld_permissions().scriptable_hosts()),
+              testing::UnorderedElementsAreArray(scriptable_hosts));
+}
+
+using ScriptingPermissionsModifierUnitTest = ExtensionServiceTestWithInstall;
 
 }  // namespace
 
@@ -88,52 +111,316 @@ TEST_F(ScriptingPermissionsModifierUnitTest, GrantAndWithholdHostPermissions) {
 
     PermissionsUpdater(profile()).InitializePermissions(extension.get());
 
-    const PermissionsData* permissions_data = extension->permissions_data();
-
     ScriptingPermissionsModifier modifier(profile(), extension);
     ASSERT_TRUE(modifier.CanAffectExtension());
 
     // By default, all permissions are granted.
-    EXPECT_THAT(GetScriptablePatternsAsStrings(*extension),
-                testing::UnorderedElementsAreArray(test_case));
-    EXPECT_THAT(GetExplicitPatternsAsStrings(*extension),
-                testing::UnorderedElementsAreArray(test_case));
-    EXPECT_TRUE(
-        permissions_data->withheld_permissions().scriptable_hosts().is_empty());
-    EXPECT_TRUE(
-        permissions_data->withheld_permissions().explicit_hosts().is_empty());
+    {
+      SCOPED_TRACE("Initial state");
+      CheckActiveHostPermissions(*extension, test_case, test_case);
+      CheckWithheldHostPermissions(*extension, {}, {});
+    }
 
     // Then, withhold host permissions.
     modifier.SetWithholdHostPermissions(true);
-
-    // Note: We don't use URLPatternSet::is_empty() here, since
-    // chrome://favicon/ can still be present in the set (it's not really a
-    // host permission and isn't withheld). GetPatternsAsStrings() ignores
-    // chrome://favicon.
-    EXPECT_THAT(GetScriptablePatternsAsStrings(*extension), testing::IsEmpty());
-    EXPECT_THAT(GetExplicitPatternsAsStrings(*extension), testing::IsEmpty());
-
-    EXPECT_THAT(
-        GetPatternsAsStrings(
-            permissions_data->withheld_permissions().scriptable_hosts()),
-        testing::UnorderedElementsAreArray(test_case));
-    EXPECT_THAT(GetPatternsAsStrings(
-                    permissions_data->withheld_permissions().explicit_hosts()),
-                testing::UnorderedElementsAreArray(test_case));
+    {
+      SCOPED_TRACE("After setting to withhold");
+      CheckActiveHostPermissions(*extension, {}, {});
+      CheckWithheldHostPermissions(*extension, test_case, test_case);
+    }
 
     // Finally, re-grant the withheld permissions.
     modifier.SetWithholdHostPermissions(false);
 
     // We should be back to our initial state - all requested permissions are
     // granted.
-    EXPECT_THAT(GetScriptablePatternsAsStrings(*extension),
-                testing::UnorderedElementsAreArray(test_case));
-    EXPECT_THAT(GetExplicitPatternsAsStrings(*extension),
-                testing::UnorderedElementsAreArray(test_case));
-    EXPECT_TRUE(
-        permissions_data->withheld_permissions().scriptable_hosts().is_empty());
-    EXPECT_TRUE(
-        permissions_data->withheld_permissions().explicit_hosts().is_empty());
+    {
+      SCOPED_TRACE("After setting to not withhold");
+      CheckActiveHostPermissions(*extension, test_case, test_case);
+      CheckWithheldHostPermissions(*extension, {}, {});
+    }
+  }
+}
+
+// Tests that with the creation flag present, requested host permissions are
+// withheld on installation, but still allow for individual permissions to be
+// granted, or all permissions be set back to not being withheld by default.
+TEST_F(ScriptingPermissionsModifierUnitTest, WithholdHostPermissionsOnInstall) {
+  InitializeEmptyExtensionService();
+
+  constexpr char kHostGoogle[] = "https://google.com/*";
+  constexpr char kHostChromium[] = "https://chromium.org/*";
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("a")
+          .AddPermissions({kHostGoogle, kHostChromium})
+          .AddContentScript("foo.js", {kHostGoogle})
+          .SetLocation(Manifest::INTERNAL)
+          .AddFlags(Extension::WITHHOLD_PERMISSIONS)
+          .Build();
+
+  // Initialize the permissions and have the prefs built and stored.
+  PermissionsUpdater(profile()).InitializePermissions(extension.get());
+  ExtensionPrefs::Get(profile())->OnExtensionInstalled(
+      extension.get(), Extension::State::ENABLED, syncer::StringOrdinal(), "");
+
+  ScriptingPermissionsModifier modifier(profile(), extension);
+  ASSERT_TRUE(modifier.CanAffectExtension());
+
+  // With the flag present, permissions should have been withheld.
+  {
+    SCOPED_TRACE("Initial state");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium},
+                                 {kHostGoogle});
+  }
+
+  // Grant one of the permissions manually.
+  modifier.GrantHostPermission(GURL(kHostChromium));
+
+  {
+    SCOPED_TRACE("After granting single");
+    CheckActiveHostPermissions(*extension, {kHostChromium}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle}, {kHostGoogle});
+  }
+
+  // Finally, re-grant the withheld permissions.
+  modifier.SetWithholdHostPermissions(false);
+
+  // All requested permissions should now be granted granted.
+  {
+    SCOPED_TRACE("After setting to not withhold");
+    CheckActiveHostPermissions(*extension, {kHostGoogle, kHostChromium},
+                               {kHostGoogle});
+    CheckWithheldHostPermissions(*extension, {}, {});
+  }
+}
+
+// Tests that reloading an extension after withholding host permissions on
+// installation retains the correct state and any changes that have been made
+// since installation.
+TEST_F(ScriptingPermissionsModifierUnitTest,
+       WithholdOnInstallPreservedOnReload) {
+  InitializeEmptyExtensionService();
+
+  constexpr char kHostGoogle[] = "https://google.com/*";
+  constexpr char kHostChromium[] = "https://chromium.org/*";
+  TestExtensionDir test_extension_dir;
+  test_extension_dir.WriteManifest(
+      R"({
+           "name": "foo",
+           "manifest_version": 2,
+           "version": "1",
+           "permissions": ["https://google.com/*", "https://chromium.org/*"]
+         })");
+  ChromeTestExtensionLoader loader(profile());
+  loader.add_creation_flag(Extension::WITHHOLD_PERMISSIONS);
+  loader.set_pack_extension(true);
+  scoped_refptr<const Extension> extension =
+      loader.LoadExtension(test_extension_dir.UnpackedPath());
+  // Cache the ID, since the extension will be invalidated across reloads.
+  ExtensionId extension_id = extension->id();
+
+  auto reload_extension = [this, &extension_id]() {
+    TestExtensionRegistryObserver observer(ExtensionRegistry::Get(profile()));
+    service()->ReloadExtension(extension_id);
+    return base::WrapRefCounted(observer.WaitForExtensionLoaded());
+  };
+
+  // Permissions start withheld due to creation flag and remain withheld after
+  // reload.
+  {
+    SCOPED_TRACE("Initial state");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Reload after initial state");
+    extension = reload_extension();
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  // Grant one of the permissions and check it persists after reload.
+  ScriptingPermissionsModifier(profile(), extension)
+      .GrantHostPermission(GURL(kHostGoogle));
+  {
+    SCOPED_TRACE("Granting single");
+    CheckActiveHostPermissions(*extension, {kHostGoogle}, {});
+    CheckWithheldHostPermissions(*extension, {kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Reload after granting single");
+    // TODO(tjudkins): We shouldn't have to explicitly call to grant
+    // permissions here, but at the moment when withholding host permissions on
+    // installation and then granting a permission, the reload or update detects
+    // that as a privilege increase and disables the extension.
+    service()->GrantPermissionsAndEnableExtension(extension.get());
+    extension = reload_extension();
+    CheckActiveHostPermissions(*extension, {kHostGoogle}, {});
+    CheckWithheldHostPermissions(*extension, {kHostChromium}, {});
+  }
+
+  // Set permissions not to be withheld at all and check it persists after
+  // reload.
+  ScriptingPermissionsModifier(profile(), extension)
+      .SetWithholdHostPermissions(false);
+  {
+    SCOPED_TRACE("Setting to not withhold");
+    CheckActiveHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+    CheckWithheldHostPermissions(*extension, {}, {});
+  }
+
+  {
+    SCOPED_TRACE("Reload after setting to not withhold");
+    // TODO(tjudkins): We shouldn't have to explicitly call to grant
+    // permissions here, but at the moment when withholding host permissions on
+    // installation and then granting a permission, the reload or update detects
+    // that as a privilege increase and disables the extension.
+    service()->GrantPermissionsAndEnableExtension(extension.get());
+    extension = reload_extension();
+    CheckActiveHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+    CheckWithheldHostPermissions(*extension, {}, {});
+  }
+
+  // Finally, set permissions to be withheld again and check it persists after
+  // reload.
+  ScriptingPermissionsModifier(profile(), extension)
+      .SetWithholdHostPermissions(true);
+  {
+    SCOPED_TRACE("Setting back to withhold");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Reload after setting back to withhold");
+    extension = reload_extension();
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+}
+
+// Tests that updating an extension after withholding host permissions on
+// installation retains the correct state and any changes that have been made
+// since installation.
+TEST_F(ScriptingPermissionsModifierUnitTest,
+       WithholdOnInstallPreservedOnUpdate) {
+  InitializeEmptyExtensionService();
+
+  constexpr char kHostGoogle[] = "https://google.com/*";
+  constexpr char kHostChromium[] = "https://chromium.org/*";
+  TestExtensionDir test_extension_dir;
+  constexpr char kManifestTemplate[] =
+      R"({
+           "name": "foo",
+           "manifest_version": 2,
+           "version": "%s",
+           "permissions": ["https://google.com/*", "https://chromium.org/*"]
+         })";
+
+  test_extension_dir.WriteManifest(base::StringPrintf(kManifestTemplate, "1"));
+  // We need to use a pem file here for consistent update IDs.
+  const base::FilePath pem_path =
+      data_dir().AppendASCII("permissions/update.pem");
+  scoped_refptr<const Extension> extension = PackAndInstallCRX(
+      test_extension_dir.UnpackedPath(), pem_path, INSTALL_NEW,
+      Extension::WITHHOLD_PERMISSIONS, Manifest::Location::INTERNAL);
+  // Cache the ID, since the extension will be invalidated across updates.
+  ExtensionId extension_id = extension->id();
+  // Hold onto references for the extension dirs so they don't get deleted
+  // outside the lambda.
+  std::vector<std::unique_ptr<TestExtensionDir>> extension_dirs;
+
+  auto update_extension = [this, &extension_id, &pem_path, &kManifestTemplate,
+                           &extension_dirs](const char* version) {
+    auto update_version = std::make_unique<TestExtensionDir>();
+    update_version->WriteManifest(
+        base::StringPrintf(kManifestTemplate, version));
+    PackCRXAndUpdateExtension(extension_id, update_version->UnpackedPath(),
+                              pem_path, ENABLED);
+    scoped_refptr<const Extension> updated_extension =
+        registry()->GetInstalledExtension(extension_id);
+
+    EXPECT_EQ(version, updated_extension->version().GetString());
+    extension_dirs.push_back(std::move(update_version));
+    return updated_extension;
+  };
+
+  // Permissions start withheld due to creation flag and remain withheld after
+  // update.
+  {
+    SCOPED_TRACE("Initial state");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Update after initial state");
+    extension = update_extension("2");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  // Grant one of the permissions and check it persists after update.
+  ScriptingPermissionsModifier(profile(), extension)
+      .GrantHostPermission(GURL(kHostGoogle));
+  {
+    SCOPED_TRACE("Granting single");
+    CheckActiveHostPermissions(*extension, {kHostGoogle}, {});
+    CheckWithheldHostPermissions(*extension, {kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Update after granting single");
+    // TODO(tjudkins): We shouldn't have to explicitly call to grant
+    // permissions here, but at the moment when withholding host permissions on
+    // installation and then granting a permission, the reload or update detects
+    // that as a privilege increase and disables the extension.
+    service()->GrantPermissionsAndEnableExtension(extension.get());
+    extension = update_extension("3");
+    CheckActiveHostPermissions(*extension, {kHostGoogle}, {});
+    CheckWithheldHostPermissions(*extension, {kHostChromium}, {});
+  }
+
+  // Set permissions not to be withheld at all and check it persists after
+  // update.
+  ScriptingPermissionsModifier(profile(), extension)
+      .SetWithholdHostPermissions(false);
+  {
+    SCOPED_TRACE("Setting to not withhold");
+    CheckActiveHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+    CheckWithheldHostPermissions(*extension, {}, {});
+  }
+
+  {
+    SCOPED_TRACE("Update after setting to not withhold");
+    // TODO(tjudkins): We shouldn't have to explicitly call to grant
+    // permissions here, but at the moment when withholding host permissions on
+    // installation and then granting a permission, the reload or update detects
+    // that as a privilege increase and disables the extension.
+    service()->GrantPermissionsAndEnableExtension(extension.get());
+    extension = update_extension("4");
+    CheckActiveHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+    CheckWithheldHostPermissions(*extension, {}, {});
+  }
+
+  // Finally, set permissions to be withheld again and check it persists after
+  // update.
+  ScriptingPermissionsModifier(profile(), extension)
+      .SetWithholdHostPermissions(true);
+  {
+    SCOPED_TRACE("Setting back to withhold");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
+  }
+
+  {
+    SCOPED_TRACE("Update after setting back to withhold");
+    extension = update_extension("5");
+    CheckActiveHostPermissions(*extension, {}, {});
+    CheckWithheldHostPermissions(*extension, {kHostGoogle, kHostChromium}, {});
   }
 }
 

chrome/browser/ui/webui/extensions/extensions_internals_source.cc

@@ -116,6 +116,8 @@ base::Value CreationFlagsToList(int creation_flags) {
     flags_value.Append("WAS_INSTALLED_BY_OEM");
   if (creation_flags & extensions::Extension::MAY_BE_UNTRUSTED)
     flags_value.Append("MAY_BE_UNTRUSTED");
+  if (creation_flags & extensions::Extension::WITHHOLD_PERMISSIONS)
+    flags_value.Append("WITHHOLD_PERMISSIONS");
   return flags_value;
 }
 

extensions/browser/extension_prefs.cc

@@ -1997,6 +1997,8 @@ void ExtensionPrefs::PopulateExtensionInfoPrefs(
     extension_dict->SetBoolean(kPrefBlacklist, true);
   if (dnr_ruleset_checksum)
     extension_dict->SetInteger(kPrefDNRRulesetChecksum, *dnr_ruleset_checksum);
+  if (extension->creation_flags() & Extension::WITHHOLD_PERMISSIONS)
+    extension_dict->SetBoolean(kGrantExtensionAllHostPermissions, false);
 
   base::FilePath::StringType path = MakePathRelative(install_directory_,
                                                      extension->path());

extensions/common/extension.cc

@@ -126,7 +126,7 @@ bool IsManifestSupported(int manifest_version,
 
 }  // namespace
 
-const int Extension::kInitFromValueFlagBits = 14;
+const int Extension::kInitFromValueFlagBits = 15;
 
 const char Extension::kMimeType[] = "application/x-chrome-extension";
 

extensions/common/extension.h

@@ -140,6 +140,10 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
     // instead of the usual |TYPE_EXTENSION|.
     FOR_LOGIN_SCREEN = 1 << 13,
 
+    // |WITHHOLD_PERMISSIONS| indicates that on installation the user indicated
+    // for permissions to be withheld from the extension by default.
+    WITHHOLD_PERMISSIONS = 1 << 14,
+
     // When adding new flags, make sure to update kInitFromValueFlagBits.
   };