Null out LocalDOMWindow::m_frame on navigation.

Null out LocalDOMWindow::frame() on navigation.

In Blink, LocalDOMWindow::frame() is only set to null when the
LocalFrame is destroyed. Multiple LocalDOMWindow objects can hold a
reference to the same LocalFrame.

It turns out that this is dangerous and a persistent source of XSS bugs.
Code that creates scriptable objects for a frame needs to remember to
call DOMWindow::isCurrentlyDisplayedInFrame() to verify that the
creating context is still active in the frame. If this check is left
out, the created object can often trigger XSS.

Instead of depending on developers to remember to add this check where
needed, Blink now nulls out LocalDOMWindow::frame() as soon as it
navigates away from a LocalDOMWindow. Code in Blink already handles the
null case, since this is already something that can happen. Code that
improperly handles this case will tend to crash (suboptimal but safe),
and in general, failures won't result in XSS, since a detached frame
cannot be reattached.

BUG=525330
Committed: https://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a
Cr-Commit-Position: refs/heads/master@{#351496}

[dcheng, 2015-09-28 06:31:33]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[dcheng, 2015-09-28 06:44:15]

This change has a few implications for backwards compatibility: if a page
captures a reference to its own Window object in a closure, assigns that closure
to another same-origin Window object, and then navigates away, many properties
of captured Window object (e.g. opener,
parent, etc) will start returning null. As far as I can tell, this is the cause
of all the test failures in this patch.

It turns out that this is not a serious problem: Gecko (and if memory serves me
correctly, Trident as well) already disallow execution of closures from an
inactive Window. If you feel OK with this approach, I will update the tests.

The other question is if this should be an intent to implement since it does
have an observable change: how about "Intent to Implement and Ship: Make Chrome
less XSS-able"? =)

[dcheng, 2015-09-28 06:45:09]

https://codereview.chromium.org/1374533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/1374533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:670: if
(!isCurrentlyDisplayedInFrame() && (!m_navigator || m_navigator->frame())) {
I tested with the test case in
https://code.google.com/p/chromium/issues/detail?id=522791 and confirmed that it
does not work any more.

https://codereview.chromium.org/1374533002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1374533002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1369: }
I'm not sure why, but the presubmit checks are randomly complaining about this
and line 1456, so I went ahead and fixed them.

[haraken, 2015-09-28 06:57:21]

I'll defer the decision to jochen@.

On 2015/09/28 06:44:15, dcheng wrote:
> This change has a few implications for backwards compatibility: if a page
> captures a reference to its own Window object in a closure, assigns that
closure
> to another same-origin Window object, and then navigates away, many properties
> of captured Window object (e.g. opener,
> parent, etc) will start returning null. As far as I can tell, this is the
cause
> of all the test failures in this patch.

BTW, how is this change related to the inner/outer global object? The reason we
have to keep alive an inner global object of the already navigated window is
that we have to keep properties of the navigated window as long as the window is
referenced in a closure or something.

> It turns out that this is not a serious problem: Gecko (and if memory serves
me
> correctly, Trident as well) already disallow execution of closures from an
> inactive Window. If you feel OK with this approach, I will update the tests.
> 
> The other question is if this should be an intent to implement since it does
> have an observable change: how about "Intent to Implement and Ship: Make
Chrome
> less XSS-able"? =)

[dcheng, 2015-09-28 08:05:17]

On 2015/09/28 at 06:57:21, haraken wrote:
> I'll defer the decision to jochen@.
> 
> On 2015/09/28 06:44:15, dcheng wrote:
> > This change has a few implications for backwards compatibility: if a page
> > captures a reference to its own Window object in a closure, assigns that
closure
> > to another same-origin Window object, and then navigates away, many
properties
> > of captured Window object (e.g. opener,
> > parent, etc) will start returning null. As far as I can tell, this is the
cause
> > of all the test failures in this patch.
> 
> BTW, how is this change related to the inner/outer global object? The reason
we have to keep alive an inner global object of the already navigated window is
that we have to keep properties of the navigated window as long as the window is
referenced in a closure or something.

I might be wrong, but the only way I found to capture the inner global is to
create a closure in that execution context, which implicitly captures the inner
global. So you can do something tricky like this:

// window.parent is same origin.
window.parent.foo = function() {
  // Must use window.opener.log since a detached window cannot log.
  opener.log(document.URL);
  opener.log(window.document.URL);
};
window.location = '/post-message-to-parent.html';

// in parent:
function onMessage() {
  foo();  // Attempt to execute the closure created by the already-navigated
window.
}

In Blink/WebKit, the result of those two lines is:
document.URL => /path/of/original-page.html
window.document.URL => /post-message-to-parent.html

With this change, <captured inner global>.opener returns null, so opener.log()
just fails. If the test is updated to capture the original value of
window.opener, then the results are:
document.URL => /path/of/original-page.html
window.document.URL => Cannot read property 'document' of null (<captured inner
global>.window() is null)

Normally, this would be a problem for backwards compatibility. However, I think
it is actually not a problem because other browsers don't even let you execute
closures from navigated contexts: Firefox throws an exception when you attempt
to execute a closure from a navigated away page
(NS_ERROR_XPC_SECURITY_MANAGER_VETO) and my recollection is IE does the same
thing (but I don't have it available to confirm atm).

> 
> > It turns out that this is not a serious problem: Gecko (and if memory serves
me
> > correctly, Trident as well) already disallow execution of closures from an
> > inactive Window. If you feel OK with this approach, I will update the tests.
> > 
> > The other question is if this should be an intent to implement since it does
> > have an observable change: how about "Intent to Implement and Ship: Make
Chrome
> > less XSS-able"? =)

[jochen (gone - plz use gerrit), 2015-09-28 11:34:35]

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

[jochen (gone - plz use gerrit), 2015-09-28 11:34:35]

Toon, can you comment on this?

[jochen (gone - plz use gerrit), 2015-09-28 11:34:58]

(anyway, matching firefox seems fine, so I'd go ahead and update the layout
tests)

[dcheng, 2015-09-29 07:50:38]

OK, I fixed the layout tests (generally, by capturing any variables into the
closure rather than try to dereference them from the captured global).

I also went ahead and added a layout test, since this doesn't seem to be very
well covered.

[jochen (gone - plz use gerrit), 2015-09-29 07:51:57]

lgtm

[dcheng, 2015-09-29 07:53:25]

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html
(right):

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html:4:
f[0].location =
"http://127.0.0.1:8000/security/frameNavigation/resources/fail.html";
Incidentally... this test was actually failing when run manually without my
changes.

+japhet... is there any DRT callback to dump out loading attempts? 

Alternatively, maybe using a JS URL to try to alert here would be good enough?

[haraken, 2015-09-29 07:54:41]

LGTM

[Nate Chapin, 2015-09-29 21:48:59]

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

[Nate Chapin, 2015-09-29 21:49:00]

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html
(right):

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html:4:
f[0].location =
"http://127.0.0.1:8000/security/frameNavigation/resources/fail.html";
On 2015/09/29 07:53:25, dcheng wrote:
> Incidentally... this test was actually failing when run manually without my
> changes.
> 
> +japhet... is there any DRT callback to dump out loading attempts? 
> 
> Alternatively, maybe using a JS URL to try to alert here would be good enough?

How was it failing? There's a DRT mode that will dump resource messages, but I
don't see that in use for any users of this file.

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:125: return frame() &&
frame()->host();
Is this host() check necessary? Can a DOMWindow and Frame remain attached while
the Frame is detached from the host?

[dcheng, 2015-09-29 21:53:53]

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html
(right):

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html:4:
f[0].location =
"http://127.0.0.1:8000/security/frameNavigation/resources/fail.html";
On 2015/09/29 at 21:49:00, Nate Chapin wrote:
> On 2015/09/29 07:53:25, dcheng wrote:
> > Incidentally... this test was actually failing when run manually without my
> > changes.
> > 
> > +japhet... is there any DRT callback to dump out loading attempts? 
> > 
> > Alternatively, maybe using a JS URL to try to alert here would be good
enough?
> 
> How was it failing? There's a DRT mode that will dump resource messages, but I
don't see that in use for any users of this file.

I believe this test is attempting to test that this navigation is blocked. Since
there's no event to listen to in this circumstance, the test just completes and
optimistically hopes that the navigation wasn't scheduled.

However, if you ran it locally, it did navigate to fail.html and pop up an
alert().

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1374533002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:125: return frame() &&
frame()->host();
On 2015/09/29 at 21:49:00, Nate Chapin wrote:
> Is this host() check necessary? Can a DOMWindow and Frame remain attached
while the Frame is detached from the host?

This doesn't affect when LocalDOMWindow::frame() becomes null during frame
detach. I did try adding setDOMWindow(nullptr) to LocalFrame::detach(), but that
broke a lot of code that assumes that Frame::document() will be non-null during
detach.

[dcheng, 2015-09-29 23:29:20]

dcheng@chromium.org changed reviewers:
+ blink-reviews@chromium.org, vogelheim@chromium.org, yhirano@chromium.org
- haraken@chromium.org, jochen@chromium.org, verwaest@chromium.org

[haraken, 2015-09-29 23:41:28]

On 2015/09/28 08:05:17, dcheng wrote:
> On 2015/09/28 at 06:57:21, haraken wrote:
> > I'll defer the decision to jochen@.
> > 
> > On 2015/09/28 06:44:15, dcheng wrote:
> > > This change has a few implications for backwards compatibility: if a page
> > > captures a reference to its own Window object in a closure, assigns that
> closure
> > > to another same-origin Window object, and then navigates away, many
> properties
> > > of captured Window object (e.g. opener,
> > > parent, etc) will start returning null. As far as I can tell, this is the
> cause
> > > of all the test failures in this patch.
> > 
> > BTW, how is this change related to the inner/outer global object? The reason
> we have to keep alive an inner global object of the already navigated window
is
> that we have to keep properties of the navigated window as long as the window
is
> referenced in a closure or something.
> 
> I might be wrong, but the only way I found to capture the inner global is to
> create a closure in that execution context, which implicitly captures the
inner
> global. So you can do something tricky like this:
> 
> // window.parent is same origin.
> window.parent.foo = function() {
>   // Must use window.opener.log since a detached window cannot log.
>   opener.log(document.URL);
>   opener.log(window.document.URL);
> };
> window.location = '/post-message-to-parent.html';
> 
> // in parent:
> function onMessage() {
>   foo();  // Attempt to execute the closure created by the already-navigated
> window.
> }
> 
> In Blink/WebKit, the result of those two lines is:
> document.URL => /path/of/original-page.html
> window.document.URL => /post-message-to-parent.html
> 
> With this change, <captured inner global>.opener returns null, so opener.log()
> just fails. If the test is updated to capture the original value of
> window.opener, then the results are:
> document.URL => /path/of/original-page.html
> window.document.URL => Cannot read property 'document' of null (<captured
inner
> global>.window() is null)
> 
> Normally, this would be a problem for backwards compatibility. However, I
think
> it is actually not a problem because other browsers don't even let you execute
> closures from navigated contexts: Firefox throws an exception when you attempt
> to execute a closure from a navigated away page
> (NS_ERROR_XPC_SECURITY_MANAGER_VETO) and my recollection is IE does the same
> thing (but I don't have it available to confirm atm).

Does this mean that after landing this change, we no longer need to keep the
inner object alive after the navigation? (It significantly simplifies the
global-object-tweaking logic in WindowProxy and improves security.)

[dcheng, 2015-09-30 00:37:55]

On 2015/09/29 at 23:41:28, haraken wrote:
> On 2015/09/28 08:05:17, dcheng wrote:
> > On 2015/09/28 at 06:57:21, haraken wrote:
> > > I'll defer the decision to jochen@.
> > > 
> > > On 2015/09/28 06:44:15, dcheng wrote:
> > > > This change has a few implications for backwards compatibility: if a
page
> > > > captures a reference to its own Window object in a closure, assigns that
> > closure
> > > > to another same-origin Window object, and then navigates away, many
> > properties
> > > > of captured Window object (e.g. opener,
> > > > parent, etc) will start returning null. As far as I can tell, this is
the
> > cause
> > > > of all the test failures in this patch.
> > > 
> > > BTW, how is this change related to the inner/outer global object? The
reason
> > we have to keep alive an inner global object of the already navigated window
is
> > that we have to keep properties of the navigated window as long as the
window is
> > referenced in a closure or something.
> > 
> > I might be wrong, but the only way I found to capture the inner global is to
> > create a closure in that execution context, which implicitly captures the
inner
> > global. So you can do something tricky like this:
> > 
> > // window.parent is same origin.
> > window.parent.foo = function() {
> >   // Must use window.opener.log since a detached window cannot log.
> >   opener.log(document.URL);
> >   opener.log(window.document.URL);
> > };
> > window.location = '/post-message-to-parent.html';
> > 
> > // in parent:
> > function onMessage() {
> >   foo();  // Attempt to execute the closure created by the already-navigated
> > window.
> > }
> > 
> > In Blink/WebKit, the result of those two lines is:
> > document.URL => /path/of/original-page.html
> > window.document.URL => /post-message-to-parent.html
> > 
> > With this change, <captured inner global>.opener returns null, so
opener.log()
> > just fails. If the test is updated to capture the original value of
> > window.opener, then the results are:
> > document.URL => /path/of/original-page.html
> > window.document.URL => Cannot read property 'document' of null (<captured
inner
> > global>.window() is null)
> > 
> > Normally, this would be a problem for backwards compatibility. However, I
think
> > it is actually not a problem because other browsers don't even let you
execute
> > closures from navigated contexts: Firefox throws an exception when you
attempt
> > to execute a closure from a navigated away page
> > (NS_ERROR_XPC_SECURITY_MANAGER_VETO) and my recollection is IE does the same
> > thing (but I don't have it available to confirm atm).
> 
> Does this mean that after landing this change, we no longer need to keep the
inner object alive after the navigation? (It significantly simplifies the
global-object-tweaking logic in WindowProxy and improves security.)

Unfortunately, I think we would still need to keep it alive: things like
'window.document' are still linked. This change by itself doesn't stop the
inactive closure from executing: it just tries to stop the class of XSS problems
when an inactive Window allows cross-origin interaction by creating objects
linked with a frame that's already displaying another Window.

Do you know if there's a way to disallow execution of closures created in
contexts that are no longer active? When I talked with jochen@, I got the
impression that this wasn't possible today.

[dcheng, 2015-09-30 00:46:18]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[dcheng, 2015-09-30 00:46:18]

PTAL at the test changes for crash-on-querying-event-path.html. I found some
concerning things during my investigation, and want to make sure you're OK with
these changes.

https://codereview.chromium.org/1374533002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/crash-on-querying-event-path.html
(left):

https://codereview.chromium.org/1374533002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/crash-on-querying-event-path.html:19:
iframe.src = null;
This test was flaking, so I did some investigation:

1) It doesn't actually need the http server, so I copied it into fast/events.
This was enough to unflake the test by itself.

2) I was unable to get either version of the test (fast/events vs
http/tests/dom) to crash with the code changes in
https://codereview.chromium.org/533633002 reverted (even with no test changes).

3) I synced back to 2014-Sep-02 and rebuilt without vogelheim@'s fix. Then both
test cases started reliably crashing in ASAN.

4) I tweaked the test a bit to eliminate setTimeout and discovered:
- iframe navigation is *not* required to make it crash.
- it crashes at the gc() calls, before even accessing event.path.

5) In fact, even with the code fixes reapplied, both versions of the test still
crash the same way in ASAN at Blink commit r181234.

6) The test (both versions) do not crash in ASAN at ToT.

I don't honestly know what to make of #4-#6. With no navigation required, all
the objects should still be live: there's nothing tricky happening with
lifetimes. My only conclusion is that there's something really wrong with GC
tracing in this version of Chrome.

Since both versions of the test appear to crash ASAN the same way, I opted to
remove the http test and just keep the modified version in fast/events.

[haraken, 2015-09-30 01:33:41]

On 2015/09/30 00:37:55, dcheng wrote:
> On 2015/09/29 at 23:41:28, haraken wrote:
> > On 2015/09/28 08:05:17, dcheng wrote:
> > > On 2015/09/28 at 06:57:21, haraken wrote:
> > > > I'll defer the decision to jochen@.
> > > > 
> > > > On 2015/09/28 06:44:15, dcheng wrote:
> > > > > This change has a few implications for backwards compatibility: if a
> page
> > > > > captures a reference to its own Window object in a closure, assigns
that
> > > closure
> > > > > to another same-origin Window object, and then navigates away, many
> > > properties
> > > > > of captured Window object (e.g. opener,
> > > > > parent, etc) will start returning null. As far as I can tell, this is
> the
> > > cause
> > > > > of all the test failures in this patch.
> > > > 
> > > > BTW, how is this change related to the inner/outer global object? The
> reason
> > > we have to keep alive an inner global object of the already navigated
window
> is
> > > that we have to keep properties of the navigated window as long as the
> window is
> > > referenced in a closure or something.
> > > 
> > > I might be wrong, but the only way I found to capture the inner global is
to
> > > create a closure in that execution context, which implicitly captures the
> inner
> > > global. So you can do something tricky like this:
> > > 
> > > // window.parent is same origin.
> > > window.parent.foo = function() {
> > >   // Must use window.opener.log since a detached window cannot log.
> > >   opener.log(document.URL);
> > >   opener.log(window.document.URL);
> > > };
> > > window.location = '/post-message-to-parent.html';
> > > 
> > > // in parent:
> > > function onMessage() {
> > >   foo();  // Attempt to execute the closure created by the
already-navigated
> > > window.
> > > }
> > > 
> > > In Blink/WebKit, the result of those two lines is:
> > > document.URL => /path/of/original-page.html
> > > window.document.URL => /post-message-to-parent.html
> > > 
> > > With this change, <captured inner global>.opener returns null, so
> opener.log()
> > > just fails. If the test is updated to capture the original value of
> > > window.opener, then the results are:
> > > document.URL => /path/of/original-page.html
> > > window.document.URL => Cannot read property 'document' of null (<captured
> inner
> > > global>.window() is null)
> > > 
> > > Normally, this would be a problem for backwards compatibility. However, I
> think
> > > it is actually not a problem because other browsers don't even let you
> execute
> > > closures from navigated contexts: Firefox throws an exception when you
> attempt
> > > to execute a closure from a navigated away page
> > > (NS_ERROR_XPC_SECURITY_MANAGER_VETO) and my recollection is IE does the
same
> > > thing (but I don't have it available to confirm atm).
> > 
> > Does this mean that after landing this change, we no longer need to keep the
> inner object alive after the navigation? (It significantly simplifies the
> global-object-tweaking logic in WindowProxy and improves security.)
> 
> Unfortunately, I think we would still need to keep it alive: things like
> 'window.document' are still linked. This change by itself doesn't stop the
> inactive closure from executing: it just tries to stop the class of XSS
problems
> when an inactive Window allows cross-origin interaction by creating objects
> linked with a frame that's already displaying another Window.

Thanks for the clarification, make sense!
 
> Do you know if there's a way to disallow execution of closures created in
> contexts that are no longer active? When I talked with jochen@, I got the
> impression that this wasn't possible today.

jochen@ should know better :)

[dcheng, 2015-09-30 02:25:36]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-09-30 02:25:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1374533002/#ps80001
(title: "try to fix crash-on-querying-event-path.html")

[commit-bot: I haz the power, 2015-09-30 02:26:04]

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

[commit-bot: I haz the power, 2015-09-30 04:03:24]

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

[commit-bot: I haz the power, 2015-09-30 04:03:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2015-09-30 04:04:45]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-09-30 04:05:21]

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

[commit-bot: I haz the power, 2015-09-30 05:05:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-30 05:06:02]

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