18553 - [DOGFOOD] addEventListener allows sniffing keystrokes (at least)

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[joro]

addEventListener may be used to sniff keystrokes from other windows, probably do
other mischief. It allows receiving events from other windows, e.g. entering
information in a form.
The code is:
----------------------------------------------
a=window.open("http://www.yahoo.com");
function ev(event)
{
/* This could have been done better, but this is only a demo */
document.forms[0].elements[0].value += String.fromCharCode(event.which);
}
a.addEventListener("keyup",ev,true);
----------------------------------------------

Comment 1

[Norris Boyd]

marking dogfood for consideration by PDT at jar's request.

Comment 2

[leger]

Putting on PDT+ radar.

Comment 3

[Norris Boyd]

Tom, any suggestions on how to fix this one? I assume we need to do something
similar to the checks in 4.x.

Comment 4

[joki (gone)]

Yeah, if 4.x we didn't allow listening to events from frames/windows from other
domains.  I assume we can do the same here.  We just have to decide whether to
do the checking at listener registration or event dispatch.  I'd lean towards
registration off the top of my head.

Comment 5

[Norris Boyd]

joki: Can you point me to where the registration occurs?

Comment 6

[Norris Boyd]

So event listeners can be added via a number of techniques.  We'll have
to decide where we want to do checks.  I'll use nsGlobalWindow as an
example but much of this code also lives in nsDocument and
nsGenericElement.  They come in through attribute name checks in
nsGenericHTMLElement::SetAttribute, they come in through script property
name checks in each SetProperty calls in all three classes mentioned.
And they come in through AddEventListener (and AddEventListenerByIID,
and internal interface) in each of the three classes.  These then enter
the nsEventListenerManager code through slightly different paths due to
differences between script and non-script handlers.  I'm guessing we
want to handle this in the central path inside nsEventListenerManager if
we can.  The most central location in there is in the
nsEventListenerManager::AddEventListener method (same name as the
function in question but different interface).

        -tom

Comment 7

[leger]

Dogfood, putting on M12 radar.

Comment 8

[Norris Boyd]

I notice that GlobalWindowImpl::EnableExternalCapture() is defined, but
unimplemented. Was this intended for this purpose?

Comment 9

[Norris Boyd]

Proposal for fix:

Add nsIPrincipal* parameter to nsEventListenerManager::AddEventListener.
Propagate the principal down from the various callers--the principal will
represent the creator of the window/document that is having a listener added.
Then in nsEventListenerManager::AddEventListener, I'll add a call to get the
current script (if any) that is attempting to add a listener and only allow it
to proceed if it matches the principal that was passed down. (We could allow
overriding the same origin check with a privilege.)

How does that sound?

Comment 10

[Norris Boyd]

Fixed. there's still additional work to do to get allowed listening to work
after bug 17351 is fixed, but it's no different than other problems revealed by
that bug.

Comment 11

[dshea]

Windows NT 1999120208 Comm
Verified
...'[Exception... "Security error"'...

Comment 12

[leger]

Bulk moving all Browser Security bugs to new Security: General component.  The 
previous Security component for Browser will be deleted.

Comment 13

[bsharma]

Reopening the bug. I ran the above url on the 2001-09-20-Branch build and
whatever I type in the yahoo search window it appears in the test case browser
window.

Comment 14

[bsharma]

Verified on Linux and Mac as well and the above bug is reproducible on both of
these platforms as well.

Comment 15

[Jimmy Lee]

Changing Status Whiteboard from [PDT+] to just PDT.

Comment 16

[Nisheeth Ranjan]

-> mitch

Comment 17

[Jaime Rodriguez, Jr.]

Pls provide a ETA in the Status Whiteboard.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Looking at it...

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - always delete event handlers when loading new document, including the first

Comment 20

[Mitchell Stoltz (not reading bugmail)]

The problem is that immediately after a window is opened, the location of the
window is about:blank, and anyone can add an event listener to about:blank. When
the window contents load, its URL and principal change, but the event listeners
remain. Normally, when a new document loads, everything previously added to the
window object gets blown away, except when the previous document is about:blank,
as it is in this case. We had a similar problem with JS watchpoints; now those
are cleared with every new document load, including the first.

The patch above clears event listeners every time; this solves the security
problem. I did some basic precheckin tests, and nothing seems to be broken
internally, but this may break some sites which expect things added to the
window object before the page laods to persist after the page has loaded. We may
be violating a de facto or explicit standard here. Unfortunately, I don't see
any other way of fixing this rather serious security problem. I will get this
reviewed and checked in on the trunk, and see what breaks.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 51360 [details] [diff] [review]
Patch - always delete event handlers when loading new document, including the first

r=heikki for fix on the trunk so we get some test coverage.

In the origibal code JS_ClearScope() was called after removing event listeners; in this new code that might happen. can this cause leaks/state  whackiness or something?

Comment 22

[vidur (gone)]

Comment on attachment 51360 [details] [diff] [review]
Patch - always delete event handlers when loading new document, including the first

sr=vidur

I have concerns about regressions in pages that do expect event handlers set on about:blank to stick around (onload handlers, for example), but I can't think of a reasonable, simple alternative to plugging this hole.

Comment 23

[Jaime Rodriguez, Jr.]

pls stop by the PIT on Monday @ noon for PDT review.

Comment 24

[Johnny Stenback (:jst)]

I share Vidur's concerns about this change, I tried tracking down this change to
figure out if this code was added to fix a specific bug, bug I can't find a
reference to a specific bug.

I'm quite sure this will regress pages that open windows and set onload handlers
to figure out when the first document in the window is done loading. This is
something that can be done in 4.x, but it seems like IE 5 doesn't support this
so maybe it's not such a big deal.

I'm ok with testing this fix out on the trunk for a while, but I wouldn't put
this on the branch w/o letting it sit on the trunk for a few days first.

Comment 25

[Mitchell Stoltz (not reading bugmail)]

I've checked it in on the trunk - how many days do we have before the branch is
done with?

Johnny, what if we deleted all event handlers except onLoad? Allowing onLoad
handlers to persist doesn't reveal much information (other than the load time,
and I can live with that). How difficult would that be, and would it eliminate
some potential problems?

Comment 26

[Johnny Stenback (:jst)]

[from my reply to private email from mstoltz wrt clearing all except onload]

I think that would be a reasonable thing to do for now. I think I now know why
the current code works the way it does, we don't mind if all event handlers are
cleared when we leave about:blank, that's not the real reason for this code. The
reason we don't call RemoveAllListeners() in GlobalWindowImpl::SetNewDocument()
when we're "leaving" about:blank is to make w=window.open(); w.onload=...;
w.document.write(...) work w/o loosing the onload, or any other, event handlers.
IMO we should at some point fix this so that we either know in SetNewDocument()
if we should remove event listeners, or we should make document.open() not call
SetNewDocument()...

If plugging this security hole at this point is important then removing all
event handlers except onload would probably be ok, if it's not all that
important then I'd say leave it as is until we have time to fix the real problem
here.

[end of my reply]

As for how hard writing the code that only clears all except onload I don't know
exactly, it might even be easier to fix this the right way. To fix this the
right way, or a more correct way at least, we'd need a flag to
nsIScriptGlobalObject::SetNewDocument() that would tell us wether or not to
clear event handlers, this would require an API change in a private interface,
but code wise doing that should be trivial.

Comment 27

[Mitchell Stoltz (not reading bugmail)]

I don't understand - under what conditions do we not want to remove event 
handlers in SetNewDocument? Where does the value of that flag come from?

Comment 28

[Johnny Stenback (:jst)]

When GlobalWindowImpl::SetNewDocument() is called from nsHTMLDocument::Open() we
do *not* want to remove event listeners, in all other cases it's ok to remove
them AFAIK.

Comment 29

[Mitchell Stoltz (not reading bugmail)]

Sounds reasonable...and easier than my proposal. If a script calls
document.open(), then it assumes ownership of the document.

Are there any other cases that don't involve window.open which involve setting
event handlers before the first document loads, and which we shouldn't break?
Any other behaviors that script writers count on?

Comment 30

[Mitchell Stoltz (not reading bugmail)]

Attachment: New patch - always delete event listeners in setNewDocument *except* when called from document.open

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 51589 [details] [diff] [review]
New patch - always delete event listeners in setNewDocument *except* when called from document.open

Looks good to me, sr=jst

Comment 32

[Jaime Rodriguez, Jr.]

pls land this on the trunk, and let's look at it again tomorrow for the branch.

Comment 33

[Mitchell Stoltz (not reading bugmail)]

Checked in on the trunk (yesterday - the change is in today's builds). No
problems reported yet. Let's consider putting it on the branch tomorrow.

Comment 34

[Mitchell Stoltz (not reading bugmail)]

Checked in on the branch with verbal PDT+ from Clayton and Jaime.

Comment 35

[bsharma]

Verified on 20001-10-10-Branch build on WinNT

Ran the test case and it works fine.