[Backport] CVE-2018-18345

Lock down blob/filesystem URL creation with a stronger CPSP::CanCommitURL()

ChildProcessSecurityPolicy::CanCommitURL() is a security check that's
supposed to tell whether a given renderer process is allowed to commit
a given URL.  It is currently used to validate (1) blob and filesystem
URL creation, and (2) Origin headers.  Currently, it has scheme-based
checks that disallow things like web renderers creating
blob/filesystem URLs in chrome-extension: origins, but it cannot stop
one web origin from creating those URLs for another origin.

This CL locks down its use for (1) to also consult
CanAccessDataForOrigin().  With site isolation, this will check origin
locks and ensure that foo.com cannot create blob/filesystem URLs for
other origins.

For now, this CL does not provide the same enforcements for (2),
Origin header validation, which has additional constraints that need
to be solved first (see https://crbug.com/515309).

Bug: 886976, 888001
Reviewed-on: https://chromium-review.googlesource.com/1235343
Change-Id: I7f784240ff7a0295e03f786317e8f227e0fceceb
Reviewed-by: Michael Brüning <michael.bruning@qt.io>
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

2 files changed, 39 insertions, 9 deletions

diff --git a/chromium/content/browser/child_process_security_policy_impl.cc b/chromium/content/browser/child_process_security_policy_impl.cc
index f75d7083620..f9f08dfe049 100644
--- a/chromium/content/browser/child_process_security_policy_impl.cc
+++ b/chromium/content/browser/child_process_security_policy_impl.cc
@@ -664,7 +664,8 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL(
 }
 
 bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
-                                                  const GURL& url) {
+                                                  const GURL& url,
+                                                  bool check_origin_locks) {
   if (!url.is_valid())
     return false;  // Can't commit invalid URLs.
 
@@ -679,9 +680,19 @@ bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
       return false;
 
     url::Origin origin(url);
-    return origin.unique() || CanCommitURL(child_id, GURL(origin.Serialize()));
+    return origin.unique() ||
+           CanCommitURL(child_id, GURL(origin.Serialize()), check_origin_locks);
   }
 
+  // With site isolation, a URL from a site may only be committed in a process
+  // dedicated to that site.  This check will ensure that |url| can't commit if
+  // the process is locked to a different site.  Note that this check is only
+  // effective for processes that are locked to a site, but even with strict
+  // site isolation, currently not all processes are locked (e.g., extensions
+  // or <webview> tags - see ShouldLockToOrigin()).
+  if (check_origin_locks && !CanAccessDataForOrigin(child_id, url))
+    return false;
+
   {
     base::AutoLock lock(lock_);
 
@@ -689,12 +700,7 @@ bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
     // schemes_okay_to_commit_in_any_process_ here, which is stricter than
     // IsWebSafeScheme().
     //
-    // TODO(creis, nick): https://crbug.com/515309: in generalized Site
-    // Isolation and/or --site-per-process, there will be no such thing as a
-    // scheme that is okay to commit in any process. Instead, an URL from a site
-    // that is isolated may only be committed in a process dedicated to that
-    // site, so CanCommitURL will need to rely on explicit, per-process grants.
-    // Note how today, even with extension isolation, the line below does not
+    // TODO(creis, nick): https://crbug.com/515309: The line below does not
     // enforce that http pages cannot commit in an extension process.
     if (base::ContainsKey(schemes_okay_to_commit_in_any_process_, url.scheme()))
       return true;
@@ -709,6 +715,11 @@ bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
   }
 }
 
+bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
+                                                  const GURL& url) {
+  return CanCommitURL(child_id, url, true /* check_origin_lock */);
+}
+
 bool ChildProcessSecurityPolicyImpl::CanSetAsOriginHeader(int child_id,
                                                           const GURL& url) {
   if (!url.is_valid())
@@ -723,7 +734,12 @@ bool ChildProcessSecurityPolicyImpl::CanSetAsOriginHeader(int child_id,
 
   // If this process can commit |url|, it can use |url| as an origin for
   // outbound requests.
-  if (CanCommitURL(child_id, url))
+  //
+  // TODO(alexmos): This should eventually also check the origin lock, but
+  // currently this is not done due to certain corner cases involving HTML
+  // imports and layout tests that simulate requests from isolated worlds.  See
+  // https://crbug.com/515309.
+  if (CanCommitURL(child_id, url, false /* check_origin_lock */))
     return true;
 
   // Allow schemes which may come from scripts executing in isolated worlds;
diff --git a/chromium/content/browser/child_process_security_policy_impl.h b/chromium/content/browser/child_process_security_policy_impl.h
index 15c8d886d9b..6b560db8ce9 100644
--- a/chromium/content/browser/child_process_security_policy_impl.h
+++ b/chromium/content/browser/child_process_security_policy_impl.h
@@ -148,6 +148,20 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // Revoke read raw cookies permission.
   void RevokeReadRawCookies(int child_id);
 
+  // A version of the public ChildProcessSecurityPolicy::CanCommitURL() which
+  // takes an additional bool |check_origin_lock|, specifying whether to
+  // reject |url| if it does not match the origin lock on process |child_id|.
+  // Passing true for |check_origin_lock| provides stronger enforcement with
+  // strict site isolation; it is only set to false by features (e.g., Origin
+  // header validation) that aren't yet ready for this enforcement. This
+  // function should *not* be used by new features; use the public
+  // ChildProcessSecurityPolicy::CanCommitURL() instead, which internally calls
+  // this with |check_origin_lock| being true.
+  //
+  // TODO(alexmos): Remove |check_origin_lock| and check origin locks
+  // unconditionally once https://crbug.com/515309 is fixed.
+  bool CanCommitURL(int child_id, const GURL& url, bool check_origin_lock);
+
   // Whether the given origin is valid for an origin header. Valid origin
   // headers are commitable URLs plus suborigin URLs.
   bool CanSetAsOriginHeader(int child_id, const GURL& url);