Re-check existence of GuestViewContainer before requesting attach.

Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523
Review-Url: https://codereview.chromium.org/2695633008
Cr-Commit-Position: refs/heads/master@{#451330}
Committed: https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527ee464bd1c1bdd

[wjmaclean, 2017-02-16 15:43:40]

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

[commit-bot: I haz the power, 2017-02-16 15:44:14]

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-02-16 16:34:10]

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

[commit-bot: I haz the power, 2017-02-16 16:34:11]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-02-16 16:44:59]

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

[commit-bot: I haz the power, 2017-02-16 16:45:32]

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

[wjmaclean, 2017-02-16 16:47:05]

Description was changed from

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

BUG=683523
==========

to

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523
==========

[wjmaclean, 2017-02-16 17:00:03]

wjmaclean@chromium.org changed reviewers:
+ kenrb@chromium.org, lfg@chromium.org

[wjmaclean, 2017-02-16 17:00:57]

I'm not sure if we should be concerned about other null pointers in
AttachIFrameGuest(), but this CL at least handles the test case in the bug.

[wjmaclean, 2017-02-16 17:01:32]

On 2017/02/16 17:00:57, wjmaclean wrote:
> I'm not sure if we should be concerned about other null pointers in
> AttachIFrameGuest(), but this CL at least handles the test case in the bug.

PTAL?

[commit-bot: I haz the power, 2017-02-16 17:38:31]

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

[commit-bot: I haz the power, 2017-02-16 17:38:32]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-02-16 20:00:58]

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:180:
guest_view::GuestViewContainer::FromID(element_instance_id)) {
GuestViewContainer already has a weak ptr factory, can you use that instead? It
will be safer than comparing pointers like this.

Also, I think this can be moved above just after evaluating javascript for the
params, so we don't need to create the request.

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:252: //
It's possible that code executed during the |params| enumeration above
Can we use the same approach as above: Get the RenderFrame before executing user
code, grabbing a weak ptr to it, then executing the user code and checking the
weak ptr after?

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:262: //
GuestView object's RenderFrame exists, that it will have a local parent?
The frame comes from getIframeContentWindow in javascript. Even if the parent
could be null, I think it will be just a crash, not a UaF.

[wjmaclean, 2017-02-16 21:25:13]

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

[wjmaclean, 2017-02-16 21:26:16]

lfg@ - I think I've addressed your suggestions (though I couldn't find a WeakPtr
already in RenderFrame, so I did something a little different there).

PTAL?

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:180:
guest_view::GuestViewContainer::FromID(element_instance_id)) {
On 2017/02/16 20:00:58, lfg wrote:
> GuestViewContainer already has a weak ptr factory, can you use that instead?
It
> will be safer than comparing pointers like this.
> 
> Also, I think this can be moved above just after evaluating javascript for the
> params, so we don't need to create the request.

Done.

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:252: //
It's possible that code executed during the |params| enumeration above
On 2017/02/16 20:00:58, lfg wrote:
> Can we use the same approach as above: Get the RenderFrame before executing
user
> code, grabbing a weak ptr to it, then executing the user code and checking the
> weak ptr after?

How about if we grab the parent_frame WebLocalFrame* and hang on to that. It's
use later is just as a lookup token.

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:262: //
GuestView object's RenderFrame exists, that it will have a local parent?
On 2017/02/16 20:00:57, lfg wrote:
> The frame comes from getIframeContentWindow in javascript. Even if the parent
> could be null, I think it will be just a crash, not a UaF.

Acknowledged.

[commit-bot: I haz the power, 2017-02-16 21:26:40]

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

[lfg, 2017-02-16 22:09:13]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2017-02-16 22:09:14]

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:252: //
It's possible that code executed during the |params| enumeration above
On 2017/02/16 21:26:16, wjmaclean wrote:
> On 2017/02/16 20:00:58, lfg wrote:
> > Can we use the same approach as above: Get the RenderFrame before executing
> user
> > code, grabbing a weak ptr to it, then executing the user code and checking
the
> > weak ptr after?
> 
> How about if we grab the parent_frame WebLocalFrame* and hang on to that. It's
> use later is just as a lookup token.

I think this is fine, the WebFrame may be closed while executing javascript, but
oilpan won't delete it immediatelly.

+dcheng@ to confirm this.

[dcheng, 2017-02-16 22:29:14]

On 2017/02/16 22:09:14, lfg wrote:
>
https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
> File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
> (right):
> 
>
https://codereview.chromium.org/2695633008/diff/20001/extensions/renderer/gue...
> extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:252: //
> It's possible that code executed during the |params| enumeration above
> On 2017/02/16 21:26:16, wjmaclean wrote:
> > On 2017/02/16 20:00:58, lfg wrote:
> > > Can we use the same approach as above: Get the RenderFrame before
executing
> > user
> > > code, grabbing a weak ptr to it, then executing the user code and checking
> the
> > > weak ptr after?
> > 
> > How about if we grab the parent_frame WebLocalFrame* and hang on to that.
It's
> > use later is just as a lookup token.
> 
> I think this is fine, the WebFrame may be closed while executing javascript,
but
> oilpan won't delete it immediatelly.
> 
> +dcheng@ to confirm this.

I think we probably don't want to rely on the implementation detail of WebFrame:
it is garbage collected, but that's not something exposed publicly through the
Blink API. We should consider frames to be invalid as soon as WebFrame::close()
is called, even if you know it will remain around for a little while longer.

[lfg, 2017-02-16 22:48:39]

On 2017/02/16 22:29:14, dcheng wrote:
> 
> I think we probably don't want to rely on the implementation detail of
WebFrame:
> it is garbage collected, but that's not something exposed publicly through the
> Blink API. We should consider frames to be invalid as soon as
WebFrame::close()
> is called, even if you know it will remain around for a little while longer.

We could just get a weak ptr to the RenderFrame instead, though I'm not sure
about exposing that through the content/public interface. Alternatively,
there's also the possibility of defining a RenderFrameObserver and listening
for the FrameDetached callback.

[commit-bot: I haz the power, 2017-02-16 23:53:49]

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

[commit-bot: I haz the power, 2017-02-16 23:53:50]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-02-17 15:00:58]

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

[wjmaclean, 2017-02-17 15:01:08]

Ptal?

[commit-bot: I haz the power, 2017-02-17 15:01: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-02-17 15:34:13]

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

[commit-bot: I haz the power, 2017-02-17 15:34:14]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wjmaclean, 2017-02-17 15:38:52]

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

[commit-bot: I haz the power, 2017-02-17 15:39:05]

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

[lfg, 2017-02-17 16:06:10]

https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:278: if
(!embedder_parent_frame)
This shouldn't be needed anymore.

[wjmaclean, 2017-02-17 16:18:01]

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

[commit-bot: I haz the power, 2017-02-17 16:18:20]

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

[wjmaclean, 2017-02-17 16:18:47]

https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/gue...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/2695633008/diff/80001/extensions/renderer/gue...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:278: if
(!embedder_parent_frame)
On 2017/02/17 16:06:10, lfg wrote:
> This shouldn't be needed anymore.

Removed.

[lfg, 2017-02-17 16:29:54]

lgtm

[commit-bot: I haz the power, 2017-02-17 17:19:51]

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

[commit-bot: I haz the power, 2017-02-17 17:19:52]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-02-17 17:20:08]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2017-02-17 17:20:37]

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-02-17 17:30:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487352008264200,
"parent_rev": "fe5f6cb97ac57902e159afdfe42e30b4f8cf8ebd", "commit_rev":
"87dd40c9b8ae1fb8c3eb15bf527ee464bd1c1bdd"}

[commit-bot: I haz the power, 2017-02-17 17:30:54]

Description was changed from

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523
==========

to

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523

Review-Url: https://codereview.chromium.org/2695633008
Cr-Commit-Position: refs/heads/master@{#451330}
Committed:
https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527e...
==========

[commit-bot: I haz the power, 2017-02-17 17:30:55]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527e...

[alph, 2017-02-18 16:40:44]

Description was changed from

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523

Review-Url: https://codereview.chromium.org/2695633008
Cr-Commit-Position: refs/heads/master@{#451330}
Committed:
https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527e...
==========

to

==========
Re-check existence of GuestViewContainer before requesting attach.

During GuestView attach, it's possible for the GuestViewContainer
pointer to become stale while enumerating the GuestView object's
properties, resulting in a use-after-free when the attach request is
queued.

This CL forces a re-check that the container pointer is valid after
the property enumeration takes place, and also checks that the container
hasn't changed.

In the case of AttachIFrameGuest(), it verifies that the RenderFrame
containing the GuestView object doesn't disappear during |params| enumeration.

BUG=683523

Review-Url: https://codereview.chromium.org/2695633008
Cr-Commit-Position: refs/heads/master@{#451330}
Committed:
https://chromium.googlesource.com/chromium/src/+/87dd40c9b8ae1fb8c3eb15bf527e...
==========