257431 - Tabbrowser accepts non-trusted events

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

XBL doesn't allow XUL-nodes to receive non-trusted events, but tabbox installs
listeners in non-XBL components where you don't have these checks anymore. As a
result, an HTML page can send a Ctrl-Tab event that will be processed by the
tabbrowser component.

Comment 1

[Wladimir Palant]

Attachment: Testcase (to be opened in a new tab)

Comment 2

[Daniel Veditz [:dveditz]]

->jst for investigation

Wladimir: is this a spin-off from bug 252326 comment 38? At one point it sounded
like you guys were contemplating fixing the trusted event bugs under that umbrella.

Can you do more damage than switch to (and probably close) tabs? I don't think
this gives you any access to the contents of the other tabs. You could try to
switch focus to yourself and hope the victim was typing something juicy in
another tab at the time.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is there a good reason to have access points for this member on both
nsIDOMNSEvent and nsIPrivateDOMEvent?

Comment 5

[Johnny Stenback (:jst)]

Not really, other than the fact that some people might be using
nsIPrivateDOMEvent outside our codebase... which isn't really much of an excuse.
Forgot to update the IID for nsIDOMNSEvent btw. Let me know if you think I
should go through the code and eliminate this getter from nsIPrivateDOMEvent,
maybe do that for the trunk only, and leave it on the aviary branch just to keep
the regression risk down?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm fine with eliminating the other getter on trunk only; I agree that we
shouldn't touch it on branch.

sr=bzbarsky with that (and the iid change; good catch!)

Comment 7

[Wladimir Palant]

(In reply to comment #2)
> Wladimir: is this a spin-off from bug 252326 comment 38? At one point it sounded
> like you guys were contemplating fixing the trusted event bugs under that
umbrella.

Not quite. The trusted events mentioned there don't seem to be abusable. The
current issue came up when testing for this bug, but this is a proper
non-trusted event that shouldn't be accepted by trusted content.

Comment 8

[Wladimir Palant]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

Argh, this will conflict with my changes for bug 252326. But yes, the patch
looks fine for me with Boris' corrections.

Maybe we want to have a general solution for this in the long term.
EventListenerManager shouldn't be triggering trusted event handlers on
non-trusted events, though I'm not quite sure how this can be implemented.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

could untrusted events override .isTrusted somehow? maybe via __defineGetter__
or somesuch?

Comment 10

[Wladimir Palant]

Tried it - yes, they can.

event.__defineGetter__("isTrusted", function(){return "This is a fake"});

I seem to remember that things like this wouldn't work for events a couple of
versions ago...

Comment 11

[Wladimir Palant]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

Can't have it this way, any attribute or method of the event can be easily
faked.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

we have an XPCNativeWrapper (sp) which is designed to help cases like this.

Comment 13

[Johnny Stenback (:jst)]

Wladimir, did you test that defining a getter for isTrusted actually makes the
patch not work? Unless I missed something that shouldn't be the case since the
event should be re-wrapped when it's accessed in the event handler in the chrome
code, and thus it should get a new fresh JS object that doesn't have the getter
on it.

Comment 14

[Wladimir Palant]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

You are right, I've only tested it with untrusted content. r+ then, with Boris'
corrections.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

Marking bz's sr+

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

Requesting aviary and 1.7 approval.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Aviary patch with IID changed

Comment 18

[Johnny Stenback (:jst)]

Attachment: Trunk patch

Comment 19

[Johnny Stenback (:jst)]

Attachment: Trunk patch

Forgot to diff toolkit.

Comment 20

[Johnny Stenback (:jst)]

Attachment: Trunk patch

Oh, and then there's SeaMonkey too. I think that's is it for now, realy :-)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157634 [details] [diff] [review]
Trunk patch

sr=bzbarsky

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 157585 [details] [diff] [review]
Expose whether an event is trusted or not, and make the tabbrowser code only listen to trusted events.

a=asa for branch checkin.

Comment 23

[Johnny Stenback (:jst)]

Attachment: 1.7 diff for checkin.

Comment 24

[Johnny Stenback (:jst)]

Fixed on aviary and 1.7 branches.

Comment 25

[Johnny Stenback (:jst)]

Attachment: Trunk patch merged to the current trunk

Comment 26

[Johnny Stenback (:jst)]

Fixed.

Comment 27

[Asa Dotzler [:asa]]

I've tested this in the 1.7.3 candidate Windows build on XP (Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910) and it appears to
not be fixed. Either that or I don't understand the testcase. When I open the
testcase in another tab, my two tabs flash back and forth really fast for about
5 seconds. Is that expected after the fix?

Comment 28

[Johnny Stenback (:jst)]

Hmm, looking.

Comment 29

[Johnny Stenback (:jst)]

This was never checked in on the 1.7.2 branch (but it's on the 1.7 branch).

Comment 30

[Daniel Veditz [:dveditz]]

This was not landed on the 1_7_2 branch (for the immanent 1.7.3 security
release), just the 1.7x branch. The 1.7.3 fixes are flagged with "fixed1.7.2+"
in the status whiteboard. (now that the fixed1.7.3 keyword was changed to 1.7.x
I could update the whiteboard text, but that would just generate bugspam.)

The bug allows abuse, but unless we find something worse than swapping your tabs
around it doesn't appear to allow exploits. The patch looks small enough, we
probably could safely take it in 1.7.3 if you want it.

Comment 31

[Frederic Crozat]

Hmm, is there a reason why security flag was never cleared ?