Always use the caller's context in PepperTryCatchV8

Always use the caller's context in PepperTryCatchV8

This removes the DCHECK from PepperTryCatch which checks that the current
context is equal to the plugin context. This assumption is not always true.
In particular, if we call into the plugin directly from another frame (which
is in the same origin) the current context will differ. The same is true for background scripts. This should be ok; we should always use the calling context so as to not leak v8 objects across contexts.

BUG=412062
Committed: https://crrev.com/1630477a5ebc6cfab27704b54ca89a76a3036c6a
Cr-Commit-Position: refs/heads/master@{#294291}

[raymes, 2014-09-09 03:43:50]

raymes@chromium.org changed reviewers:
+ dmichael@chromium.org, vrk@chromium.org

[dmichael (off chromium), 2014-09-09 21:38:06]

tl;dr: I think what we want is to just assume there's a context. In places where
we don't have one, we can enter the plugin's context outside this class. But
this class doesn't know enough to be sure that the mainWorldScriptContext is the
right one.

https://codereview.chromium.org/555583003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/555583003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:75: // called into the plugin from a
frame that differs from the plugin's frame.
d'oh...

I don't think this is the right thing to do. If the context is different, I
think that implies that there's an extension using a background content script.
(Sorry I didn't remember this issue when reviewing before; it's subtle). We
probably should just use the caller's context in all cases (and maybe not enter
directly).

If the caller doesn't *have* a context, make them choose one explicitly. Cases
where the *plugin itself* initiates the interaction with JS, you might not have
a context, but it should be fine to use the "main world" one. When dispatching a
MessageEvent, we serialize the data part of the message, and it will be
reconstituted for each target event handler in their appropriate
"world"/context. So in that case the originating context doesn't matter. For
Flash...  we probably haven't thought hard about it, and there might not be a
good answer.

Anyway, we don't want a case where a background content script can get a v8
object from the wrong "world" or "isolate". I might explain this wrong, but I
think that can constitute a "capability leak". E.g., see
http://www.adambarth.com/papers/2009/barth-weinberger-song.pdf

The idea being if some extension accesses the plugin in the DOM, and they get
back a JavaScript object that is from the "main world" instead of the background
context's world, they might be able to access other bits of JavaScript objects
from the "main world", which is not supposed to be available to the extension.

[raymes, 2014-09-09 23:29:58]

I sort of understand - thanks for the explanation :) PTAL at the updated patch.

This class is only used in cases where javascript calls into the plugin, so it
should always have a context of some sort.

When we call in the other direction, we use the PepperTryCatchVar which enters
the plugin context. Do you think that is ok?

[raymes, 2014-09-09 23:54:37]

Actually after thinking about it some more I'm not completely sure it's an
issue. 

I could trigger the DCHECK by doing the following:
1) Creating an iframe with a plugin inside of it.
2) Calling into the plugin in the frame, e.g.
document.getElementById('iframe').contentDocument.getElementById('plugin').postMessage

The outer frame has a different context to the inner frame. However, you are
only allowed to do this sort of cross-frame communication when both frames exist
in the same origin. So I don't think there is a security concern here. If
another context can script into the plugin, then I assume it already has access
to the plugins context directly.

WDYT?

[raymes, 2014-09-10 00:29:42]

raymes@chromium.org changed reviewers:
+ abarth@chromium.org

[raymes, 2014-09-10 00:29:43]

+abarth for his thoughts on this

[raymes, 2014-09-10 05:43:29]

raymes@chromium.org changed reviewers:
+ jochen@chromium.org

[raymes, 2014-09-10 05:43:30]

+jochen as well in case he gets to it before abarth

[jochen (gone - plz use gerrit), 2014-09-10 07:37:30]

lgtm

[dmichael (off chromium), 2014-09-10 15:55:34]

lgtm

[dmichael (off chromium), 2014-09-10 16:22:46]

On 2014/09/09 23:54:37, raymes wrote:
> Actually after thinking about it some more I'm not completely sure it's an
> issue. 
> 
> I could trigger the DCHECK by doing the following:
> 1) Creating an iframe with a plugin inside of it.
> 2) Calling into the plugin in the frame, e.g.
>
document.getElementById('iframe').contentDocument.getElementById('plugin').postMessage
> 
> The outer frame has a different context to the inner frame. However, you are
> only allowed to do this sort of cross-frame communication when both frames
exist
> in the same origin. So I don't think there is a security concern here.
From the poking around I've done, I've learned that iframes always get their own
v8 context. I might not have guessed it, but it makes sense to me.

Some apps will use an iframe to sort of "sandbox" part of their app, and it
isn't appropriate for them to share JavaScript objects with each other, even
though the they can access each others' DOM. I can imagine a case where a frame
wants to load a same-origin iframe to maybe run some script it doesn't
completely trust. It gets DOM access, but it still makes sense to still limit
JavaScript cross-talk, since you can. I'd like to understand the threat model
more myself...  but knowing that the v8/blink team decided to use a separate
context for iframes, it's definitely not appropriate for the outer frame's
JavaScript context to get an object from the inner frame's context and vice
versa. The "worlds" shouldn't be mixed like that.

> If
> another context can script into the plugin, then I assume it already has
access
> to the plugins context directly.
It has access to its DOM, but not its JavaScript objects. I don't fully grasp
the possible security problems (others like abarth have more knowledge and
imagination for such things), but it still wouldn't be correct behavior. And
it's at least as much of a threat in the case of background content scripts.

The point is, the calling context is the right one, so that any converted
objects or exceptions that pop out are in the caller's context. Using the "main
world" context is only be right when the caller is from the "main world"
context.


> 
> WDYT?
tl;dr, I think your latest patch set is right, so lgtm. Thanks!

[dmichael (off chromium), 2014-09-10 17:13:56]

I updated the subject and description; hope that's OK. Edit as desired, but I
realized it was not accurate anymore.

I'm trying to educate myself about v8 Contexts and isolates, and ran across some
reading material in case you want to read them too:
https://developers.google.com/v8/embed#contexts
https://docs.google.com/a/google.com/document/d/1DeEaAQlN4ZCVkaUzhHIhHOQVvF2a...
(internal only :-( )
https://groups.google.com/a/chromium.org/d/msg/blink-dev/SJZVnlbxFOY/NwsBS5wB...

[raymes, 2014-09-11 00:07:19]

The CQ bit was checked by raymes@chromium.org

[raymes, 2014-09-11 00:08:40]

On 2014/09/10 17:13:56, dmichael wrote:
> I updated the subject and description; hope that's OK. Edit as desired, but I
> realized it was not accurate anymore.
> 
> I'm trying to educate myself about v8 Contexts and isolates, and ran across
some
> reading material in case you want to read them too:
> https://developers.google.com/v8/embed#contexts
>
https://docs.google.com/a/google.com/document/d/1DeEaAQlN4ZCVkaUzhHIhHOQVvF2a...
> (internal only :-( )
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/SJZVnlbxFOY/NwsBS5wB...

Thanks David. I had a bit of a look at the thread you linked yesterday - it's
mind boggling stuff to me :)

[commit-bot: I haz the power, 2014-09-11 00:13:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555583003/40001

[commit-bot: I haz the power, 2014-09-11 01:54:03]

Committed patchset #3 (id:40001) as 83d055ac1aee6eb1d4f79976d4b18d940d1cdff7

[commit-bot: I haz the power, 2014-09-11 03:09:06]

Patchset 3 (id:??) landed as
https://crrev.com/1630477a5ebc6cfab27704b54ca89a76a3036c6a
Cr-Commit-Position: refs/heads/master@{#294291}