Whitelisting exts and plugins from cross-site document blocking

Whitelisting exts and plugins from cross-site document blocking

BUG=268640
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221215

[dsjang, 2013-08-31 02:43:35]

This is the first attempt to exclude extension/plugin processes from the
cross-site document blocking checking.

This is only working for excluding extension background pages, not not working
in the following cases:

1) plugins : it seems like plugin processes are not getting network resources
directly from the browser process, but getting it from its parent renderer
process which spawned the plugin process. I saw that flash's crossdomain.xml is
blocked in the renderer process which spanwed the flash, not  in the flash
process.

2) breaking content module hierarchy : cl upload script complains about the fact
that chrome/renderer includes something from content/child.

This CL is for asking you for sharing your ideas how to solve the above
problems.

Thanks in advance, and have a good holiday!

[Charlie Reis, 2013-09-03 16:43:23]

On 2013/08/31 02:43:35, dsjang wrote:
> This is the first attempt to exclude extension/plugin processes from the
> cross-site document blocking checking.
> 
> This is only working for excluding extension background pages, not not working
> in the following cases:
> 
> 1) plugins : it seems like plugin processes are not getting network resources
> directly from the browser process, but getting it from its parent renderer
> process which spawned the plugin process. I saw that flash's crossdomain.xml
is
> blocked in the renderer process which spanwed the flash, not  in the flash
> process.

Argh!  We'll probably have to change plugins to not send their network requests
through the renderer process in the long run.  If we're sure that this is the
only reason we're seeing <object> results in the data, we can probably just
disregard them for now, with a recommendation to change that behavior.

> 2) breaking content module hierarchy : cl upload script complains about the
fact
> that chrome/renderer includes something from content/child.

Suggestion below.

> 
> This CL is for asking you for sharing your ideas how to solve the above
> problems.
> 
> Thanks in advance, and have a good holiday!

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:69: #include
"content/child/site_isolation_policy.h"
Yeah, we can only include things from content/public outside the content module.

Usually, we call out to an interface on ContentRendererClient when we need
chrome/ code to help make a decision about things in content/.  We could
probably add a ContentRendererClient::ShouldEnableSiteIsolationPolicy() and call
it from somewhere in content/renderer.  You'd then put the code below into
ChromeContentRendererClient's implementation of the method.

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:360: process_type ==
switches::kRendererProcess &&
This check doesn't seem necessary.  I think we can only get here in renderer
processes (since it's in chrome/renderer).

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.cc:11: #include
"base/process/process_handle.h"
Is this just for the logging statement?  We should remove both.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.cc:50: // default, and only activated for a
rederer process backed-up by
nit: renderer

Also, "backed up by RenderProcessImpl (so we exclude plugin processes too)" is
redundant with "renderer process."

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.cc:58: LOG(ERROR) <<
"setpolicyactivationflag:" << flag << ":" <<
We should remove this.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.cc:69: if (!g_policy_activated) {
nit: No braces needed on one-line body.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.h:59: static void
SetPolicyActivationFlag(bool flag);
SetPolicyEnabled seems like a simpler name.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.h:173: static bool g_policy_activated;
g_policy_enabled

[dsjang, 2013-09-03 19:13:11]

According to jam, ResourceLoaderBridge::requestor_pid can be used to distinguish
responses for plugins from usual responses. The patch excludes any responses
with non-zero requestor_pid value from the UMA data collection, and blocking
too.

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:69: #include
"content/child/site_isolation_policy.h"
On 2013/09/03 16:43:23, creis wrote:
> Yeah, we can only include things from content/public outside the content
module.
> 
> Usually, we call out to an interface on ContentRendererClient when we need
> chrome/ code to help make a decision about things in content/.  We could
> probably add a ContentRendererClient::ShouldEnableSiteIsolationPolicy() and
call
> it from somewhere in content/renderer.  You'd then put the code below into
> ChromeContentRendererClient's implementation of the method.

Done.

https://codereview.chromium.org/23842002/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:360: process_type ==
switches::kRendererProcess &&
On 2013/09/03 16:43:23, creis wrote:
> This check doesn't seem necessary.  I think we can only get here in renderer
> processes (since it's in chrome/renderer).

Done.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.cc:58: LOG(ERROR) <<
"setpolicyactivationflag:" << flag << ":" <<
On 2013/09/03 16:43:23, creis wrote:
> We should remove this.

Done.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.h:59: static void
SetPolicyActivationFlag(bool flag);
On 2013/09/03 16:43:23, creis wrote:
> SetPolicyEnabled seems like a simpler name.

Done.

https://codereview.chromium.org/23842002/diff/1/content/child/site_isolation_...
content/child/site_isolation_policy.h:173: static bool g_policy_activated;
On 2013/09/03 16:43:23, creis wrote:
> g_policy_enabled

Done.

[Charlie Reis, 2013-09-03 20:22:10]

Looks like SiteIsolationPolicyBrowserTest.CrossSiteDocumentBlockingForMimeType
started failing?

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1312: // SiteIsolationPolicy
for a renderer process without having the extension
nit: without having -> that does not have

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1319: // the extension flag
on.
This comment is redundant with the one above.

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1320: DCHECK(process_type ==
switches::kRendererProcess);
If this passes the try jobs, then I think we can remove the process_type check.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:49: // default, and only activated for a
rederer process backed-up by
You missed my comment about this in the earlier patch set.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:73: // if |origin_pid| is non-zero, it
means that this response is for the plugin
for the -> for a

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:76: // the browser process so that we can
apply the cross-site document blocking
I agree with everything but the last part.  The goal is to make them talk
directly to the browser process so that we do *not* try to apply the document
blocking policy to them.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.h:58: static void SetPolicyEnabled(bool
flag);
flag -> enabled
(here and in the .cc file)

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.h:173: static bool g_policy_enabled;
nit: Add comment about what this does (i.e., false by default, but enables UMA
logging and cross-site document blocking), and add a blank line below it.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.cc:191: return false;
I think we can return true here.  Reasoning: this is only called within renderer
processes, and if any embedders of content don't have extensions, then it's fine
for this to be enabled.  We only need to make sure it's disabled when we know
it's an extension.

No need to change the phrasing elsewhere.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:255: // renderer process.
Currently, we apply the policy only to a render process
render -> renderer
(here and below)

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:256: // running on a normal
page from the internet, excluding render processes for
internet -> web

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:257: // extensions.
Unfortunately, there's no easy way to exclude a renderer
nit: Drop "excluding render processes for extensions," since the concept of
extensions doesn't belong in content.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:258: // process for plugins
here, but our ultimate goal is to exclude plugin
This last sentence is a bit confusing to me.  We don't create a renderer process
for plugins-- it's sort of the other way around.  Can you rephrase?

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
content/renderer/render_process_impl.cc:77: // Turn on the cross-site document
blocking.
nit: drop "the", add "for renderer processes."

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
content/renderer/render_process_impl.cc:78:
DCHECK(GetContentClient()->renderer());
No need for this DCHECK.  It's no different than crashing when we access it
below.

[dsjang, 2013-09-03 22:31:30]

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1312: // SiteIsolationPolicy
for a renderer process without having the extension
On 2013/09/03 20:22:10, creis wrote:
> nit: without having -> that does not have

Done.

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1319: // the extension flag
on.
On 2013/09/03 20:22:10, creis wrote:
> This comment is redundant with the one above.

Done.

https://codereview.chromium.org/23842002/diff/11001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1320: DCHECK(process_type ==
switches::kRendererProcess);
On 2013/09/03 20:22:10, creis wrote:
> If this passes the try jobs, then I think we can remove the process_type
check.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:49: // default, and only activated for a
rederer process backed-up by
On 2013/09/03 20:22:10, creis wrote:
> You missed my comment about this in the earlier patch set.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:73: // if |origin_pid| is non-zero, it
means that this response is for the plugin
On 2013/09/03 20:22:10, creis wrote:
> for the -> for a

Done.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.cc:76: // the browser process so that we can
apply the cross-site document blocking
On 2013/09/03 20:22:10, creis wrote:
> I agree with everything but the last part.  The goal is to make them talk
> directly to the browser process so that we do *not* try to apply the document
> blocking policy to them.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.h:58: static void SetPolicyEnabled(bool
flag);
On 2013/09/03 20:22:10, creis wrote:
> flag -> enabled
> (here and in the .cc file)

Done.

https://codereview.chromium.org/23842002/diff/11001/content/child/site_isolat...
content/child/site_isolation_policy.h:173: static bool g_policy_enabled;
On 2013/09/03 20:22:10, creis wrote:
> nit: Add comment about what this does (i.e., false by default, but enables UMA
> logging and cross-site document blocking), and add a blank line below it.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.cc:191: return false;
On 2013/09/03 20:22:10, creis wrote:
> I think we can return true here.  Reasoning: this is only called within
renderer
> processes, and if any embedders of content don't have extensions, then it's
fine
> for this to be enabled.  We only need to make sure it's disabled when we know
> it's an extension.
> 
> No need to change the phrasing elsewhere.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:255: // renderer process.
Currently, we apply the policy only to a render process
On 2013/09/03 20:22:10, creis wrote:
> render -> renderer
> (here and below)

Done.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:256: // running on a normal
page from the internet, excluding render processes for
On 2013/09/03 20:22:10, creis wrote:
> internet -> web

Done.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:257: // extensions.
Unfortunately, there's no easy way to exclude a renderer
On 2013/09/03 20:22:10, creis wrote:
> nit: Drop "excluding render processes for extensions," since the concept of
> extensions doesn't belong in content.

Done.

https://codereview.chromium.org/23842002/diff/11001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:258: // process for plugins
here, but our ultimate goal is to exclude plugin
On 2013/09/03 20:22:10, creis wrote:
> This last sentence is a bit confusing to me.  We don't create a renderer
process
> for plugins-- it's sort of the other way around.  Can you rephrase?

Done.

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
content/renderer/render_process_impl.cc:77: // Turn on the cross-site document
blocking.
On 2013/09/03 20:22:10, creis wrote:
> nit: drop "the", add "for renderer processes."

Done.

https://codereview.chromium.org/23842002/diff/11001/content/renderer/render_p...
content/renderer/render_process_impl.cc:78:
DCHECK(GetContentClient()->renderer());
On 2013/09/03 20:22:10, creis wrote:
> No need for this DCHECK.  It's no different than crashing when we access it
> below.

Done.

[Charlie Reis, 2013-09-03 22:59:17]

LGTM with nits about the comments.

John, can you confirm that this is what you had in mind for detecting requests
from plugins?  Thanks for your help!

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1309: // SiteIsolationPolicy
is off by default. We would like to activate the
nit: drop "the"

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1311: // processes running an
normal web page from the Internet. We only turn on
nit: a normal

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1313: // flag on. Currently,
this cannot be turned off for resource requests from
No need to mention plugins here, since that's explained elsewhere.  Feel free to
drop the last sentence.

https://codereview.chromium.org/23842002/diff/6001/content/child/site_isolati...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/6001/content/child/site_isolati...
content/child/site_isolation_policy.cc:51: // default, and only activated for a
renderer process, not for extensions.
nit: only activated in renderer processes.

(Don't mention extensions here.)

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
content/public/renderer/content_renderer_client.h:254: // Returns true if we
apply the cross-site document blocking policy to this
nit: if we should

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
content/public/renderer/content_renderer_client.h:257: // policy is not applied
to plugin processes too, and it's done in
Better, but I'm still having trouble following the last sentence.  Perhaps:

The policy should not be applied to plugin processes, which currently proxy
their network requests through renderer processes.  As a result,
SiteIsolationPolicy checks for plugin requests in renderer processes at runtime
and ignores them.

[jam, 2013-09-04 00:32:23]

lgtm

https://codereview.chromium.org/23842002/diff/48001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/48001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:142: 
nit: why add blank line? follow the convention...

https://codereview.chromium.org/23842002/diff/48001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/48001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:259: // plugin requests in
renderer processes at runtime and ignores them.
nit: it's a bit odd that this comment talks about plugins, when this callback
has nothing to do with plugins. so i would take that part out.

[dsjang, 2013-09-04 00:37:37]

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1309: // SiteIsolationPolicy
is off by default. We would like to activate the
On 2013/09/03 22:59:17, creis wrote:
> nit: drop "the"

Done.

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1311: // processes running an
normal web page from the Internet. We only turn on
On 2013/09/03 22:59:17, creis wrote:
> nit: a normal

Done.

https://codereview.chromium.org/23842002/diff/6001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1313: // flag on. Currently,
this cannot be turned off for resource requests from
On 2013/09/03 22:59:17, creis wrote:
> No need to mention plugins here, since that's explained elsewhere.  Feel free
to
> drop the last sentence.

Done.

https://codereview.chromium.org/23842002/diff/6001/content/child/site_isolati...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/23842002/diff/6001/content/child/site_isolati...
content/child/site_isolation_policy.cc:51: // default, and only activated for a
renderer process, not for extensions.
On 2013/09/03 22:59:17, creis wrote:
> nit: only activated in renderer processes.
> 
> (Don't mention extensions here.)

Done.

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
content/public/renderer/content_renderer_client.h:254: // Returns true if we
apply the cross-site document blocking policy to this
On 2013/09/03 22:59:17, creis wrote:
> nit: if we should

Done.

https://codereview.chromium.org/23842002/diff/6001/content/public/renderer/co...
content/public/renderer/content_renderer_client.h:257: // policy is not applied
to plugin processes too, and it's done in
On 2013/09/03 22:59:17, creis wrote:
> Better, but I'm still having trouble following the last sentence.  Perhaps:
> 
> The policy should not be applied to plugin processes, which currently proxy
> their network requests through renderer processes.  As a result,
> SiteIsolationPolicy checks for plugin requests in renderer processes at
runtime
> and ignores them.

Done.

https://codereview.chromium.org/23842002/diff/48001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/48001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:142: 
On 2013/09/04 00:32:24, jam wrote:
> nit: why add blank line? follow the convention...

Done.

https://codereview.chromium.org/23842002/diff/48001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23842002/diff/48001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:259: // plugin requests in
renderer processes at runtime and ignores them.
On 2013/09/04 00:32:24, jam wrote:
> nit: it's a bit odd that this comment talks about plugins, when this callback
> has nothing to do with plugins. so i would take that part out.

Done.

[commit-bot: I haz the power, 2013-09-04 00:41:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsjang@chromium.org/23842002/57001

[commit-bot: I haz the power, 2013-09-04 17:17:58]

Change committed as 221215