[DNR] Check for all hosts for API permission privilege increase

If an extension uses all hosts (e.g. by specifying <all_urls> in host
permissions), an API permission kHostsAll is created somewhere down the
line. This permission is used to provide a permission message, but
kHostsAll is not added as an API permission when checking if there is
a privilege increase between a set of granted vs requested API
permissions. As a result, an extension can be reported for a privilege
increase even when it shouldn't.

This CL fixes this issue by adding kHostsAll for privilege increase
checks to make sure the right messages are compared.

Bug: 1014505, 512344
Change-Id: Ie20045c6760b50dae5949b9cceaf10467fcf1534
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2042387Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741542}

chrome/common/extensions/permissions/chrome_permission_message_provider.cc

@@ -106,18 +106,6 @@ void ChromePermissionMessageProvider::AddAPIPermissions(
     PermissionIDSet* permission_ids) const {
   for (const APIPermission* permission : permissions.apis())
     permission_ids->InsertAll(permission->GetPermissions());
-
-  // A special hack: The warning message for declarativeWebRequest
-  // permissions speaks about blocking parts of pages, which is a
-  // subset of what the "<all_urls>" access allows. Therefore we
-  // display only the "<all_urls>" warning message if both permissions
-  // are required.
-  // TODO(treib): The same should apply to other permissions that are implied by
-  // "<all_urls>" (aka APIPermission::kHostsAll), such as kTab. This would
-  // happen automatically if we didn't differentiate between API/Manifest/Host
-  // permissions here.
-  if (permissions.ShouldWarnAllHosts())
-    permission_ids->erase(APIPermission::kDeclarativeWebRequest);
 }
 
 void ChromePermissionMessageProvider::AddManifestPermissions(
@@ -161,12 +149,20 @@ bool ChromePermissionMessageProvider::IsAPIOrManifestPrivilegeIncrease(
   AddAPIPermissions(granted_permissions, &granted_ids);
   AddManifestPermissions(granted_permissions, &granted_ids);
 
+  // <all_urls> is processed as APIPermission::kHostsAll and should be included
+  // when checking permission messages.
+  if (granted_permissions.ShouldWarnAllHosts())
+    granted_ids.insert(APIPermission::kHostsAll);
+
   // We compare |granted_ids| against the set of permissions that would be
   // granted if the requested permissions are allowed.
   PermissionIDSet potential_total_ids = granted_ids;
   AddAPIPermissions(requested_permissions, &potential_total_ids);
   AddManifestPermissions(requested_permissions, &potential_total_ids);
 
+  if (requested_permissions.ShouldWarnAllHosts())
+    potential_total_ids.insert(APIPermission::kHostsAll);
+
   // For M62, we added a new permission ID for new tab page overrides. Consider
   // the addition of this permission to not result in a privilege increase for
   // the time being.

chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc

@@ -53,12 +53,14 @@ class ChromePermissionMessageProviderUnittest : public testing::Test {
   }
 
   bool IsPrivilegeIncrease(const APIPermissionSet& granted_permissions,
-                           const APIPermissionSet& requested_permissions) {
+                           const URLPatternSet& granted_hosts,
+                           const APIPermissionSet& requested_permissions,
+                           const URLPatternSet& requested_hosts) {
     return message_provider_->IsPrivilegeIncrease(
         PermissionSet(granted_permissions.Clone(), ManifestPermissionSet(),
-                      URLPatternSet(), URLPatternSet()),
+                      granted_hosts.Clone(), URLPatternSet()),
         PermissionSet(requested_permissions.Clone(), ManifestPermissionSet(),
-                      URLPatternSet(), URLPatternSet()),
+                      requested_hosts.Clone(), URLPatternSet()),
         Manifest::TYPE_EXTENSION);
   }
 
@@ -202,4 +204,31 @@ TEST_F(ChromePermissionMessageProviderUnittest, PowerfulPermissions) {
   }
 }
 
+// Checks that granted hosts that may cause API permission messages are
+// processed as part of IsPrivilegeIncrease. Regression test for
+// crbug.com/1014505.
+TEST_F(ChromePermissionMessageProviderUnittest, PrivilegeIncreaseAllUrls) {
+  APIPermissionSet granted_permissions;
+  granted_permissions.insert(APIPermission::kWebRequest);
+
+  extensions::URLPatternSet granted_hosts;
+  granted_hosts.AddPattern(URLPattern(URLPattern::SCHEME_ALL, "<all_urls>"));
+
+  APIPermissionSet requested_permissions;
+  requested_permissions.insert(APIPermission::kWebRequest);
+  requested_permissions.insert(APIPermission::kDeclarativeNetRequest);
+
+  extensions::URLPatternSet requested_hosts;
+  requested_hosts.AddPattern(URLPattern(URLPattern::SCHEME_ALL, "<all_urls>"));
+
+  // While |kDeclarativeNetRequest| would cause a permission message, the
+  // inclusion of <all_urls> for both granted and request permissions should
+  // subsume the permission message for |kDeclarativeNetRequest| with its own
+  // message. Since this message would be identical between
+  // |granted_permissions| and |requested_permissions|, there should not be a
+  // privilege increase.
+  EXPECT_FALSE(IsPrivilegeIncrease(granted_permissions, granted_hosts,
+                                   requested_permissions, requested_hosts));
+}
+
 }  // namespace extensions

chrome/common/extensions/permissions/permission_set_unittest.cc

@@ -59,12 +59,6 @@ size_t IndexOf(const PermissionMessages& warnings, const std::string& warning) {
   return warnings.size();
 }
 
-PermissionIDSet MakePermissionIDSet(APIPermission::ID id) {
-  PermissionIDSet set;
-  set.insert(id);
-  return set;
-}
-
 PermissionIDSet MakePermissionIDSet(APIPermission::ID id1,
                                     APIPermission::ID id2) {
   PermissionIDSet set;
@@ -672,8 +666,7 @@ TEST(PermissionsTest, IsPrivilegeIncrease) {
       {"hosts6", false},        // http://a.com -> http://a.com + http://a.co.uk
       {"permissions1", false},  // tabs -> tabs
       {"permissions2", true},   // tabs -> tabs,bookmarks
-      // TODO(crbug.com/512344): This is wrong, kAllHosts implies kTabs.
-      {"permissions3", true},          // http://*/* -> http://*/*,tabs
+      {"permissions3", false},  // http://*/* -> http://*/*,tabs
       {"permissions5", true},   // bookmarks -> bookmarks,history
       {"equivalent_warnings", false},  // tabs --> tabs, webNavigation
 
@@ -969,7 +962,7 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kTab);
     URLPatternSet hosts;
-    hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
+    hosts.AddPattern(URLPattern(URLPattern::SCHEME_HTTP, "*://*/*"));
     PermissionSet permissions(std::move(api_permissions),
                               ManifestPermissionSet(), std::move(hosts),
                               URLPatternSet());
@@ -982,7 +975,7 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kTopSites);
     URLPatternSet hosts;
-    hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
+    hosts.AddPattern(URLPattern(URLPattern::SCHEME_HTTP, "*://*/*"));
     PermissionSet permissions(std::move(api_permissions),
                               ManifestPermissionSet(), std::move(hosts),
                               URLPatternSet());
@@ -996,13 +989,14 @@ TEST(PermissionsTest, SuppressedPermissionMessages) {
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kDeclarativeWebRequest);
     URLPatternSet hosts;
-    hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
+    hosts.AddPattern(URLPattern(URLPattern::SCHEME_HTTP, "*://*/*"));
     PermissionSet permissions(std::move(api_permissions),
                               ManifestPermissionSet(), std::move(hosts),
                               URLPatternSet());
     EXPECT_TRUE(PermissionSetProducesMessage(
         permissions, Manifest::TYPE_EXTENSION,
-        MakePermissionIDSet(APIPermission::kHostsAll)));
+        MakePermissionIDSet(APIPermission::kHostsAll,
+                            APIPermission::kDeclarativeWebRequest)));
   }
   {
     // BrowsingHistory warning suppresses all history read/write warnings.