Last Comment Bug 637248 - Make Event.isTrusted Unforgeable
: Make Event.isTrusted Unforgeable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla27
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on: 863954 776864 854836 921033
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-27 22:59 PST by Garrett Smith
Modified: 2013-09-29 02:41 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
isTrusted flag set to true -- popup is blocked (322 bytes, text/html)
2011-03-01 12:55 PST, Garrett Smith
no flags Details
patch (1.59 KB, patch)
2013-03-26 03:53 PDT, Olli Pettay [:smaug]
bzbarsky: review+
Details | Diff | Review
+one more test (1.64 KB, patch)
2013-06-25 06:49 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
v3 (4.17 KB, patch)
2013-06-25 11:00 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
patch, v4 (7.22 KB, patch)
2013-09-27 11:58 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
v5 (7.25 KB, patch)
2013-09-27 12:05 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
v6 (8.86 KB, patch)
2013-09-27 15:21 PDT, Olli Pettay [:smaug]
bzbarsky: review+
Details | Diff | Review
v7 (8.90 KB, patch)
2013-09-28 03:32 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review

Description Garrett Smith 2011-02-27 22:59:24 PST
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] 2011-02-28 03:54:55 PST
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 2011-02-28 14:26:37 PST
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] 2011-03-01 04:15:07 PST
(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 2011-03-01 12:55:39 PST
Created attachment 516000 [details]
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, jst@mozilla.com) 2011-03-02 14:35:25 PST
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 2012-10-01 09:50:04 PDT
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 [:bz] 2012-10-01 09:51:57 PDT
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 2013-02-20 04:38:25 PST
(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] 2013-02-20 04:52:38 PST
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 [:bz] 2013-02-20 05:29:18 PST
> 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 2013-02-20 06:21:12 PST
(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] 2013-02-20 08:55:39 PST
(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 [:bz] 2013-02-20 08:58:12 PST
showModalDialog spins the event loop, which sets aside the JS stack, iirc...
Comment 14 Igor Bukanov 2013-02-21 02:59:48 PST
(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 2013-03-26 02:52:29 PDT
(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] 2013-03-26 03:53:47 PDT
Created attachment 729463 [details] [diff] [review]
patch

Assigning to isTrusted works still in instances of inherited interfaces.
Comment 17 Boris Zbarsky [:bz] 2013-03-26 09:12:17 PDT
Comment on attachment 729463 [details] [diff] [review]
patch

r=me
Comment 18 Igor Bukanov 2013-03-26 10:25:50 PDT
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 [:bz] 2013-03-26 11:28:35 PDT
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 2013-04-02 04:39:16 PDT
(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] 2013-06-25 06:49:02 PDT
Created attachment 767172 [details] [diff] [review]
+one more test
Comment 22 Olli Pettay [:smaug] 2013-06-25 06:52:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6a2e7042123c
Comment 23 Olli Pettay [:smaug] 2013-06-25 07:18:39 PDT
But that doesn't compile everywhere because of http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamEvent.webidl
Comment 24 Olli Pettay [:smaug] 2013-06-25 07:25:17 PDT
For some odd reason self->GetIsTrusted(rv); is generated for JS-implemented events, but
self->IsTrusted(); for C++ implemented.
Comment 25 :Ms2ger 2013-06-25 09:46:09 PDT
Mm, presumably we assume that all JS-implemented code can throw... Andrew?
Comment 26 Andrew McCreight [:mccr8] 2013-06-25 10:02:03 PDT
Yeah, we assume everything in a JS-implemented method can throw.
Comment 28 Olli Pettay [:smaug] 2013-06-25 11:28:00 PDT
try still doesn't like the patch.
Comment 29 Boris Zbarsky [:bz] 2013-06-25 14:43:20 PDT
> 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] 2013-09-27 11:58:08 PDT
Created attachment 811296 [details] [diff] [review]
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] 2013-09-27 12:05:33 PDT
Created attachment 811303 [details] [diff] [review]
v5

just assert unforgeability
Comment 32 Boris Zbarsky [:bz] 2013-09-27 13:01:43 PDT
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] 2013-09-27 13:03:31 PDT
(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 [:bz] 2013-09-27 13:05:48 PDT
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 36 Boris Zbarsky [:bz] 2013-09-27 20:03:13 PDT
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 38 Carsten Book [:Tomcat] 2013-09-29 02:41:28 PDT
https://hg.mozilla.org/mozilla-central/rev/8918bc282c8a

Note You need to log in before you can comment on or make changes to this bug.