[Extensions Click-to-Script] Update permissions sectioning

Update the logic used to partition permissions between granted and
withheld with click-to-script. This patch addresses a few issues:
- Previously, permissions granted with runtime host permissions were
  added to the active permissions entry in the ExtensionPrefs. This
  meant there was a chance that extensions would continue to be
  affected after the feature was disabled.
- Previously, we would try to grant an entire origin to an extension,
  even if only a certain path was requested. This could result in a
  crash, since we later CHECK'd that all granted permissions were
  strictly contained within the withheld permissions of an extension.
- The user should not need to specify an exact URLPattern at the
  chrome://extensions page in order to grant permission to an extension.
  That is, the user should be able to allow the extension on
  "google.com", rather than
  "google.com/some-specific-path-the-extension-specified".
  (This is dependent on the changes here, but will be fully resolved in
  a followup.)

To fix this, we change runtime granted host permissions to only affect
the runtime granted permissions preference (and not change active
permissions in the preferences). When calculating permissions, we
determine the set of permissions requested by the extension and
intersect it with the permissions in the runtime granted permissions.

This patch also allows extensions to be granted more permissions than
they explicitly request. This means that if an extension requests only
https://google.com/maps, it can be granted https://google.com/*. This is
necessary for runtime host permissions, where permissions granted may be
broader than explicitly requested. For security purposes, even though
the granted permissions may exceed the requested permissions, the
permissions active on the extension object (and in the extension
process) will always be strictly bound by the requested permissions. To
achieve this, we leverage the new intersection logic in URLPatterns
introduced in https://crbug.com/867549.

These two changes are interdependent (and cannot be submitted as two
CLs) because by no longer adding runtime-granted permissions to the
active permissions set, we introduce a need to find the semantic
intersection of the runtime-granted permissions and the requested
permissions. Otherwise, because of the way we partitioned permissions,
we would not grant any permissions if an extension requested *://*/*
and the user granted https://google.com/*.

Bug: 867547

Change-Id: I161cc699c95989f86be5b1743aee422ac5b41221
Reviewed-on: https://chromium-review.googlesource.com/1153986
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579678}

chrome/browser/extensions/permissions_updater.h

@@ -78,7 +78,10 @@ class PermissionsUpdater {
 
   // Grants |permissions| that were withheld at installation and granted at
   // runtime to |extension|, updating the active permission set and notifying
-  // any observers.
+  // any observers. |permissions| may contain permissions that were not
+  // explicitly requested by the extension; if this happens, those permissions
+  // will be added to the runtime-granted permissions in the preferences, but
+  // will not be granted to the extension object or process itself.
   // NOTE: This should only be used for granting permissions through the runtime
   // host permissions feature.
   void GrantRuntimePermissions(const Extension& extension,
@@ -156,15 +159,15 @@ class PermissionsUpdater {
     kNone = 0,
     kGrantedPermissions = 1 << 0,
     kRuntimeGrantedPermissions = 1 << 1,
+    kActivePermissions = 1 << 2,
   };
 
-  // Sets the |extension|'s active permissions to |active| and records the
-  // change in the prefs. If |withheld| is non-null, also sets the extension's
-  // withheld permissions to |withheld|. Otherwise, |withheld| permissions are
-  // not changed.
+  // Sets the |extension|'s active permissions to |active|, and calculates and
+  // sets the |extension|'s new withheld permissions. If |update_prefs| is true,
+  // also updates the set of active permissions in the extension preferences.
   void SetPermissions(const Extension* extension,
                       std::unique_ptr<const PermissionSet> active,
-                      std::unique_ptr<const PermissionSet> withheld);
+                      bool update_prefs);
 
   // Dispatches specified event to the extension.
   void DispatchEvent(const std::string& extension_id,
@@ -189,17 +192,26 @@ class PermissionsUpdater {
       const URLPatternSet& default_runtime_blocked_hosts,
       const URLPatternSet& default_runtime_allowed_hosts);
 
-  // Adds the given |permissions| to |extension|. Updates the preferences
-  // according to |permission_store_mask|.
+  // Adds the given |active_permissions_to_add| to |extension|'s current
+  // active permissions (i.e., the permissions associated with the |extension|
+  // object and the extension's process). Updates the preferences according to
+  // |permission_store_mask| with |prefs_permissions_to_add|.
+  // The sets of |prefs_permissions_to_add| and |active_permissions_to_add| may
+  // differ in the case of granting a wider set of permissions than what the
+  // extension explicitly requested, as described in GrantRuntimePermissions().
   void AddPermissionsImpl(const Extension& extension,
-                          const PermissionSet& permissions,
-                          int permission_store_mask);
-
-  // Removes the given |permissions| to |extension|. Updates the preferences
-  // according to |permission_store_mask|.
+                          const PermissionSet& active_permissions_to_add,
+                          int permission_store_mask,
+                          const PermissionSet& prefs_permissions_to_add);
+
+  // Removes the given |active_permissions_to_remove| from |extension|'s current
+  // active permissions. Updates the preferences according to
+  // |permission_store_mask| with |prefs_permissions_to_remove|. As above, the
+  // permission sets may be different.
   void RemovePermissionsImpl(const Extension& extension,
-                             const PermissionSet& permissions,
-                             int permission_store_mask);
+                             const PermissionSet& active_permissions_to_remove,
+                             int permission_store_mask,
+                             const PermissionSet& prefs_permissions_to_remove);
 
   // The associated BrowserContext.
   content::BrowserContext* browser_context_;

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -732,4 +732,116 @@ TEST_F(PermissionsUpdaterTest, ChromeFaviconIsNotARevokableHost) {
   }
 }
 
+// Tests runtime-granting permissions beyond what are explicitly requested by
+// the extension.
+TEST_F(PermissionsUpdaterTest, GrantingBroadRuntimePermissions) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kRuntimeHostPermissions);
+  InitializeEmptyExtensionService();
+
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("extension")
+          .AddPermission("https://maps.google.com/*")
+          .Build();
+
+  const URLPattern kMapsPattern(Extension::kValidHostPermissionSchemes,
+                                "https://maps.google.com/*");
+  const URLPattern kAllGooglePattern(Extension::kValidHostPermissionSchemes,
+                                     "https://*.google.com/*");
+
+  // Withhold host permissions. Effective hosts should be empty.
+  PermissionsUpdater updater(profile());
+  updater.InitializePermissions(extension.get());
+  ScriptingPermissionsModifier(profile(), extension)
+      .SetWithholdHostPermissions(true);
+  EXPECT_TRUE(extension->permissions_data()
+                  ->active_permissions()
+                  .effective_hosts()
+                  .is_empty());
+
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+  {
+    // Verify initial state. The extension "active" permissions in preferences
+    // represent the permissions that would be active on the extension without
+    // the runtime host permissions feature. Thus, this should include the
+    // requested host permissions, and nothing more.
+    std::unique_ptr<const PermissionSet> active_prefs =
+        prefs->GetActivePermissions(extension->id());
+    EXPECT_TRUE(active_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_FALSE(
+        active_prefs->effective_hosts().ContainsPattern(kAllGooglePattern));
+
+    // Runtime granted permissions should not contain any permissions (all
+    // hosts are withheld).
+    std::unique_ptr<const PermissionSet> runtime_granted_prefs =
+        prefs->GetRuntimeGrantedPermissions(extension->id());
+    EXPECT_FALSE(
+        runtime_granted_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_FALSE(runtime_granted_prefs->effective_hosts().ContainsPattern(
+        kAllGooglePattern));
+  }
+
+  // Grant permission to all google.com domains.
+  const PermissionSet runtime_permissions(
+      APIPermissionSet(), ManifestPermissionSet(),
+      URLPatternSet({kAllGooglePattern}), URLPatternSet());
+  updater.GrantRuntimePermissions(*extension, runtime_permissions);
+
+  // The extension object's permission should never include un-requested
+  // permissions, so it should only include maps.google.com.
+  EXPECT_TRUE(extension->permissions_data()
+                  ->active_permissions()
+                  .effective_hosts()
+                  .ContainsPattern(kMapsPattern));
+  EXPECT_FALSE(extension->permissions_data()
+                   ->active_permissions()
+                   .effective_hosts()
+                   .ContainsPattern(kAllGooglePattern));
+
+  {
+    // The active permissions in preferences should reflect the extension's
+    // permission state without the runtime host permissions feature, so should
+    // still include exactly the requested permissions.
+    std::unique_ptr<const PermissionSet> active_prefs =
+        prefs->GetActivePermissions(extension->id());
+    EXPECT_TRUE(active_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_FALSE(
+        active_prefs->effective_hosts().ContainsPattern(kAllGooglePattern));
+    // The runtime-granted permissions should include all permissions that have
+    // been granted, which in this case includes google.com subdomains.
+    std::unique_ptr<const PermissionSet> runtime_granted_prefs =
+        prefs->GetRuntimeGrantedPermissions(extension->id());
+    EXPECT_TRUE(
+        runtime_granted_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_TRUE(runtime_granted_prefs->effective_hosts().ContainsPattern(
+        kAllGooglePattern));
+  }
+
+  // Revoke the host permission.
+  updater.RevokeRuntimePermissions(*extension, runtime_permissions);
+
+  EXPECT_FALSE(extension->permissions_data()
+                   ->active_permissions()
+                   .effective_hosts()
+                   .ContainsPattern(kMapsPattern));
+
+  {
+    // Active permissions in the preferences should remain constant (unaffected
+    // by the runtime host permissions feature).
+    std::unique_ptr<const PermissionSet> active_prefs =
+        prefs->GetActivePermissions(extension->id());
+    EXPECT_TRUE(active_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_FALSE(
+        active_prefs->effective_hosts().ContainsPattern(kAllGooglePattern));
+    // The runtime granted preferences should be empty again.
+    std::unique_ptr<const PermissionSet> runtime_granted_prefs =
+        prefs->GetRuntimeGrantedPermissions(extension->id());
+    EXPECT_FALSE(
+        runtime_granted_prefs->effective_hosts().ContainsPattern(kMapsPattern));
+    EXPECT_FALSE(runtime_granted_prefs->effective_hosts().ContainsPattern(
+        kAllGooglePattern));
+  }
+}
+
 }  // namespace extensions

chrome/browser/extensions/scripting_permissions_modifier.h

@@ -74,17 +74,15 @@ class ScriptingPermissionsModifier {
 
   // 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, and
-  // |withheld_permissions_out| with the set of all withheld permissions. 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.
+  // with the set of all 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.
   static void WithholdPermissionsIfNecessary(
       const Extension& extension,
       const ExtensionPrefs& extension_prefs,
       const PermissionSet& permissions,
-      std::unique_ptr<const PermissionSet>* granted_permissions_out,
-      std::unique_ptr<const PermissionSet>* withheld_permissions_out);
+      std::unique_ptr<const PermissionSet>* granted_permissions_out);
 
   // Returns the subset of active permissions which can be withheld.
   std::unique_ptr<const PermissionSet> GetRevokablePermissions() const;

chrome/browser/extensions/scripting_permissions_modifier_unittest.cc

@@ -512,4 +512,57 @@ TEST_F(ScriptingPermissionsModifierUnitTest,
   EXPECT_THAT(GetEffectivePatternsAsStrings(*extension), testing::IsEmpty());
 }
 
+// Tests granting runtime permissions for a full host when the extension only
+// wants to run on a subset of that host.
+TEST_F(ScriptingPermissionsModifierUnitTest,
+       GrantingHostPermissionsBeyondRequested) {
+  RuntimeHostPermissionsEnabledScope enabled_scope;
+  InitializeEmptyExtensionService();
+
+  DictionaryBuilder content_script;
+  content_script
+      .Set("matches", ListBuilder().Append("https://google.com/maps").Build())
+      .Set("js", ListBuilder().Append("foo.js").Build());
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("test")
+          .SetManifestKey("content_scripts",
+                          ListBuilder().Append(content_script.Build()).Build())
+          .Build();
+
+  // At installation, all permissions granted.
+  ScriptingPermissionsModifier modifier(profile(), extension);
+  EXPECT_THAT(GetEffectivePatternsAsStrings(*extension),
+              testing::UnorderedElementsAre("https://google.com/maps"));
+
+  // Withhold host permissions.
+  modifier.SetWithholdHostPermissions(true);
+  EXPECT_THAT(GetEffectivePatternsAsStrings(*extension), testing::IsEmpty());
+
+  // Grant the requested host permission. We grant origins (rather than just
+  // paths), but we don't over-grant permissions to the actual extension object.
+  // The active permissions on the extension should be restricted to the
+  // permissions explicitly requested (google.com/maps), but the granted
+  // permissions in preferences will be the full host (google.com).
+  modifier.GrantHostPermission(GURL("https://google.com/maps"));
+  EXPECT_THAT(GetEffectivePatternsAsStrings(*extension),
+              testing::UnorderedElementsAre("https://google.com/maps"));
+  EXPECT_THAT(GetPatternsAsStrings(
+                  modifier.GetRevokablePermissions()->effective_hosts()),
+              // Subtle: revokable permissions include permissions either in
+              // the runtime granted permissions preference or active on the
+              // extension object. In this case, that includes both google.com/*
+              // and google.com/maps.
+              testing::UnorderedElementsAre("https://google.com/maps",
+                                            "https://google.com/*"));
+
+  // Remove the granted permission. This should remove the permission from both
+  // the active permissions on the extension object and the entry in the
+  // preferences.
+  modifier.RemoveAllGrantedHostPermissions();
+  EXPECT_THAT(GetEffectivePatternsAsStrings(*extension), testing::IsEmpty());
+  EXPECT_THAT(GetPatternsAsStrings(
+                  modifier.GetRevokablePermissions()->effective_hosts()),
+              testing::IsEmpty());
+}
+
 }  // namespace extensions

extensions/common/permissions/permission_set.cc

@@ -73,7 +73,8 @@ std::unique_ptr<const PermissionSet> PermissionSet::CreateDifference(
 // static
 std::unique_ptr<const PermissionSet> PermissionSet::CreateIntersection(
     const PermissionSet& set1,
-    const PermissionSet& set2) {
+    const PermissionSet& set2,
+    URLPatternSet::IntersectionBehavior intersection_behavior) {
   APIPermissionSet apis;
   APIPermissionSet::Intersection(set1.apis(), set2.apis(), &apis);
 
@@ -82,15 +83,10 @@ std::unique_ptr<const PermissionSet> PermissionSet::CreateIntersection(
                                       set2.manifest_permissions(),
                                       &manifest_permissions);
 
-  // TODO(https://crbug.com/867549): Audit callers of CreateIntersection() and
-  // determine what the proper intersection behavior is. Likely, we'll want to
-  // introduce an argument to specify it.
-  constexpr auto kIntersectionBehavior =
-      URLPatternSet::IntersectionBehavior::kPatternsContainedByBoth;
   URLPatternSet explicit_hosts = URLPatternSet::CreateIntersection(
-      set1.explicit_hosts(), set2.explicit_hosts(), kIntersectionBehavior);
+      set1.explicit_hosts(), set2.explicit_hosts(), intersection_behavior);
   URLPatternSet scriptable_hosts = URLPatternSet::CreateIntersection(
-      set1.scriptable_hosts(), set2.scriptable_hosts(), kIntersectionBehavior);
+      set1.scriptable_hosts(), set2.scriptable_hosts(), intersection_behavior);
 
   return base::WrapUnique(new PermissionSet(apis, manifest_permissions,
                                             explicit_hosts, scriptable_hosts));

extensions/common/permissions/permission_set.h

@@ -48,9 +48,13 @@ class PermissionSet {
 
   // Creates a new permission set equal to the intersection of |set1| and
   // |set2|.
+  // TODO(https://crbug.com/867549): Audit callers of CreateIntersection() and
+  // have them determine the proper intersection behavior.
   static std::unique_ptr<const PermissionSet> CreateIntersection(
       const PermissionSet& set1,
-      const PermissionSet& set2);
+      const PermissionSet& set2,
+      URLPatternSet::IntersectionBehavior intersection_behavior =
+          URLPatternSet::IntersectionBehavior::kPatternsContainedByBoth);
 
   // Creates a new permission set equal to the union of |set1| and |set2|.
   static std::unique_ptr<const PermissionSet> CreateUnion(