Intent to Implement and Ship: Event.isTrusted attribute

[Ken Buchanan]

Contact emails

ke...@chromium.org, rby...@chromium.org

Spec

http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events

Summary

Event.isTrusted is an attribute that is true when the event was generated by a user action, and false when the event was created or modified by script, or dispatched via dispatchEvent.

Motivation

This is primarily intended to be used by extensions. Password Alert wants to know whether a given event originated from a script running in the main world, or if it came from the user, and we see this as a reasonable thing for an extension to want to know. We expect it to have limited usefulness on the open web, which is why it had not been implemented to date, because scripts should be able to achieve the same result with using custom attributes.

Compatibility Risk

We don't expect any compatibility problems. Firefox and Internet Explorer implement Event.isTrusted.

Ongoing technical constraints

None.

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes.

OWP launch tracking bug

https://code.google.com/p/chromium/issues/detail?id=334015

Link to entry on the feature dashboard

https://www.chromestatus.com/features/6461137440735232

Requesting approval to ship?

Yes

[Fady Samuel]

Hmm, out of curiosity what does this mean for the android WebView (or any embeddings in an environment where input events can be modified or sent synthetically), and Chrome Custom Tabs?

Fady

[PhistucK]

On Mon, Jun 22, 2015 at 9:36 PM, Ken Buchanan <ke...@chromium.org> wrote:

because scripts should be able to achieve the same result with using custom attributes.

​Can you elaborate?

Are you referring to this?

var event = new Event("hi");

event.isTrusted = true;

window.addEventListener("hi", console.log.bind(console));

window.dispatchEvent(event);

If so, implementing isTrusted should eliminate this, as it should be a read only and non configurable property, right?​

☆PhistucK

[Ken Buchanan]

On Mon, Jun 22, 2015 at 2:46 PM, Fady Samuel <fsa...@chromium.org> wrote:

Hmm, out of curiosity what does this mean for the android WebView (or any embeddings in an environment where input events can be modified or sent synthetically), and Chrome Custom Tabs?

Do you have any specific concerns with those cases? Script-modified events would have isTrusted == false, but wouldn't otherwise have any behavior changed. 

[Ken Buchanan]

Right, the attribute will be marked as Unforgeable, but scripts could still do the same with a differently named attribute.

There have been a couple of discussions about this on a private security team list, and the prevailing opinion is that Unforgeability does not really provide much value in this case, because it seems to imply a security boundary between two scripts running in the same world and context, which doesn't exist. (i.e. you can't have one script that doesn't trust another script loaded from the same page.) Adding it will match the other implementations, though, and the spec requires it to be read-only.

[Fady Samuel]

Chatted with Andrew Grieve offline: there isn't really a way to distinguish between programmatically generated input events and user generated input on Android. I was thinking if that was possible, we could toggle that bit based on whether the input was generated by the user or programmatically. 

Fady

[Rick Byers]

On Mon, Jun 22, 2015 at 3:18 PM, Fady Samuel <fsa...@chromium.org> wrote:

Chatted with Andrew Grieve offline: there isn't really a way to distinguish between programmatically generated input events and user generated input on Android. I was thinking if that was possible, we could toggle that bit based on whether the input was generated by the user or programmatically. 

Personally I don't think we'd want to do this even if we could.  I consider "isTrusted"  to be confusingly named.  Instead, matching the semantics defined by the spec, it should be named something like "isNotDispatchedByScript".  But given that other browsers have shipped this API for awhile and we do believe there's value in exposing this bit, we might as well just use the name everyone else uses here.

[Jochen Eisinger]

Given that this already ships in other browser and gets requested over and over again by developers, I think we should implement this feature as well.

lgtm to implement & ship, but please don't try to rush this into M45

[PhistucK]

On Mon, Jun 22, 2015 at 10:12 PM, Ken Buchanan <ke...@google.com> wrote:

Right, the attribute will be marked as Unforgeable, but scripts could still do the same with a differently named attribute.

I do not follow. A differently name​d attribute would not be set by the browser and known to be trusted. What does that have to do with the purpose of isTrusted?

☆PhistucK

[Ken Buchanan]

To clarify, as it applies to regular web pages being loaded, we don't believe this is meaningfully different. isTrusted does not really convey any kind of security guarantee.

In previous discussions there had been objections to implementing this on the basis that it did not have valid use cases on the open web, because there is no meaningful notion of trust if you have untrusted scripts running in your context. The motivation for implementing it now is that it is useful for event handlers in isolated worlds, which do correspond to real security boundaries.

[PhistucK]

On Mon, Jun 22, 2015 at 11:37 PM, Ken Buchanan <ke...@chromium.org> wrote:

isTrusted does not really convey any kind of security guarantee.

You keep repeating that, but why not?

For example, a script ​that clicks on the "Purchase" button (or whatever button it is that can cost the user some money without even entering any payment details). If the handler is within a self evaluating anonymous function, third party scripts cannot call the handler by themselves, so isTrusted will be meaningful here. The script can even be a content script, for that matter, of a malicious extension.

Anyway, I understand that we are in a disagreement here, or else it would have been implemented much sooner.

Will content script initiated events have isTrusted = true? I hope not.

☆PhistucK

[Adam Barth]

On Mon, Jun 22, 2015 at 1:56 PM, PhistucK <phis...@gmail.com> wrote:

On Mon, Jun 22, 2015 at 11:37 PM, Ken Buchanan <ke...@chromium.org> wrote:

isTrusted does not really convey any kind of security guarantee.

You keep repeating that, but why not?

For example, a script ​that clicks on the "Purchase" button (or whatever button it is that can cost the user some money without even entering any payment details). If the handler is within a self evaluating anonymous function, third party scripts cannot call the handler by themselves, so isTrusted will be meaningful here.

Sadly, it won't be.  The anonymous function is running in a JS environment controlled by an attacker.  No JavaScript property is what it seems.  For example, the attacker can add an isTrusted getter to the MouseEvent prototype that always returns true.  The isTrusted property isn't reliable enough for security decisions.  (IMHO, isTrusted is a bad feature because it will confuse too many people into thinking it has some security value.)

[PhistucK]

Since it is not configurable and it is read only, an attacker should not be able to add that getter.

If this does not apply because the lack of configurability is not inherited from Event, this probably needs to be fixed and defined as non configurable on every Event subclass?

I see that Internet Explorer does not protect this property :( -

var a = document.createEvent("Event")

...

a.initEvent("hi", false, false);

undefined

a.isTrusted

false

a.isTrusted = true

true

a.isTrusted

false

Object.defineProperty(a, "isTrusted", {get: function () { return true}})

...

a.isTrusted

true

What a waste of time.

☆PhistucK

[Fady Samuel]

It looks like Gecko supports a wantsUntrusted parameter on addEventListener: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

This seems like a more robust API.

Fady

[Rick Byers]

There is no way to realistically achieve the kind of security you're looking for in JS PhistucK.  That is the whole reason browsers rely on isolated worlds when they need a JS security boundary.  You'd basically end up having to make almost every API ' [Unforgeable]', and then it's not JS anymore.  AFAICT there is no real debate on this point in the browser security community (please see all the previous threads on isTrusted), so please let's not pollute this intent thread further with this debate (you can either trust the security experts like Ken or start a new thread elsewhere - eg. maybe securi...@chromium.org).

The only real question here is given that JS doesn't have any meaningful notion of 'trust' within a single world, does implementing isTrusted in blink provide enough benefit to justify the confusion it raises.  I know Mozilla (and I'm sure also IE) understood that isTrusted doesn't actually provide any 'trust' properties when they implemented it.  My opinion is that the time for this debate was when this API was being spec'd and before it shipped in one of the major browsers.  Now that it's part of the web platform (for better or worse), and given there are legitimate use cases for it, I support just following the other vendors.

But if we thought there was a credible path to remove this confusing API from the web platform, then I'd also support solving the use cases with a new better-named API.

Rick

[Ken Buchanan]

On Mon, Jun 22, 2015 at 5:01 PM, Adam Barth <aba...@chromium.org> wrote: 

(IMHO, isTrusted is a bad feature because it will confuse too many people into thinking it has some security value.)

I would prefer it had a better name, but I also don't think it will do any harm. Ultimately, sites vulnerable to XSS are still vulnerable to XSS, with or without people checking isTrusted on their events.

[TAMURA, Kent]

LGTM to ship.  It improves compatibility.

--

TAMURA Kent
Software Engineer, Google

[Boris Zbarsky]

On 6/22/15 2:01 PM, Adam Barth wrote:
> For example, the attacker can add an isTrusted getter to the
> MouseEvent prototype that always returns true.

The isTrusted property is specified to be a non-configurable own
property on Event objects. So if you just do event.isTrusted you will
get the right value no matter what someone has done with
MouseEvent.prototype.... as long as "event" is an actual Event instance.

Now guaranteeing _that_ is what might be hard. For example, someone
might have swapped out EventTarget.prototype.addEventListener out from
under you, and is now feeding you totally made-up objects instead of
actual Event instances.

-Boris

[Adam Barth]

On Tue, Jun 23, 2015 at 1:12 AM Boris Zbarsky <bzba...@mit.edu> wrote:

On 6/22/15 2:01 PM, Adam Barth wrote:
> For example, the attacker can add an isTrusted getter to the
> MouseEvent prototype that always returns true.

The isTrusted property is specified to be a non-configurable own
property on Event objects.  So if you just do event.isTrusted you will
get the right value no matter what someone has done with
MouseEvent.prototype.... as long as "event" is an actual Event instance.

Will that security property continue to hold once ECMAScript lets you subclass host objects?  If not, does that mean isTrusted will prevent future authors from subclassing Event?

Adam

[Boris Zbarsky]

On 6/23/15 8:25 AM, Adam Barth wrote:
> Will that security property continue to hold once ECMAScript lets you
> subclass host objects?

I don't see why it wouldn't.

> If not, does that mean isTrusted will prevent future authors from
> subclassing Event?

Subclassing Event should work fine. All that happens, conceptually, is
that the Event constructor, when called, defines a non-configurable
property on the object it's created before returning it.

The important thing for this to work is that the Event object not be
exposed to script after it's been allocated but before the constructor
has had a chance to define this non-configurable property. But the
subclassing setup in ES6 provides such a guarantee, as far as I can
tell, so we should be ok.

-Boris

P.S. I agree with your larger point, though: unless your code is
extension code or has locked down addEventListener or other globals via
things like SES or whatnot, you lose simply by not being able to ensure
your event objects are actual Event objects.

[Ken Buchanan]

This thread has 2 LGTMs. Are there any other owners who would be okay with this, or else have concerns they want discussed further?

[Philip Jägenstedt]

I'm leaning towards thinking that we should do this, but have some questions.

First, I take it https://dom.spec.whatwg.org/#dom-event-istrusted is what you'll try to implement, not http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events?

Per spec, https://dom.spec.whatwg.org/#concept-event-fire is the only place that sets isTrusted to true, but in Blink I don't think there is a common code path for firing events that is taken for all internally created events but never for script-created events. How will isTrusted be implemented? I strongly doubt that all specs actually refer to "fire an event named e" when firing events, so to implement it pedantically would result in some pretty confusing mess.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Rick Byers]

On Mon, Jun 29, 2015 at 8:14 AM, Philip Jägenstedt <phi...@opera.com> wrote:

I'm leaning towards thinking that we should do this, but have some questions.

First, I take it https://dom.spec.whatwg.org/#dom-event-istrusted is what you'll try to implement, not http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events?

The latter is defined in terms of http://www.w3.org/TR/dom/ which is just a snapshot of the former, so the semantics should be the same.  I'm not aware of any relevant differences, but please let me know if I've missed something!

Per spec, https://dom.spec.whatwg.org/#concept-event-fire is the only place that sets isTrusted to true, but in Blink I don't think there is a common code path for firing events that is taken for all internally created events but never for script-created events. How will isTrusted be implemented? I strongly doubt that all specs actually refer to "fire an event named e" when firing events, so to implement it pedantically would result in some pretty confusing mess.

It's a good point that this is going to take some care, but from initial experience with identifying MouseEvents dispatched from script, I don't expect it to be a huge issue.  EventTarget::dispatchEvent is (AFAIK) the only script entrypoint for dispatching events, so we can focus on ensuring we always set isTrusted to false there and that we never call it internally (a few fixes for that were needed here).  We should probably take some extra steps in review to minimize the risk of mistakes (eg. rename some methods / add comments to clarify the different paths).

[Philip Jägenstedt]

On Mon, Jun 29, 2015 at 2:55 PM, Rick Byers <rby...@chromium.org> wrote:

On Mon, Jun 29, 2015 at 8:14 AM, Philip Jägenstedt <phi...@opera.com> wrote:

I'm leaning towards thinking that we should do this, but have some questions.

First, I take it https://dom.spec.whatwg.org/#dom-event-istrusted is what you'll try to implement, not http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events?

The latter is defined in terms of http://www.w3.org/TR/dom/ which is just a snapshot of the former, so the semantics should be the same.  I'm not aware of any relevant differences, but please let me know if I've missed something!

What the UI Events spec says seems mostly superfluous on top of DOM, so I'm wondering if anything in it will inform implementation. In particular, this bit looks like something we might regret: "Most untrusted events should not trigger default actions, with the exception of the click event. This event always triggers the default action, even if the isTrusted attribute is false (this behavior is retained for backward-compatibility)."

The way it's phrased in the UI Events spec assumes that default actions are a feature of the event and its event target, which is how it's actually implemented with defaultEventHandler(). However, I think Anne's ambition is that default actions should always be taken by the code that dispatches the event. This is the topic of a long-winded spec bug:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230

Now, the exemption for click events was implemented in https://codereview.chromium.org/894913002/ but I wonder if adding isTrusted would change anything around this? It would be nice to have use counters to see if this case is actually required for web compat. (Note that the click() method is different, as the default action could be run by click() itself.)

Per spec, https://dom.spec.whatwg.org/#concept-event-fire is the only place that sets isTrusted to true, but in Blink I don't think there is a common code path for firing events that is taken for all internally created events but never for script-created events. How will isTrusted be implemented? I strongly doubt that all specs actually refer to "fire an event named e" when firing events, so to implement it pedantically would result in some pretty confusing mess.

It's a good point that this is going to take some care, but from initial experience with identifying MouseEvents dispatched from script, I don't expect it to be a huge issue.  EventTarget::dispatchEvent is (AFAIK) the only script entrypoint for dispatching events, so we can focus on ensuring we always set isTrusted to false there and that we never call it internally (a few fixes for that were needed here).  We should probably take some extra steps in review to minimize the risk of mistakes (eg. rename some methods / add comments to clarify the different paths).

Setting isTrusted in dispatchEvent() would be observably different from what the spec says, though. The simplest thing would be if isTrusted has the correct value from time the event is created and never changes, but that is also the most complicated fix because there's no split between internal and external ways to create events...

Philip

[Rick Byers]

On Mon, Jun 29, 2015 at 5:39 PM, Philip Jägenstedt <phi...@opera.com> wrote:

On Mon, Jun 29, 2015 at 2:55 PM, Rick Byers <rby...@chromium.org> wrote:

On Mon, Jun 29, 2015 at 8:14 AM, Philip Jägenstedt <phi...@opera.com> wrote:

I'm leaning towards thinking that we should do this, but have some questions.

First, I take it https://dom.spec.whatwg.org/#dom-event-istrusted is what you'll try to implement, not http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events?

The latter is defined in terms of http://www.w3.org/TR/dom/ which is just a snapshot of the former, so the semantics should be the same.  I'm not aware of any relevant differences, but please let me know if I've missed something!

What the UI Events spec says seems mostly superfluous on top of DOM, so I'm wondering if anything in it will inform implementation. In particular, this bit looks like something we might regret: "Most untrusted events should not trigger default actions, with the exception of the click event. This event always triggers the default action, even if the isTrusted attribute is false (this behavior is retained for backward-compatibility)."

The way it's phrased in the UI Events spec assumes that default actions are a feature of the event and its event target, which is how it's actually implemented with defaultEventHandler(). However, I think Anne's ambition is that default actions should always be taken by the code that dispatches the event. This is the topic of a long-winded spec bug:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230

I think this issue is really orthogonal to whether or not we expose an isTrusted API.  We've already been trying to move closer to the spec here (since it's how IE and Firefox behavior), that's what this in-progress CL is about.  That might be a good discussion for blink-dev as well (eg. I have raised some concerns about the extensibility implications of the design as spec'd), but other than the overlapping spec language I think it's entirely decoupled from the isTrusted API.

Now, the exemption for click events was implemented in https://codereview.chromium.org/894913002/ but I wonder if adding isTrusted would change anything around this? It would be nice to have use counters to see if this case is actually required for web compat. (Note that the click() method is different, as the default action could be run by click() itself.)

Per spec, https://dom.spec.whatwg.org/#concept-event-fire is the only place that sets isTrusted to true, but in Blink I don't think there is a common code path for firing events that is taken for all internally created events but never for script-created events. How will isTrusted be implemented? I strongly doubt that all specs actually refer to "fire an event named e" when firing events, so to implement it pedantically would result in some pretty confusing mess.

It's a good point that this is going to take some care, but from initial experience with identifying MouseEvents dispatched from script, I don't expect it to be a huge issue.  EventTarget::dispatchEvent is (AFAIK) the only script entrypoint for dispatching events, so we can focus on ensuring we always set isTrusted to false there and that we never call it internally (a few fixes for that were needed here).  We should probably take some extra steps in review to minimize the risk of mistakes (eg. rename some methods / add comments to clarify the different paths).

Setting isTrusted in dispatchEvent() would be observably different from what the spec says, though. The simplest thing would be if isTrusted has the correct value from time the event is created and never changes, but that is also the most complicated fix because there's no split between internal and external ways to create events...

The spec requires that JS-created events have isTrusted set to false on creation AND by dispatchEvent (eg. you can't re-dispatch a real mouse event and still have it be considered trusted).  It's the latter that's the primary security property IMHO (since it's the primary way events would be shared between isolated worlds).  Of course we'd also follow the spec in terms of setting isTrusted to false from the JS-invoked event constructors, but I don't think we need the same level of extra care here - it's not a security hole if we miss a place.

Philip

[Philip Jägenstedt]

I agree that it is orthogonal, but it's what
http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events is about so I
wanted to make sure we're definitely only talking about exposing
isTrusted as per https://dom.spec.whatwg.org/, not using isTrusted
internally to change any existing behavior? (Not we shouldn't ever,
just for clarity.)

Oh, you're right, isTrusted is always initialized to false and then
there are two dispatch paths, one for scripts, dispatchEvent(), and
one internal, "fire an event named e."

Things aren't that cleanly separated internally, but it's a matter for
the reviewer to make sure this is correct and doesn't invite mistakes.

LGTM3, with the recognition that this doesn't solve a security problem
between scripts in the same page, but it's already shipped elsewhere
and doesn't seem broken or confusing enough to try to change the spec
and all other browsers.

Philip

[Rick Byers]

Yep, thank you for clarifying! 

Yeah, still my problem ;-)

LGTM3, with the recognition that this doesn't solve a security problem
between scripts in the same page, but it's already shipped elsewhere
and doesn't seem broken or confusing enough to try to change the spec
and all other browsers.

Great, thanks!  The tradeoffs here are definitely non-obvious and debatable.  I agree that better interoperability is a strong reason to just ship this as spec'd.

Philip