PermissionsData: Prevent race on accessing default policy host restrictions.

This CL resolves a race where multiple threads can access the default policy
host restrictions. This can lead to crashes in the wild. Fix this by controlling
access to the default policy host restictions via a global lock.

BUG=978407

Change-Id: Ic5d5f4e42980017f4e0b4d56b1f4d9f79983223a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1700323
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677113}

extensions/browser/renderer_startup_helper.cc

@@ -138,9 +138,9 @@ void RendererStartupHelper::InitializeProcess(
   // of the ExtensionSettings policy.
   ExtensionMsg_UpdateDefaultPolicyHostRestrictions_Params params;
   params.default_policy_blocked_hosts =
-      PermissionsData::default_policy_blocked_hosts().Clone();
+      PermissionsData::default_policy_blocked_hosts();
   params.default_policy_allowed_hosts =
-      PermissionsData::default_policy_allowed_hosts().Clone();
+      PermissionsData::default_policy_allowed_hosts();
   process->Send(new ExtensionMsg_UpdateDefaultPolicyHostRestrictions(params));
 
   // Loaded extensions.

extensions/common/permissions/permissions_data.cc

@@ -7,7 +7,7 @@
 #include <utility>
 
 #include "base/command_line.h"
-#include "base/lazy_instance.h"
+#include "base/no_destructor.h"
 #include "base/stl_util.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
@@ -33,11 +33,24 @@ struct DefaultPolicyRestrictions {
   URLPatternSet allowed_hosts;
 };
 
-// URLs an extension can't interact with. An extension can override these
-// settings by declaring its own list of blocked and allowed hosts using
-// policy_blocked_hosts and policy_allowed_hosts.
-base::LazyInstance<DefaultPolicyRestrictions>::Leaky
-    default_policy_restrictions = LAZY_INSTANCE_INITIALIZER;
+// Lock to access the default policy restrictions. This should never be acquired
+// before PermissionsData instance level |runtime_lock_| to prevent deadlocks.
+base::Lock& GetDefaultPolicyRestrictionsLock() {
+  static base::NoDestructor<base::Lock> lock;
+  return *lock;
+}
+
+// Returns the default policy restrictions i.e. the URLs an extension can't
+// interact with. An extension can override these settings by declaring its own
+// list of blocked and allowed hosts using policy_blocked_hosts and
+// policy_allowed_hosts. Must be called with the default policy restriction lock
+// already acquired.
+DefaultPolicyRestrictions& GetDefaultPolicyRestrictions() {
+  static base::NoDestructor<DefaultPolicyRestrictions>
+      default_policy_restrictions;
+  GetDefaultPolicyRestrictionsLock().AssertAcquired();
+  return *default_policy_restrictions;
+}
 
 class AutoLockOnValidThread {
  public:
@@ -140,39 +153,33 @@ bool PermissionsData::AllUrlsIncludesChromeUrls(
 
 bool PermissionsData::UsesDefaultPolicyHostRestrictions() const {
   DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread());
-  return uses_default_policy_host_restrictions;
+  return uses_default_policy_host_restrictions_;
 }
 
-const URLPatternSet& PermissionsData::default_policy_blocked_hosts() {
-  return default_policy_restrictions.Get().blocked_hosts;
+URLPatternSet PermissionsData::default_policy_blocked_hosts() {
+  base::AutoLock lock(GetDefaultPolicyRestrictionsLock());
+  return GetDefaultPolicyRestrictions().blocked_hosts.Clone();
 }
 
-const URLPatternSet& PermissionsData::default_policy_allowed_hosts() {
-  return default_policy_restrictions.Get().allowed_hosts;
+URLPatternSet PermissionsData::default_policy_allowed_hosts() {
+  base::AutoLock lock(GetDefaultPolicyRestrictionsLock());
+  return GetDefaultPolicyRestrictions().allowed_hosts.Clone();
 }
 
-const URLPatternSet PermissionsData::policy_blocked_hosts() const {
-  base::AutoLock auto_lock(runtime_lock_);
-  return PolicyBlockedHostsUnsafe().Clone();
-}
-
-const URLPatternSet& PermissionsData::PolicyBlockedHostsUnsafe() const {
-  runtime_lock_.AssertAcquired();
-  if (uses_default_policy_host_restrictions)
+URLPatternSet PermissionsData::policy_blocked_hosts() const {
+  if (uses_default_policy_host_restrictions_)
     return default_policy_blocked_hosts();
-  return policy_blocked_hosts_unsafe_;
-}
 
-const URLPatternSet PermissionsData::policy_allowed_hosts() const {
   base::AutoLock auto_lock(runtime_lock_);
-  return PolicyAllowedHostsUnsafe().Clone();
+  return policy_blocked_hosts_unsafe_.Clone();
 }
 
-const URLPatternSet& PermissionsData::PolicyAllowedHostsUnsafe() const {
-  runtime_lock_.AssertAcquired();
-  if (uses_default_policy_host_restrictions)
+URLPatternSet PermissionsData::policy_allowed_hosts() const {
+  if (uses_default_policy_host_restrictions_)
     return default_policy_allowed_hosts();
-  return policy_allowed_hosts_unsafe_;
+
+  base::AutoLock auto_lock(runtime_lock_);
+  return policy_allowed_hosts_unsafe_.Clone();
 }
 
 void PermissionsData::BindToCurrentThread() const {
@@ -194,21 +201,22 @@ void PermissionsData::SetPolicyHostRestrictions(
   AutoLockOnValidThread lock(runtime_lock_, thread_checker_.get());
   policy_blocked_hosts_unsafe_ = policy_blocked_hosts.Clone();
   policy_allowed_hosts_unsafe_ = policy_allowed_hosts.Clone();
-  uses_default_policy_host_restrictions = false;
+  uses_default_policy_host_restrictions_ = false;
 }
 
 void PermissionsData::SetUsesDefaultHostRestrictions() const {
   AutoLockOnValidThread lock(runtime_lock_, thread_checker_.get());
-  uses_default_policy_host_restrictions = true;
+  uses_default_policy_host_restrictions_ = true;
 }
 
 // static
 void PermissionsData::SetDefaultPolicyHostRestrictions(
     const URLPatternSet& default_policy_blocked_hosts,
     const URLPatternSet& default_policy_allowed_hosts) {
-  default_policy_restrictions.Get().blocked_hosts =
+  base::AutoLock lock(GetDefaultPolicyRestrictionsLock());
+  GetDefaultPolicyRestrictions().blocked_hosts =
       default_policy_blocked_hosts.Clone();
-  default_policy_restrictions.Get().allowed_hosts =
+  GetDefaultPolicyRestrictions().allowed_hosts =
       default_policy_allowed_hosts.Clone();
 }
 
@@ -492,9 +500,17 @@ const PermissionSet* PermissionsData::GetTabSpecificPermissions(
 }
 
 bool PermissionsData::IsPolicyBlockedHostUnsafe(const GURL& url) const {
+  // We don't use [default_]policy_[blocked|allowed]_hosts() to avoid copying
+  // URLPatternSet.
+  if (uses_default_policy_host_restrictions_) {
+    base::AutoLock lock(GetDefaultPolicyRestrictionsLock());
+    return GetDefaultPolicyRestrictions().blocked_hosts.MatchesURL(url) &&
+           !GetDefaultPolicyRestrictions().allowed_hosts.MatchesURL(url);
+  }
+
   runtime_lock_.AssertAcquired();
-  return PolicyBlockedHostsUnsafe().MatchesURL(url) &&
-         !PolicyAllowedHostsUnsafe().MatchesURL(url);
+  return policy_blocked_hosts_unsafe_.MatchesURL(url) &&
+         !policy_allowed_hosts_unsafe_.MatchesURL(url);
 }
 
 PermissionsData::PageAccess PermissionsData::CanRunOnPage(

extensions/common/permissions/permissions_data.h

@@ -252,27 +252,27 @@ class PermissionsData {
   // This should only be used for 1. Serialization when initializing renderers
   // or 2. Called from utility methods above. For all other uses, call utility
   // methods instead (e.g. CanAccessPage()).
-  static const URLPatternSet& default_policy_blocked_hosts();
+  static URLPatternSet default_policy_blocked_hosts();
 
   // Returns list of hosts this extension may interact with regardless of
   // what is defined by policy_blocked_hosts().
   // This should only be used for 1. Serialization when initializing renderers
   // or 2. Called from utility methods above. For all other uses, call utility
   // methods instead (e.g. CanAccessPage()).
-  static const URLPatternSet& default_policy_allowed_hosts();
+  static URLPatternSet default_policy_allowed_hosts();
 
   // Returns list of hosts this extension may not interact with by policy.
   // This should only be used for 1. Serialization when initializing renderers
   // or 2. Called from utility methods above. For all other uses, call utility
   // methods instead (e.g. CanAccessPage()).
-  const URLPatternSet policy_blocked_hosts() const;
+  URLPatternSet policy_blocked_hosts() const;
 
   // Returns list of hosts this extension may interact with regardless of
   // what is defined by policy_blocked_hosts().
   // This should only be used for 1. Serialization when initializing renderers
   // or 2. Called from utility methods above. For all other uses, call utility
   // methods instead (e.g. CanAccessPage()).
-  const URLPatternSet policy_allowed_hosts() const;
+  URLPatternSet policy_allowed_hosts() const;
 
   // Check if a specific URL is blocked by policy from extension use at runtime.
   bool IsPolicyBlockedHost(const GURL& url) const {
@@ -308,14 +308,6 @@ class PermissionsData {
   // You must acquire the runtime_lock_ before calling.
   bool IsPolicyBlockedHostUnsafe(const GURL& url) const;
 
-  // Same as policy_blocked_hosts but instead returns a reference.
-  // You must acquire runtime_lock_ before calling this.
-  const URLPatternSet& PolicyBlockedHostsUnsafe() const;
-
-  // Same as policy_allowed_hosts but instead returns a reference.
-  // You must acquire runtime_lock_ before calling this.
-  const URLPatternSet& PolicyAllowedHostsUnsafe() const;
-
   // The associated extension's id.
   std::string extension_id_;
 
@@ -353,7 +345,7 @@ class PermissionsData {
 
   // If the ExtensionSettings policy is not being used, or no per-extension
   // exception to the default policy was declared for this extension.
-  mutable bool uses_default_policy_host_restrictions = true;
+  mutable bool uses_default_policy_host_restrictions_ = true;
 
   mutable TabPermissionsMap tab_specific_permissions_;