CORB shouldn't make an exception for plugins, unless Flash actually runs

Overview
========

Flash has its own CORS-like mechanism (crossdomain.xml-based) and
therefore CORB (Cross-Origin Read Blocking) cannot be enforced for
requests initiated by Flash.

This CL avoids making an exception for plugins, unless the given
renderer process is actually hosting a Flash plugin (and is therefore
capable of proxying network requests on behalf of Flash).  This
means that the exception won't take place unless the user has
approved running Flash (via click-to-play / content settings /
enterprise policy - see the bug for more details).


Details
=======

This CL introduces a global set that stores process IDs of renderers
that host Flash.  This set lives either in the NetworkService process or
(if NetworkService feature is disabled) in the IO thread of the browser
process.  In both cases the global set is implemented and exposed by new
static methods of network::CrossOriginReadBlocking class.

The CL populates the global set from
PluginServiceImpl::FindOrStartPpapiPluginProcess after all the security
checks have been done and the plugin process is ready to be used or
launched.

The CL consults the global set before deciding to make a CORB exception
for a plugin request.  This is done from network::URLLoader (used if
NetworkService feature is enabled) and from
CrossSiteDocumentResourceHandler (used otherwise).

The CL removes items from the global set when RenderProcessHostImpl is
destroyed.


Bug: 874515
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I50484807c921a4daea08be8a00c67a3cf9c82cf0
Reviewed-on: https://chromium-review.googlesource.com/1178885
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585124}

content/browser/loader/cross_site_document_resource_handler.cc

@@ -579,6 +579,8 @@ void CrossSiteDocumentResourceHandler::OnResponseCompleted(
 
 bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders(
     const network::ResourceResponse& response) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   // Delegate most decisions to CrossOriginReadBlocking::ResponseAnalyzer.
   analyzer_ =
       std::make_unique<network::CrossOriginReadBlocking::ResponseAnalyzer>(
@@ -612,14 +614,22 @@ bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders(
   if (!info || info->GetChildID() == -1)
     return false;
 
-  // Don't block plugin requests.
-  // TODO(lukasza): Only disable CORB for plugins with universal access (see
-  // PepperURLLoaderHost::has_universal_access_), because only such plugins may
-  // have their own CORS-like mechanisms - e.g. crossdomain.xml in Flash).  We
-  // should still enforce CORB for other kinds of plugins (i.e. ones without
-  // universal access).
+  // Don't block some plugin requests.
+  //
+  // Note that in practice this exception only only matters to Flash and test
+  // plugins (both can issue requests without CORS and both will be covered by
+  // CORB::ShouldAllowForPlugin below).
+  //
+  // This exception is not needed for:
+  // - PNaCl (which always issues CORS requests)
+  // - PDF (which is already covered by the exception for chrome-extension://...
+  //   initiators and therefore doesn't need another exception here;
+  //   additionally PDF doesn't _really_ make *cross*-origin requests - it just
+  //   seems that way because of the usage of the Chrome extension).
   if (info->GetResourceType() == RESOURCE_TYPE_PLUGIN_RESOURCE &&
-      is_nocors_plugin_request_) {
+      is_nocors_plugin_request_ &&
+      network::CrossOriginReadBlocking::ShouldAllowForPlugin(
+          info->GetChildID())) {
     return false;
   }
 

content/browser/loader/cross_site_document_resource_handler_unittest.cc

@@ -345,7 +345,13 @@ const TestScenario kScenarios[] = {
         kVerdictPacketForHeadersBasedVerdict,       // verdict_packet
     },
     {
-        "Allowed: Cross-site fetch HTML from Flash without CORS",
+        // Blocked, because the unit test doesn't make a call to
+        // CrossOriginReadBlocking::AddExceptionForFlash (simulating a behavior
+        // of a compromised renderer that only pretends to be hosting Flash).
+        //
+        // The regular scenario is covered by integration tests:
+        // OutOfProcessPPAPITest.URLLoaderTrusted.
+        "Blocked: Cross-site fetch HTML from Flash without CORS",
         __LINE__,
         "http://www.b.com/plugin.html",             // target_url
         RESOURCE_TYPE_PLUGIN_RESOURCE,              // resource_type
@@ -357,7 +363,7 @@ const TestScenario kScenarios[] = {
         false,                                      // simulate_range_response
         AccessControlAllowOriginHeader::kOmit,      // cors_response
         {"<html><head>this should sniff as HTML"},  // packets
-        Verdict::kAllow,                            // verdict
+        Verdict::kBlock,                            // verdict
         kVerdictPacketForHeadersBasedVerdict,       // verdict_packet
     },
     {

content/browser/plugin_service_impl.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -40,6 +41,7 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/webplugininfo.h"
+#include "ppapi/shared_impl/ppapi_permissions.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 
 namespace content {
@@ -179,6 +181,21 @@ PpapiPluginProcessHost* PluginServiceImpl::FindOrStartPpapiPluginProcess(
     return nullptr;
   }
 
+  // Flash has its own flavour of CORS, so CORB needs to allow all responses
+  // and rely on Flash to enforce same-origin policy.  See also
+  // https://crbug.com/874515 and https://crbug.com/816318#c5.
+  //
+  // Note that ppapi::PERMISSION_FLASH is present not only in the Flash plugin.
+  // This permission is also present in plugins added from the cmdline and so
+  // will be also present for "PPAPI Tests" plugin used for
+  // OutOfProcessPPAPITest.URLLoaderTrusted and related tests.
+  //
+  // TODO(lukasza, laforge): https://crbug.com/702995: Remove the code below
+  // once Flash support is removed from Chromium (probably around 2020 - see
+  // https://www.chromium.org/flash-roadmap).
+  if (info->permissions & ppapi::PERMISSION_FLASH)
+    RenderProcessHostImpl::AddCorbExceptionForPlugin(render_process_id);
+
   PpapiPluginProcessHost* plugin_host =
       FindPpapiPluginProcess(plugin_path, profile_data_directory, origin_lock);
   if (plugin_host)

content/browser/renderer_host/render_process_host_impl.cc

@@ -194,6 +194,7 @@
 #include "ppapi/buildflags/buildflags.h"
 #include "services/device/public/mojom/battery_monitor.mojom.h"
 #include "services/device/public/mojom/constants.mojom.h"
+#include "services/network/cross_origin_read_blocking.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/network_switches.h"
 #include "services/network/public/mojom/network_service.mojom.h"
@@ -1155,6 +1156,54 @@ void GetNetworkChangeManager(
   GetNetworkService()->GetNetworkChangeManager(std::move(request));
 }
 
+void AddCorbExceptionForPluginOnUIThread(int process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  RenderProcessHost* process = RenderProcessHostImpl::FromID(process_id);
+  process->CleanupCorbExceptionForPluginUponDestruction();
+
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService))
+    GetNetworkService()->AddCorbExceptionForPlugin(process_id);
+}
+
+// This is the entry point (i.e. this is called on the IO thread *before*
+// we post a task for AddCorbExceptionForPluginOnUIThread).
+void AddCorbExceptionForPluginOnIOThread(int process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Without NetworkService the exception list is stored directly in the browser
+  // process.
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService))
+    network::CrossOriginReadBlocking::AddExceptionForPlugin(process_id);
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&AddCorbExceptionForPluginOnUIThread, process_id));
+}
+
+void RemoveCorbExceptionForPluginOnIOThread(int process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Without NetworkService the exception list is stored directly in the browser
+  // process.
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService))
+    network::CrossOriginReadBlocking::RemoveExceptionForPlugin(process_id);
+}
+
+// This is the entry point (i.e. this is called on the UI thread *before*
+// we post a task for RemoveCorbExceptionForPluginOnIOThread).
+void RemoveCorbExceptionForPluginOnUIThread(int process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+    GetNetworkService()->RemoveCorbExceptionForPlugin(process_id);
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&RemoveCorbExceptionForPluginOnIOThread, process_id));
+  }
+}
+
 }  // namespace
 
 // Held by the RPH and used to control an (unowned) ConnectionFilterImpl from
@@ -1538,6 +1587,9 @@ RenderProcessHostImpl::~RenderProcessHostImpl() {
   }
 
   GetMemoryDumpProvider().RemoveHost(this);
+
+  if (cleanup_corb_exception_for_plugin_upon_destruction_)
+    RemoveCorbExceptionForPluginOnUIThread(GetID());
 }
 
 bool RenderProcessHostImpl::Init() {
@@ -1905,6 +1957,17 @@ void RenderProcessHostImpl::DelayProcessShutdownForUnload(
       timeout);
 }
 
+// static
+void RenderProcessHostImpl::AddCorbExceptionForPlugin(int process_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  AddCorbExceptionForPluginOnIOThread(process_id);
+}
+
+void RenderProcessHostImpl::CleanupCorbExceptionForPluginUponDestruction() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  cleanup_corb_exception_for_plugin_upon_destruction_ = true;
+}
+
 void RenderProcessHostImpl::RegisterMojoInterfaces() {
   auto registry = std::make_unique<service_manager::BinderRegistry>();
 

content/browser/renderer_host/render_process_host_impl.h

@@ -228,6 +228,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   void LockToOrigin(const GURL& lock_url) override;
   void BindCacheStorage(blink::mojom::CacheStorageRequest request,
                         const url::Origin& origin) override;
+  void CleanupCorbExceptionForPluginUponDestruction() override;
 
   mojom::RouteProvider* GetRemoteRouteProvider();
 
@@ -424,6 +425,11 @@ class CONTENT_EXPORT RenderProcessHostImpl
     return file_system_manager_impl_.get();
   }
 
+  // Adds a CORB (Cross-Origin Read Blocking) exception for |process_id|.  The
+  // exception will be removed when the corresponding RenderProcessHostImpl is
+  // destroyed (see |cleanup_corb_exception_for_plugin_upon_destruction_|).
+  static void AddCorbExceptionForPlugin(int process_id);
+
  protected:
   // A proxy for our IPC::Channel that lives on the IO thread.
   std::unique_ptr<IPC::ChannelProxy> channel_;
@@ -856,6 +862,8 @@ class CONTENT_EXPORT RenderProcessHostImpl
   std::unique_ptr<mojo::Binding<viz::mojom::CompositingModeReporter>>
       compositing_mode_reporter_;
 
+  bool cleanup_corb_exception_for_plugin_upon_destruction_ = false;
+
   base::WeakPtrFactory<RenderProcessHostImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderProcessHostImpl);

content/public/browser/render_process_host.h

@@ -457,6 +457,16 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
   // be posted back on the UI thread).
   void PostTaskWhenProcessIsReady(base::OnceClosure task);
 
+  // Controls whether the destructor of RenderProcessHost*Impl* will end up
+  // cleaning the memory used by the exception added via
+  // RenderProcessHostImpl::AddCorbExceptionForPlugin.
+  //
+  // TODO(lukasza): https://crbug.com/652474: This method shouldn't be part of
+  // the //content public API, because it shouldn't be called by anyone other
+  // than RenderProcessHostImpl (from underneath
+  // RenderProcessHostImpl::AddCorbExceptionForPlugin).
+  virtual void CleanupCorbExceptionForPluginUponDestruction() = 0;
+
   // Static management functions -----------------------------------------------
 
   // Possibly start an unbound, spare RenderProcessHost. A subsequent creation

content/public/test/mock_render_process_host.cc

@@ -436,6 +436,8 @@ void MockRenderProcessHost::BindCacheStorage(
   cache_storage_request_ = std::move(request);
 }
 
+void MockRenderProcessHost::CleanupCorbExceptionForPluginUponDestruction() {}
+
 void MockRenderProcessHost::FilterURL(bool empty_allowed, GURL* url) {
   RenderProcessHostImpl::FilterURL(this, empty_allowed, url);
 }

content/public/test/mock_render_process_host.h

@@ -147,6 +147,7 @@ class MockRenderProcessHost : public RenderProcessHost {
   void LockToOrigin(const GURL& lock_url) override;
   void BindCacheStorage(blink::mojom::CacheStorageRequest request,
                         const url::Origin& origin) override;
+  void CleanupCorbExceptionForPluginUponDestruction() override;
 
   // IPC::Sender via RenderProcessHost.
   bool Send(IPC::Message* msg) override;

services/network/cross_origin_read_blocking.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <algorithm>
+#include <set>
 #include <string>
 #include <unordered_set>
 
@@ -15,6 +16,8 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/no_destructor.h"
+#include "base/stl_util.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "net/base/mime_sniffer.h"
@@ -191,6 +194,11 @@ void BlockResponseHeaders(
   headers->RemoveHeaders(names_of_headers_to_remove);
 }
 
+std::set<int>& GetPluginProxyingProcesses() {
+  static base::NoDestructor<std::set<int>> set;
+  return *set;
+}
+
 }  // namespace
 
 MimeType CrossOriginReadBlocking::GetCanonicalMimeType(
@@ -859,4 +867,23 @@ void CrossOriginReadBlocking::ResponseAnalyzer::LogBlockedResponse() {
   LogBytesReadForSniffing();
 }
 
+// static
+void CrossOriginReadBlocking::AddExceptionForPlugin(int process_id) {
+  std::set<int>& plugin_proxies = GetPluginProxyingProcesses();
+  plugin_proxies.insert(process_id);
+}
+
+// static
+bool CrossOriginReadBlocking::ShouldAllowForPlugin(int process_id) {
+  std::set<int>& plugin_proxies = GetPluginProxyingProcesses();
+  return base::ContainsKey(plugin_proxies, process_id);
+}
+
+// static
+void CrossOriginReadBlocking::RemoveExceptionForPlugin(int process_id) {
+  std::set<int>& plugin_proxies = GetPluginProxyingProcesses();
+  size_t number_of_elements_removed = plugin_proxies.erase(process_id);
+  DCHECK_EQ(1u, number_of_elements_removed);
+}
+
 }  // namespace network

services/network/cross_origin_read_blocking.h

@@ -195,6 +195,23 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CrossOriginReadBlocking {
     kYes,
   };
 
+  // Notifies CORB that |process_id| is proxying requests on behalf of a
+  // universal-access plugin and therefore CORB should stop blocking requests
+  // marked as RESOURCE_TYPE_PLUGIN_RESOURCE.
+  //
+  // TODO(lukasza, laforge): https://crbug.com/702995: Remove the static
+  // ...ForPlugin methods once Flash support is removed from Chromium (probably
+  // around 2020 - see https://www.chromium.org/flash-roadmap).
+  static void AddExceptionForPlugin(int process_id);
+
+  // Returns true if CORB should ignore a request initiated by a universal
+  // access plugin - i.e. if |process_id| has been previously passed to
+  // AddExceptionForPlugin.
+  static bool ShouldAllowForPlugin(int process_id);
+
+  // Reverts AddExceptionForPlugin.
+  static void RemoveExceptionForPlugin(int process_id);
+
  private:
   CrossOriginReadBlocking();  // Not instantiable.
 

services/network/network_service.cc

@@ -6,6 +6,7 @@
 
 #include <map>
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -31,6 +32,7 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
 #include "services/network/crl_set_distributor.h"
+#include "services/network/cross_origin_read_blocking.h"
 #include "services/network/mojo_net_log.h"
 #include "services/network/network_context.h"
 #include "services/network/network_usage_accumulator.h"
@@ -431,6 +433,16 @@ void NetworkService::SetCryptConfig(mojom::CryptConfigPtr crypt_config) {
 }
 #endif
 
+void NetworkService::AddCorbExceptionForPlugin(uint32_t process_id) {
+  DCHECK_NE(mojom::kBrowserProcessId, process_id);
+  CrossOriginReadBlocking::AddExceptionForPlugin(process_id);
+}
+
+void NetworkService::RemoveCorbExceptionForPlugin(uint32_t process_id) {
+  DCHECK_NE(mojom::kBrowserProcessId, process_id);
+  CrossOriginReadBlocking::RemoveExceptionForPlugin(process_id);
+}
+
 net::HttpAuthHandlerFactory* NetworkService::GetHttpAuthHandlerFactory() {
   if (!http_auth_handler_factory_) {
     http_auth_handler_factory_ = net::HttpAuthHandlerFactory::CreateDefault(

services/network/network_service.h

@@ -146,6 +146,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkService
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   void SetCryptConfig(mojom::CryptConfigPtr crypt_config) override;
 #endif
+  void AddCorbExceptionForPlugin(uint32_t process_id) override;
+  void RemoveCorbExceptionForPlugin(uint32_t process_id) override;
 
   // Returns the shared HttpAuthHandlerFactory for the NetworkService, creating
   // one if needed.

services/network/public/mojom/network_service.mojom

@@ -286,4 +286,16 @@ interface NetworkService {
   // encrypted cookies can be read or set.
   [EnableIf=needs_crypt_config]
   SetCryptConfig(CryptConfig crypt_config);
+
+  // Notifies CORB (Cross-Origin Read Blocking) that |process_id| is proxying
+  // requests on behalf of a universal-access plugin and therefore CORB should
+  // stop blocking requests marked as RESOURCE_TYPE_PLUGIN_RESOURCE.
+  //
+  // TODO(lukasza, laforge): https://crbug.com/702995: Remove the ...ForPlugin
+  // methods once Flash support is removed from Chromium (probably around 2020
+  // - see https://www.chromium.org/flash-roadmap).
+  AddCorbExceptionForPlugin(uint32 process_id);
+
+  // Reverts AddCorbExceptionForPlugin.
+  RemoveCorbExceptionForPlugin(uint32 process_id);
 };

services/network/url_loader.cc

@@ -352,7 +352,9 @@ URLLoader::URLLoader(
 
   is_nocors_corb_excluded_request_ =
       resource_type_ == factory_params_->corb_excluded_resource_type &&
-      request.fetch_request_mode == mojom::FetchRequestMode::kNoCORS;
+      request.fetch_request_mode == mojom::FetchRequestMode::kNoCORS &&
+      CrossOriginReadBlocking::ShouldAllowForPlugin(
+          factory_params_->process_id);
 
   throttling_token_ = network::ScopedThrottlingToken::MaybeCreate(
       url_request_->net_log().source().id, request.throttling_profile_id);

services/network/url_loader_unittest.cc

@@ -2262,6 +2262,8 @@ TEST_F(URLLoaderTest, CorbExcludedWithNoCors) {
   std::unique_ptr<URLLoader> url_loader;
   mojom::URLLoaderFactoryParams params;
   params.corb_excluded_resource_type = kResourceType;
+  params.process_id = 123;
+  CrossOriginReadBlocking::AddExceptionForPlugin(123);
   url_loader = std::make_unique<URLLoader>(
       context(), nullptr /* network_service_client */,
       DeleteLoaderCallback(&delete_run_loop, &url_loader),
@@ -2279,6 +2281,47 @@ TEST_F(URLLoaderTest, CorbExcludedWithNoCors) {
 
   // The request body is allowed through because CORB isn't applied.
   ASSERT_NE(std::string(), body);
+
+  CrossOriginReadBlocking::RemoveExceptionForPlugin(123);
+}
+
+// This simulates a renderer that pretends to be proxying requests for Flash
+// (when browser didn't actually confirm that Flash is hosted by the given
+// process via CrossOriginReadBlocking::AddExceptionForPlugin).  We should still
+// apply CORB in this case.
+TEST_F(URLLoaderTest, CorbEffectiveWithNoCorsWhenNoActualPlugin) {
+  int kResourceType = 1;
+  ResourceRequest request =
+      CreateResourceRequest("GET", test_server()->GetURL("/hello.html"));
+  request.resource_type = kResourceType;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNoCORS;
+  request.request_initiator = url::Origin::Create(GURL("http://foo.com/"));
+
+  base::RunLoop delete_run_loop;
+  mojom::URLLoaderPtr loader;
+  std::unique_ptr<URLLoader> url_loader;
+  mojom::URLLoaderFactoryParams params;
+  params.corb_excluded_resource_type = kResourceType;
+  params.process_id = 234;
+  // No call to CrossOriginReadBlocking::AddExceptionForPlugin(123) - this is
+  // what we primarily want to cover in this test.
+  url_loader = std::make_unique<URLLoader>(
+      context(), nullptr /* network_service_client */,
+      DeleteLoaderCallback(&delete_run_loop, &url_loader),
+      mojo::MakeRequest(&loader), 0, request, false,
+      client()->CreateInterfacePtr(), TRAFFIC_ANNOTATION_FOR_TESTS, &params,
+      0 /* request_id */, resource_scheduler_client(), nullptr,
+      nullptr /* network_usage_accumulator */);
+
+  client()->RunUntilResponseBodyArrived();
+  std::string body = ReadBody();
+
+  client()->RunUntilComplete();
+
+  delete_run_loop.Run();
+
+  // The request body should be blocked by CORB.
+  ASSERT_EQ(std::string(), body);
 }
 
 }  // namespace network