Require a process ID when looking up RFHs by FrameTreeNode ID.

Require a process ID when looking up RFHs by FrameTreeNode ID.

This helps avoid security bugs where callers store a FrameTreeNode ID
and later assume it is for the same RenderFrameHost.  However, a
cross-process navigation may have taken place, leading to a higher or
lower privileged page.

Because extension APIs use the old approach, the previous API is left
as an unsafe option, with comments encouraging callers to avoid it.

BUG=715541
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2856653004
Cr-Commit-Position: refs/heads/master@{#469410}
Committed: https://chromium.googlesource.com/chromium/src/+/f71a263681dc50097672cc55e0204c7b0e789a48

[Charlie Reis, 2017-05-01 21:35:31]

Description was changed from

==========
Require a process ID when looking up RFHs by FrameTreeNode ID.

This helps avoid security bugs where callers store a FrameTreeNode ID
and later assume it is for the same RenderFrameHost.  However, a
cross-process navigation may have taken place, leading to a higher or
lower privileged page.

Because extension APIs use the old approach, the previous API is left
as an unsafe option, with comments encouraging callers to avoid it.

BUG=715541
TEST=No behavior change.
==========

to

==========
Require a process ID when looking up RFHs by FrameTreeNode ID.

This helps avoid security bugs where callers store a FrameTreeNode ID
and later assume it is for the same RenderFrameHost.  However, a
cross-process navigation may have taken place, leading to a higher or
lower privileged page.

Because extension APIs use the old approach, the previous API is left
as an unsafe option, with comments encouraging callers to avoid it.

BUG=715541
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-05-01 21:48:59]

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

[commit-bot: I haz the power, 2017-05-01 21:49:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-05-01 21:56:58]

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

[Charlie Reis, 2017-05-01 21:56:59]

Nick, can you take a look?

https://codereview.chromium.org/2856653004/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right):

https://codereview.chromium.org/2856653004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:148:
navigation_handle->GetStartingSiteInstance()->GetProcess()->GetID();
This is only ok because it matches the limitations described in the comment
above.  All of this will go away as soon as we track initiators.

[ncarter (slow), 2017-05-01 22:15:32]

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
content/public/browser/navigation_handle.h:86: // taking place in the main
frame, the value returned is -1.
Deprecate this?

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:254: int process_id) = 0;
I agree with your choice of ordering these params, given how the function is
currently named. But both process_id and frame_tree_node_id want to be the first
parameter here, so I think there's some worry about the ordering being confused
here. At the very least this is one of those "look up the decl to double-check
the ordering."

Is there anything we can do to make this more obvious at the call site?

[Charlie Reis, 2017-05-01 22:35:41]

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

[commit-bot: I haz the power, 2017-05-01 22:36:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-05-01 22:36:51]

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
content/public/browser/navigation_handle.h:86: // taking place in the main
frame, the value returned is -1.
On 2017/05/01 22:15:32, ncarter wrote:
> Deprecate this?

Interesting.  Yes, the only other caller is
ExtensionApiFrameIdMap::GetParentFrameId, and that could just call
GetFrameTreeNodeId on GetParentFrame.

Done.

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2856653004/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:254: int process_id) = 0;
On 2017/05/01 22:15:32, ncarter wrote:
> I agree with your choice of ordering these params, given how the function is
> currently named. But both process_id and frame_tree_node_id want to be the
first
> parameter here, so I think there's some worry about the ordering being
confused
> here. At the very least this is one of those "look up the decl to double-check
> the ordering."
> 
> Is there anything we can do to make this more obvious at the call site?
> 

Heh, yeah, there is.  :)
https://bugs.chromium.org/p/chromium/issues/detail?id=715541#c4

Then again, I'm not super eager to convert all process IDs or even just FTN IDs
to gpu::IdType32<>, with potential bikeshedding around where IdType lives.  :) 
(If we did, though, we could make FTN IDs 64-bit while we're at it, and decide
what to do with extension APIs that expect them to be 32-bit.)

Not sure I have other suggestions, though, and I agree it's a bit unfortunate. 
You're right that I almost put process_id first (to be consistent with other
FromID methods that take a routing ID), but that felt more error prone here due
to the name.

I'm inclined to proceed for now, but let me know if you think we should tackle
something like the IdType conversion first.

[ncarter (slow), 2017-05-01 22:51:17]

lgtm

[dmazzoni, 2017-05-01 22:56:37]

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

[dmazzoni, 2017-05-01 22:56:38]

https://codereview.chromium.org/2856653004/diff/40001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2856653004/diff/40001/content/public/browser/...
content/public/browser/web_contents.h:248: // part of this tab. Returns nullptr
if |process_id| does not match the
Is there a better term you can use than "tab"? That doesn't cover all of the
other
uses of WebContents.

I think you mean, part of the same frame tree including all embedded
WebContents, right?

[Charlie Reis, 2017-05-01 23:10:09]

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

[commit-bot: I haz the power, 2017-05-01 23:11:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-05-01 23:12:35]

creis@chromium.org changed reviewers:
+ clamy@chromium.org, jialiul@chromium.org, melandory@chromium.org,
rdevlin.cronin@chromium.org

[Charlie Reis, 2017-05-01 23:12:36]

Thanks!

clamy@: Can you review the NavigationHandle changes, to double check that you're
ok with the API change?

jialiul@: Can you review safe_browsing?

melandory@: Can you review components/subresource_filter?

rdevlin.cronin@: Can you review extension_api_frame_id_map.cc?  (I can't think
of a way to switch that code to the safer option.)

https://codereview.chromium.org/2856653004/diff/40001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2856653004/diff/40001/content/public/browser/...
content/public/browser/web_contents.h:248: // part of this tab. Returns nullptr
if |process_id| does not match the
On 2017/05/01 22:56:37, dmazzoni wrote:
> Is there a better term you can use than "tab"? That doesn't cover all of the
> other
> uses of WebContents.
> 
> I think you mean, part of the same frame tree including all embedded
> WebContents, right?

Thanks.  I changed it to "frame tree," explicitly noting that it does not
include inner WebContents (since FindByID doesn't loop over them).

[Jialiu Lin, 2017-05-01 23:14:35]

lgtm for c/b/safe_browsing
Thanks!

[commit-bot: I haz the power, 2017-05-02 00:15:45]

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

[commit-bot: I haz the power, 2017-05-02 00:15:46]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-02 02:04:29]

https://codereview.chromium.org/2856653004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2856653004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:926: if (!frame ||
current_frame_host->GetProcess()->GetID() != process_id)
If !frame is true, wouldn't we have crashed on line 925?

https://codereview.chromium.org/2856653004/diff/60001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2856653004/diff/60001/content/public/browser/...
content/public/browser/navigation_handle.h:88: virtual RenderFrameHost*
GetParentFrame() = 0;
If we were worried before about callers storing the render frame tree node id
and not expected a change, is this better?  Is it safer to store a raw ptr to a
RenderFrameHost?

[Charlie Harrison, 2017-05-02 02:45:49]

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

[Charlie Harrison, 2017-05-02 02:45:50]

subresource_filter LGTM thanks for the nicer nav handle API.

[Charlie Reis, 2017-05-04 03:22:49]

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

[commit-bot: I haz the power, 2017-05-04 03:23:06]

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-05-04 03:33:26]

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

[commit-bot: I haz the power, 2017-05-04 03:33:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2017-05-04 05:39:59]

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

[commit-bot: I haz the power, 2017-05-04 05:40:11]

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-05-04 06:49:29]

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

[commit-bot: I haz the power, 2017-05-04 06:49:31]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-04 16:29:57]

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

[Charlie Reis, 2017-05-04 16:29:58]

[+ekaramad, CC lazyboy]

Heh, I waited just long enough to update this to hit a conflict with
https://codereview.chromium.org/2858143002 in ExtensionNavigationThrottle, but
it's easily resolved.

Devlin, can you take another look?

Ehsan, can you check extension_navigation_throttle.cc?

https://codereview.chromium.org/2856653004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2856653004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:926: if (!frame ||
current_frame_host->GetProcess()->GetID() != process_id)
On 2017/05/02 02:04:29, Devlin wrote:
> If !frame is true, wouldn't we have crashed on line 925?

Ha, yep!  Thanks for catching that.

https://codereview.chromium.org/2856653004/diff/60001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2856653004/diff/60001/content/public/browser/...
content/public/browser/navigation_handle.h:88: virtual RenderFrameHost*
GetParentFrame() = 0;
On 2017/05/02 02:04:29, Devlin wrote:
> If we were worried before about callers storing the render frame tree node id
> and not expected a change, is this better?

Yes, since the parent RenderFrameHost doesn't change during the navigation, and
the navigation would be canceled if it did.  (More specifically, a cross-process
navigation in the parent FrameTreeNode will synchronously delete the subtree of
FrameTreeNodes and RenderFrameHosts, which will delete any of their
NavigationHandles as well.  Same if the parent is detached or closed.)

>  Is it safer to store a raw ptr to a RenderFrameHost?

I don't think this should be a problem, given that we already expose a raw
pointer to this frame's RenderFrameHost below (after the NavigationHandle is
ready to commit).  Exposing the FTN ID here just seemed to be a hurdle to the
actual uses, which wasn't adding much value in the NavigationHandle case when
there's no risk of it changing.

https://codereview.chromium.org/2856653004/diff/120001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2856653004/diff/120001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:111:
content::RenderFrameHost* ancestor = navigation_handle()->GetParentFrame();
ekaramad/lazyboy: I'm further simplifying your recent refactor from
https://codereview.chromium.org/2858143002, now that we can just grab the parent
RFH directly.  (Otherwise, the FindFrameByFrameTreeNodeId call is a bit risky in
general, in case people tried to use navigating_frame in other ways.)

[Devlin, 2017-05-04 17:56:34]

lgtm

[EhsanK, 2017-05-04 18:01:39]

Thanks Charlie and sorry for the random clash. LGTM.

https://codereview.chromium.org/2856653004/diff/120001/extensions/browser/ext...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2856653004/diff/120001/extensions/browser/ext...
extensions/browser/extension_navigation_throttle.cc:111:
content::RenderFrameHost* ancestor = navigation_handle()->GetParentFrame();
On 2017/05/04 16:29:58, Charlie Reis wrote:
> ekaramad/lazyboy: I'm further simplifying your recent refactor from
> https://codereview.chromium.org/2858143002, now that we can just grab the
parent
> RFH directly.  (Otherwise, the FindFrameByFrameTreeNodeId call is a bit risky
in
> general, in case people tried to use navigating_frame in other ways.)

Thanks! This is quite useful in my current works so looking forward to a quick
rebase asap. Also I guess it should be more efficient given that we do not check
all frames for a matching ID.

[Charlie Reis, 2017-05-04 18:56:31]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-05-04 18:56:32]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
csharrison@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2856653004/#ps120001
(title: "Simplify ExtNavThrottle")

[commit-bot: I haz the power, 2017-05-04 18:57:19]

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-05-04 19:03:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493924191519010,
"parent_rev": "51c5530a57e76042a510e45ec360de638d9217ca", "commit_rev":
"f71a263681dc50097672cc55e0204c7b0e789a48"}

[commit-bot: I haz the power, 2017-05-04 19:04:06]

Description was changed from

==========
Require a process ID when looking up RFHs by FrameTreeNode ID.

This helps avoid security bugs where callers store a FrameTreeNode ID
and later assume it is for the same RenderFrameHost.  However, a
cross-process navigation may have taken place, leading to a higher or
lower privileged page.

Because extension APIs use the old approach, the previous API is left
as an unsafe option, with comments encouraging callers to avoid it.

BUG=715541
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Require a process ID when looking up RFHs by FrameTreeNode ID.

This helps avoid security bugs where callers store a FrameTreeNode ID
and later assume it is for the same RenderFrameHost.  However, a
cross-process navigation may have taken place, leading to a higher or
lower privileged page.

Because extension APIs use the old approach, the previous API is left
as an unsafe option, with comments encouraging callers to avoid it.

BUG=715541
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2856653004
Cr-Commit-Position: refs/heads/master@{#469410}
Committed:
https://chromium.googlesource.com/chromium/src/+/f71a263681dc50097672cc55e020...
==========

[commit-bot: I haz the power, 2017-05-04 19:04:08]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f71a263681dc50097672cc55e020...