PlzNavigate: Enforce frame-src CSP on the browser.

PlzNavigate: Enforce frame-src CSP on the browser.

Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.

This is based on arthursonzogni's patch
https://codereview.chromium.org/2655463006/ that I'm trying to land as he's OOO
and it's required to experiment with PlzNavigate on Canary.

BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[clamy, 2017-03-02 16:47:12]

Description was changed from

==========
PlzNavigate: Enforce frame-src CSP on the browser.

Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.

This is based on arthursonzogni's patch
https://codereview.chromium.org/2655463006/ that I'm trying to land as he's OOO
and it's required to experiment with PlzNavigate on Canary.

BUG=685074
==========

to

==========
PlzNavigate: Enforce frame-src CSP on the browser.

Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.

This is based on arthursonzogni's patch
https://codereview.chromium.org/2655463006/ that I'm trying to land as he's OOO
and it's required to experiment with PlzNavigate on Canary.

BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2017-03-02 19:09:42]

Description was changed from

==========
PlzNavigate: Enforce frame-src CSP on the browser.

Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.

This is based on arthursonzogni's patch
https://codereview.chromium.org/2655463006/ that I'm trying to land as he's OOO
and it's required to experiment with PlzNavigate on Canary.

BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Enforce frame-src CSP on the browser.

Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.

This is based on arthursonzogni's patch
https://codereview.chromium.org/2655463006/ that I'm trying to land as he's OOO
and it's required to experiment with PlzNavigate on Canary.

BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2017-03-02 19:09:42]

clamy@chromium.org changed reviewers:
+ alexmos@chromium.org, creis@chromium.org

[clamy, 2017-03-02 19:12:46]

@alexmos, creis: PTAL
As arthursonzogni is OOO and CSP support in PlzNavigate is high priority I'm
picking up his patches and trying to get them to land ASAP.

The latest patchset here should have you comments addressed Alex.
Charlie, I've tried to fix the issue with
SiteDetailsBrowserTest.PlatformAppsNotIsolated by modifying the code in
RenderFrameHostManager so that we do not create an OOPIF if the SitePartitions
would be different from the parent frame. From the test, it seems we do that
when --site-per-process is enabled, so I still allow the switch in that case.
Let me know if this should be changed.

[clamy, 2017-03-02 19:12:58]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:13:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 20:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 20:46:10]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-03-03 16:55:57]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2017-03-03 16:56:26]

@nasko: could you PTAL at this since Charlie is OOO? Thanks!

[nasko, 2017-03-03 23:04:24]

Just a handful of comments. The main concern is whether this CL should just
cancel the navigation, as it is today to minimize change of behavior.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:194: is_redirect))
nit: {} needed because of the two line if statement.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:195: return
NavigationThrottle::BLOCK_REQUEST;
I thought the idea was to return CANCELLED here, so we preserve the existing
behavior of not displaying an error page. Why can't we do this first, to
minimize the change of behavior in this specific CL, then in a follow up address
this specifically?

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:197: return
NavigationThrottle::PROCEED;
I wonder if it will be better to fail closed, the return value to be BLOCK
instead of PROCEED, so we notice bugs quickly :).

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:177: void ResetCSPHeaders();
nit: I'd capitalize it as ResetCspHeaders, a bit easier to read and compatible
with the style guide.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:279:
CSPContext::SetSelf(origin);
Calling this here means that this method is no longer simple setter. It should
be changed to CamelCased method in the cc file.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:817: // SiteInstance
would use the same storage partition as its parent. If
nit: StoragePartition

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:820: // of its parent.
That seems like a good property to enforce long term, but I'm a bit weary of
introducing it as part of this patch, as it will be hard to discover.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:831:
(!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&
Why check for UseDedicatedProcessesForAllSites? If we want to uniformly enforce
that no frame in the same WebContents uses a different StoragePartition, then
UseDedicatedProcessesForAllSites shouldn't factor in that decision.

https://codereview.chromium.org/2727633005/diff/20001/content/common/content_...
File content/common/content_security_policy/csp_context.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/common/content_...
content/common/content_security_policy/csp_context.h:30: bool
AllowContentSecurityPolicy(CSPDirective::Name directive_name,
nit: IsAllowedByCsp?

[arthursonzogni, 2017-03-06 15:10:11]

arthursonzogni@chromium.org changed reviewers:
+ arthursonzogni@chromium.org

[arthursonzogni, 2017-03-06 15:10:13]

Thanks Camille! I will rebase from you patch.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/ancestor_throttle.cc (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:194: is_redirect))
On 2017/03/03 23:04:23, nasko (slow) wrote:
> nit: {} needed because of the two line if statement.

Done.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:195: return
NavigationThrottle::BLOCK_REQUEST;
On 2017/03/03 23:04:23, nasko (slow) wrote:
> I thought the idea was to return CANCELLED here, so we preserve the existing
> behavior of not displaying an error page. Why can't we do this first, to
> minimize the change of behavior in this specific CL, then in a follow up
address
> this specifically?

I had two options: returning BLOCK_REQUEST or CANCEL.
I used BLOCK_REQUEST instead of CANCEL because when an error page is loaded, it
fires the 'load' event and restrict some data the embedder of the iframe could
get.

This is necessary, because an evil embedded could tries to load a page and
detect if an redirect happens  (to know if the user is logged in for instance,
or to know some data based on the URL) by using the CSP source-expressions.

We could try to make return CANCEL and make another set of IPC to tell blink to
achieve the same result, but after some discussions, we decide that loading an
error page was the best option.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:197: return
NavigationThrottle::PROCEED;
On 2017/03/03 23:04:23, nasko (slow) wrote:
> I wonder if it will be better to fail closed, the return value to be BLOCK
> instead of PROCEED, so we notice bugs quickly :).

Sorry, I don't understand :) Do you meant reversing the condition?

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:177: void ResetCSPHeaders();
On 2017/03/03 23:04:23, nasko (slow) wrote:
> nit: I'd capitalize it as ResetCspHeaders, a bit easier to read and compatible
> with the style guide. 

Done.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:279:
CSPContext::SetSelf(origin);
On 2017/03/03 23:04:23, nasko (slow) wrote:
> Calling this here means that this method is no longer simple setter. It should
> be changed to CamelCased method in the cc file.

Done.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:817: // SiteInstance
would use the same storage partition as its parent. If
On 2017/03/03 23:04:23, nasko (slow) wrote:
> nit: StoragePartition

Done.

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:820: // of its parent.
On 2017/03/03 23:04:23, nasko (slow) wrote:
> That seems like a good property to enforce long term, but I'm a bit weary of
> introducing it as part of this patch, as it will be hard to discover.

I moved it to:
https://codereview.chromium.org/2732883003

https://codereview.chromium.org/2727633005/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:831:
(!SiteIsolationPolicy::UseDedicatedProcessesForAllSites() &&
On 2017/03/03 23:04:23, nasko (slow) wrote:
> Why check for UseDedicatedProcessesForAllSites? If we want to uniformly
enforce
> that no frame in the same WebContents uses a different StoragePartition, then
> UseDedicatedProcessesForAllSites shouldn't factor in that decision.

I checked with Camille. It was because what is written in
SiteDetailsBrowserTest.PlatformAppsNotIsolated about --isolate-extensions. This
comment is no more valid since Chrome no longer allow external web content
inside sandboxed page in Platform Apps V2.
I removed it in https://codereview.chromium.org/2732883003

Not using a different process when UseDedicatedProcessesForAllSites is true is a
little bit weird though.

https://codereview.chromium.org/2727633005/diff/20001/content/common/content_...
File content/common/content_security_policy/csp_context.h (right):

https://codereview.chromium.org/2727633005/diff/20001/content/common/content_...
content/common/content_security_policy/csp_context.h:30: bool
AllowContentSecurityPolicy(CSPDirective::Name directive_name,
On 2017/03/03 23:04:23, nasko (slow) wrote:
> nit: IsAllowedByCsp?

Done.

[nasko, 2017-03-09 05:37:36]

Hey Arthur, which CL will be the authoritative for this work? This CL or
https://codereview.chromium.org/2655463006/?

[arthursonzogni, 2017-03-09 13:17:38]

On 2017/03/09 05:37:36, nasko (slow) wrote:
> Hey Arthur, which CL will be the authoritative for this work? This CL or
> https://codereview.chromium.org/2655463006/

Yes, I took back Camille's work in
https://codereview.chromium.org/2655463006
and it is the authoritative one. Sorry if there was some confusion.