Sandbox detached iframes a bit more

Sandbox detached iframes a bit more

Disallow cross-origin wrapper creation from them

BUG=616225
R=haraken@chromium.org

Committed: https://crrev.com/c0f67c5511dacb91d9484a737e162c02d9f53269
Cr-Commit-Position: refs/heads/master@{#398260}

[haraken, 2016-06-06 07:46:25]

LGTM to try. When I tried to forbid wrapper creations on all detached iframes,
it broke a lot of things. It is clever to limit the target to cross-origin
Locations and see how it goes.

Can we add a test for the change?

[jochen (gone - plz use gerrit), 2016-06-06 07:58:20]

On 2016/06/06 at 07:46:25, haraken wrote:
> LGTM to try. When I tried to forbid wrapper creations on all detached iframes,
it broke a lot of things. It is clever to limit the target to cross-origin
Locations and see how it goes.
> 
> Can we add a test for the change?

I suspect http/tests/security/location-change-from-detached-iframe.html will
fail. If yes, I'll update the test accordingly

[jochen (gone - plz use gerrit), 2016-06-07 08:40:29]

On 2016/06/06 at 07:58:20, jochen wrote:
> On 2016/06/06 at 07:46:25, haraken wrote:
> > LGTM to try. When I tried to forbid wrapper creations on all detached
iframes, it broke a lot of things. It is clever to limit the target to
cross-origin Locations and see how it goes.
> > 
> > Can we add a test for the change?
> 
> I suspect http/tests/security/location-change-from-detached-iframe.html will
fail. If yes, I'll update the test accordingly

It didn't fail. Actually, if a test would fail, we'd have a security bug. Since
the referenced bug handily points such a bug out, I tested my patch on a
revision before that fix, and it also caught the bug.

[jochen (gone - plz use gerrit), 2016-06-07 08:40:34]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-06-07 08:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2042743002/#ps60001
(title: "updates")

[commit-bot: I haz the power, 2016-06-07 08:40:39]

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

[haraken, 2016-06-07 08:46:09]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2016-06-07 08:46:09]

+yukishiino: Do you know if wrapper creation (and other DOM operation) should be
allowed on cross-origin detached iframes?

My best guess is that:

- Per the spec, DOM operations on detached iframes are allowed (so this CL is
doing a wrong thing).
- However, behaviors of browsers are broken in many ways.
- In practice, it would be safer to disallow DOM operations at least for
cross-origin iframes.

[jochen (gone - plz use gerrit), 2016-06-07 08:47:05]

On 2016/06/07 at 08:46:09, haraken wrote:
> +yukishiino: Do you know if wrapper creation (and other DOM operation) should
be allowed on cross-origin detached iframes?
> 
> My best guess is that:
> 
> - Per the spec, DOM operations on detached iframes are allowed (so this CL is
doing a wrong thing).
> - However, behaviors of browsers are broken in many ways.
> - In practice, it would be safer to disallow DOM operations at least for
cross-origin iframes.

btw, I still allow wrapper creation if the security origin checks out.

[jochen (gone - plz use gerrit), 2016-06-07 08:47:39]

https://codereview.chromium.org/2042743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp (right):

https://codereview.chromium.org/2042743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp:116: if
(callingWindow->document()->getSecurityOrigin()->canAccessCheckSuborigins(targetWindow->document()->getSecurityOrigin()))
here ^^^

[Yuki, 2016-06-07 08:56:31]

On 2016/06/07 08:46:09, haraken wrote:
> +yukishiino: Do you know if wrapper creation (and other DOM operation) should
be
> allowed on cross-origin detached iframes?

I don't know any rule that *disallows* wrapper creation on detached iframes
regardless of whether same origin or cross origin.  Thus, I think it's allowed.


> My best guess is that:
> 
> - Per the spec, DOM operations on detached iframes are allowed (so this CL is
> doing a wrong thing).
> - However, behaviors of browsers are broken in many ways.
> - In practice, it would be safer to disallow DOM operations at least for
> cross-origin iframes.

Totally agreed.


LGTM to try, too.

[commit-bot: I haz the power, 2016-06-07 09:46:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-07 09:48:29]

Description was changed from

==========
Sandbox detached iframes a bit more

Disallow cross-origin wrapper creation from them

BUG=616225
R=haraken@chromium.org
==========

to

==========
Sandbox detached iframes a bit more

Disallow cross-origin wrapper creation from them

BUG=616225
R=haraken@chromium.org

Committed: https://crrev.com/c0f67c5511dacb91d9484a737e162c02d9f53269
Cr-Commit-Position: refs/heads/master@{#398260}
==========

[commit-bot: I haz the power, 2016-06-07 09:48:30]

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