637248 - Make Event.isTrusted Unforgeable

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Garrett Smith]

The W3C DOM 3 Core draft and D3E drafts both define the isTrusted flag.

http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-event-type-isTrusted
| isTrusted of type boolean, readonly, introduced in DOM Level 3
|
| Used to indicate whether this event was generated by the 
| user agent (trusted) or by script (untrusted). See trusted 
| events for more details.


So the isTrusted flag is readonly, which means scripts can't set it. But look, it's trivial to create a property `isTrusted` on a synthesized event:

var e = document.createEvent("Events");
e.__defineGetter__("isTrusted", function(){return true});
e.initEvent("click", false, false);
document.addEventListener("click", function(ev){alert(ev.isTrusted);}, false);
document.dispatchEvent(e);

Result is true.

Comment 1

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

Why is this security sensitive? If a web page marks the JS wrappers of its own
events trusted, I don't quite see where the problem is.

Comment 2

[Garrett Smith]

Sounds like you think I made a bad call by filing this as security sensitive and maybe I did. But here's why I did that:

I did not want to give away a free tip on "how to open popups in Firefox" so I checked the security bug. 

The problem is that synthesized clicks are used to usurp pop-up blockers. 

When the isTrusted readonly boolean attribute on events is true, that means the event was created by the browser itself, not by any script running on the page.

So then if events with `isTrusted` is true can be interpreted as a browser-generated, hence trustable, popups from that event should be allowed, not blocked, right?

Any site that wants to open popups can set the isTrusted flag to true, rendering that flag meaningless.

Comment 3

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

(In reply to comment #2) 
> So then if events with `isTrusted` is true can be interpreted as a
> browser-generated, hence trustable,
Your code does not change the isTrusted flag of the C++ object.
And browser chrome should use that flag (via right kinds of js wrappers)

But are you saying you can actually use synthesized click events to effectively
disable popup blocker? That certainly would be bug to fix asap.
Do you have a minimal testcase? Please attach it using "Add an attachment".

Comment 4

[Garrett Smith]

Attachment: isTrusted flag set to true -- popup is blocked

Example shows that setting isTrusted cannot be used to usurp popup blocker. So this is not a security issue. Is it a bug at all?

Comment 5

[Johnny Stenback (:jst)]

I think this is invalid, isTrusted is only read by C++ code, which doesn't see JS properties set like this, or by trusted chrome JS, which also doesn't see such properties.

Garret, you made the right call to file this as a security bug, even if it turns out not to be. Better safe than sorry.

Marking INVALID.

Comment 6

[Igor Bukanov]

This a valid bug. 

Consider a website that is explicitly targeted by malware like a bank. Such website may want to do extra measures to protect its users. Detecting synthesized events may be a reasonable short-term strategy for such site. Many malware infections do not have a full control over the site. For example, they can only inject extra JavaScript code to the page without replacing the current code. In such cases defensive programming on the site works.

So it would be very helpful if event.isTrusted cannot be overwritten from JavaScript. As currently the property cannot be trusted, one is forced to use workarounds like using deprecated function.caller to look for JS frames that invoked the event handler.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Please raise a spec issue on http://ie.microsoft.com/testdrive/Performance/MazeSolver/Default.html if you think isTrusted should be [Unforgeable].

Comment 8

[Igor Bukanov]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Please raise a spec issue on
> http://ie.microsoft.com/testdrive/Performance/MazeSolver/Default.html if you
> think isTrusted should be [Unforgeable].

That link goes to a game. Is it the right one?

In any case, the issue is that isTrusted is defined on the prototype of the event object in the same way as other event DOM attributes resulting in a configurable property on the protype. I suppose this can be special-cased in the specs, but a better solution is to replace the attribute with a method on the document (preferably unconfigurable) like document.isTrustedEvent(event). This asserts that the event is a trusted object generated in response to a user action. 

In the mean time as a workaround one can use at least 2 methods to assert the trusted status of an event:

1. Extract the isTrusted getter and call it on the event object:

var getter = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(document.createEvent("MouseEvents")), "isTrusted").get;

getter.call(event)  // This returns false for the above example

2. In the event listener check for arguments.caller. If it is not null, then there is JS code on the stack and presumably the event was simulated.

Comment 9

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

Spec issue should be filed in https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WebAppsWG
Component: DOM (not DOM3 Events)

Adding a method to document sounds pretty ugly, but we could perhaps make
event.isTrusted [Unforgeable]

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That link goes to a game. Is it the right one?

Probably not; I assume I meant a spec link as in comment 9.

> In any case, the issue is that isTrusted is defined on the prototype of the event object 

Right, that's what [Unforgeable] would change: it would define it as a non-configurable accessor property on the event object itself.

> If it is not null, then there is JS code on the stack and presumably the event was
> simulated.

This will presumably stop working with bug 835643, but even now there are lots of trusted events that can fire with JS on the stack.

Comment 11

[Igor Bukanov]

(In reply to Boris Zbarsky (:bz) from comment #10)
> > That link goes to a game. Is it the right one?
>
> This will presumably stop working with bug 835643, but even now there are
> lots of trusted events that can fire with JS on the stack.

Well, this is what one can use in older browsers like MSIE 8 as a workaround. In newer browsers with Function.bind one can dispatch an event while avoiding any JS code on the stack during the listener execution using:

setTimeout(document.dispatchEvent.bind(document, syntheticEvent), 0)

Comment 12

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

(Not really relevant to this bug, but even in that case there can be JS code on stack. Just use showModalDialog)

Comment 13

[Boris Zbarsky [:bzbarsky]]

showModalDialog spins the event loop, which sets aside the JS stack, iirc...

Comment 14

[Igor Bukanov]

(In reply to Olli Pettay [:smaug] from comment #9)
> Spec issue should be filed in
> https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WebAppsWG
> Component: DOM (not DOM3 Events)

Filed bug - https://www.w3.org/Bugs/Public/show_bug.cgi?id=21068

Comment 15

[Igor Bukanov]

(In reply to Igor Bukanov from comment #14)
> Filed bug - https://www.w3.org/Bugs/Public/show_bug.cgi?id=21068

That bug is now fixed and the next draft require isTrusted to be [Unforgeable]

Comment 16

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

Attachment: patch

Assigning to isTrusted works still in instances of inherited interfaces.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 729463 [details] [diff] [review]
patch

r=me

Comment 18

[Igor Bukanov]

Should the test case in the patch also check that

delete Object.getPrototypeOf(w).isTrusted

after e.isTrusted still remains false?

Comment 19

[Boris Zbarsky [:bzbarsky]]

It could, but note that there is no isTrusted on the prototype after this patch.  I suppose the test could just check _that_.

Comment 20

[Igor Bukanov]

(In reply to Boris Zbarsky (:bz) from comment #19)
> It could, but note that there is no isTrusted on the prototype after this
> patch.  I suppose the test could just check _that_.

Yes, that would be nice - it also would give a reliable hint about how to distinguish the current FF versus one with the patch applied.

Comment 21

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

Attachment: +one more test

Comment 22

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

https://tbpl.mozilla.org/?tree=Try&rev=6a2e7042123c

Comment 23

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

But that doesn't compile everywhere because of http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamEvent.webidl

Comment 24

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

For some odd reason self->GetIsTrusted(rv); is generated for JS-implemented events, but
self->IsTrusted(); for C++ implemented.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Mm, presumably we assume that all JS-implemented code can throw... Andrew?

Comment 26

[Andrew McCreight [:mccr8]]

Yeah, we assume everything in a JS-implemented method can throw.

Comment 27

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

Attachment: v3

https://tbpl.mozilla.org/?tree=Try&rev=bdc1069efaa7

Comment 28

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

try still doesn't like the patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Yeah, we assume everything in a JS-implemented method can throw.

Hmm, that breaks down for stuff that actually comes from unforgeables on non-JS-implemented interfaces.  We should fix that...

Comment 30

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

Attachment: patch, v4

This is a bit ugly. Unforgeable is tricky for the current event codegen setup, and couldn't figure out anything simpler.

https://tbpl.mozilla.org/?tree=Try&rev=f44c18d32644

Comment 31

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

Attachment: v5

just assert unforgeability

Comment 32

[Boris Zbarsky [:bzbarsky]]

The real issue with unforgeable is that it inherits down into the descendant interfaces, right?

How about we just flag unforgeable attrs with the interface they originally came from (in the parser), so you can just skip unforgeable stuff that got inherited, not just isTrusted?

Comment 33

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

(In reply to Boris Zbarsky [:bz] from comment #32)
> The real issue with unforgeable is that it inherits down into the descendant
> interfaces, right?
yes
 
> How about we just flag unforgeable attrs with the interface they originally
> came from (in the parser), so you can just skip unforgeable stuff that got
> inherited, not just isTrusted?
Sounds good. I guess I need to dive into the parser code.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Basically, put it in IDLInterface.finish() right before we do the whole "Make sure we don't shadow any of the [Unforgeable] attributes" thing.

Comment 35

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

Attachment: v6

https://tbpl.mozilla.org/?tree=Try&rev=0e30a4aa6e96

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811408 [details] [diff] [review]
v6

I would prefer that we had an originatingInterface on all unforgeable attrs, not just the ones that are on interfaces that have descendants...

r=me with that fixed.

Comment 37

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

Attachment: v7

https://hg.mozilla.org/integration/mozilla-inbound/rev/8918bc282c8a

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8918bc282c8a