|request_initiator_site_lock = <opaque>| for the process-wide factory.

The CL modifies URLLoaderFactoryParamsHelper::CreateForRendererProcess
so that it sets |request_initiator_site_lock| of the process-wide
factory to an opaque origin.  This change is based on
https://crbug.com/1105794 which shows that there are indeed no cases in
the wild where a process-wide factory is used with a non-opaque
initiator origin.  This intuitively makes sense - the process-wide
factory should only be used in frames that have not yet committed any
navigation and such frame should not (yet) have a non-opaque origin.

Additionally, thanks to the change described above, there is no longer a
need for site/eTLD+1 comparisons in VerifyRequestInitiatorLock nor for
SiteInstanceImpl::GetRequestInitiatorSiteLock.

Fixed: 1098938
Change-Id: I9e8b53139a2418636c84e783f86ea8d7be34eed9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2274591Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797542}

content/browser/renderer_host/render_process_host_impl.h

@@ -942,7 +942,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Creates a URLLoaderFactory that can be used by the renderer process,
   // without binding it to a specific frame or an origin.
   //
-  // TODO(kinuko, lukasza): https://crbug.com/1098938: Remove, once all
+  // TODO(kinuko, lukasza): https://crbug.com/1114822: Remove, once all
   // URLLoaderFactories vended to a renderer process are associated with a
   // specific origin and an execution context (e.g. a frame, a service worker or
   // any other kind of worker).

content/browser/site_instance_impl.cc

@@ -1185,28 +1185,6 @@ bool SiteInstanceImpl::ShouldLockProcess(
   return true;
 }
 
-// static
-base::Optional<url::Origin> SiteInstanceImpl::GetRequestInitiatorSiteLock(
-    const ProcessLock& lock) {
-  // The following schemes are safe for sites that require a process lock:
-  // - data: - locking |request_initiator| to an opaque origin
-  // - http/https - requiring |request_initiator| to match |site_url| with
-  //   DomainIs (i.e. suffix-based) comparison.
-  if (lock.matches_scheme(url::kHttpScheme) ||
-      lock.matches_scheme(url::kHttpsScheme) ||
-      lock.matches_scheme(url::kDataScheme)) {
-    url::Origin origin = url::Origin::Create(lock.lock_url());
-    // Only return an opaque origin if it's a data url. If http/https creates an
-    // opaque origin, then the url was invalid to begin with.
-    if (!origin.opaque() || lock.matches_scheme(url::kDataScheme))
-      return origin;
-  }
-
-  // Other schemes might not be safe to use as |request_initiator_site_lock|.
-  // One example is chrome-guest://...
-  return base::nullopt;
-}
-
 void SiteInstanceImpl::RenderProcessHostDestroyed(RenderProcessHost* host) {
   DCHECK_EQ(process_, host);
   process_->RemoveObserver(this);

content/browser/site_instance_impl.h

@@ -9,7 +9,6 @@
 #include <stdint.h>
 
 #include "base/observer_list.h"
-#include "base/optional.h"
 #include "content/browser/isolation_context.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/common/content_export.h"
@@ -421,17 +420,6 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
                                 const GURL& site_url,
                                 const bool is_guest);
 
-  // Converts |lock| into an origin that can be used as
-  // |URLLoaderFactoryParams::request_initiator_site_lock|.
-  // This means that the returned origin can be safely used in a eTLD+1
-  // comparison against |network::ResourceRequest::request_initiator|.
-  //
-  // base::nullopt is returned if |lock| cannot be used as a
-  // |request_initiator_site_lock| (e.g. in case of site_url =
-  // chrome-guest://...).
-  static base::Optional<url::Origin> GetRequestInitiatorSiteLock(
-      const ProcessLock& lock);
-
   // Return an ID of the next BrowsingInstance to be created.  This ID is
   // guaranteed to be higher than any ID of an existing BrowsingInstance.
   // This is useful when process model decisions need to be scoped only to

content/browser/url_loader_factory_params_helper.cc

@@ -6,9 +6,7 @@
 
 #include "base/command_line.h"
 #include "base/optional.h"
-#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
-#include "content/browser/site_instance_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
@@ -35,14 +33,10 @@ namespace {
 // network::ResourceRequest::request_initiator, except when
 // |is_for_isolated_world|.  See also the doc comment for
 // extensions::URLLoaderFactoryManager::CreateFactory.
-//
-// TODO(kinuko, lukasza): https://crbug.com/1098938: Make
-// |request_initiator_site_lock| non-optional, once
-// URLLoaderFactoryParamsHelper::CreateForRendererProcess is removed.
 network::mojom::URLLoaderFactoryParamsPtr CreateParams(
     RenderProcessHost* process,
     const url::Origin& origin,
-    const base::Optional<url::Origin>& request_initiator_site_lock,
+    const url::Origin& request_initiator_site_lock,
     bool is_trusted,
     const base::Optional<base::UnguessableToken>& top_frame_token,
     const net::IsolationInfo& isolation_info,
@@ -57,8 +51,7 @@ network::mojom::URLLoaderFactoryParamsPtr CreateParams(
 
   // "chrome-guest://..." is never used as a main or isolated world origin.
   DCHECK_NE(kGuestScheme, origin.scheme());
-  DCHECK(!request_initiator_site_lock.has_value() ||
-         request_initiator_site_lock->scheme() != kGuestScheme);
+  DCHECK_NE(kGuestScheme, request_initiator_site_lock.scheme());
 
   network::mojom::URLLoaderFactoryParamsPtr params =
       network::mojom::URLLoaderFactoryParams::New();
@@ -205,13 +198,13 @@ URLLoaderFactoryParamsHelper::CreateForWorker(
 network::mojom::URLLoaderFactoryParamsPtr
 URLLoaderFactoryParamsHelper::CreateForRendererProcess(
     RenderProcessHost* process) {
-  // Attempt to use the process lock as |request_initiator_site_lock|.
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
-  ProcessLock process_lock = policy->GetProcessLock(process->GetID());
-  base::Optional<url::Origin> request_initiator_site_lock =
-      SiteInstanceImpl::GetRequestInitiatorSiteLock(process_lock);
+  // Lock the |request_initiator| to an opaque origin - before something commits
+  // in a frame, requests initiated by such frame should use an opaque
+  // |request_initiator|.  See also https://crbug.com/1105794 and
+  // https://crbug.com/1098938.
+  url::Origin request_initiator_site_lock = url::Origin();
 
-  // Since this function is about to get deprecated (crbug.com/1098938), it
+  // Since this function is about to get deprecated (crbug.com/1114822), it
   // should be fine to not add support for isolation info thus using an empty
   // NetworkIsolationKey.
   //

content/browser/url_loader_factory_params_helper.h

@@ -72,7 +72,7 @@ class URLLoaderFactoryParamsHelper {
       mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
           coep_reporter);
 
-  // TODO(kinuko, lukasza): https://crbug.com/1098938: Remove, once all
+  // TODO(kinuko, lukasza): https://crbug.com/1114822: Remove, once all
   // URLLoaderFactories vended to a renderer process are associated with a
   // specific origin and an execution context (e.g. a frame, a service worker or
   // any other kind of worker).

content/renderer/render_frame_impl.cc

@@ -1089,7 +1089,7 @@ CreateDefaultURLLoaderFactory() {
 //
 // AVOID: prefer to use an origin/frame/worker-specific factory (e.g. via
 // content::RenderFrameImpl::FrameURLLoaderFactory::CreateURLLoader).  See
-// also https://crbug.com/1098938.
+// also https://crbug.com/1114822.
 //
 // It is invalid to call this in an incomplete env where
 // RenderThreadImpl::current() returns nullptr (e.g. in some tests).

content/renderer/render_frame_impl.h

@@ -1025,7 +1025,7 @@ class CONTENT_EXPORT RenderFrameImpl
   // Returns a mostly empty bundle, with a fallback that uses a process-wide,
   // direct-network factory.
   //
-  // TODO(lukasza): https://crbug.com/1098938: Remove once the fallback is no
+  // TODO(lukasza): https://crbug.com/1114822: Remove once the fallback is no
   // longer needed.
   scoped_refptr<ChildURLLoaderFactoryBundle> GetLoaderFactoryBundleFallback();
 

docs/security/compromised-renderers.md

@@ -387,7 +387,7 @@ below.
 - `request_initiator_site_lock` may be missing in unlocked renderer
   processes on Android (for example affecting protections of CORB, CORP,
   Sec-Fetch-Site and in the future SameSite cookies and Origin
-  protections).  See also https://crbug.com/1098938.
+  protections).  See also https://crbug.com/1114906.
 
 
 ## Renderer processes hosting DevTools frontend

services/network/cors/cors_url_loader_factory.cc

@@ -381,10 +381,11 @@ bool CorsURLLoaderFactory::IsValidRequest(const ResourceRequest& request,
       break;
 
     case InitiatorLockCompatibility::kNoLock:
-      // TODO(lukasza): https://crbug.com/1098938: Browser process should always
+      // TODO(lukasza): https://crbug.com/1114906: Browser process should always
       // specify the request_initiator_site_lock in URLLoaderFactories given to
-      // a renderer process.  Once https://crbug.com/1098938 is fixed, the case
-      // below should return |false| (i.e. = bad message).
+      // a renderer process.  Once issues blocking https://crbug.com/1114906 are
+      // fixed, the case below should return |false| and call
+      // mojo::ReportBadMessage.
       break;
 
     case InitiatorLockCompatibility::kNoInitiator:

services/network/public/cpp/initiator_lock_compatibility.cc

@@ -30,32 +30,17 @@ InitiatorLockCompatibility VerifyRequestInitiatorLock(
     return InitiatorLockCompatibility::kNoInitiator;
   const url::Origin& initiator = request_initiator.value();
 
-  // TODO(lukasza, nasko): Also consider equality of precursor origins (e.g. if
-  // |initiator| is opaque, then it's precursor origin should match the |lock|
-  // [or |lock|'s precursor if |lock| is also opaque]).
-  if (initiator.opaque() || (initiator == lock))
+  if (initiator == lock)
     return InitiatorLockCompatibility::kCompatibleLock;
 
-  // TODO(lukasza): https://crbug.com/1098938: Return kIncorrectLock if
-  // the origins do not match exactly in the previous if statement.  This should
-  // be possible to do once we no longer vend process-wide factory.
-  if (!initiator.opaque() && !lock.opaque() &&
-      initiator.scheme() == lock.scheme() &&
-      initiator.GetURL().SchemeIsHTTPOrHTTPS()) {
-    if (initiator.GetURL().HostIsIPAddress()) {
-      // For IP addresses, we require host equality (allowing ports to differ,
-      // since site_url ignores ports).  See also https://crbug.com/1051674.
-      if (initiator.host() == lock.host())
-        return InitiatorLockCompatibility::kCompatibleLock;
-    } else {
-      // For non-IP-address origins, we require sites (eTLD+1) to match
-      // (again ignoring ports).
-      std::string lock_domain = lock.host();
-      if (!lock_domain.empty() && lock_domain.back() == '.')
-        lock_domain.erase(lock_domain.length() - 1);
-      if (initiator.DomainIs(lock_domain))
-        return InitiatorLockCompatibility::kCompatibleLock;
-    }
+  // Opaque |initiator| is always allowed.  In particular, a factory locked to a
+  // non-opaque |lock| may be used by an opaque |initiator| - for example when
+  // the factory is inherited by a data: URL frame.
+  if (initiator.opaque()) {
+    // TODO(lukasza, nasko): Also consider equality of precursor origins (e.g.
+    // if |initiator| is opaque, then it's precursor origin should match the
+    // |lock| [or |lock|'s precursor if |lock| is also opaque]).
+    return InitiatorLockCompatibility::kCompatibleLock;
   }
 
   return InitiatorLockCompatibility::kIncorrectLock;

services/network/public/cpp/initiator_lock_compatibility_unittest.cc

@@ -66,24 +66,19 @@ TEST(InitiatorLockCompatibilityTest, VerifyRequestInitiatorSiteLock) {
   EXPECT_EQ(InitiatorLockCompatibility::kIncorrectLock,
             VerifyRequestInitiatorLock(foo_example_com, bar_example_com));
 
-  // Site-URL-based comparisons.
-  //
-  // TODO(lukasza): These should result in kIncorrectLock eventually (once
-  // request_initiator_site_lock becomes request_initiator_origin_lock - see
-  // https://crbug.com/1098938.
-  EXPECT_EQ(InitiatorLockCompatibility::kCompatibleLock,
+  // Site != origin.
+  EXPECT_EQ(InitiatorLockCompatibility::kIncorrectLock,
             VerifyRequestInitiatorLock(example_com, bar_foo_example_com));
-  EXPECT_EQ(InitiatorLockCompatibility::kCompatibleLock,
+  EXPECT_EQ(InitiatorLockCompatibility::kIncorrectLock,
             VerifyRequestInitiatorLock(foo_example_com, bar_foo_example_com));
   EXPECT_EQ(
-      InitiatorLockCompatibility::kCompatibleLock,
+      InitiatorLockCompatibility::kIncorrectLock,
       VerifyRequestInitiatorLock(ip_origin1, ip_origin1_with_different_port));
 
-  // The trailing dot is not important (at least for site-URL-based
-  // comparisons).
-  EXPECT_EQ(InitiatorLockCompatibility::kCompatibleLock,
+  // The trailing dot.
+  EXPECT_EQ(InitiatorLockCompatibility::kIncorrectLock,
             VerifyRequestInitiatorLock(foo_example_com_dot, foo_example_com));
-  EXPECT_EQ(InitiatorLockCompatibility::kCompatibleLock,
+  EXPECT_EQ(InitiatorLockCompatibility::kIncorrectLock,
             VerifyRequestInitiatorLock(foo_example_com, foo_example_com_dot));
 }
 

services/network/public/mojom/network_context.mojom

@@ -601,8 +601,14 @@ struct URLLoaderFactoryParams {
 
   // If specified, then |request_initiator_site_lock| locks
   // |ResourceRequest::request_initiator| to the specified origin.
-  // TODO(lukasza): https://crbug.com/1098938: 1) Make this non-optional,
-  // 2) Make this an *origin* lock (rather than a *site* lock).
+  //
+  // SECURITY NOTE: |request_initiator_site_lock| should be present in all
+  // factories that may be vended to a Renderer process.
+  // |request_initiator_site_lock| may be missing only in factories used by the
+  // Browser process.
+  //
+  // TODO(lukasza): https://crbug.com/918967: Make this an *origin* lock
+  // (rather than a *site* lock).
   url.mojom.Origin? request_initiator_site_lock;
 
   // Cross-origin read blocking (CORB) configuration.

third_party/blink/web_tests/external/wpt/fetch/corb/script-resource-with-nonsniffable-types.tentative.sub.html

@@ -42,12 +42,7 @@ function test(mime_type, is_blocking_expected) {
     });
 
     // www1 is cross-origin, so the HTTP response is CORB-eligible.
-    //
-    // TODO(lukasza@chromium.org): Once https://crbug.com/888079 and
-    // https://crbug.com/1098938 are fixed, we should use a cross-*origin*
-    // rather than cross-*site* URL below (e.g. s/hosts[alt]/domains/g).
-    // See also https://crbug.com/918660 for more context.
-    var src_prefix = "http://{{hosts[alt][www1]}}:{{ports[http][0]}}/fetch/corb/resources/sniffable-resource.py";
+    var src_prefix = "http://{{domains[www1]}}:{{ports[http][0]}}/fetch/corb/resources/sniffable-resource.py";
     body = `window['${script_has_run_token}'] = true;`
     script.src = src_prefix + "?type=" + mime_type + "&body=" + encodeURIComponent(body);
     document.body.appendChild(script)