Make Event.isTrusted Unforgeable

RESOLVED FIXED in mozilla27

Status

()

Core
DOM: Events
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Garrett Smith, Assigned: smaug)

Tracking

(Depends on: 1 bug)

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
(Reporter)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
(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".
(Reporter)

Comment 4

7 years ago
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?
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
Last Resolved: 7 years ago
Resolution: --- → INVALID

Comment 6

5 years ago
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].

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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.

Comment 11

5 years ago
(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)
(Assignee)

Comment 12

5 years ago
(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...

Updated

5 years ago
Depends on: 776864

Comment 14

5 years ago
(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

5 years ago
(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)

Updated

5 years ago
Assignee: nobody → bugs
Summary: Event isTrusted is Trivial to shadow to JS code → Make Event.isTrusted Unforgeable
(Assignee)

Comment 16

5 years ago
Created attachment 729463 [details] [diff] [review]
patch

Assigning to isTrusted works still in instances of inherited interfaces.
(Assignee)

Updated

5 years ago
Depends on: 854836
(Assignee)

Updated

5 years ago
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+

Comment 18

5 years ago
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_.

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
Created attachment 767172 [details] [diff] [review]
+one more test
(Assignee)

Comment 22

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6a2e7042123c
(Assignee)

Comment 23

4 years ago
But that doesn't compile everywhere because of http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamEvent.webidl
(Assignee)

Comment 24

4 years ago
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.
(Assignee)

Comment 27

4 years ago
Created attachment 767330 [details] [diff] [review]
v3

https://tbpl.mozilla.org/?tree=Try&rev=bdc1069efaa7
Attachment #767172 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 921033
(Assignee)

Comment 30

4 years ago
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
Attachment #811296 - Flags: review?(bzbarsky)
(Assignee)

Comment 31

4 years ago
Created attachment 811303 [details] [diff] [review]
v5

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?
(Assignee)

Comment 33

4 years ago
(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.
(Assignee)

Comment 35

4 years ago
Created attachment 811408 [details] [diff] [review]
v6

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+
(Assignee)

Comment 37

4 years ago
Created attachment 811497 [details] [diff] [review]
v7

https://hg.mozilla.org/integration/mozilla-inbound/rev/8918bc282c8a
https://hg.mozilla.org/mozilla-central/rev/8918bc282c8a
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.