InitiatorLockCompatibility UMA: Break out plugins and extensions.

This CL breaks-out two additional special-cases out of the old
kIncorrectLock category:

- kExcludedScheme will be used for chrome-extension, since
  non-allowlisted extensions will use an incompatible request_initiator
  until https://crbug.com/940068 is fixed.

- kExcludedUniversalAccessPlugin will be used for requests from
  renderers which are embedding a universal-access plugin
  (like Flash) - the renderer is proxying requests on behalf of the
  (potentially cross-origin = request_initiator-incompatible) plugin.

The CL adds or modifies tests, so that if we started to treat an
incorrect lock as a bad message, then the tests would fail without the
additional exceptions outlined above.

Bug: 920634
Change-Id: I93f14a43d6569c010898a662c250d2bda0613fca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762677
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691013}

chrome/browser/net/system_network_context_manager.cc

@@ -78,6 +78,10 @@
 #include "ui/base/l10n/l10n_util.h"
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/common/constants.h"
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+
 namespace {
 
 // The global instance of the SystemNetworkContextmanager.
@@ -562,6 +566,11 @@ void SystemNetworkContextManager::OnNetworkServiceCreated(
          "text/csv"});
   }
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  network_service->ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      extensions::kExtensionScheme, base::DoNothing::Once());
+#endif
+
   int max_connections_per_proxy =
       local_state_->GetInteger(prefs::kMaxConnectionsPerProxy);
   if (max_connections_per_proxy != -1)

chrome/test/data/cors-ok.txt

+cors-ok.txt - body

chrome/test/data/cors-ok.txt.mock-http-headers

+HTTP/1.1 200 OK
+Access-Control-Allow-Origin: *
+Content-Type: text/plain; charset=utf-8

content/shell/browser/web_test/web_test_message_filter.cc

@@ -13,6 +13,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/content_index_context.h"
+#include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/test/web_test_support.h"
@@ -28,6 +29,7 @@
 #include "content/test/mock_platform_notification_service.h"
 #include "net/base/completion_once_callback.h"
 #include "net/base/net_errors.h"
+#include "services/network/public/mojom/network_service.mojom.h"
 #include "storage/browser/database/database_tracker.h"
 #include "storage/browser/fileapi/isolated_context.h"
 #include "storage/browser/quota/quota_manager.h"
@@ -60,6 +62,14 @@ ContentIndexContext* GetContentIndexContext(const url::Origin& origin) {
   return storage_partition->GetContentIndexContext();
 }
 
+void ExcludeSchemeFromRequestInitiatorSiteLockChecksOnUIThread(
+    const std::string& scheme,
+    base::OnceClosure completion_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  GetNetworkService()->ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      scheme, std::move(completion_callback));
+}
+
 }  // namespace
 
 WebTestMessageFilter::WebTestMessageFilter(
@@ -109,6 +119,9 @@ bool WebTestMessageFilter::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(WebTestHostMsg_ReadFileToString, OnReadFileToString)
     IPC_MESSAGE_HANDLER(WebTestHostMsg_RegisterIsolatedFileSystem,
                         OnRegisterIsolatedFileSystem)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(
+        WebTestHostMsg_ExcludeSchemeFromRequestInitiatorSiteLockChecks,
+        OnExcludeSchemeFromRequestInitiatorSiteLockChecks)
     IPC_MESSAGE_HANDLER(WebTestHostMsg_ClearAllDatabases, OnClearAllDatabases)
     IPC_MESSAGE_HANDLER(WebTestHostMsg_SetDatabaseQuota, OnSetDatabaseQuota)
     IPC_MESSAGE_HANDLER(WebTestHostMsg_SimulateWebNotificationClick,
@@ -156,6 +169,20 @@ void WebTestMessageFilter::OnRegisterIsolatedFileSystem(
   policy->GrantReadFileSystem(render_process_id_, *filesystem_id);
 }
 
+void WebTestMessageFilter::OnExcludeSchemeFromRequestInitiatorSiteLockChecks(
+    const std::string& scheme,
+    IPC::Message* reply_msg) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  base::OnceClosure completion_callback =
+      base::BindOnce(base::IgnoreResult(&IPC::Sender::Send), this, reply_msg);
+
+  base::PostTask(
+      FROM_HERE, {BrowserThread::UI},
+      base::BindOnce(&ExcludeSchemeFromRequestInitiatorSiteLockChecksOnUIThread,
+                     scheme, base::Passed(std::move(completion_callback))));
+}
+
 void WebTestMessageFilter::OnClearAllDatabases() {
   DCHECK(database_tracker_->task_runner()->RunsTasksInCurrentSequence());
   database_tracker_->DeleteDataModifiedSince(base::Time(),

content/shell/browser/web_test/web_test_message_filter.h

@@ -62,6 +62,9 @@ class WebTestMessageFilter : public BrowserMessageFilter {
   void OnRegisterIsolatedFileSystem(
       const std::vector<base::FilePath>& absolute_filenames,
       std::string* filesystem_id);
+  void OnExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      const std::string& scheme,
+      IPC::Message* reply_msg);
   void OnClearAllDatabases();
   void OnSetDatabaseQuota(int quota);
   void OnSimulateWebNotificationClick(

content/shell/common/web_test/web_test_messages.h

@@ -25,6 +25,10 @@ IPC_SYNC_MESSAGE_ROUTED1_1(WebTestHostMsg_ReadFileToString,
 IPC_SYNC_MESSAGE_ROUTED1_1(WebTestHostMsg_RegisterIsolatedFileSystem,
                            std::vector<base::FilePath> /* absolute_filenames */,
                            std::string /* filesystem_id */)
+IPC_SYNC_MESSAGE_ROUTED1_0(
+    WebTestHostMsg_ExcludeSchemeFromRequestInitiatorSiteLockChecks,
+    std::string /* scheme */)
+
 IPC_MESSAGE_ROUTED0(WebTestHostMsg_ClearAllDatabases)
 IPC_MESSAGE_ROUTED1(WebTestHostMsg_SetDatabaseQuota, int /* quota */)
 IPC_MESSAGE_ROUTED3(WebTestHostMsg_SimulateWebNotificationClick,

content/shell/renderer/web_test/blink_test_runner.cc

@@ -677,6 +677,12 @@ void BlinkTestRunner::ForceTextInputStateUpdate(WebLocalFrame* frame) {
   ForceTextInputStateUpdateForRenderFrame(RenderFrame::FromWebFrame(frame));
 }
 
+void BlinkTestRunner::ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+    const std::string& scheme) {
+  Send(new WebTestHostMsg_ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      routing_id(), scheme));
+}
+
 // RenderViewObserver  --------------------------------------------------------
 
 void BlinkTestRunner::DidClearWindowObject(WebLocalFrame* frame) {

content/shell/renderer/web_test/blink_test_runner.h

@@ -138,6 +138,8 @@ class BlinkTestRunner : public RenderViewObserver,
   float GetDeviceScaleFactor() const override;
   void RunIdleTasks(base::OnceClosure callback) override;
   void ForceTextInputStateUpdate(blink::WebLocalFrame* frame) override;
+  void ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      const std::string& scheme) override;
 
   // Resets a RenderView to a known state for web tests. It is used both when
   // a RenderView is created and when reusing an existing RenderView for the

content/shell/test_runner/test_runner_for_specific_view.cc

@@ -660,6 +660,15 @@ void TestRunnerForSpecificView::SetIsolatedWorldInfo(
   web_view()->FocusedFrame()->ClearIsolatedWorldCSPForTesting(world_id);
 
   web_view()->FocusedFrame()->SetIsolatedWorldInfo(world_id, info);
+
+  if (!info.security_origin.IsNull()) {
+    // Isolated world's origin may differ from the main world origin and trigger
+    // security checks when it doesn't match request_initiator_site_lock.  To
+    // avoid this, we need to explicitly exclude the isolated world's scheme
+    // from these security checks.
+    delegate()->ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+        info.security_origin.Protocol().Utf8());
+  }
 }
 
 void TestRunner::InsertStyleSheet(const std::string& source_code) {

content/shell/test_runner/web_test_delegate.h

@@ -258,6 +258,11 @@ class WebTestDelegate {
   // Forces a text input state update for the client of WebFrameWidget
   // associated with |frame|.
   virtual void ForceTextInputStateUpdate(blink::WebLocalFrame* frame) = 0;
+
+  // Synchronously waits for the browser process to notify the NetworkService
+  // that |scheme| should be excluded from request_initiator_site_lock checks.
+  virtual void ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      const std::string& scheme) = 0;
 };
 
 }  // namespace test_runner

ppapi/tests/test_url_loader.cc

@@ -711,7 +711,8 @@ std::string TestURLLoader::TestTrustedHttpRequests() {
   // Trusted requests with custom referrer should succeed.
   {
     pp::URLRequestInfo request(instance_);
-    request.SetCustomReferrerURL("http://www.google.com/");
+    request.SetCustomReferrerURL("http://www.referer.com/");
+    request.SetHeaders("Referer: http://www.referer.com/");
 
     int32_t rv = OpenTrusted(request, NULL);
     if (rv != PP_OK)

services/network/cors/cors_url_loader_factory.cc

@@ -13,6 +13,7 @@
 #include "net/base/load_flags.h"
 #include "services/network/cors/cors_url_loader.h"
 #include "services/network/cors/preflight_controller.h"
+#include "services/network/cross_origin_read_blocking.h"
 #include "services/network/initiator_lock_compatibility.h"
 #include "services/network/loader_util.h"
 #include "services/network/network_context.h"
@@ -191,22 +192,45 @@ bool CorsURLLoaderFactory::IsSane(const NetworkContext* context,
     }
   }
 
+  // Compare |request_initiator| and |request_initiator_site_lock_|.
   InitiatorLockCompatibility initiator_lock_compatibility =
-      process_id_ == mojom::kBrowserProcessId
-          ? InitiatorLockCompatibility::kBrowserProcess
-          : VerifyRequestInitiatorLock(request_initiator_site_lock_,
-                                       request.request_initiator);
+      VerifyRequestInitiatorLock(process_id_, request_initiator_site_lock_,
+                                 request.request_initiator);
   UMA_HISTOGRAM_ENUMERATION(
       "NetworkService.URLLoader.RequestInitiatorOriginLockCompatibility",
       initiator_lock_compatibility);
-  // TODO(lukasza): Enforce the origin lock.
-  // - https://crbug.com/766694: In the long-term kIncorrectLock should trigger
-  //   a renderer kill, but this can't be done until HTML Imports are gone.
-  // - https://crbug.com/515309: The lock should apply to Origin header (and
-  //   SameSite cookies) in addition to CORB (which was taken care of in
-  //   https://crbug.com/871827).  Here enforcement most likely would mean
-  //   setting |url_request_|'s initiator to something other than
-  //   |request.request_initiator| (opaque origin?  lock origin?).
+  switch (initiator_lock_compatibility) {
+    case InitiatorLockCompatibility::kCompatibleLock:
+    case InitiatorLockCompatibility::kBrowserProcess:
+    case InitiatorLockCompatibility::kExcludedScheme:
+    case InitiatorLockCompatibility::kExcludedUniversalAccessPlugin:
+      break;
+
+    case InitiatorLockCompatibility::kNoLock:
+      // TODO(lukasza): https://crbug.com/891872: Browser process should always
+      // specify the request_initiator_site_lock in URLLoaderFactories given to
+      // a renderer process.  Once https://crbug.com/891872 is fixed, the case
+      // below should return |false| (i.e. = bad message).
+      DCHECK_NE(process_id_, mojom::kBrowserProcessId);
+      break;
+
+    case InitiatorLockCompatibility::kNoInitiator:
+      // Requests from the renderer need to always specify an initiator.
+      DCHECK_NE(process_id_, mojom::kBrowserProcessId);
+      // TODO(lukasza): Report this as a bad message.
+      break;
+
+    case InitiatorLockCompatibility::kIncorrectLock:
+      // Requests from the renderer need to always specify a correct initiator.
+      DCHECK_NE(process_id_, mojom::kBrowserProcessId);
+      // TODO(lukasza): Report this as a bad message (or use the lock instead
+      // of the renderer-reported value).  Before we can do this, we need to
+      // ensure via UMA that this rarely happens or has low impact.  One known
+      // case are probably non-universal-access plugins (like PNaCl) which
+      // wouldn't be covered by the kExcludedUniversalAccessPlugin exception
+      // above.
+      break;
+  }
 
   if (context) {
     net::HttpRequestHeaders::Iterator header_iterator(

services/network/initiator_lock_compatibility.cc

@@ -6,9 +6,12 @@
 
 #include <string>
 
+#include "base/containers/flat_set.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "services/network/cross_origin_read_blocking.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/mojom/network_context.mojom.h"
@@ -18,6 +21,16 @@
 
 namespace network {
 
+namespace {
+
+base::flat_set<std::string>&
+GetSchemesExcludedFromRequestInitiatorSiteLockChecks() {
+  static base::NoDestructor<base::flat_set<std::string>> s_scheme;
+  return *s_scheme;
+}
+
+}  // namespace
+
 InitiatorLockCompatibility VerifyRequestInitiatorLock(
     const base::Optional<url::Origin>& request_initiator_site_lock,
     const base::Optional<url::Origin>& request_initiator) {
@@ -65,9 +78,34 @@ InitiatorLockCompatibility VerifyRequestInitiatorLock(
       return InitiatorLockCompatibility::kCompatibleLock;
   }
 
+  // TODO(lukasza): https://crbug.com/940068: Stop excluding specific schemes
+  // after request_initiator=website also for requests from isolated worlds.
+  if (base::Contains(GetSchemesExcludedFromRequestInitiatorSiteLockChecks(),
+                     initiator.scheme())) {
+    return InitiatorLockCompatibility::kExcludedScheme;
+  }
+
   return InitiatorLockCompatibility::kIncorrectLock;
 }
 
+InitiatorLockCompatibility VerifyRequestInitiatorLock(
+    uint32_t process_id,
+    const base::Optional<url::Origin>& request_initiator_site_lock,
+    const base::Optional<url::Origin>& request_initiator) {
+  if (process_id == mojom::kBrowserProcessId)
+    return InitiatorLockCompatibility::kBrowserProcess;
+
+  InitiatorLockCompatibility result = VerifyRequestInitiatorLock(
+      request_initiator_site_lock, request_initiator);
+
+  if (result == InitiatorLockCompatibility::kIncorrectLock &&
+      CrossOriginReadBlocking::ShouldAllowForPlugin(process_id)) {
+    result = InitiatorLockCompatibility::kExcludedUniversalAccessPlugin;
+  }
+
+  return result;
+}
+
 url::Origin GetTrustworthyInitiator(
     const base::Optional<url::Origin>& request_initiator_site_lock,
     const base::Optional<url::Origin>& request_initiator) {
@@ -91,4 +129,11 @@ url::Origin GetTrustworthyInitiator(
   return request_initiator.value();
 }
 
+void ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+    const std::string& scheme) {
+  base::flat_set<std::string>& excluded_schemes =
+      GetSchemesExcludedFromRequestInitiatorSiteLockChecks();
+  excluded_schemes.insert(scheme);
+}
+
 }  // namespace network

services/network/initiator_lock_compatibility.h

@@ -28,7 +28,8 @@ enum class InitiatorLockCompatibility {
   // and RenderProcessHostImpl::CreateURLLoaderFactoryWithOptionalOrigin.
   kNoLock = 1,
 
-  // |request_initiator| is missing.
+  // |request_initiator| is missing.  This indicates that the renderer has a bug
+  // or has been compromised by an attacker.
   kNoInitiator = 2,
 
   // |request.request_initiator| is compatible with
@@ -43,13 +44,33 @@ enum class InitiatorLockCompatibility {
   // - HTML Imports (see https://crbug.com/871827#c9).
   kIncorrectLock = 4,
 
-  kMaxValue = kIncorrectLock,
+  // Covered by ExcludeSchemeFromRequestInitiatorSiteLockChecks.
+  kExcludedScheme = 5,
+
+  // Covered by CrossOriginReadBlocking::ShouldAllowForPlugin.
+  kExcludedUniversalAccessPlugin = 6,
+
+  kMaxValue = kExcludedUniversalAccessPlugin,
 };
 
 // Verifies if |request.request_initiator| matches
 // |factory_params.request_initiator_site_lock|.
+//
+// This overload should only be called for requests from renderer processes
+// (ones that are not coverd by the kExcludedPlugin exception).
+COMPONENT_EXPORT(NETWORK_SERVICE)
+InitiatorLockCompatibility VerifyRequestInitiatorLock(
+    const base::Optional<url::Origin>& request_initiator_site_lock,
+    const base::Optional<url::Origin>& request_initiator);
+
+// Verifies if |request.request_initiator| matches
+// |factory_params.request_initiator_site_lock|.
+//
+// This overload takes into account exception for the browser process and/or for
+// renderer processes that embed universal-access plugins.
 COMPONENT_EXPORT(NETWORK_SERVICE)
 InitiatorLockCompatibility VerifyRequestInitiatorLock(
+    uint32_t process_id,
     const base::Optional<url::Origin>& request_initiator_site_lock,
     const base::Optional<url::Origin>& request_initiator);
 
@@ -65,10 +86,23 @@ InitiatorLockCompatibility VerifyRequestInitiatorLock(
 // |request_initiator| should come from net::URLRequest::initiator() or
 // network::ResourceRequest::request_initiator which may be initially set in an
 // untrustworthy process (eg: renderer process).
+//
+// TODO(lukasza): Remove this function if https://crrev.com/c/1661114 sticks
+// (i.e. if ResourceRequest::request_initiator is sanitized and made trustworthy
+// by CorsURLLoaderFactory::CreateLoaderAndStart and IsSane).
 url::Origin GetTrustworthyInitiator(
     const base::Optional<url::Origin>& request_initiator_site_lock,
     const base::Optional<url::Origin>& request_initiator);
 
+// Registers a scheme that should not be subject to
+// |request_initiator_site_lock| checks (e.g. a scheme that is typically
+// used in isolated worlds, with a separate origin, such as
+// "chrome-extensions").
+//
+// TODO(lukasza): https://crbug.com/940068: Remove this method once isolated
+// worlds use the same |request_initiator| as the main world.
+void ExcludeSchemeFromRequestInitiatorSiteLockChecks(const std::string& scheme);
+
 }  // namespace network
 
 #endif  // SERVICES_NETWORK_INITIATOR_LOCK_COMPATIBILITY_H_

services/network/network_service.cc

@@ -47,6 +47,7 @@
 #include "services/network/cross_origin_read_blocking.h"
 #include "services/network/dns_config_change_manager.h"
 #include "services/network/http_auth_cache_copier.h"
+#include "services/network/initiator_lock_compatibility.h"
 #include "services/network/net_log_exporter.h"
 #include "services/network/network_context.h"
 #include "services/network/network_usage_accumulator.h"
@@ -620,6 +621,14 @@ void NetworkService::AddExtraMimeTypesForCorb(
   CrossOriginReadBlocking::AddExtraMimeTypesForCorb(mime_types);
 }
 
+void NetworkService::ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+    const std::string& scheme,
+    mojom::NetworkService::
+        ExcludeSchemeFromRequestInitiatorSiteLockChecksCallback callback) {
+  network::ExcludeSchemeFromRequestInitiatorSiteLockChecks(scheme);
+  std::move(callback).Run();
+}
+
 void NetworkService::OnMemoryPressure(
     base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {
   base::MemoryPressureListener::NotifyMemoryPressure(memory_pressure_level);

services/network/network_service.h

@@ -161,6 +161,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkService
   void RemoveCorbExceptionForPlugin(uint32_t process_id) override;
   void AddExtraMimeTypesForCorb(
       const std::vector<std::string>& mime_types) override;
+  void ExcludeSchemeFromRequestInitiatorSiteLockChecks(
+      const std::string& scheme,
+      mojom::NetworkService::
+          ExcludeSchemeFromRequestInitiatorSiteLockChecksCallback callback)
+      override;
   void OnMemoryPressure(base::MemoryPressureListener::MemoryPressureLevel
                             memory_pressure_level) override;
   void OnPeerToPeerConnectionsCountChange(uint32_t count) override;

services/network/public/mojom/network_service.mojom

@@ -323,6 +323,15 @@ interface NetworkService {
   // kMimeHandlerViewInCrossProcessFrame feature ships.
   AddExtraMimeTypesForCorb(array<string> mime_types);
 
+  // Registers a scheme that should not be subject to
+  // |request_initiator_site_lock| checks (e.g. a scheme that is typically
+  // used in isolated worlds, with a separate origin, such as
+  // "chrome-extension").
+  //
+  // TODO(lukasza): https://crbug.com/940068: Remove this method once isolated
+  // worlds use the same |request_initiator| as the main world.
+  ExcludeSchemeFromRequestInitiatorSiteLockChecks(string scheme) => ();
+
   // Called when the system is low on memory.
   OnMemoryPressure(mojo_base.mojom.MemoryPressureLevel memory_pressure_level);
 

tools/metrics/histograms/enums.xml

@@ -51884,6 +51884,14 @@ Called by update_net_trust_anchors.py.-->
     |request.request_initiator| is non-opaque/unique and differs from
     |factory_params_.request_initiator_site_lock|.
   </int>
+  <int value="5" label="ExcludedScheme">
+    Scheme excluded from request_initiator_site_lock checks (e.g.
+    chrome-extension).
+  </int>
+  <int value="6" label="ExcludedUniversalAccessPlugin">
+    Requests from a renderer processes that embeds an universal access plugin
+    (like Flash).
+  </int>
 </enum>
 
 <enum name="RequestMediaKeySystemAccessStatus">