trusted event reinitialization allows data-theft

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: shutdown, Assigned: jst)

Tracking

({fixed-aviary1.0.5, fixed1.7.9})

Trunk
x86
Windows 98
fixed-aviary1.0.5, fixed1.7.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +
blocking1.8b3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: 

Mozilla has the special "trusted" flag on event objects, and uses it to
deny untrusted events to do some special operations. So untrusted
events raised by web pages cannot do clipboard copy, paste, etc...

However, malicious page can hijack trusted events by reinitializing
such events using DOM initEvent() family in the event handlers,
and can perform limited subset of the privileged operations.

Reproducible: Always

Steps to Reproduce:
1. open the testcase.
2. press "alt-a" key.
Actual Results:  
trusted events can be reinitialized by initEvent() family.

Expected Results:  
initEvent() family during dispatching-phase must be ignored.
So, trusted events cannot be hijacked by web pages.

http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-initEvent
According to DOM Level2 Events Spec, initEvent() family
called during event dispatching-phase must be ignored.
(Reporter)

Comment 1

13 years ago
Created attachment 185396 [details]
testcase

testcase: clipboard text and local file can be stealed.

works on firefox 1.0.4 and firefox trunk 2005060423.
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050604 Firefox/1.0+
Assignee: events → jst
Blocks: 256195
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b3+
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
Whiteboard: [sg:fix]
(Assignee)

Comment 2

13 years ago
Fix coming up.
(Assignee)

Comment 3

13 years ago
Created attachment 185875 [details] [diff] [review]
Prevent events from ever being initialized or dispatched more than once.

There may be a more efficient way to do this, with less code even. But I'm not
sure where that would be to catch all cases, so for now this is probably the
safest approach. Alternatively I guess we could move the code that marks events
as being dispatched already into nsEventListenerManager::HandleEvent() as I
*think* that all event handler invocations *always* go through there. And if no
handler is ever called, there's no *real* reason to mark the event since no
untrusted code could ever get a reference to the event (unless it was created
by script, in which case it'll be properly marked trustes/untrusted anyways).

Thoughts?
Attachment #185875 - Flags: superreview?(dveditz)
Attachment #185875 - Flags: review?(bzbarsky)
I don't see why you shouldn't be able to dispatch an event twice. If the event
is already being dispatched, then dispatching it again should raise
DISPATCH_REQUEST_ERR. Once an event has been passed to the dispatchEvent method,
 then calling initEventXXX() on it should do nothing.

Once a trusted event has finished being dispatched, its trusted flag should be
reset, so that if it is redispatched, it doesn't do any damage.

Beyond that I see no reason to stop redispatching or reinitialising.
Comment on attachment 185875 [details] [diff] [review]
Prevent events from ever being initialized or dispatched more than once.

It's going to take me a day or two to get to this.... If I don't by the 19th,
though, then I won't be able to until July.

Comment 6

13 years ago
If bz can't get to this soon, can anyone else jump in here and get this fixed
for 1.0.5/1.7.9?
Comment on attachment 185875 [details] [diff] [review]
Prevent events from ever being initialized or dispatched more than once.

>-#define NS_PRIV_EVENT_UNTRUSTED_PERMITTED 0x0800
>+#define NS_EVENT_FLAG_DISPATCH_STARTED    0x0800
>+#define NS_PRIV_EVENT_UNTRUSTED_PERMITTED 0x1000

bump the untrusted define to 0x8000 so we don't move it around each time we add
a new flag. (Potentially breaks existing add-ons otherwise binary compatible
with unchanged interfaces.)

sr=dveditz
Attachment #185875 - Flags: superreview?(dveditz) → superreview+
Whiteboard: [sg:fix] → [sg:fix] need r=, a=, landing
Attachment #185875 - Flags: review?(bzbarsky) → review?(bugmail)
(Assignee)

Comment 8

13 years ago
Created attachment 186958 [details] [diff] [review]
Same as above but unmark the dispatch bit when we're done dispatching an event, and permit re-dispatching of DOM events (but reset trusted flags if the caller isn't trusted).

To be on the safe side, this makes is so that re-dispatching an event still
works as long as the event isn't in the process of being dispatched already.
This also makes sure that if an event is re-dispatched and the event is
trusted, it makes sure the caller is also trusted. If not, it unsets the
trusted bit when re-initializing the event. This code piggy-backs on the
NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY bit to know that an event has already
been dispatched, the bit gets set when we leave the DOM event loop, and it's
checked when dispatching an event.

diff -w coming up.
(Assignee)

Comment 9

13 years ago
Created attachment 186960 [details] [diff] [review]
diff -w of the above for review.

This should fix this bug while letting people reuse events in a safe way.
Attachment #186960 - Flags: superreview?(bryner)
Attachment #186960 - Flags: review?(dveditz)
(Assignee)

Comment 10

13 years ago
*** Bug 292943 has been marked as a duplicate of this bug. ***
Attachment #186960 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

13 years ago
Attachment #185875 - Attachment is obsolete: true
Attachment #185875 - Flags: superreview+
Attachment #185875 - Flags: review?(bugmail)
Comment on attachment 186960 [details] [diff] [review]
diff -w of the above for review.

r=dveditz
a=dveditz for branches and trunk
Attachment #186960 - Flags: review?(dveditz)
Attachment #186960 - Flags: review+
Attachment #186960 - Flags: approval1.8b3+
Attachment #186960 - Flags: approval1.7.9+
Attachment #186960 - Flags: approval-aviary1.0.5+
Whiteboard: [sg:fix] need r=, a=, landing → [sg:fix] need landing
(Assignee)

Comment 12

13 years ago
Created attachment 186981 [details] [diff] [review]
Aviary and mozilla1.7 branch version of the above
(Assignee)

Comment 13

13 years ago
Fixed on trunk and aviary branches. There are some differences between 1.7 and
aviary branches that make this hard to apply there, need to figure that out
before landing on the 1.7 branch.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0.5
Resolution: --- → FIXED

Comment 14

13 years ago
dveditz and jst need to discuss any discreprencies with their checkins on the
1.7 branch to get this patch checked in there.

Comment 15

13 years ago
Comment on attachment 186981 [details] [diff] [review]
Aviary and mozilla1.7 branch version of the above

Per 1.0.5 meeting, needs check-in by dveditz.
Attachment #186981 - Attachment description: Aviary version of the above. → Aviary version of the above. [needs check-in]

Updated

13 years ago
Whiteboard: [sg:fix] need landing → [sg:fix] needs landing by dveditz
Checked into mozilla 1.7 branch for jst
Keywords: fixed1.7.9
Whiteboard: [sg:fix] needs landing by dveditz → [sg:fix]
Attachment #186981 - Attachment description: Aviary version of the above. [needs check-in] → Aviary and mozilla1.7 branch version of the above
Don't early returns on various errors in HandleDOMEvent methods mean we won't
remove the "dispatching" flag from some events?
Depends on: 289940
(Assignee)

Comment 18

13 years ago
Created attachment 188994 [details] [diff] [review]
Changes missing from the branches.
Attachment #188994 - Flags: superreview?(dveditz)
Attachment #188994 - Flags: review?(dveditz)
(Assignee)

Comment 19

13 years ago
Created attachment 188995 [details] [diff] [review]
Missed one part of the missing code.

This unsets the NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY bit when
re-initializing events to make them dispatchable again. Dveditz found this part
was missing.
Attachment #188994 - Attachment is obsolete: true
Attachment #188995 - Flags: superreview?(dveditz)
Attachment #188995 - Flags: review?(dveditz)
(Assignee)

Updated

13 years ago
Attachment #188994 - Flags: superreview?(dveditz)
Attachment #188994 - Flags: review?(dveditz)
Comment on attachment 188995 [details] [diff] [review]
Missed one part of the missing code.

r/sr=dveditz
Attachment #188995 - Flags: superreview?(dveditz)
Attachment #188995 - Flags: superreview+
Attachment #188995 - Flags: review?(dveditz)
Attachment #188995 - Flags: review+
(Assignee)

Comment 21

13 years ago
Missing changes landed on the aviary *and* 1.7 branches now.

Comment 22

13 years ago
v.fixed on Aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050711 Firefox/1.0.5 using attached testcase.  Alt-A no longer
compromises your clipboard.
Adding distributors
FF1.0.5 advisories published
Group: security

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.