291653 - Event handlers can fire with window.document set to new document

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Brian Ryner (not reading)]

There is a small window of time between the time we begin parsing a new document
and the time that the old document is destroyed (250ms, the paint suppression
delay).  During this time, it is still possible for event handlers in the old
document to fire if they were initiated by a user action.  When this happens,
window.document will be set to the newly-loading document, so the event handler
can grab whatever it wants from there.  Through some creative use of event
handlers (like mousemove), a malicious site could make it quite easy for a user
to trigger the event handler.

I discovered this in the context of a larger set of testing for fast back, so I
will try to reduce the testcase and post it here.

Comment 1

[Brian Ryner (not reading)]

I may have overstated the extent of this bug.  I can only get it to happen when
the new URL is on the same server as the old page.  Feel free to mark invalid if
this is as intended.

Comment 2

[Brendan Eich [:brendan]]

Is a same-origin check suppressing any across servers?  That's cool, but it
still does not match what happened with Netscape 2-4.  Maybe Gecko has always
been this way, though.  I'm pretty sure IE isolates documents, using distinct
inner window objects (which would help us in a couple of different ways: faster
yet safe caching of object principals; set-aside rather than copy-out/in for
fast-back navigation; this bug).

/be

Comment 3

[Hixie (not reading bugmail)]

The events should still be firing, but they should be firing on the original
document, for sure.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Is a same-origin check suppressing any across servers?

Almost certainly.

We have existing bugs on this, actually, mostly filed on the errors event
handlers get (because the window scope has been cleared, so the functions, etc,
they expect aren't there).

Comment 5

[chris hofmann]

should we close this out, or re-evaluate this area after all the event and wrapper work that has gone on in the last 2 years?

Comment 6

[Boris Zbarsky [:bzbarsky]]

The bug as filed is still here.  "window" is the outer window, so window.document is the new document, as described in comment 0.  The window scope is not cleared anymore, so more stuff will "work".

I'm not really sure what comment 3 is trying to say.  Say I have the following:

  var win = window.open();

Then in the newly opened window I have an event handler that compares window.document and parent.win.document.  I would expect to get back the same document from both.

Comment 7

[Lucas Adamski [:ladamski]]

Marking "sg:moderate" for now, however with some open questions regarding what sort of stuff will work now.

a) is the issue still same-origin only?
b) does it still require user interaction?

May re-evaluate severity based upon a) and b)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

A testcase would be good here... Is it possible to do anything bad with this? Don't security checks prevent any real access to the document?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yes, a testcase would be great, because things have changed since 2005.

Comment 10

[Johnny Stenback (:jst)]

Olli, can you help drive this forward, or decide that this is a non-issue and close this out? We're unlikely to get a testcase here unless we write it ourselves, if we can't do that we should close this as incomplete, or somesuch.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

This should be fixed long ago. When window.document is set to new document, outer window should
already have a new inner window, which means that the listeners in the old inner window don't fire
anymore.
But I'll try to verify this somehow.

Comment 12

[Johnny Stenback (:jst)]

Olli, any luck verifying here?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

This is just based on code inspection:
For nodes in the document nsContentUtils::GetContextForEventHandlers uses
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?mark=3854-3857#3843
which checks that right innerWindow is used (fix for Bug 403167).

But I'm not yet sure about window handling. Looking some more code...

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

So window event handlers can run when new document is set, but that works only with 
same origin documents.
It is not yet clear what prevent that with different origin docs.
I don't see exceptions but somehow http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#1533 fails.

Anyway, I think we should handle windows like we handle nodes: if innerWindow isn't the current
innerwindow, listener shouldn't run.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

bholley says that XOW does the security check in this case.

I still wonder why there isn't any security warning in the error console.

Comment 16

[Johnny Stenback (:jst)]

(In reply to Olli Pettay [:smaug] from comment #14)
> Anyway, I think we should handle windows like we handle nodes: if
> innerWindow isn't the current innerwindow, listener shouldn't run.

Agreed.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Make event listener handling more consistent.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/e2f76393d2d0