Modifies WindowProxy::setSecurityToken so that the frame's SecurityOrigin is taken into account whe…

Modifies WindowProxy::setSecurityToken so that the frame's SecurityOrigin is taken into account when setting the token for an extension.

Chrome extensions (or, in general, scripts that run in isolated worlds) can no longer do cross-origin access to the parent window or other iframes.

If the domain of one frame is explicitly set through the document.domain accessor from the main world, the security tokens of all the isolated worlds associated with that frame are also updated.

BUG=529682
Committed: https://crrev.com/cf073253ae5c3e2f12166977eeddec1a1bcc016d
git-svn-id: svn://svn.chromium.org/blink/trunk@202267 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[jochen (gone - plz use gerrit), 2015-09-10 14:16:43]

jochen@chromium.org changed reviewers:
+ creis@chromium.org, haraken@chromium.org, jochen@chromium.org,
kalman@chromium.org

[jochen (gone - plz use gerrit), 2015-09-10 14:16:43]

lgtm

what do others think?

Enrico, can you trigger tryjobs?

[epertoso, 2015-09-10 14:17:03]

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

[commit-bot: I haz the power, 2015-09-10 14:17:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327263002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327263002/1

[jochen (gone - plz use gerrit), 2015-09-10 14:19:01]

also, could you write a layout test for this?

there's window.testRunner.setIsolatedWorldSecurityOrigin and
evaluateScriptInIsolatedWorld

[commit-bot: I haz the power, 2015-09-10 15:37:54]

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

[commit-bot: I haz the power, 2015-09-10 15:37:55]

Dry run: This issue passed the CQ dry run.

[haraken, 2015-09-10 23:34:20]

Layout test sounds nice.

Also please add more explanations to the CL description so that others can
follow what the CL is doing.

https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
Source/bindings/core/v8/WindowProxy.cpp:465: token = frameSecurityToken + token;

Just help me understand: Why do we need to add a per-frame information to the
token? What we need is to add a per-world information (e.g., world id) to the
token, isn't it?

[epertoso, 2015-09-11 13:18:22]

https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
Source/bindings/core/v8/WindowProxy.cpp:465: token = frameSecurityToken + token;
On 2015/09/10 at 23:34:20, haraken wrote:
> Just help me understand: Why do we need to add a per-frame information to the
token? What we need is to add a per-world information (e.g., world id) to the
token, isn't it?

The rationale here is: the extension can access some domain, e.g.
'http://www.abc.de/*'. 

A page on abc.de has an iframe, whose source is still
'http://www.abc.de/something', so the extension script is injected in that frame
too.

IIUC, there are now two worlds for the same script, but the same-origin policy
doesn't prevent the iframe from accessing the content of the document in which
it is contained, because they have the same protocol, host and port.

[haraken, 2015-09-11 13:22:20]

On 2015/09/11 13:18:22, epertoso wrote:
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> File Source/bindings/core/v8/WindowProxy.cpp (right):
> 
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> Source/bindings/core/v8/WindowProxy.cpp:465: token = frameSecurityToken +
token;
> On 2015/09/10 at 23:34:20, haraken wrote:
> > Just help me understand: Why do we need to add a per-frame information to
the
> token? What we need is to add a per-world information (e.g., world id) to the
> token, isn't it?
> 
> The rationale here is: the extension can access some domain, e.g.
> 'http://www.abc.de/*'. 
> 
> A page on abc.de has an iframe, whose source is still
> 'http://www.abc.de/something', so the extension script is injected in that
frame
> too.
> 
> IIUC, there are now two worlds for the same script, but the same-origin policy
> doesn't prevent the iframe from accessing the content of the document in which
> it is contained, because they have the same protocol, host and port.

There should be 1 world, 2 frames, and thus 1*2=2 contexts in that scenario.

See the section 2 of this document:
https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1...

[jochen (gone - plz use gerrit), 2015-09-11 13:25:53]

On 2015/09/11 at 13:22:20, haraken wrote:
> On 2015/09/11 13:18:22, epertoso wrote:
> >
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > File Source/bindings/core/v8/WindowProxy.cpp (right):
> > 
> >
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > Source/bindings/core/v8/WindowProxy.cpp:465: token = frameSecurityToken +
token;
> > On 2015/09/10 at 23:34:20, haraken wrote:
> > > Just help me understand: Why do we need to add a per-frame information to
the
> > token? What we need is to add a per-world information (e.g., world id) to
the
> > token, isn't it?
> > 
> > The rationale here is: the extension can access some domain, e.g.
> > 'http://www.abc.de/*'. 
> > 
> > A page on abc.de has an iframe, whose source is still
> > 'http://www.abc.de/something', so the extension script is injected in that
frame
> > too.
> > 
> > IIUC, there are now two worlds for the same script, but the same-origin
policy
> > doesn't prevent the iframe from accessing the content of the document in
which
> > it is contained, because they have the same protocol, host and port.
> 
> There should be 1 world, 2 frames, and thus 1*2=2 contexts in that scenario.
> 
> See the section 2 of this document:
https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1...

No, we're talking about content scripts here, do there are 2 worlds.

Before the change, the isolated worlds have the same security token, even though
they're cross origin

[haraken, 2015-09-11 13:28:35]

On 2015/09/11 13:25:53, jochen wrote:
> On 2015/09/11 at 13:22:20, haraken wrote:
> > On 2015/09/11 13:18:22, epertoso wrote:
> > >
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > > File Source/bindings/core/v8/WindowProxy.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > > Source/bindings/core/v8/WindowProxy.cpp:465: token = frameSecurityToken +
> token;
> > > On 2015/09/10 at 23:34:20, haraken wrote:
> > > > Just help me understand: Why do we need to add a per-frame information
to
> the
> > > token? What we need is to add a per-world information (e.g., world id) to
> the
> > > token, isn't it?
> > > 
> > > The rationale here is: the extension can access some domain, e.g.
> > > 'http://www.abc.de/*'. 
> > > 
> > > A page on abc.de has an iframe, whose source is still
> > > 'http://www.abc.de/something', so the extension script is injected in that
> frame
> > > too.
> > > 
> > > IIUC, there are now two worlds for the same script, but the same-origin
> policy
> > > doesn't prevent the iframe from accessing the content of the document in
> which
> > > it is contained, because they have the same protocol, host and port.
> > 
> > There should be 1 world, 2 frames, and thus 1*2=2 contexts in that scenario.
> > 
> > See the section 2 of this document:
>
https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1...
> 
> No, we're talking about content scripts here, do there are 2 worlds.
> 
> Before the change, the isolated worlds have the same security token, even
though
> they're cross origin

ah, makes sense. LGTM once a layout test is added :)

[jochen (gone - plz use gerrit), 2015-09-11 13:30:48]

Looking at the code some more, I wonder whether we'd then need to update
the isolated worlds' security tokens, when we update the security token of
the main world

On Fri, Sep 11, 2015, 3:28 PM  <haraken@chromium.org> wrote:

> On 2015/09/11 13:25:53, jochen wrote:
> > On 2015/09/11 at 13:22:20, haraken wrote:
> > > On 2015/09/11 13:18:22, epertoso wrote:
> > > >
>
>
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > > > File Source/bindings/core/v8/WindowProxy.cpp (right):
> > > >
> > > >
>
>
>
https://codereview.chromium.org/1327263002/diff/1/Source/bindings/core/v8/Win...
> > > > Source/bindings/core/v8/WindowProxy.cpp:465: token =
> > frameSecurityToken +
> > token;
> > > > On 2015/09/10 at 23:34:20, haraken wrote:
> > > > > Just help me understand: Why do we need to add a per-frame
> > information
> to
> > the
> > > > token? What we need is to add a per-world information (e.g., world
> > id) to
> > the
> > > > token, isn't it?
> > > >
> > > > The rationale here is: the extension can access some domain, e.g.
> > > > 'http://www.abc.de/*'.
> > > >
> > > > A page on abc.de has an iframe, whose source is still
> > > > 'http://www.abc.de/something', so the extension script is injected
> in
> > that
> > frame
> > > > too.
> > > >
> > > > IIUC, there are now two worlds for the same script, but the
> > same-origin
> > policy
> > > > doesn't prevent the iframe from accessing the content of the document
> > in
> > which
> > > > it is contained, because they have the same protocol, host and port.
> > >
> > > There should be 1 world, 2 frames, and thus 1*2=2 contexts in that
> > scenario.
> > >
> > > See the section 2 of this document:
>
>
>
https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1...
>
> > No, we're talking about content scripts here, do there are 2 worlds.
>
> > Before the change, the isolated worlds have the same security token, even
> though
> > they're cross origin
>
> ah, makes sense. LGTM once a layout test is added :)
>
>
> https://codereview.chromium.org/1327263002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[epertoso, 2015-09-14 08:53:39]

Added a layout test.

[jochen (gone - plz use gerrit), 2015-09-14 08:56:35]

still lgtm

https://codereview.chromium.org/1327263002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/resources/cross-frame-iframe-for-parent-isolated-world.html
(right):

https://codereview.chromium.org/1327263002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/resources/cross-frame-iframe-for-parent-isolated-world.html:1:
<html>
nit. add <!DOCTYPE html>

[haraken, 2015-09-14 09:01:56]

> Looking at the code some more, I wonder whether we'd then need to update
> the isolated worlds' security tokens, when we update the security token of
> the main world

What about this concern? Are we planning to fix it in a follow-up CL?

[epertoso, 2015-09-14 09:13:50]

https://codereview.chromium.org/1327263002/diff/20001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/resources/cross-frame-iframe-for-parent-isolated-world.html
(right):

https://codereview.chromium.org/1327263002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/resources/cross-frame-iframe-for-parent-isolated-world.html:1:
<html>
On 2015/09/14 at 08:56:35, jochen wrote:
> nit. add <!DOCTYPE html>

Done.

[epertoso, 2015-09-14 15:13:39]

On 2015/09/14 at 09:01:56, haraken wrote:
> > Looking at the code some more, I wonder whether we'd then need to update
> > the isolated worlds' security tokens, when we update the security token of
> > the main world
> 
> What about this concern? Are we planning to fix it in a follow-up CL?

I addressed it in this CL.

Please take another look.

[haraken, 2015-09-14 15:29:00]

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:147: WindowProxy* proxy =
m_windowProxyManager->existingWindowProxy(isolatedContext.first->world());

I think we should call windowProxy, not existingWindowProxy.

Note that existingWindowProxy is for devtools only (see the comment on
WindowProxyManager::existingWindowProxy).

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:149:
proxy->setSecurityToken(isolatedContext.second);

How about calling updateSecurityOrigin, just like we do for the main world proxy
at line 143?

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:462: if
(frameSecurityOrigin->domainWasSetInDOM() || frameSecurityToken.isEmpty() ||
frameSecurityToken == "null") {

Would you help me understand why we need the domainWasSetInDOM() check? (It
would be worth having a comment.)

[epertoso, 2015-09-14 15:59:37]

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:147: WindowProxy* proxy =
m_windowProxyManager->existingWindowProxy(isolatedContext.first->world());
On 2015/09/14 at 15:28:59, haraken wrote:
> I think we should call windowProxy, not existingWindowProxy.
> 
> Note that existingWindowProxy is for devtools only (see the comment on
WindowProxyManager::existingWindowProxy).

But windowProxy() will create a proxy if there isn't one already. Do we want
this?

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:149:
proxy->setSecurityToken(isolatedContext.second);
On 2015/09/14 at 15:28:59, haraken wrote:
> How about calling updateSecurityOrigin, just like we do for the main world
proxy at line 143?

updateSecurityOrigin has an ASSERT that implies it's expected to be called only
for the main world.

We could remove the assert, if you prefer.

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:462: if
(frameSecurityOrigin->domainWasSetInDOM() || frameSecurityToken.isEmpty() ||
frameSecurityToken == "null") {
On 2015/09/14 at 15:29:00, haraken wrote:
> Would you help me understand why we need the domainWasSetInDOM() check? (It
would be worth having a comment.)

Added a comment.

[haraken, 2015-09-14 23:46:04]

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:147: WindowProxy* proxy =
m_windowProxyManager->existingWindowProxy(isolatedContext.first->world());
On 2015/09/14 15:59:37, epertoso wrote:
> On 2015/09/14 at 15:28:59, haraken wrote:
> > I think we should call windowProxy, not existingWindowProxy.
> > 
> > Note that existingWindowProxy is for devtools only (see the comment on
> WindowProxyManager::existingWindowProxy).
> 
> But windowProxy() will create a proxy if there isn't one already. Do we want
> this?

I can't come up with a scenario where the isolated world doesn't have a
WindowProxy when we reach here.

Even if it happens, a right thing would be to create a WindowProxy and set a
security origin on it. Otherwise, the WindowProxy will be instantiated without
setting the security origin, which seems wrong.

So, calling windowProxy() would be a right thing here, I think.

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:149:
proxy->setSecurityToken(isolatedContext.second);
On 2015/09/14 15:59:37, epertoso wrote:
> On 2015/09/14 at 15:28:59, haraken wrote:
> > How about calling updateSecurityOrigin, just like we do for the main world
> proxy at line 143?
> 
> updateSecurityOrigin has an ASSERT that implies it's expected to be called
only
> for the main world.
> 
> We could remove the assert, if you prefer.

I think it does make sense to remove the assert now, since we're adding the
security origin to isolated worlds in this CL.

[Charlie Reis, 2015-09-14 23:50:14]

creis@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Charlie Reis, 2015-09-15 06:08:17]

[+Devlin]

Rubber stamp LGTM from a security policy perspective.  I haven't found any
extensions yet that would be obviously broken by this, and we can keep an eye
out for reports if this lands.

(I defer to haraken@ for the details.)

[epertoso, 2015-09-15 08:24:58]

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:147: WindowProxy* proxy =
m_windowProxyManager->existingWindowProxy(isolatedContext.first->world());
On 2015/09/14 at 23:46:04, haraken wrote:
> On 2015/09/14 15:59:37, epertoso wrote:
> > On 2015/09/14 at 15:28:59, haraken wrote:
> > > I think we should call windowProxy, not existingWindowProxy.
> > > 
> > > Note that existingWindowProxy is for devtools only (see the comment on
> > WindowProxyManager::existingWindowProxy).
> > 
> > But windowProxy() will create a proxy if there isn't one already. Do we want
> > this?
> 
> I can't come up with a scenario where the isolated world doesn't have a
WindowProxy when we reach here.
> 
> Even if it happens, a right thing would be to create a WindowProxy and set a
security origin on it. Otherwise, the WindowProxy will be instantiated without
setting the security origin, which seems wrong.
> 
> So, calling windowProxy() would be a right thing here, I think.

Done.

https://codereview.chromium.org/1327263002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptController.cpp:149:
proxy->setSecurityToken(isolatedContext.second);
On 2015/09/14 at 23:46:04, haraken wrote:
> On 2015/09/14 15:59:37, epertoso wrote:
> > On 2015/09/14 at 15:28:59, haraken wrote:
> > > How about calling updateSecurityOrigin, just like we do for the main world
> > proxy at line 143?
> > 
> > updateSecurityOrigin has an ASSERT that implies it's expected to be called
only
> > for the main world.
> > 
> > We could remove the assert, if you prefer.
> 
> I think it does make sense to remove the assert now, since we're adding the
security origin to isolated worlds in this CL.

Done.

[haraken, 2015-09-15 10:47:58]

LGTM

[epertoso, 2015-09-15 10:48:21]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2015-09-15 10:48:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1327263002/#ps120001
(title: "Update")

[commit-bot: I haz the power, 2015-09-15 10:48:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327263002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327263002/120001

[commit-bot: I haz the power, 2015-09-15 11:57:54]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202267

[commit-bot: I haz the power, 2015-09-23 12:44:17]

Patchset 7 (id:??) landed as
https://crrev.com/cf073253ae5c3e2f12166977eeddec1a1bcc016d