[Extensions] Match wildcards when trying to determine increasing host privileges.

Bug: 65337
Change-Id: I60eccbb32f782f98f147c95f8bbbebf7f92a7c34
Reviewed-on: https://chromium-review.googlesource.com/1082009
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565034}

chrome/common/extensions/permissions/chrome_permission_message_provider.cc

@@ -229,9 +229,6 @@ bool ChromePermissionMessageProvider::IsHostPrivilegeIncrease(
   const URLPatternSet& granted_list = granted_permissions.effective_hosts();
   const URLPatternSet& requested_list = requested_permissions.effective_hosts();
 
-  // TODO(jstritar): This is overly conservative with respect to subdomains.
-  // For example, going from *.google.com to www.google.com will be
-  // considered an elevation, even though it is not (http://crbug.com/65337).
   std::set<std::string> requested_hosts_set(
       permission_message_util::GetDistinctHosts(requested_list, false, false));
   std::set<std::string> granted_hosts_set(
@@ -240,7 +237,34 @@ bool ChromePermissionMessageProvider::IsHostPrivilegeIncrease(
       base::STLSetDifference<std::set<std::string>>(requested_hosts_set,
                                                     granted_hosts_set);
 
-  return !requested_hosts_only.empty();
+  // Try to match any domain permissions against existing domain permissions
+  // that overlap, so that migrating from *.example.com -> foo.example.com
+  // does not constitute a permissions increase, even though the strings are
+  // not exactly the same.
+  for (const auto& requested : requested_hosts_only) {
+    bool host_matched = false;
+    const base::StringPiece unmatched(requested);
+    for (const auto& granted : granted_hosts_set) {
+      if (granted.size() > 2 && granted[0] == '*' && granted[1] == '.') {
+        const base::StringPiece stripped_granted(granted.data() + 1,
+                                                 granted.length() - 1);
+        // If the unmatched host ends with the the granted host,
+        // after removing the '*', then it's a match. In addition,
+        // because we consider having access to "*.domain.com" as
+        // granting access to "domain.com" then compare the string
+        // with both the "*" and the "." removed.
+        if (unmatched.ends_with(stripped_granted) ||
+            unmatched == stripped_granted.substr(1)) {
+          host_matched = true;
+          break;
+        }
+      }
+    }
+    if (!host_matched) {
+      return true;
+    }
+  }
+  return false;
 }
 
 }  // namespace extensions

chrome/common/extensions/permissions/permission_set_unittest.cc

@@ -1582,87 +1582,131 @@ TEST(PermissionsTest, GetDistinctHosts_FirstInListIs4thBestRcd) {
 }
 
 TEST(PermissionsTest, IsHostPrivilegeIncrease) {
-  Manifest::Type type = Manifest::TYPE_EXTENSION;
+  const struct {
+    struct host_spec {
+      int schemes;
+      std::string pattern;
+    };
+    std::vector<host_spec> initial_hosts;
+    std::vector<host_spec> final_hosts;
+    Manifest::Type type;
+    bool is_increase;
+    bool reverse_is_increase;
+  } test_cases[] = {
+      // Order doesn't matter.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://www.google.com/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       false},
+      // Paths are ignored.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://www.google.com/*"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       false},
+      // RCDs are ignored.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/*"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       false},
+      // Subdomain wildcards are handled properly.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://*.google.com.hk/*"}},
+       Manifest::TYPE_EXTENSION,
+       true,
+       false},
+      // Different domains count as different hosts.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://www.google.com/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.example.org/path"}},
+       Manifest::TYPE_EXTENSION,
+       true,
+       false},
+      // Different subdomains count as different hosts.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://mail.google.com/*"}},
+       Manifest::TYPE_EXTENSION,
+       true,
+       true},
+      // Moving from all subdomains to the domain should not be
+      // an increase in permissions. However, moving from just
+      // the domain to all of the subdomains should be.
+      {{{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://*.google.com/*"}},
+       {{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://google.com/*"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       true},
+      // Platform apps should not have host permissions increases.
+      {{{URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"},
+        {URLPattern::SCHEME_HTTP, "http://www.google.com/path"}},
+       {{URLPattern::SCHEME_HTTP, "http://mail.google.com/*"}},
+       Manifest::TYPE_PLATFORM_APP,
+       false,
+       false},
+      // Test that subdomain wildcard matching from crbug.com://65337
+      // works.
+      {{{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://*.google.com/"},
+        {URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://mail.google.com/"}},
+       {{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://inbox.google.com/"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       true},
+      // Test the "all_urls" meta-pattern.
+      {{{URLPattern::SCHEME_ALL, "<all_urls>"}},
+       {{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS,
+         "*://inbox.google.com/"}},
+       Manifest::TYPE_EXTENSION,
+       false,
+       true},
+      // Test expanding from any .com host to any host in any TLD.
+      // TODO(crbug.com/849906): Should this really be a permissions increase?
+      {{{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, "*://*.com/*"}},
+       {{URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS, "*://*/*"}},
+       Manifest::TYPE_EXTENSION,
+       true,
+       false},
+  };
+  const ManifestPermissionSet empty_manifest_permissions;
+  const APIPermissionSet empty_permissions;
+  const URLPatternSet empty_scriptable_hosts;
   const PermissionMessageProvider* provider = PermissionMessageProvider::Get();
-  ManifestPermissionSet empty_manifest_permissions;
-  URLPatternSet elist1;
-  URLPatternSet elist2;
-  URLPatternSet slist1;
-  URLPatternSet slist2;
-  std::unique_ptr<const PermissionSet> set1;
-  std::unique_ptr<const PermissionSet> set2;
-  APIPermissionSet empty_perms;
-  elist1.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"));
-  elist1.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
-
-  // Test that the host order does not matter.
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"));
-
-  set1.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist1,
-                               slist1));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set2, *set1, type));
-
-  // Test that paths are ignored.
-  elist2.ClearPatterns();
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/*"));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set2, *set1, type));
-
-  // Test that RCDs are ignored.
-  elist2.ClearPatterns();
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/*"));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set2, *set1, type));
-
-  // Test that subdomain wildcards are handled properly.
-  elist2.ClearPatterns();
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://*.google.com.hk/*"));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-  EXPECT_TRUE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  // TODO(jstritar): Does not match subdomains properly. http://crbug.com/65337
-  // EXPECT_FALSE(provider->IsPrivilegeIncrease(set2, set1, type));
-
-  // Test that different domains count as different hosts.
-  elist2.ClearPatterns();
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://www.example.org/path"));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-  EXPECT_TRUE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set2, *set1, type));
-
-  // Test that different subdomains count as different hosts.
-  elist2.ClearPatterns();
-  elist2.AddPattern(
-      URLPattern(URLPattern::SCHEME_HTTP, "http://mail.google.com/*"));
-  set2.reset(new PermissionSet(empty_perms, empty_manifest_permissions, elist2,
-                               slist2));
-  EXPECT_TRUE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_TRUE(provider->IsPrivilegeIncrease(*set2, *set1, type));
-
-  // Test that platform apps do not have host permissions increases.
-  type = Manifest::TYPE_PLATFORM_APP;
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set1, *set2, type));
-  EXPECT_FALSE(provider->IsPrivilegeIncrease(*set2, *set1, type));
+  for (size_t i = 0; i < base::size(test_cases); ++i) {
+    URLPatternSet explicit_hosts1;
+    URLPatternSet explicit_hosts2;
+    const auto& test_case = test_cases[i];
+    for (const auto& initial_host : test_case.initial_hosts) {
+      explicit_hosts1.AddPattern(
+          URLPattern(initial_host.schemes, initial_host.pattern));
+    }
+    for (const auto& final_host : test_case.final_hosts) {
+      explicit_hosts2.AddPattern(
+          URLPattern(final_host.schemes, final_host.pattern));
+    }
+    const PermissionSet set1(empty_permissions, empty_manifest_permissions,
+                             explicit_hosts1, empty_scriptable_hosts);
+    const PermissionSet set2(empty_permissions, empty_manifest_permissions,
+                             explicit_hosts2, empty_scriptable_hosts);
+    EXPECT_EQ(test_case.is_increase,
+              provider->IsPrivilegeIncrease(set1, set2, test_case.type))
+        << "Failure at index " << i;
+    EXPECT_EQ(test_case.reverse_is_increase,
+              provider->IsPrivilegeIncrease(set2, set1, test_case.type))
+        << "Failure at index " << i;
+  }
 }
 
 TEST(PermissionsTest, GetAPIsAsStrings) {