1330113 - X-ray wrappers share event listener attributes with untrusted code

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Igor Kononuchenko]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

1. In WebExtension code (you have to install simple web extension, and in content script) write: 
```
window.onerror = function() {
  console.log('error')
}
```
2. Open console and write: `window.onerror` then write `window.onerror.something`




Actual results:

It equals function. And then when you execute `window.onerror.something` you will get an exception. 


Expected results:

`window.onerror` should be `undefined` by default

Comment 1

[Andy McKay]

Luca to reproduce and investigate.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

I tried to reproduce it locally and start looking into it.

Follows some additional detail about this issue.

a "security wrapped function object " is assigned to onerror property, the web content javascript cannot call it (and errors are logged in the console), but it is nevertheless defined (which is unexpected).

by looking a bit more deeply in it, we can notice that:

- if from the content script we try to assign a function to a non existent window property, from the web content that new property is still undefined (as expected),

- if the property is an existent attribute defined in the WebIDL (e.g. onload, onerror, oncontextmenu, etc), the property value is set to the wrapped function (which cannot be used and it will raise exception if we try to call it from the web content).

This issue doesn't look like a "WebExtensions specific" issue, but it seems to be valid for any non-content javascript code that tries to do the same. 

Ideally, what it should happen here is that:

- the value assigned from the addon to the WebIDL attribute should be visible only from the non-content javascript code;

- similarly, if it is the web content to set a value on an existent WebIDL attribute, from the content script we should get an undefined value when we try to access the related property on the "security wrapped window" (while currently, the content script gets the value as assigned from the web content).

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

NOTE: as a workaround to the particular scenario described in Comment 0, from a content script it would be better to subscribe the error event using the addEventListener method (which should behave correctly on both the browsers).

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Kris, do you know if there is any existent plans for implementing something similar to the behavior described in the last part of Comment 2?

Comment 5

[Kris Maglione [:kmag]]

This is something we could implement, but I'm not sure whether it's entirely desirable. Or if it's worth the effort, given that `addEventListener` already handles this correctly. But I suppose it is a bit of a footgun for people who aren't expecting it.

Comment 6

[Andrew Overholt [:overholt]]

So the console is running in the same context as the Web Extension? Is this intentional?

Comment 7

[Kris Maglione [:kmag]]

No, the extension code is running in a Sandbox that inherits from the content window. The console code is running in the context of the content window that it inherits from. The problem is less for code executed in the console, though as for code executed from page scripts.

Comment 8

[Andrew Overholt [:overholt]]

Thanks for the explanation, Kris! I'll set the priority here as "backlog (aka P3)" but am happy to change that if it's incorrect.

Comment 9

[Kris Maglione [:kmag]]

That's OK with me.

Comment 12

[sami.vanttinen]

Would it be possible to add a detection for this to the Addon review process? At least then the developers would be notified not to use events directly, but to implement addEventListener() instead.