[DOGFOOD] addEventListener allows sniffing keystrokes (at least)

VERIFIED FIXED in mozilla0.9.5

Status

()

Core
Security
P1
major
VERIFIED FIXED
18 years ago
15 years ago

People

(Reporter: joro, Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
mozilla0.9.5
x86
Windows 95
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] [ETA 10/3], URL)

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
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);
----------------------------------------------

Updated

18 years ago
Status: NEW → ASSIGNED
Summary: addEventListener allows sniffing keystrokes (at least) → [DOGFOOD] addEventListener allows sniffing keystrokes (at least)

Comment 1

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

Updated

18 years ago
Whiteboard: [PDT+]

Comment 2

18 years ago
Putting on PDT+ radar.

Comment 3

18 years ago
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

18 years ago
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

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

Comment 6

18 years ago
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

Updated

18 years ago
Target Milestone: M12

Comment 7

18 years ago
Dogfood, putting on M12 radar.

Updated

18 years ago
Whiteboard: [PDT+] → [PDT+] Try for 12/3 -- risky

Comment 8

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

Comment 9

18 years ago
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?

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] Try for 12/3 -- risky → [PDT+]

Comment 10

18 years ago
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.

Updated

18 years ago
Blocks: 20203

Updated

18 years ago
QA Contact: junruh → dshea

Updated

18 years ago
Status: RESOLVED → VERIFIED

Comment 11

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

Updated

17 years ago
Component: Security → Security: General

Comment 12

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

Updated

17 years ago
No longer blocks: 20203

Comment 13

16 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 14

16 years ago
Verified on Linux and Mac as well and the above bug is reproducible on both of
these platforms as well.
Keywords: nsbranch+
QA Contact: dshea → bsharma
Target Milestone: M12 → ---

Comment 15

16 years ago
Changing Status Whiteboard from [PDT+] to just PDT.
Whiteboard: [PDT+] → PDT

Comment 16

16 years ago
-> mitch
Assignee: nboyd → mstoltz
Status: REOPENED → NEW

Comment 17

16 years ago
Pls provide a ETA in the Status Whiteboard.
Whiteboard: PDT → [PDT] [ETA ?]
(Assignee)

Comment 18

16 years ago
Looking at it...
Group: security?
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → mozilla0.9.5
(Assignee)

Comment 19

16 years ago
Created attachment 51360 [details] [diff] [review]
Patch - always delete event handlers when loading new document, including the first
(Assignee)

Updated

16 years ago
Whiteboard: [PDT] [ETA ?] → [PDT] [ETA 10/1]
(Assignee)

Comment 20

16 years ago
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 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?
Attachment #51360 - Flags: review+

Comment 22

16 years ago
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.
Attachment #51360 - Flags: superreview+

Comment 23

16 years ago
pls stop by the PIT on Monday @ noon for PDT review.
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.
(Assignee)

Comment 25

16 years ago
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?
[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.
(Assignee)

Comment 27

16 years ago
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?
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.
(Assignee)

Comment 29

16 years ago
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?
(Assignee)

Comment 30

16 years ago
Created attachment 51589 [details] [diff] [review]
New patch - always delete event listeners in setNewDocument *except* when called from document.open
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
Attachment #51589 - Flags: superreview+
Attachment #51589 - Flags: review+

Comment 32

16 years ago
pls land this on the trunk, and let's look at it again tomorrow for the branch.
Whiteboard: [PDT] [ETA 10/1] → [PDT] [ETA 10/3]
(Assignee)

Comment 33

16 years ago
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.
(Assignee)

Comment 34

16 years ago
Checked in on the branch with verbal PDT+ from Clayton and Jaime.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [PDT] [ETA 10/3] → [PDT+] [ETA 10/3]

Comment 35

16 years ago
Verified on 20001-10-10-Branch build on WinNT

Ran the test case and it works fine.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

15 years ago
Group: security?
You need to log in before you can comment on or make changes to this bug.