Closed Bug 637248 Opened 13 years ago Closed 11 years ago

Make Event.isTrusted Unforgeable

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: dhtmlkitchen, Assigned: smaug, NeedInfo)

References

Details

Attachments

(5 files, 3 obsolete files)

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.
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.
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.
(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".
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?
Group: core-security
Summary: Event isTrusted is Trivial to Set → Event isTrusted is Trivial to shadow to JS code
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Version: 1.9.2 Branch → unspecified
Please raise a spec issue on http://ie.microsoft.com/testdrive/Performance/MazeSolver/Default.html if you think isTrusted should be [Unforgeable].
(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.
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]
> 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.
(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)
(Not really relevant to this bug, but even in that case there can be JS code on stack. Just use showModalDialog)
showModalDialog spins the event loop, which sets aside the JS stack, iirc...
Depends on: 776864
(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
(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]
Assignee: nobody → bugs
Summary: Event isTrusted is Trivial to shadow to JS code → Make Event.isTrusted Unforgeable
Attached patch patchSplinter Review
Assigning to isTrusted works still in instances of inherited interfaces.
Depends on: 854836
Attachment #729463 - Attachment description: patch which doesn't work → patch
Attachment #729463 - Flags: review?(bzbarsky)
Comment on attachment 729463 [details] [diff] [review]
patch

r=me
Attachment #729463 - Flags: review?(bzbarsky) → review+
Should the test case in the patch also check that

delete Object.getPrototypeOf(w).isTrusted

after e.isTrusted still remains false?
It could, but note that there is no isTrusted on the prototype after this patch.  I suppose the test could just check _that_.
(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.
Attached patch +one more test (obsolete) — Splinter Review
For some odd reason self->GetIsTrusted(rv); is generated for JS-implemented events, but
self->IsTrusted(); for C++ implemented.
Mm, presumably we assume that all JS-implemented code can throw... Andrew?
OS: Mac OS X → All
Hardware: PowerPC → All
Yeah, we assume everything in a JS-implemented method can throw.
try still doesn't like the patch.
> 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...
Depends on: 863954
Depends on: 921033
Attached patch patch, v4 (obsolete) — Splinter Review
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
Attachment #811296 - Flags: review?(bzbarsky)
Attached patch v5 (obsolete) — Splinter Review
just assert unforgeability
Attachment #811296 - Attachment is obsolete: true
Attachment #811296 - Flags: review?(bzbarsky)
Attachment #811303 - Flags: review?(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?
(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.
Basically, put it in IDLInterface.finish() right before we do the whole "Make sure we don't shadow any of the [Unforgeable] attributes" thing.
Attached patch v6Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=0e30a4aa6e96
Attachment #811303 - Attachment is obsolete: true
Attachment #811303 - Flags: review?(bzbarsky)
Attachment #811408 - Flags: review?(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.
Attachment #811408 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/8918bc282c8a
Status: REOPENED → RESOLVED
Closed: 13 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: