OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

Currently with OOPIFs, when a page from site A embeds a cross-site
frame from site B, and the subframe is blocked due to X-Frame-Options
or CSP frame-ancestors, the blocking happens in process B, after the
frame gets transferred to a new process, but before commit (or rather,
DidFailProvisionalLoad, which is what happens because of the block).

There are several problems with this:

1. The renderer for frame B gets killed while trying to forward the
  load event to the parent process, so that it can get fired on the
  <iframe> element there. (see issue 584845).

2. As a result, the load event for the <iframe> element in process A
never fires as it should.  (Firing this event is necessary so that the
blocking can't be observed.)

3. The blocked subframe is kept in process A, but it still has
A's origin, meaning it appears as a same-site frame to its parent,
rather than a cross-site frame, as it should after the block.  Normal
blocking logic handles this by setting a unique origin on the blocked
frame, but this happens in process B, and process A never learns about
that.  In addition, the browser process still thinks the subframe's
origin is A.

4. The load that was started for the subframe in process A never gets
stopped.  In particular, this is problematic for layout tests, which
wait for DidStopLoading in process A to end the test.

Basically, process A never finds out that the subframe failed to load
in a different process due to CSP/XFO blocking, and can't take
appropriate action.

The proper way to fix this is by enforcing XFO/CSP in the browser
process, so that we don't need to create the unnecessary new process
in the first place.  However, that is a longer-term effort (see
https://crbug.com/555418).  This CL provides a stop-gap solution of
forwarding the notification about the block from process B to process
A, so that process A can (1) set the subframe's origin to unique, and
(2) fire the load event on its FrameOwner.  The browser->renderer part
of this notification can be reused once the browser process can do the
blocking.

This CL also generically fixes the case of stopping the load in the
current RenderFrameHost when a pending RenderFrameHost never commits,
which solves problem 4 and guards any future such problems.

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[alexmos, 2016-02-19 06:52:09]

Description was changed from

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

[Draft only, do not land]

BUG=584845
==========

to

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

[Draft only, do not land]

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-02-25 21:19:31]

Description was changed from

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

[Draft only, do not land]

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

Currently with OOPIFs, when a page from site A embeds a cross-site
frame from site B, and the subframe is blocked due to X-Frame-Options
or CSP frame-ancestors, the blocking happens in process B, after the
frame gets transferred to a new process, but before commit (or rather,
DidFailProvisionalLoad, which is what happens because of the block).

There are several problems with this:

1. The renderer for frame B gets killed while trying to forward the
  load event to the parent process, so that it can get fired on the
  <iframe> element there. (see issue 584845).

2. As a result, the load event for the <iframe> element in process A
never fires as it should.  (Firing this event is necessary so that the
blocking can't be observed.)

3. The blocked (empty) subframe is kept in process A, but it still has
A's origin, meaning it appears as a same-site frame to its parent,
rather than a cross-site frame, as it should after the block.  Normal
blocking logic handles this by setting a unique origin on the blocked
frame, but this happens in process B, and process A never learns about
that.  In addition, the browser process still thinks the subframe's
origin is A.

4. The load that was started for the subframe in process A never gets
stopped.  In particular, this is problematic for layout tests, which
wait for DidStopLoading in process A to end the test.

Basically, process A never finds out that the subframe failed to load
in a different process due to CSP/XFO blocking, and can't take
appropriate action.

The proper way to fix this is by enforcing XFO/CSP in the browser
process, so that we don't need to create the unnecessary new process
in the first place.  However, that is a longer-term effort (see
https://crbug.com/555418).  This CL provides a stop-gap solution of
forwarding the notification about the block from process B to process
A, so that process A can (1) set the subframe's origin to unique, and
(2) fire the load event on its FrameOwner.  The browser->renderer part
of this notification can be reused once the browser process can do the
blocking.

This CL also generically fixes the case of stopping the load in the
current RenderFrameHost when a pending RenderFrameHost never commits,
which solves problem 4 and guards any future such problems.

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-02-25 21:59:11]

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

[alexmos, 2016-02-25 21:59:12]

Hey Charlie, can you please take an initial look?  This looks long (sorry!), but
most of it is just plumbing to forward the notification that a subframe was
blocked due to CSP/XFO in a pending RFH to the frame's current process.  This
new IPC allows us to do the stuff done normally as part of XFO/CSP blocking in
the correct process.

One question is whether to take this approach vs. leave the blocked frame in the
new process (even though it was blocked).  That would require us to change pages
that are blocked by XFO/CSP to actually commit (so we can create proxies, etc.),
which seemed like a bigger change, given that we'll revisit this soon when
moving the enforcement to the browser process.  The spec
(https://www.w3.org/TR/CSP2/#directive-frame-ancestors) actually says to treat
the blocked frame as an 200 with an empty body, or to redirect to a friendly
error page, but this isn't what Chrome does today (it sends a DidFail; and the
blocked frame is blank, which isn't exactly a friendly error page :)).  Let me
know what you think.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:243: if (render_frame_host ==
manager->pending_frame_host())
Here, one case I was thinking about is if A embeds a subframe B (which gets
blocked), but A navigates the subframe to C in parallel to B getting blocked. 
I'm not sure if it's possible to get this race, as we'll probably clean up the
old pending RFH for B before creating the new pending RFH for C (right?).  But
regardless, this code should do the right thing even if the race is possible
(which is to not stop the new load in process A).

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:244:
manager->current_frame_host()->Stop();
I did this more generically, as opposed to stopping the load as part of
OnCancelLoadAfterXFrameOptionsOrCSPDenied, to prevent similar future cases from
falling through, as we discussed.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:244:
manager->current_frame_host()->Stop();
Note also that I'm not cleaning up the pending RFH in which the frame was
blocked, which seems consistent in general with how DidFail behaves (IIUC, the
pending RFH will be cleaned up on next navigation; there's a comment about that
above with a TODO for you :).  I actually tried cleaning up the pending RFH, but
ran into various trouble and left it alone for now.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1535:
frame_tree_node_->SetCurrentOrigin(url::Origin());
I think this matters in default Chrome even without --site-per-process, so I
always send this IPC and do this.  Previously, we would've returned the last
committed origin for a blocked frame, whereas I think it's more correct to
return a unique origin, which would match Blink's behavior.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1594: if (this !=
frame_tree_node_->current_frame_host())
This is pretty ugly, but I tried to detect whether or not to send this IPC from
Blink in the blocked case, and I thought that was even uglier.  At least this is
in one place and easy to rip out once we're done with 555418.  Also, dispatching
loads on <iframe> elements can now probably be done together with forwarding
DidStopLoading, as opposed to this separate IPC, in which case this function
will disappear entirely (but this is probably best left to a separate CL).

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

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:477:
DCHECK(request_routing_id == pending_routing_id ||
Relaxing this DCHECK was necessary to enable navigating the blocked subframe to
a different site (c.com in my test), and we discussed that it might happen more
generically.

[alexmos, 2016-02-25 22:19:33]

+site-isolation-reviews, I thought I did that when I initially sent this, but
apparently not.

[Charlie Reis, 2016-02-26 01:13:21]

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

[Charlie Reis, 2016-02-26 01:13:21]

I'm only a small way in, but I wanted to send a few initial questions.  Also
adding clamy@, since there's a chance this affects PlzNavigate as well (and
since she was recently working on the loading behavior).

On 2016/02/25 21:59:12, alexmos wrote:
> Hey Charlie, can you please take an initial look?  This looks long (sorry!),
but
> most of it is just plumbing to forward the notification that a subframe was
> blocked due to CSP/XFO in a pending RFH to the frame's current process.  This
> new IPC allows us to do the stuff done normally as part of XFO/CSP blocking in
> the correct process.
> 
> One question is whether to take this approach vs. leave the blocked frame in
the
> new process (even though it was blocked).  That would require us to change
pages
> that are blocked by XFO/CSP to actually commit (so we can create proxies,
etc.),
> which seemed like a bigger change, given that we'll revisit this soon when
> moving the enforcement to the browser process.  The spec
> (https://www.w3.org/TR/CSP2/#directive-frame-ancestors) actually says to treat
> the blocked frame as an 200 with an empty body, or to redirect to a friendly
> error page, but this isn't what Chrome does today (it sends a DidFail; and the
> blocked frame is blank, which isn't exactly a friendly error page :)).  Let me
> know what you think.

I'm partly confused-- when you say the blocked frame is blank in Chrome, doesn't
that match the "empty body" clause of the spec?  Or do we not actually commit a
new blank page?

I think I'll be able to comment more on which process the blocked page should
end up in once that's clear and I have a chance to look at the rest.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:243: if (render_frame_host ==
manager->pending_frame_host())
On 2016/02/25 21:59:12, alexmos wrote:
> Here, one case I was thinking about is if A embeds a subframe B (which gets
> blocked), but A navigates the subframe to C in parallel to B getting blocked. 
> I'm not sure if it's possible to get this race, as we'll probably clean up the
> old pending RFH for B before creating the new pending RFH for C (right?).  But
> regardless, this code should do the right thing even if the race is possible
> (which is to not stop the new load in process A).

Right, the RFHM will destroy the RFH for B before creating the one for C.

That said, ensuring that we're not hearing this from some other RFH makes sense.
 It could be one pending deletion, or even the current one.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:244:
manager->current_frame_host()->Stop();
On 2016/02/25 21:59:12, alexmos wrote:
> Note also that I'm not cleaning up the pending RFH in which the frame was
> blocked, which seems consistent in general with how DidFail behaves (IIUC, the
> pending RFH will be cleaned up on next navigation; there's a comment about
that
> above with a TODO for you :).  I actually tried cleaning up the pending RFH,
but
> ran into various trouble and left it alone for now.

Right.  I'm sad to hear that there's still trouble waiting there, since I was
hoping it would be easier after so much time had passed.  Anyway, that's
unrelated to this.

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:244:
manager->current_frame_host()->Stop();
On 2016/02/25 21:59:12, alexmos wrote:
> I did this more generically, as opposed to stopping the load as part of
> OnCancelLoadAfterXFrameOptionsOrCSPDenied, to prevent similar future cases
from
> falling through, as we discussed.

Camille, is RFHI::Stop() the right way to handle this?

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1535:
frame_tree_node_->SetCurrentOrigin(url::Origin());
On 2016/02/25 21:59:12, alexmos wrote:
> I think this matters in default Chrome even without --site-per-process, so I
> always send this IPC and do this.  Previously, we would've returned the last
> committed origin for a blocked frame, whereas I think it's more correct to
> return a unique origin, which would match Blink's behavior.

Acknowledged.

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
content/common/frame_messages.h:851:
IPC_MESSAGE_ROUTED0(FrameMsg_CancelLoadAfterXFrameOptionsOrCSPDenied)
Is there a reason to make this specific to these features?  I'm wondering if
there's a more general "blocked load" concept that this might cover.

[alexmos, 2016-02-26 02:38:05]

> On 2016/02/25 21:59:12, alexmos wrote:
> > Hey Charlie, can you please take an initial look?  This looks long (sorry!),
> but
> > most of it is just plumbing to forward the notification that a subframe was
> > blocked due to CSP/XFO in a pending RFH to the frame's current process. 
This
> > new IPC allows us to do the stuff done normally as part of XFO/CSP blocking
in
> > the correct process.
> > 
> > One question is whether to take this approach vs. leave the blocked frame in
> the
> > new process (even though it was blocked).  That would require us to change
> pages
> > that are blocked by XFO/CSP to actually commit (so we can create proxies,
> etc.),
> > which seemed like a bigger change, given that we'll revisit this soon when
> > moving the enforcement to the browser process.  The spec
> > (https://www.w3.org/TR/CSP2/#directive-frame-ancestors) actually says to
treat
> > the blocked frame as an 200 with an empty body, or to redirect to a friendly
> > error page, but this isn't what Chrome does today (it sends a DidFail; and
the
> > blocked frame is blank, which isn't exactly a friendly error page :)).  Let
me
> > know what you think.
> 
> I'm partly confused-- when you say the blocked frame is blank in Chrome,
doesn't
> that match the "empty body" clause of the spec?  Or do we not actually commit
a
> new blank page?
> 
> I think I'll be able to comment more on which process the blocked page should
> end up in once that's clear and I have a chance to look at the rest.

Today, we don't commit a new blank page.  We abort the load and send a
DidFailProvisionalLoad.  If the frame was already loaded something else, that
old page will still be showing if the frame were navigated to another URL that
was blocked (I've just verified this).  The old page will still be interactive,
but it won't be scriptable anymore from other frames (since it gained a unique
origin as a result of the block).

So today, Chrome doesn't obey the "empty body" part of the spec, and this CL
doesn't change that.

Sorry, I was confused when I wrote "blank frame", since I was thinking about the
case where the blocked URL is the frame's first navigation.  In that case, the
blank page that's visible after the block is just the initial about:blank.  I'll
go through the comments in this CL and remove any similar language about blank
frames.

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
content/common/frame_messages.h:851:
IPC_MESSAGE_ROUTED0(FrameMsg_CancelLoadAfterXFrameOptionsOrCSPDenied)
On 2016/02/26 01:13:22, Charlie Reis (slow til Mar 7) wrote:
> Is there a reason to make this specific to these features?  I'm wondering if
> there's a more general "blocked load" concept that this might cover.

Good question.  It's only specific in the sense that we do some special stuff in
the parent frame (that the spec requires) so that it can't determine whether the
child was blocked or not (i.e., dispatch the load event on the iframe element,
change the blocked frame's origin to unique).  I don't know of anything else
besides XFO+CSP that needs this at the moment, but there might be other kinds of
blocked loads in the future that might reuse this (e.g., clickjacking
protection), so maybe this whole plumbing should be renamed to 
FrameMsg_BlockedLoad, etc.  WDYT?  I really like that this would simplify all
the names. :)

[Charlie Reis, 2016-02-26 21:10:02]

On 2016/02/26 02:38:05, alexmos wrote:
> > On 2016/02/25 21:59:12, alexmos wrote:
> > > Hey Charlie, can you please take an initial look?  This looks long
(sorry!),
> > but
> > > most of it is just plumbing to forward the notification that a subframe
was
> > > blocked due to CSP/XFO in a pending RFH to the frame's current process. 
> This
> > > new IPC allows us to do the stuff done normally as part of XFO/CSP
blocking
> in
> > > the correct process.
> > > 
> > > One question is whether to take this approach vs. leave the blocked frame
in
> > the
> > > new process (even though it was blocked).  That would require us to change
> > pages
> > > that are blocked by XFO/CSP to actually commit (so we can create proxies,
> > etc.),
> > > which seemed like a bigger change, given that we'll revisit this soon when
> > > moving the enforcement to the browser process.  The spec
> > > (https://www.w3.org/TR/CSP2/#directive-frame-ancestors) actually says to
> treat
> > > the blocked frame as an 200 with an empty body, or to redirect to a
friendly
> > > error page, but this isn't what Chrome does today (it sends a DidFail; and
> the
> > > blocked frame is blank, which isn't exactly a friendly error page :)). 
Let
> me
> > > know what you think.
> > 
> > I'm partly confused-- when you say the blocked frame is blank in Chrome,
> doesn't
> > that match the "empty body" clause of the spec?  Or do we not actually
commit
> a
> > new blank page?
> > 
> > I think I'll be able to comment more on which process the blocked page
should
> > end up in once that's clear and I have a chance to look at the rest.
> 
> Today, we don't commit a new blank page.  We abort the load and send a
> DidFailProvisionalLoad.  If the frame was already loaded something else, that
> old page will still be showing if the frame were navigated to another URL that
> was blocked (I've just verified this).  The old page will still be
interactive,
> but it won't be scriptable anymore from other frames (since it gained a unique
> origin as a result of the block).
> 
> So today, Chrome doesn't obey the "empty body" part of the spec, and this CL
> doesn't change that.
> 
> Sorry, I was confused when I wrote "blank frame", since I was thinking about
the
> case where the blocked URL is the frame's first navigation.  In that case, the
> blank page that's visible after the block is just the initial about:blank. 
I'll
> go through the comments in this CL and remove any similar language about blank
> frames.

Wow, that current behavior sounds bad!  You could basically revoke the effective
origin of a page by navigating it to something that you know will be blocked. 
I'm not sure if that can lead to more interesting attacks than DoS, though it
probably would be a way to detect if the block happened or not.  (If the
attacker owns the subframe's current page and the navigation is blocked, the
current page will keep existing and can check in with the server.)

Sounds like that will need to be fixed at some point, unless there's reasons we
took that path.  If it does need to change, how disruptive will that be for the
changes in this CL?  It's worth thinking about whether that change should be
made first, or if it's relatively easy to make it later.

Anyway, I'll continue my actual review now...

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1710283003/diff/60001/content/common/frame_me...
content/common/frame_messages.h:851:
IPC_MESSAGE_ROUTED0(FrameMsg_CancelLoadAfterXFrameOptionsOrCSPDenied)
On 2016/02/26 02:38:05, alexmos wrote:
> On 2016/02/26 01:13:22, Charlie Reis (slow til Mar 7) wrote:
> > Is there a reason to make this specific to these features?  I'm wondering if
> > there's a more general "blocked load" concept that this might cover.
> 
> Good question.  It's only specific in the sense that we do some special stuff
in
> the parent frame (that the spec requires) so that it can't determine whether
the
> child was blocked or not (i.e., dispatch the load event on the iframe element,
> change the blocked frame's origin to unique).  I don't know of anything else
> besides XFO+CSP that needs this at the moment, but there might be other kinds
of
> blocked loads in the future that might reuse this (e.g., clickjacking
> protection), so maybe this whole plumbing should be renamed to 
> FrameMsg_BlockedLoad, etc.  WDYT?  I really like that this would simplify all
> the names. :)

Yeah, let's make it BlockedLoad, with documentation about all the things it does
(and the motivation).  That should make it a bit easier to follow, and possibly
easier to reuse later.

[alexmos, 2016-02-26 21:25:02]

Description was changed from

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

Currently with OOPIFs, when a page from site A embeds a cross-site
frame from site B, and the subframe is blocked due to X-Frame-Options
or CSP frame-ancestors, the blocking happens in process B, after the
frame gets transferred to a new process, but before commit (or rather,
DidFailProvisionalLoad, which is what happens because of the block).

There are several problems with this:

1. The renderer for frame B gets killed while trying to forward the
  load event to the parent process, so that it can get fired on the
  <iframe> element there. (see issue 584845).

2. As a result, the load event for the <iframe> element in process A
never fires as it should.  (Firing this event is necessary so that the
blocking can't be observed.)

3. The blocked (empty) subframe is kept in process A, but it still has
A's origin, meaning it appears as a same-site frame to its parent,
rather than a cross-site frame, as it should after the block.  Normal
blocking logic handles this by setting a unique origin on the blocked
frame, but this happens in process B, and process A never learns about
that.  In addition, the browser process still thinks the subframe's
origin is A.

4. The load that was started for the subframe in process A never gets
stopped.  In particular, this is problematic for layout tests, which
wait for DidStopLoading in process A to end the test.

Basically, process A never finds out that the subframe failed to load
in a different process due to CSP/XFO blocking, and can't take
appropriate action.

The proper way to fix this is by enforcing XFO/CSP in the browser
process, so that we don't need to create the unnecessary new process
in the first place.  However, that is a longer-term effort (see
https://crbug.com/555418).  This CL provides a stop-gap solution of
forwarding the notification about the block from process B to process
A, so that process A can (1) set the subframe's origin to unique, and
(2) fire the load event on its FrameOwner.  The browser->renderer part
of this notification can be reused once the browser process can do the
blocking.

This CL also generically fixes the case of stopping the load in the
current RenderFrameHost when a pending RenderFrameHost never commits,
which solves problem 4 and guards any future such problems.

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Handle cross-site frames being blocked by X-Frame-Options or CSP.

Currently with OOPIFs, when a page from site A embeds a cross-site
frame from site B, and the subframe is blocked due to X-Frame-Options
or CSP frame-ancestors, the blocking happens in process B, after the
frame gets transferred to a new process, but before commit (or rather,
DidFailProvisionalLoad, which is what happens because of the block).

There are several problems with this:

1. The renderer for frame B gets killed while trying to forward the
  load event to the parent process, so that it can get fired on the
  <iframe> element there. (see issue 584845).

2. As a result, the load event for the <iframe> element in process A
never fires as it should.  (Firing this event is necessary so that the
blocking can't be observed.)

3. The blocked subframe is kept in process A, but it still has
A's origin, meaning it appears as a same-site frame to its parent,
rather than a cross-site frame, as it should after the block.  Normal
blocking logic handles this by setting a unique origin on the blocked
frame, but this happens in process B, and process A never learns about
that.  In addition, the browser process still thinks the subframe's
origin is A.

4. The load that was started for the subframe in process A never gets
stopped.  In particular, this is problematic for layout tests, which
wait for DidStopLoading in process A to end the test.

Basically, process A never finds out that the subframe failed to load
in a different process due to CSP/XFO blocking, and can't take
appropriate action.

The proper way to fix this is by enforcing XFO/CSP in the browser
process, so that we don't need to create the unnecessary new process
in the first place.  However, that is a longer-term effort (see
https://crbug.com/555418).  This CL provides a stop-gap solution of
forwarding the notification about the block from process B to process
A, so that process A can (1) set the subframe's origin to unique, and
(2) fire the load event on its FrameOwner.  The browser->renderer part
of this notification can be reused once the browser process can do the
blocking.

This CL also generically fixes the case of stopping the load in the
current RenderFrameHost when a pending RenderFrameHost never commits,
which solves problem 4 and guards any future such problems.

BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-02-26 21:26:41]

content/ seems good to me once we rename.  Thanks!

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1594: if (this !=
frame_tree_node_->current_frame_host())
On 2016/02/25 21:59:12, alexmos wrote:
> This is pretty ugly, but I tried to detect whether or not to send this IPC
from
> Blink in the blocked case, and I thought that was even uglier.  At least this
is
> in one place and easy to rip out once we're done with 555418.  Also,
dispatching
> loads on <iframe> elements can now probably be done together with forwarding
> DidStopLoading, as opposed to this separate IPC, in which case this function
> will disappear entirely (but this is probably best left to a separate CL).

Acknowledged.

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

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:477:
DCHECK(request_routing_id == pending_routing_id ||
On 2016/02/25 21:59:12, alexmos wrote:
> Relaxing this DCHECK was necessary to enable navigating the blocked subframe
to
> a different site (c.com in my test), and we discussed that it might happen
more
> generically.

Yep.  I wouldn't be opposed to landing this change before the rest of the CL if
we think it can happen generally.  It seems relatively unrelated to the rest of
the blocking logic.

https://codereview.chromium.org/1710283003/diff/60001/content/test/data/frame...
File content/test/data/frame-ancestors-none.html.mock-http-headers (right):

https://codereview.chromium.org/1710283003/diff/60001/content/test/data/frame...
content/test/data/frame-ancestors-none.html.mock-http-headers:2:
Content-Security-Policy: frame-ancestors 'none'
Wow, I didn't know we could specify headers like this.  Cool!

[alexmos, 2016-02-26 22:37:56]

Thanks for reviewing, Charlie!  I've done the rename and split off the relaxed
check into a separate CL.

> > So today, Chrome doesn't obey the "empty body" part of the spec, and this CL
> > doesn't change that.
> > 
> > Sorry, I was confused when I wrote "blank frame", since I was thinking about
> the
> > case where the blocked URL is the frame's first navigation.  In that case,
the
> > blank page that's visible after the block is just the initial about:blank. 
> I'll
> > go through the comments in this CL and remove any similar language about
blank
> > frames.
> 
> Wow, that current behavior sounds bad!  You could basically revoke the
effective
> origin of a page by navigating it to something that you know will be blocked. 
> I'm not sure if that can lead to more interesting attacks than DoS, though it
> probably would be a way to detect if the block happened or not.  (If the
> attacker owns the subframe's current page and the navigation is blocked, the
> current page will keep existing and can check in with the server.)
> 
> Sounds like that will need to be fixed at some point, unless there's reasons
we
> took that path.  If it does need to change, how disruptive will that be for
the
> changes in this CL?  It's worth thinking about whether that change should be
> made first, or if it's relatively easy to make it later.

Agreed, I didn't realize how bad it is if there's already an existing document
loaded in the frame.  I'll explore what it takes to commit a blank document
instead and also run this by mkwst@ to see if there are reasons not to do it. 
If it works out, I think that would be a better approach, and it'll probably
make half of this unnecessary (the browser->renderer IPC might still be needed
once the blocking moves to the browser process).  But one potential problem with
that is that we might need to redo the way we'd commit the blank document once
CSP/XFO moves to browser process, so I'm hesitant on spending much effort there
unless it's very easy.

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

https://codereview.chromium.org/1710283003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:477:
DCHECK(request_routing_id == pending_routing_id ||
On 2016/02/26 21:26:41, Charlie Reis (slow til Mar 7) wrote:
> On 2016/02/25 21:59:12, alexmos wrote:
> > Relaxing this DCHECK was necessary to enable navigating the blocked subframe
> to
> > a different site (c.com in my test), and we discussed that it might happen
> more
> > generically.
> 
> Yep.  I wouldn't be opposed to landing this change before the rest of the CL
if
> we think it can happen generally.  It seems relatively unrelated to the rest
of
> the blocking logic.

Agreed.  I split this off into https://codereview.chromium.org/1736393002/ and
just sent to you.

[Charlie Reis, 2016-02-27 00:02:15]

On 2016/02/26 22:37:56, alexmos wrote:
> Thanks for reviewing, Charlie!  I've done the rename and split off the relaxed
> check into a separate CL.
> 
> > > So today, Chrome doesn't obey the "empty body" part of the spec, and this
CL
> > > doesn't change that.
> > > 
> > > Sorry, I was confused when I wrote "blank frame", since I was thinking
about
> > the
> > > case where the blocked URL is the frame's first navigation.  In that case,
> the
> > > blank page that's visible after the block is just the initial about:blank.

> > I'll
> > > go through the comments in this CL and remove any similar language about
> blank
> > > frames.
> > 
> > Wow, that current behavior sounds bad!  You could basically revoke the
> effective
> > origin of a page by navigating it to something that you know will be
blocked. 
> > I'm not sure if that can lead to more interesting attacks than DoS, though
it
> > probably would be a way to detect if the block happened or not.  (If the
> > attacker owns the subframe's current page and the navigation is blocked, the
> > current page will keep existing and can check in with the server.)
> > 
> > Sounds like that will need to be fixed at some point, unless there's reasons
> we
> > took that path.  If it does need to change, how disruptive will that be for
> the
> > changes in this CL?  It's worth thinking about whether that change should be
> > made first, or if it's relatively easy to make it later.
> 
> Agreed, I didn't realize how bad it is if there's already an existing document
> loaded in the frame.  I'll explore what it takes to commit a blank document
> instead and also run this by mkwst@ to see if there are reasons not to do it. 
> If it works out, I think that would be a better approach, and it'll probably
> make half of this unnecessary (the browser->renderer IPC might still be needed
> once the blocking moves to the browser process).  But one potential problem
with
> that is that we might need to redo the way we'd commit the blank document once
> CSP/XFO moves to browser process, so I'm hesitant on spending much effort
there
> unless it's very easy.

Sounds good.  I'll grant my LGTM for content/ now, in case it turns out the
behavior change is too disruptive for the moment.  (This CL seems to be a good
intermediate point that avoids lots of ugly problems we have today.)

[alexmos, 2016-03-04 01:32:08]

On 2016/02/27 00:02:15, Charlie Reis (OOO til Mar 7) wrote:
> On 2016/02/26 22:37:56, alexmos wrote:
> > Thanks for reviewing, Charlie!  I've done the rename and split off the
relaxed
> > check into a separate CL.
> > 
> > > > So today, Chrome doesn't obey the "empty body" part of the spec, and
this
> CL
> > > > doesn't change that.
> > > > 
> > > > Sorry, I was confused when I wrote "blank frame", since I was thinking
> about
> > > the
> > > > case where the blocked URL is the frame's first navigation.  In that
case,
> > the
> > > > blank page that's visible after the block is just the initial
about:blank.
> 
> > > I'll
> > > > go through the comments in this CL and remove any similar language about
> > blank
> > > > frames.
> > > 
> > > Wow, that current behavior sounds bad!  You could basically revoke the
> > effective
> > > origin of a page by navigating it to something that you know will be
> blocked. 
> > > I'm not sure if that can lead to more interesting attacks than DoS, though
> it
> > > probably would be a way to detect if the block happened or not.  (If the
> > > attacker owns the subframe's current page and the navigation is blocked,
the
> > > current page will keep existing and can check in with the server.)
> > > 
> > > Sounds like that will need to be fixed at some point, unless there's
reasons
> > we
> > > took that path.  If it does need to change, how disruptive will that be
for
> > the
> > > changes in this CL?  It's worth thinking about whether that change should
be
> > > made first, or if it's relatively easy to make it later.
> > 
> > Agreed, I didn't realize how bad it is if there's already an existing
document
> > loaded in the frame.  I'll explore what it takes to commit a blank document
> > instead and also run this by mkwst@ to see if there are reasons not to do
it. 
> > If it works out, I think that would be a better approach, and it'll probably
> > make half of this unnecessary (the browser->renderer IPC might still be
needed
> > once the blocking moves to the browser process).  But one potential problem
> with
> > that is that we might need to redo the way we'd commit the blank document
once
> > CSP/XFO moves to browser process, so I'm hesitant on spending much effort
> there
> > unless it's very easy.
> 
> Sounds good.  I'll grant my LGTM for content/ now, in case it turns out the
> behavior change is too disruptive for the moment.  (This CL seems to be a good
> intermediate point that avoids lots of ugly problems we have today.)

Just to give an update here - I'm going to close this CL for now without landing
it.  I ended up going with the alternate approach that changes CSP/XFO blocking
to commit a blank document instead of canceling:
https://codereview.chromium.org/1742923002.  That turned out to be an easier
Blink-only change that makes this behavior more sane, fixes spec compliance, and
fixes all the OOPIF problems for free.  I also extracted out the test I wrote
here and landed that separately in https://codereview.chromium.org/1754383002. 
We can revisit this CL to see if any of this plumbing might be needed once
CSP/XFO moves to the browser process.