New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 874515 link

Starred by 1 user
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Aug 31
Cc:
jam@chromium.org
█ nick@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 786673



Sign in to add a comment

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

Project Member Reported by lukasza@chromium.org, Aug 15 
It seems okay to assume that actually running Flash is outside of Site Isolation's threat model:

- According to https://www.chromium.org/flash-roadmap Flash will be disabled in July 2019 and removed altogether in December 2020.

- Even today, running Flash requires clearing some hurdles first:
  - The user has to click-to-play and/or allow the plugin on a given page via content settings (see the attached screenshot)
  - The enterprise policy must not block the plugin (see https://www.chromium.org/administrators/policy-list-3#PluginsBlockedForUrls)

- Site Isolation cannot protect against compromised Flash plugin processes (which can issue any cross-origin requests while pretending that they consult crossdomain.xml unless CORB implements parsing and stateful awareness of crossdomain.xml status)


So - when CORB sees a network request from a given |render_process_id|, it should ignore RESOURCE_TYPE_PLUGIN_RESOURCE if only if |render_process_id| is *actually* hosting Flash (and therefore proxying network requests for Flash).  This should make Site Isolation and CORB robust against compromised renderers which pretend that they are proxying requests on behalf of Flash (without actually running Flash and clearing browser-controlled hurdles outlined at the top here).


Rough implementation idea:
1. Have a global set of |flash_hosting_render_process_ids| (in the Browser process or the NetworkService process) and consult this set when making CORB block-or-allow decision
2. Add a |render_process_id_ to this set only after content::PluginServiceImpl::FindOrStartPpapiPluginProcess verifies that
   2.1) the plugin it is dealing with is either ppapi::PERMISSION_FLASH or ppapi::PERMISSION_TESTING (*)
   2.2) the plugin is allowed to run

There will be some NetworkService-specific and CrossSiteDocumentResourceHandler-specific implementation parts, but hopefully some of the implementation can be done in CrossOriginReadBlocking class and shared.

(*) Note that PERMISSION_PDF doesn't need special-handling, because 1) CORB doesn't block PDF documents today and 2) PDF requests come with initiator=chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai and are therefore covered by extensions-specific CORB exception.


Note that this bug is an alternative, simpler way of enforcing Site Isolation to the approach proposed in issue 855171 (Plugins should use a separate URLLoaderFactory)

Comment 1 by lukasza@chromium.org, Aug 15

flash-content-settings-dialog.png
17.0 KB View Download

Comment 2 by lukasza@chromium.org, Aug 16

Cc: jam@chromium.org
WIP CL @ https://chromium-review.googlesource.com/1178885
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d706632fc05eea8322a682428ae8903586ba4ba

commit 7d706632fc05eea8322a682428ae8903586ba4ba
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Aug 20 15:42:56 2018

Update Flash-related aspects of the Site Isolation threat model.

Bug: 816318,  874515 
Change-Id: I8d165ec355036f8c190bb21e1c50ee8f9b6a4d5a
Reviewed-on: https://chromium-review.googlesource.com/1176389
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584459}
[modify] https://crrev.com/7d706632fc05eea8322a682428ae8903586ba4ba/docs/security/side-channel-threat-model.md
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc4a77252540dbe39f7e2821c7caa2cebd073920

commit bc4a77252540dbe39f7e2821c7caa2cebd073920
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Aug 22 17:21:56 2018

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}
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/browser/loader/cross_site_document_resource_handler_unittest.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/public/browser/render_process_host.h
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/cross_origin_read_blocking.h
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/network_service.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/network_service.h
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/url_loader.cc
[modify] https://crrev.com/bc4a77252540dbe39f7e2821c7caa2cebd073920/services/network/url_loader_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f94b4106732d4abfd4cc1db54e87aec519b02ae4

commit f94b4106732d4abfd4cc1db54e87aec519b02ae4
Author: Łukasz Anforowicz <lukasza@chromium.org>
Date: Wed Aug 22 22:19:44 2018

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

This reverts commit bc4a77252540dbe39f7e2821c7caa2cebd073920.

Reason for revert: Speculating that this may cause flaky test crashes:  https://crbug.com/876903 

Original change's description:
> 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}

TBR=nasko@chromium.org,jam@chromium.org,lukasza@chromium.org

Change-Id: I683e189587a2edfe8fbf516b757a6590c6441c9c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  874515 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1185941
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585281}
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/browser/loader/cross_site_document_resource_handler_unittest.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/public/browser/render_process_host.h
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/cross_origin_read_blocking.h
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/network_service.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/network_service.h
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/url_loader.cc
[modify] https://crrev.com/f94b4106732d4abfd4cc1db54e87aec519b02ae4/services/network/url_loader_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e

commit e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Aug 27 21:32:25 2018

[reland] No CORB exception for plugins, unless Flash actually runs.

This is a reland of r585124 with an extra null check in
AddCorbExceptionForPluginOnUIThread.  Original CL description
follows below.

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: I807e2d7f47f753c9a097d2bceed22e9eef1a88f9
Tbr: Nasko Oskov <nasko@chromium.org>
Tbr: John Abd-El-Malek <jam@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1191324
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586422}
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/browser/loader/cross_site_document_resource_handler_unittest.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/public/browser/render_process_host.h
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/cross_origin_read_blocking.h
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/network_service.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/network_service.h
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/url_loader.cc
[modify] https://crrev.com/e4986b2043eb1f7df28c8c94b1c76ffab2c3b85e/services/network/url_loader_unittest.cc

Comment 7 by lukasza@chromium.org, Aug 31

Status: Fixed (was: Assigned)
I think this can be closed as Fixed.

For full transparency - I think that there is a race condition that I recently realized exists, but I think we don't need to do anything about it.  The race/problem is because the "add exception for process id X" IPC from Browser->NetworkService process is not synchronized in any way with "start url request" IPCs from the Renderer (or Plugin) process to the NetworkService process.  I think this is not a big problem, because A) Flash will be deprecated soon and B) the overhead of an extra plugin process should help avoid the race in practice:
- extra process means extra delay when initially spawning the plugin process (OTOH, I do note that this delay won’t happen when reusing a plugin process from a new renderer process)
- Network requests from the plugin process don’t go directly to the NetworkService process, but are proxied through a renderer process first.
►

Sign in to add a comment