Send origin updates to frame proxies when a frame navigates to new origin.

Send origin updates to frame proxies when a frame navigates to new origin.

One current use for this is to ensure that cross-domain access error
messages (see https://crbug.com/478254) show the latest origin for
RemoteFrames.

BUG=426512, 478254
Committed: https://crrev.com/a7a4ff82f6f78186faf446e1db4d1443c8496371
Cr-Commit-Position: refs/heads/master@{#327068}

[alexmos, 2015-04-20 17:35:42]

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

[alexmos, 2015-04-20 17:35:42]

Charlie, can you please take a look?  The trickiest part here was finding a way
to test it - I ended up catching and inspecting a console message for the
cross-domain access error, which needs to reflect the right origin.  Currently,
that's also the only use case I know of where this matters.

This will need a couple of CLs to land first: the Blink error message CL
(https://codereview.chromium.org/1085973003/), and also a CL to allow origin
updates in RemoteSecurityContexts (https://codereview.chromium.org/1096873002/).

[Charlie Reis, 2015-04-20 20:22:48]

Quick sanity check before I review the code itself: do you know of other places
we'll need this besides the error messages?

Just wanted to make sure we need this (from a least privilege perspective),
since otherwise it would be nice not to tell possibly exploited renderer
processes about everywhere the user visits in that BrowsingInstance.  I know we
need the replicated origin on the ancestor chain for a bunch of checks, but as
you mentioned, that can't change without destroying the descendant.  In that
sense, I wonder if we only need replicated origins on RemoteFrames when they
have a LocalFrame descendant.

For the error messages case, I could imagine filling in the actual origin in the
browser process, but maybe that's a lot easier said than done.  Worth comparing
against this alternative, though.

[alexmos, 2015-04-20 22:06:12]

On 2015/04/20 20:22:48, Charlie Reis wrote:
> Quick sanity check before I review the code itself: do you know of other
places
> we'll need this besides the error messages?
> 
> Just wanted to make sure we need this (from a least privilege perspective),
> since otherwise it would be nice not to tell possibly exploited renderer
> processes about everywhere the user visits in that BrowsingInstance.  I know
we
> need the replicated origin on the ancestor chain for a bunch of checks, but as
> you mentioned, that can't change without destroying the descendant.  In that
> sense, I wonder if we only need replicated origins on RemoteFrames when they
> have a LocalFrame descendant.
> 
> For the error messages case, I could imagine filling in the actual origin in
the
> browser process, but maybe that's a lot easier said than done.  Worth
comparing
> against this alternative, though.

Yes, good point about leaking history.  I've thought some more about this, and I
think this may matter if the proxy in question gets a local descendant after an
origin change.  For example, suppose you have:

      Site A ------------ proxies for B
        |--Site B ------- proxies for A
        +--Site A ------- proxies for B(replicated_origin=A)

Suppose the second frame navigates to C.  Without this CL, you would have:

      Site A ------------ proxies for B, C
        |--Site B ------- proxies for A, C
        +--Site C ------- proxies for A(replicated_origin=C),
B(replicated_origin=A)

Suppose C appends a new frame on site B:
      Site A ------------ proxies for B, C
        |--Site B ------- proxies for A, C
        +--Site C ------- proxies for A(replicated_origin=C),
B(replicated_origin=A)
             |--Site B -- proxies for A, C

This will append a local child to the proxy for C in B's process.  Now, if that
new child frame evaluates location.ancestorOrigins (or needs ancestors for a
security check), its parent's origin should be "C".  Without this CL, it would
be "A".  

With this CL, the proxy's replicated_origin would be updated from "A" to "C"
after the navigation to C.  To reduce history leaks, maybe we could buffer the
origin updates and only send them to the proxy in question in the last step,
when the proxy gets a local descendant.  (The attacker shouldn't be able to
create a child frame under another renderer's frame to exploit this.)  That
would break the error messages, though.  The frame navigation checks themselves
(like Frame::canNavigate) should still work despite using an incorrect origin,
since they only care about same-origin vs. cross-origin, though this feels a bit
shaky if the checks (or our policy) change in some way in the future.  Filling
in the origin for error messages in the browser process seems pretty complex --
we could also just not use a remote frame's origin in the message if we don't
have it for now.  Long-term, it seems that checks like Frame::canNavigate should
be enforced in the browser process, which would let us generate correct and
complete error messages there if/when that happens.

If we do go forward with this, I can also write a test for the above scenario,
which would be much better than the hacky test I have currently (though the
current test could still be useful to keep for testing error messages with
OOPIF).

[Charlie Reis, 2015-04-20 22:40:36]

On 2015/04/20 22:06:12, alexmos wrote:
> On 2015/04/20 20:22:48, Charlie Reis wrote:
> > Quick sanity check before I review the code itself: do you know of other
> places
> > we'll need this besides the error messages?
> > 
> > Just wanted to make sure we need this (from a least privilege perspective),
> > since otherwise it would be nice not to tell possibly exploited renderer
> > processes about everywhere the user visits in that BrowsingInstance.  I know
> we
> > need the replicated origin on the ancestor chain for a bunch of checks, but
as
> > you mentioned, that can't change without destroying the descendant.  In that
> > sense, I wonder if we only need replicated origins on RemoteFrames when they
> > have a LocalFrame descendant.
> > 
> > For the error messages case, I could imagine filling in the actual origin in
> the
> > browser process, but maybe that's a lot easier said than done.  Worth
> comparing
> > against this alternative, though.
> 
> Yes, good point about leaking history.  I've thought some more about this, and
I
> think this may matter if the proxy in question gets a local descendant after
an
> origin change.  For example, suppose you have:
> 
>       Site A ------------ proxies for B
>         |--Site B ------- proxies for A
>         +--Site A ------- proxies for B(replicated_origin=A)
> 
> Suppose the second frame navigates to C.  Without this CL, you would have:
> 
>       Site A ------------ proxies for B, C
>         |--Site B ------- proxies for A, C
>         +--Site C ------- proxies for A(replicated_origin=C),
> B(replicated_origin=A)
> 
> Suppose C appends a new frame on site B:
>       Site A ------------ proxies for B, C
>         |--Site B ------- proxies for A, C
>         +--Site C ------- proxies for A(replicated_origin=C),
> B(replicated_origin=A)
>              |--Site B -- proxies for A, C
> 
> This will append a local child to the proxy for C in B's process.  Now, if
that
> new child frame evaluates location.ancestorOrigins (or needs ancestors for a
> security check), its parent's origin should be "C".  Without this CL, it would
> be "A".  
> 
> With this CL, the proxy's replicated_origin would be updated from "A" to "C"
> after the navigation to C.  To reduce history leaks, maybe we could buffer the
> origin updates and only send them to the proxy in question in the last step,
> when the proxy gets a local descendant.  (The attacker shouldn't be able to
> create a child frame under another renderer's frame to exploit this.)  That
> would break the error messages, though.  The frame navigation checks
themselves
> (like Frame::canNavigate) should still work despite using an incorrect origin,
> since they only care about same-origin vs. cross-origin, though this feels a
bit
> shaky if the checks (or our policy) change in some way in the future.  Filling
> in the origin for error messages in the browser process seems pretty complex
--
> we could also just not use a remote frame's origin in the message if we don't
> have it for now.  Long-term, it seems that checks like Frame::canNavigate
should
> be enforced in the browser process, which would let us generate correct and
> complete error messages there if/when that happens.
> 
> If we do go forward with this, I can also write a test for the above scenario,
> which would be much better than the hacky test I have currently (though the
> current test could still be useful to keep for testing error messages with
> OOPIF).

Yes, when a LocalFrame gets added as a subframe to a RemoteFrame, we would have
to send origin updates for all of its ancestors (maybe only if they've changed,
as you mention).

Also, I agree that Frame::canNavigate is really something that should eventually
be enforced in the browser process, where we have the authoritative info.  That
would also let us handle the error messages there, as you mention.

It sounds like your current CL (dynamic origin updates) is a bit more sane and
convenient at the moment, but less in line with where we'd like to end up down
the road.  It may be that it's worthwhile landing it for now, with some plans to
undo it when we can move Frame::canNavigate?  Let's chat in person a bit
tomorrow about how much work is on each path, and we can decide which way to go.

Thanks!

[Charlie Reis, 2015-04-21 19:10:30]

Ok, sounds like we'll need enough of this CL either way that we should proceed. 
We may be more selective about the updates in the future (e.g., only for
ancestor chains, if we can move things like Frame::canNavigate to the browser
process), but this is useful in the meantime.

Can you add some comments to frame_replication_state.h about how often its
fields are updated, and maybe when we know that they're needed?  (Not sure if
the second one is just bound to get out of date, but it might be nice to say
that origins or only strictly needed on the ancestor chain.)

Otherwise LGTM with nits.

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

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:142:
render_manager_.OnDidUpdateName(name);
Should this have a similar "only notify if there's a change" check?

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:266: void
FrameTreeNode::SetCurrentOrigin(const url::Origin& origin) {
nit: Same order as .h file (before SetFrameName).

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

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:99: void SetFrameName(const
std::string& name);
Can you put a similar comment here, since it also notifies proxies?

https://codereview.chromium.org/1098763003/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1098763003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1870: DepictFrameTree(root));
I like this new feature.  :)

https://codereview.chromium.org/1098763003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1886: "Unsafe JavaScript attempt
to initiate navigation*"));
Yeah, this is a bit fragile if the error message ever changes, but I suppose it
will let us detect whether the replication worked from the browser process.

[alexmos, 2015-04-22 23:15:29]

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

[alexmos, 2015-04-22 23:15:30]

Thanks!  +kenrb@ for IPC review.

> Can you add some comments to frame_replication_state.h about how often its
> fields are updated, and maybe when we know that they're needed?  (Not sure if
> the second one is just bound to get out of date, but it might be nice to say
> that origins or only strictly needed on the ancestor chain.)

I added some comments -- do you mind taking a look for a sanity check?

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

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:142:
render_manager_.OnDidUpdateName(name);
On 2015/04/21 19:10:30, Charlie Reis wrote:
> Should this have a similar "only notify if there's a change" check?

Done.

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:266: void
FrameTreeNode::SetCurrentOrigin(const url::Origin& origin) {
On 2015/04/21 19:10:30, Charlie Reis wrote:
> nit: Same order as .h file (before SetFrameName).

Done.

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

https://codereview.chromium.org/1098763003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:99: void SetFrameName(const
std::string& name);
On 2015/04/21 19:10:30, Charlie Reis wrote:
> Can you put a similar comment here, since it also notifies proxies?

Done.

[Charlie Reis, 2015-04-22 23:41:25]

Thanks!  LGTM with nit.

https://codereview.chromium.org/1098763003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1098763003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:47: // |origin| is updated whenever a
frame navigation commits.  |name| is
These look great, but maybe it would help to put them by each member instead of
here at the top.  That will make it easier to add similar comments for new
members.

[alexmos, 2015-04-23 00:07:44]

https://codereview.chromium.org/1098763003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1098763003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:47: // |origin| is updated whenever a
frame navigation commits.  |name| is
On 2015/04/22 23:41:25, Charlie Reis wrote:
> These look great, but maybe it would help to put them by each member instead
of
> here at the top.  That will make it easier to add similar comments for new
> members.

Done.

[kenrb, 2015-04-27 16:57:20]

ipc lgtm, sorry for missing this last week.

[alexmos, 2015-04-27 16:58:56]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-04-27 16:58:56]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1098763003/#ps100001
(title: "Fix Windows builds (add missing base:: to UTF16toASCII)")

[commit-bot: I haz the power, 2015-04-27 16:59:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098763003/100001

[commit-bot: I haz the power, 2015-04-27 18:00:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-27 18:01:56]

Patchset 6 (id:??) landed as
https://crrev.com/a7a4ff82f6f78186faf446e1db4d1443c8496371
Cr-Commit-Position: refs/heads/master@{#327068}