821809 - run jetpack content scripts with nsExpandedPrincipal

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Splitting this off from the discussion in bug 821521 comment 11.

bsmedberg says that self.port is sensitive. If so, this is a problem, because jetpack runs with content principals. It's shielded by default by giving the jetpack compartment wantXrays, which means that it gets Xray wrappers even for same-origin access. But if the content script ever waives Xray vision to place an expando object on the page, it's effectively granting access to content.

suppose the content script does:

contentWindow.wrappedJSObject.foo = {};

Content can then do:

var port = (Object.getPrototypeOf(foo).constructor.constructor)('return self.port')();

Effectively, we traverse {} -> Object.prototype -> Object -> Function, at which point we can create a new function in the content script's scope, and return self.port.

Comment 1

[Bobby Holley (:bholley)]

Note that this is all just theoretical, I haven't tested it. But from what I know about how this  stuff works, this is what will happen.

What do you think, gabor? More priority to my suggestion to make jetpack use nsEP? ;-)

Comment 2

[Alexandre Poirot [:ochameau]]

We know that. Content script can be unsafe when using either wrappedJSOBject or the global object unsafeWindow. For now, we are warning about that unsafe practice.
We are discussing about getting rid of unsafeWindow and it will be only meaningfull if we get rid of these wrappedJSObject attributes as well.

One idea behind the pipe mechanism used behind `Port` object is that, in case of security leak, you don't get full set of priviledges but only what Port listeners do in addon module.

Don't get me wrong here, I'm not saying we should not improve content scripts. I just tried to draw the current state of them.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> We know that. Content script can be unsafe when using either wrappedJSOBject
> or the global object unsafeWindow. For now, we are warning about that unsafe
> practice.
> We are discussing about getting rid of unsafeWindow and it will be only
> meaningfull if we get rid of these wrappedJSObject attributes as well.

Well, an alternative would be what I suggest - give content scripts an nsEP (or something similar) that subsumes the content (and nothing else), but not the reverse. We'd then probably need to expand our usage of the COW machinery so that __exposedProps__ would work for content scripts too (or alternatively, we could just declare that content scripts aren't allowed to expose objects to content).

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #1)
> What do you think, gabor? More priority to my suggestion to make jetpack use
> nsEP? ;-)

I would really like to land that patch, and when it's working I like the idea of using nsEP always for consistency as well. Unfortunately because of some contract changes I'm on holiday for the rest of this year, which probably will not spped things up... 

 (In reply to Alexandre Poirot (:ochameau) from comment #2)
> We are discussing about getting rid of unsafeWindow and it will be only
> meaningfull if we get rid of these wrappedJSObject attributes as well.

Please don't do that. There are situations where we need it. Informing add-on developers about the risk they take by using it is mandatory (I've just released a blog article about this btw). And telling reviewers to allow it only in special cases is fine too. But for example, if your add-on attaches content-script to only very trusted sites. Like if you want to extend your own site with an add-on it should be possible to enable a more direct interaction between the add-on and the site, like accessing globals... in that case self.port is not a big threat either. Or if you want to attach script to gmail and yahoo mail only, it is very unlikely that gmail will abuse a security leak of an add-on, but accessing googles API would be important. For content script that is attached to any sites... probably we need some restrictions there.

(In reply to Bobby Holley (:bholley) from comment #3)
> Well, an alternative would be what I suggest - give content scripts an nsEP
> (or something similar) that subsumes the content (and nothing else), but not
> the reverse. We'd then probably need to expand our usage of the COW
> machinery so that __exposedProps__ would work for content scripts too (or
> alternatively, we could just declare that content scripts aren't allowed to
> expose objects to content).

The nice thing about using nsEP always, that we could very easily identify add-ons and create custom wrappers without affecting anything else. So I really really like this approach in general. There are two cases. The one when content-script want to expose something to content, which should be really rare, so __exposedProps__ kind of thing should be OK imo. And there is the other way around the wrappedJSOBject thingy. I would keep it as it is. But this would be an add-on breaking change, and last time (the other __exposedProps__ related change) that didn't go very smoothly... Is that patch landed even? Alex: how difficult do you think it would be to land something like this without breaking add-ons? And when can you come back to jetpack finally? :)

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Gabor Krizsanits [:krizsa :gabor] (out until 2013) from comment #4)
> 
>  (In reply to Alexandre Poirot (:ochameau) from comment #2)
> > We are discussing about getting rid of unsafeWindow and it will be only
> > meaningfull if we get rid of these wrappedJSObject attributes as well.
> 
> Please don't do that. There are situations where we need it.

I think there is some tweaks to be done without preventing addon authors to play with website JS. Here is some ideas:
- avoid exposing wrappedJSObject at all, only expose unsafeWindow but only when an attribute is set on page-mod in order to ask for JS access.
- add a new content script flag in order to evaluate it directly in website scope, so that you don't even have to use unsafeWindow. You can just do window.GmailGreasemonkeyAPI.dosomething()
- ...

The important point is that I'd really like to see JS value access be disabled by default. But I still want to expose an easy JS value access, I really don't want to tell people to pipe messages between the webpage and the content script in order to send pipe messages to the addon :o That's what is suggested for chrome addons, and what irakli suggested last time we talked about that: using eval or inject script tag from the content script and communicate through DOM event (postMessage or custom events)

> The nice thing about using nsEP always, that we could very easily identify
> add-ons and create custom wrappers without affecting anything else.

What are those nsEP ???

> Is that
> patch landed even? Alex: how difficult do you think it would be to land
> something like this without breaking add-ons? And when can you come back to
> jetpack finally? :)

I'm not sure the patch landed, but that's only to avoid breaking addons still using old jetpack version. I immediatly fixed SDK master when the patch was landed on m-c and made our test fail!
Otherwise I don't undestand what is the "land something like this without breaking add-ons"?

Comment 6

[Benjamin Smedberg]

The use case that I think we should try to preserve is addon authors prototyping new DOM APIs by making content-visible properties, either on the DOM prototypes or on individual objects such as <window>.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] (out until 2013) from comment #4)
> The nice thing about using nsEP always, that we could very easily identify
> add-ons and create custom wrappers without affecting anything else.

I don't want to make nsEP a jetpack-only construction. I think it's generally useful. For example, I plan on using it for my XBL rewrite.

Luke and I are mulling a different API than __exposedProps__, but the practical effect will be the same: content script objects are opaque to content unless they are explicitly exposed. I doubt this will break very much if anything.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> What are those nsEP ???

nsExpandedPrincipal the principal with an array of origins.

> I'm not sure the patch landed, but that's only to avoid breaking addons
> still using old jetpack version. I immediatly fixed SDK master when the
> patch was landed on m-c and made our test fail!
> Otherwise I don't undestand what is the "land something like this without
> breaking add-ons"?

So if my addon was exposing some object to content (unsafeWindow.blah = {myProp: 3}) and we land a patch that requires the content-script to specify which properties of that exposed object should be accessible (something like __exposedProps__) then the add-on will be broken right? Since, window.blah.myProp will not be visible by default from content any longer. And this is not something we can fix on SDK side. I'm not sure that this should block us from improving security here, I'm just saying that we need to address this issue.

(In reply to Bobby Holley (:bholley) from comment #7)
> I don't want to make nsEP a jetpack-only construction. I think it's
> generally useful. For example, I plan on using it for my XBL rewrite.

Sounds cool. We can flag the compartment of add-ons at creation time if it's needed.

> 
> Luke and I are mulling a different API than __exposedProps__, but the
> practical effect will be the same: content script objects are opaque to
> content unless they are explicitly exposed. I doubt this will break very
> much if anything.

Probably there won't be many add-ons affected, but I'm really not the person who could tell. Alex probably knows it better, if the case I mentioned above is common/rare/non-exiting.

Comment 9

[Andrew McCreight [:mccr8]]

Any thoughts on a security rating for this?

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Gabor Krizsanits [:krizsa :gabor] (out until 2013) from comment #8)
> Probably there won't be many add-ons affected, but I'm really not the person
> who could tell. Alex probably knows it better, if the case I mentioned above
> is common/rare/non-exiting.

Here is some examples of usage of unsafeWindow (equivalent of window.wrappedJSOBject) in order to expose content script values to the webpage:

* BugzillaJS
https://github.com/gkoberger/BugzillaJS/blob/master/includes/jsonp.js#L33-L41

They are also using unsafeWindow for localStorage (most likely due to issue with old javascript proxy layer on top of xrays), and to change location (may be a proxy issue too).

But the JSONP usage is very interesting, I don't think we can do JSONP without using wrappedJSObject, nor exposing values to the webpage?

* Openwebapp
In order to expose an API:
https://github.com/mozilla/openwebapps/blob/develop/addons/jetpack/data/mgmtapi.js#L111

* Flightdeck
For an API too:
https://github.com/mozilla/addon-builder-helper/blob/master/lib/addons-builder-helper.js#L250-L267

* Grooveshark remote
They have to expose a function to the webpage in order for it to be called by the webpage API that expect a function name and not a reference:
https://github.com/peregrinogris/Grooveshark-Remote-Control/blob/master/data/pageMod.js#L46-L52


Otherwise there is way more simplier usages of unsafeWindow just to call or access webpage values. Like all addons calling audio/video players (youtube, grooveshark, ...).

Exposing JS values to the webpage isn't a very popular practice, *but* it tends to be used by popular addons and they are doing mainly because they have to because that's the only way to implement addon feature.
Having said that, I totally agree in the need to improve default security. It would just be cool to deprecate and send warning first, then deprecate. If that's not an option, we can also communicate to addon author before the change and be able to suggest better and easier way to implement their features.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Any thoughts on a security rating for this?

Well, it depends who you ask. Apparently this is a known issue and the answer is "addons shouldn't waive Xrays unless they're interacting with a trusted page". I personally think that isn't really good enough - people are going to waive Xrays when they want to look at the page, so I think we need to solve this at the platform level.

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> Having said that, I totally agree in the need to improve default security.
> It would just be cool to deprecate and send warning first, then deprecate.
> If that's not an option, we can also communicate to addon author before the
> change and be able to suggest better and easier way to implement their
> features.

As noted, I think we can do better than just forbidding Xray waivers, by using an nsExpandedPrincipal for all content scripts. I'm sorting out some of the rough edges for doing something like this in bug 823348 and bug 821850.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Any thoughts on a security rating for this?

As said in comment 2, that security issue is known and documented to addon authors:
https://addons.mozilla.org/en-US/developers/docs/sdk/1.12/dev-guide/guides/content-scripts/accessing-the-dom.html
We had to offer addon authors a workaround in order to implement their features,
so we went for the existing greasemonkey feature. But that's something we kept as being experimental, in order to eventually remove it and replace it with some other way of accessing and exposing values from/to the webpage.


Addon reviewers can easily check for `unsafeWindow` existence in content scripts and spend more time reviewing related code.
That being said, the flaw here would be the `wrappedJSOBject` available on all nodes. We never documented their existence, so we are not expecting addon authors to use it, but they still can... I found 7 of them using it in a content script: wildfox, net-radio, springpad, resumelater, idderall, youtube autoplay, fx-share.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Bobby Holley (:bholley) from comment #11)
> As noted, I think we can do better than just forbidding Xray waivers, by
> using an nsExpandedPrincipal for all content scripts. I'm sorting out some
> of the rough edges for doing something like this in bug 823348 and bug
> 821850.

So correct me if I'm wrong: when we are using nsEP, the content script won't be able to expose anything to content (the content will get a permission exception as soon as it tries to access a JS values from nsEP)?
But the content script will still be able to call any webpage object throught wrappedJSObject?

That sounds like a good default behavior to me, as soon as we still offer a way out in order to expose JS to the webpage with additional flags/API. Otherwise we are just going to say: Sorry bugzillajs and grooveshark devs but there is no way to write your addon anymore :o

Comment 14

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> (In reply to Bobby Holley (:bholley) from comment #11)
> > As noted, I think we can do better than just forbidding Xray waivers, by
> > using an nsExpandedPrincipal for all content scripts. I'm sorting out some
> > of the rough edges for doing something like this in bug 823348 and bug
> > 821850.
> 
> So correct me if I'm wrong: when we are using nsEP, the content script won't
> be able to expose anything to content (the content will get a permission
> exception as soon as it tries to access a JS values from nsEP)?
> But the content script will still be able to call any webpage object
> throught wrappedJSObject?

Right. The idea here is that nsEP(foo.com) has no more privileges than foo.com, except that it has an asymmetric security relationship with foo.com. nsEP(foo.com) subsumes foo.com, but not the reverse.

> That sounds like a good default behavior to me, as soon as we still offer a
> way out in order to expose JS to the webpage with additional flags/API.
> Otherwise we are just going to say: Sorry bugzillajs and grooveshark devs
> but there is no way to write your addon anymore :o

Yeah. We could easily plug in the __exposedProps__ machinery, but we're trying to move away from that in favor of some kind of Components.utils.cloneInto(), which allows privileged callers to do place a structured clone of an object into content (sorta like Cu.createObjectIn() + Cu.makeObjectPropsNormal() + deep). However, exposing this stuff to content scripts means exposing something in Components.utils to non-chrome callers, which isn't great either. Could we expose the API via self.port?

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to Bobby Holley (:bholley) from comment #14)
> Yeah. We could easily plug in the __exposedProps__ machinery, but we're
> trying to move away from that in favor of some kind of
> Components.utils.cloneInto(), which allows privileged callers to do place a
> structured clone of an object into content (sorta like Cu.createObjectIn() +
> Cu.makeObjectPropsNormal() + deep). However, exposing this stuff to content
> scripts means exposing something in Components.utils to non-chrome callers,
> which isn't great either. Could we expose the API via self.port?

Oh that looks very interesting! Could you please cc me on related bugs?
And yes, I think we could expose Cu.* via self.port. 
And what about function? Does the clone calls the original method when it's being called?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot (:ochameau) from comment #15)
> (In reply to Bobby Holley (:bholley) from comment #14)
> > Yeah. We could easily plug in the __exposedProps__ machinery, but we're
> > trying to move away from that in favor of some kind of
> > Components.utils.cloneInto(), which allows privileged callers to do place a
> > structured clone of an object into content (sorta like Cu.createObjectIn() +
> > Cu.makeObjectPropsNormal() + deep). However, exposing this stuff to content
> > scripts means exposing something in Components.utils to non-chrome callers,
> > which isn't great either. Could we expose the API via self.port?
> 
> Oh that looks very interesting! Could you please cc me on related bugs?

Bug 821521 is the bug for phasing out COWs. I haven't yet filed any bugs about the replacement API - been busy.

> And yes, I think we could expose Cu.* via self.port.

Er, I don't think we want to do that (if by * you meant 'everything'). Just a very, very particular and checked API that allows content scripts to clone objects into content that they subsume.

> And what about function? Does the clone calls the original method when it's
> being called?

I believe it will be the same mechanism as the current Cu.makeObjectProps normal - wrapping the functions up in JSNatives and shipping the call via C++.

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Bobby Holley (:bholley) from comment #16)
> > And yes, I think we could expose Cu.* via self.port.
> 
> Er, I don't think we want to do that (if by * you meant 'everything'). Just
> a very, very particular and checked API that allows content scripts to clone
> objects into content that they subsume.

Sure `*` meant whatever we need from Cu, with eventual sugar and checks on top of it :o
I'll be interested to see how is that going to be used in WebAPI implemented in JS, as I assume they are going to use that instead of __exposedProps__ as well. They might already use makeObjectPropsNormal?

In any case, thanks for these clarifications!

Comment 18

[Gabor Krizsanits (INACTIVE)]

Can I haz access to bug 821850?

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] (out until 2013) from comment #18)
> Can I haz access to bug 821850?

Done.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Hi all,

We still need to get a security rating on this as this is an open security issue that comes up whenever we do triage since it is unrated and unassigned to anyone.

Who can take this bug?

Comment 21

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #20)
> We still need to get a security rating on this as this is an open security
> issue that comes up whenever we do triage since it is unrated and unassigned
> to anyone.

See comment 11. It's not totally clear what the security rating here is, but I'm just going to say sec-moderate.

> Who can take this bug?

First we need to implement an API on the platform side to allow objects to be cloned into content (this is bug 825981). Once that happens, the jetpack folks need to make this API usable from content scripts, and then switch the content scripts to nsExpandedPrincipal.

Comment 22

[Gabor Krizsanits (INACTIVE)]

I just came back from holiday so feel free to put any of this work on my plate, since this is pretty important for jetpack. Just a question, isn't the only thing we need here is the machinery of the COWs that prevent content to have access the base classes of chrome compartment like Object and String? So Object.getPrototypeOf(foo) should return the Object of the content compartment in your example and problem solved. Am I missing something, or do we want to move away from that wrapper entirely? Not just from the explicit usage of __exposedProps__?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> I just came back from holiday so feel free to put any of this work on my
> plate, since this is pretty important for jetpack. Just a question, isn't
> the only thing we need here is the machinery of the COWs that prevent
> content to have access the base classes of chrome compartment like Object
> and String? So Object.getPrototypeOf(foo) should return the Object of the
> content compartment in your example and problem solved. Am I missing
> something, or do we want to move away from that wrapper entirely? Not just
> from the explicit usage of __exposedProps__?

We've decided that prototype remapping is insane (due to its reliance on nativeCall, see bug 809652), that COWs are insane in general, and that we want to get rid of them where we can (bug 821521).

In the current world, we can already just run content scripts with nsEP and block the content from accessing anything important in the content-script compartment. However, as ochameau points out, this means that content scripts will have no way at all to expose objects to content, which will be a problem for certain addons. So I think this bug is blocked by creating the new API in bug 825981.

Comment 24

[Gabor Krizsanits (INACTIVE)]

Right, this all makes sense, thanks for summing it up for me. The exposing part could be solved probably relatively easily, but since we want to get rid of COWs anyway let's just fix this properly. I'd be glad to see a saner replacement for COWs, I'm really curious about the new version.

Comment 25

[Andrew McCreight [:mccr8]]

Any progress here, Gabor?  This still sounds relevant.

Comment 26

[Gabor Krizsanits (INACTIVE)]

After bug 877673 is resolved we should be able to use nsEP for all content-script. I'm not sure that it will be super easy to make that change though (possible add-on breakage...) I will have to think about that part too.

Comment 27

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> (In reply to Gabor Krizsanits [:krizsa :gabor] (out until 2013) from comment
> #4)
> > 
> >  (In reply to Alexandre Poirot (:ochameau) from comment #2)
> > > We are discussing about getting rid of unsafeWindow and it will be only
> > > meaningfull if we get rid of these wrappedJSObject attributes as well.
> > 
> > Please don't do that. There are situations where we need it.
> 
> I think there is some tweaks to be done without preventing addon authors to
> play with website JS. Here is some ideas:
> - avoid exposing wrappedJSObject at all, only expose unsafeWindow but only
> when an attribute is set on page-mod in order to ask for JS access.
> - add a new content script flag in order to evaluate it directly in website
> scope, so that you don't even have to use unsafeWindow. You can just do
> window.GmailGreasemonkeyAPI.dosomething()
> - ...
> 
> The important point is that I'd really like to see JS value access be
> disabled by default. But I still want to expose an easy JS value access, I
> really don't want to tell people to pipe messages between the webpage and
> the content script in order to send pipe messages to the addon :o That's
> what is suggested for chrome addons, and what irakli suggested last time we
> talked about that: using eval or inject script tag from the content script
> and communicate through DOM event (postMessage or custom events)
> 
> > The nice thing about using nsEP always, that we could very easily identify
> > add-ons and create custom wrappers without affecting anything else.
> 
> What are those nsEP ???
> 
> > Is that
> > patch landed even? Alex: how difficult do you think it would be to land
> > something like this without breaking add-ons? And when can you come back to
> > jetpack finally? :)
> 
> I'm not sure the patch landed, but that's only to avoid breaking addons
> still using old jetpack version. I immediatly fixed SDK master when the
> patch was landed on m-c and made our test fail!
> Otherwise I don't undestand what is the "land something like this without
> breaking add-ons"?


Alex that was not what I was suggesting. What I was talking about was scenario that
Gabor described perfectly. Which is add-on providing additional capabilities to a
content. Right now it requires two steps:

1. Content communication with a content script (directly or via messages)
2. Content script communication with add-on (via messages)

So basically content-scripts serve as a proxy. What I was talking about is to have an
API on the add-on side that would allow direct communication between content and addon.
By direct I mean without `content-script` proxy, but still using JSON messages.

Comment 28

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think it's important to separate use cases that add-on's have for a content-scripts or
for communication with content:


1. Add-on wishes to expose additional capabilities to a very specific page like gmail. At the
   moment such add-ons are forced to have content script + unsafeWindow.props to achieve that.

   I think the best alternative we can provide here is to let add-ons expose a message directly
   to a page content, through which content and add-on could communicate directly via plain
   JSON messages.

2. Add-on wishes to manipulate or collect data form the page(s) in this scenario add-on can
   create a content script and with nsEP it will be able to collect any data it wishes or call
   do things to a content page on add-on's behalf. Most of the uses of `unsafeWindow` were triggered
   by the fact that some interactions with exposed window were buggy. In other words same script
   executed in content itself would work, but it would not if executed in the context of content script.
   My hope is that nsEP solves this.

3. Add-on wishes to modify content page. This is very similar to 2. but add-ons may want to add
   additional buttons to a page and trigger some actions when they're clicked. Which did not worked with
   non `unsafeWindow` as there was some issues with listeners that do come form the content scripts.
   I'm afraid nsEP won't address this use case though. And if it is true we need some solution here.

4. Mix of cases above.

What I'm trying to say that changing props directly in the content from content script isn't actually a
goal for add-ons, in most cases they just try to either workaround bugs, or let content communicate with
add-on.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #28)
> 1. Add-on wishes to expose additional capabilities to a very specific page
> like gmail. At the
>    moment such add-ons are forced to have content script +
> unsafeWindow.props to achieve that.
> 
>    I think the best alternative we can provide here is to let add-ons expose
> a message directly
>    to a page content, through which content and add-on could communicate
> directly via plain
>    JSON messages.

Our plan for this is to use the new primitives that gabor built to allow nsEP content scripts to safely inject APIs into content. Did something change?

I don't understand the difference between 2 and 3. Xrays are getting better, so anything that involves the DOM (or soon-to-be-xrayable objects like typed arrays) will be doable via Xrays from an nsEP. Anything that involves mucking with script-defined expandos will require unsafeWindow/wrappedJSObject, but those will be much safer operations once we're using nsEP.

Comment 30

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Bobby Holley (:bholley) from comment #29)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #28)
> > 1. Add-on wishes to expose additional capabilities to a very specific page
> > like gmail. At the
> >    moment such add-ons are forced to have content script +
> > unsafeWindow.props to achieve that.
> > 
> >    I think the best alternative we can provide here is to let add-ons expose
> > a message directly
> >    to a page content, through which content and add-on could communicate
> > directly via plain
> >    JSON messages.
> 
> Our plan for this is to use the new primitives that gabor built to allow
> nsEP content scripts to safely inject APIs into content. Did something
> change?
> 

We can use that API to expose a message channel, although I would prefer if add-on
authors did not had to write content-script proxy. One way could be to predefine
content-scripts for them, other way could be to just use regular DOM events so creating
content-scripts or injecting APIs won't even be necessary.

> I don't understand the difference between 2 and 3. Xrays are getting better,
> so anything that involves the DOM (or soon-to-be-xrayable objects like typed
> arrays) will be doable via Xrays from an nsEP. 

You right technically there are no difference, although 2. always worked without unsafeWindow
while 3. did not. 


Anything that involves
> mucking with script-defined expandos will require
> unsafeWindow/wrappedJSObject, but those will be much safer operations once
> we're using nsEP.

Comment 31

[Gabor Krizsanits (INACTIVE)]

I'm trying to do this switch and resolving issues around it. One thing that came up while running tests is this guy: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/deprecated/traits-worker.js#267

When will this API be deprecated? In the new world this will not work. I can expose the port object to trusted pages, but not self... unless we create those trusted pages with nsEP as well, which is not possible currently but could be an option. So the question is how bad it would be to break the injectInDocument part here?

Comment 32

[Gabor Krizsanits (INACTIVE)]

As we discussed in email, cloning self to content is enough short term, but on long term probably we want to run these trusted pages with nsEP too. Which brings up a few questions... Bobby what do you think in general about this? How would that affect XBL?

Also, I tried to do the cloning but the first test failed already: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/fixtures/test-sidebar-addon-global.html?force=1

because it tries to pass in a function object into the exported function which cannot be cloned... What do you think about on the fly creating cross compartment helper functions like the ones exportFunction uses?

Comment 33

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #32)
> As we discussed in email, cloning self to content is enough short term, but
> on long term probably we want to run these trusted pages with nsEP too.

It's not clear to me why we want to do that. nsEP exists to give an asymmetric privilege relationship with a principal. If we don't want that relationship for these trusted pages, then we should just not run the associated content scripts with nsEP.

> because it tries to pass in a function object into the exported function
> which cannot be cloned... What do you think about on the fly creating cross
> compartment helper functions like the ones exportFunction uses?

I'm skeptical that it's smart to do that automatically. What's the use-case?

Comment 34

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #33)
> It's not clear to me why we want to do that. nsEP exists to give an
> asymmetric privilege relationship with a principal. If we don't want that
> relationship for these trusted pages, then we should just not run the
> associated content scripts with nsEP.

The concept is very simple. These 'pages' belong to content-script not to content.
It's bundled to the add-on just as much as content-script and have to communicate
with the add-on.
It has the same security descriptor for ease of communication. Since it belongs
to content-script, if any time it wants to do any interaction with the content
itself it should have the same security barrier there too. I really don't want
to introduce yet another messaging layer between these add-on pages (panels
and widgets really...) and content-script...

> I'm skeptical that it's smart to do that automatically. What's the use-case?

I don't really understand your concern. In general I think it's a good thing to use
the same asymmetric security setup for add-on widgets and content-scripts. Scripts
in these widgets should be considered as content-script imo. Don't forget that
we're just about to inject port into their scope for compatibility... In fact
I'm kind of worried to inject port into these scopes without having the same
security barrier there too... So the more I think about it the more it feels like the
only real solution.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #34)
> The concept is very simple. These 'pages' belong to content-script not to
> content.
> It's bundled to the add-on just as much as content-script and have to
> communicate
> with the add-on.
> It has the same security descriptor for ease of communication. Since it
> belongs
> to content-script, if any time it wants to do any interaction with the
> content
> itself it should have the same security barrier there too.

Ah ok. If these pages are generated by content scripts, it seems fine to run them with nsEP.
 
> > I'm skeptical that it's smart to do that automatically. What's the use-case?
> 
> I don't really understand your concern. In general I think it's a good thing
> to use
> the same asymmetric security setup for add-on widgets and content-scripts.
> Scripts
> in these widgets should be considered as content-script imo. Don't forget
> that
> we're just about to inject port into their scope for compatibility... In fact
> I'm kind of worried to inject port into these scopes without having the same
> security barrier there too... So the more I think about it the more it feels
> like the
> only real solution.

I don't follow. The "I'm skeptical" part was about implicitly generated FunctionForwarders. Did I misunderstand something? It would be helpful to describe the use-case of where we need that.

Comment 36

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #35)
> I don't follow. The "I'm skeptical" part was about implicitly generated
> FunctionForwarders. Did I misunderstand something? It would be helpful to
> describe the use-case of where we need that.

Sorry my bad then... So the use case are callback functions as arguments. For example
addon.on("blah", function(){...})

I understand why you're skeptical here, we talked about this back then, I share your
concerns. But it's quite a common pattern to use callback functions as arguments so
I'm still playing with the idea...

Anyway, I'm going to try to create those widgets with nsEP.

Comment 37

[Gabor Krizsanits (INACTIVE)]

As a second thought if those widgets are trusted, have the same principal as content-script, pretty much separated from the actual web contents, then we should just not use nsEP for them and their content-script and problem solved.

Comment 38

[Gabor Krizsanits (INACTIVE)]

Attachment: Using nsEP for content-scripts. v1

The idea is that for start add-on developers can get back to the old behavior by setting the 'security-membrane-for-content-script' permission flag to false in the manifest file. By default it's true.

If we attach a content-script to something that already has system principal, or trusted code (comes with the same add-on like panels) we don't use nsEP. It would not make any sense there which made things very simple in the end.

The wantExportHelpers flag in the sandbox is responsible for installing the new API on the content-script sandbox.

Comment 39

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8393392 [details] [diff] [review]
Using nsEP for content-scripts. v1

Review of attachment 8393392 [details] [diff] [review]:
-----------------------------------------------------------------

Rejecting because there's no way to opt out of securityMembrane, also we need more tests

::: addon-sdk/source/lib/sdk/content/sandbox.js
@@ +38,5 @@
>  const permissions = (metadata && metadata['permissions']) || {};
>  const EXPANDED_PRINCIPALS = permissions['cross-domain-content'] || [];
>  
> +const securityMembrane = permissions['security-membrane-for-content-script'] || true;
> +

x || true is always going to be true.

@@ +118,5 @@
>      let principals = window;
>      let wantGlobalProperties = [];
> +    const nsIScriptSecurityManager = Ci.nsIScriptSecurityManager;
> +    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].
> +      getService(Ci.nsIScriptSecurityManager);

Nit: We typically put service imports at the top of the file.

Comment 40

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #39)
> Rejecting because there's no way to opt out of securityMembrane, also we
> need more tests

I can add some more tests, but all of our tests go through these... so do you have
some specific tests in mind? I can add some around the new API but if you are concerned
about some parts of the SDK that it might be under tested and might run into issues because
of this change let me know.

> > +const securityMembrane = permissions['security-membrane-for-content-script'] || true;
> > +
> 
> x || true is always going to be true.

Thanks, for noticing. Also, I was totally unsure about this part, do I need to define this flag somewhere else? Or will it work like this? Is the 'security-membrane-for-content-script' an acceptable name for this flag or do we need something else? Most importantly, how can I test a flag like this? I wanted to add a test for this but have no idea how to do that. Where can I set permission flags for a test?

Comment 41

[Gabor Krizsanits (INACTIVE)]

Got all the info I needed per IRC.

Comment 42

[Gabor Krizsanits (INACTIVE)]

Attachment: Using nsEP for content-scripts. v2

I've added an addon test for the flag. The flag now is called unsafe-content-script and has to be set to true for the old behavior as requested. For the inject addon global thing I don't think we need any extra test, the current ones would be broken already if nsEP would be used in those cases. For the new API I only test exportFunction in the tests, since we already have tests for those on platform side. Let me know if I still miss something or if you have any questions.

Comment 43

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8397124 [details] [diff] [review]
Using nsEP for content-scripts. v2

Review of attachment 8397124 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I think we should communicate it really well with users as I expect all uses of `unsafeWindow` is going to break.

Comment 44

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #43)
> Comment on attachment 8397124 [details] [diff] [review]
> Using nsEP for content-scripts. v2
> 
> Review of attachment 8397124 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I think we should communicate it really well with users as I
> expect all uses of `unsafeWindow` is going to break.

Yes we should. I will talk to Will next week and will only land this one when all the docs, and some blogs about it are in place.

unsafeWindow in the common case will work just fine. Only thing that will be broken if someone does unsafeWindow.fuu = { ... }. So trying to expose an object, which is a very rare scenario among addons.

Comment 45

[Gabor Krizsanits (INACTIVE)]

As it turns out the SDK does this:

140     // We have to ensure that window.top and window.parent are the exact same
141     // object than window object, i.e. the sandbox global object. But not
142     // always, in case of iframes, top and parent are another window object.
143     let top = window.top === window ? content : content.top;
144     let parent = window.parent === window ? content : content.parent;
145     merge(content, {
146       // We need 'this === window === top' to be true in toplevel scope:
147       get window() content,
148       get top() top,
149       get parent() parent,
150       // Use the Greasemonkey naming convention to provide access to the
151       // unwrapped window object so the content script can access document
152       // JavaScript values.
153       // NOTE: this functionality is experimental and may change or go away
154       // at any time!
155       get unsafeWindow() window.wrappedJSObject
156     });

So essentially we are actively faking that within the content-script sandbox scope, window === this, which is a lie. I asked around and it seems like we are doing it for letting some libs do things like:

var foo = 42;

and expecting window.foo to be defined.

I think if an add-on wants that to work, they can just do the same hack explicitly, but doing it everywhere feels very wrong. Conceptually as well (explaining anything about compartments or our security layer with this hack around is pretty horrible...) and also practically, since if I leave this in, that means I have to tell people to use unsafeWindow for the new API (since that at least really is the window object unlike the one you get by this custom window getter, which is not even from the scope of the window...)

And thanks Will for pointing out that bug you found!

Comment 46

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Stop overwriting window getter in content-script sandboxes. v1

Comment 47

[Alexandre Poirot [:ochameau]]

Comment on attachment 8400020 [details] [diff] [review]
part2: Stop overwriting window getter in content-script sandboxes. v1

Review of attachment 8400020 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a big fan of this move. I don't see what is wrong with that.
Does it actualy fix something? Prevent security leaks?
The sandbox itself aims to mimic the content page global scope by inheriting from it.
And in page context window == top == this. I don't see why we would prevent this rule to be also true for content script.
If I remember correctly, by doing this, you will break jquery in content script.

Comment 48

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #47)
> Comment on attachment 8400020 [details] [diff] [review]
> part2: Stop overwriting window getter in content-script sandboxes. v1
> 
> Review of attachment 8400020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a big fan of this move. I don't see what is wrong with that.
> Does it actualy fix something? Prevent security leaks?
> The sandbox itself aims to mimic the content page global scope by inheriting
> from it.
> And in page context window == top == this. I don't see why we would prevent
> this rule to be also true for content script.
> If I remember correctly, by doing this, you will break jquery in content
> script.

Yeah that's why I wanted to pull you into this discussion even though you're not
working on the SDK any longer.
So here is the story in a nutshell. To prevent a security
hole we need to separate content from content-script a bit more. So content will
no longer have access to object from the scope of content-script.

Which means we had to invent a new API to export object from content-script to
content for those rare cases add-on developers want to do it. It's structure
cloned based, and the syntax of this API takes a parameter of 'where' to
clone/export the object. It is not just utterly confusing for this API that 'window'
is not the window object itself but not even something from that scope. But I also
don't see that we should maintain the illusion that content-script and content are
the same scope, if people need an API like this on the first place. It leads to way
to many confusion.

So in practice something like cloneInto({...}, window) will clone the object back in
the same scope... and one had to use cloneInro({...}, unsafeWindow) instead...
evalInWindow("foo", window) would just throw that window is not a window object. And
exportFunction(function(){...}, window) would export the function to the wrong scope
again.

If someone needs some library that relies on the window == top == this assumption, he
can add the same hack to his script, and we should write a guide for this for sure. Or
we can add a flag for it in the manifest. But in the general case I think it's very
misleading.

Comment 49

[Alexandre Poirot [:ochameau]]

Can't we make cloneInto smart enough to redirect it to the content window if we pass the sandbox global?
The sandbox global is the window object. It has all window properties. I think it is only different in our mind because of the internal of xpconnect. But I'm not sure it is considered as much different for userscript developers.
The only justification I see here is related to the internal implementation of your stuff.

But behind this very precise detail, you have to figure out the big picture. That's the most important.
I would suggest you to take a look at greasemonkey. They are using nsEP! And they have a whole bunch of userscript in the wild. I'm pretty sure 90% of them are using nsEP flawlessly.
https://github.com/greasemonkey/greasemonkey/blob/master/components/greasemonkey.js#L71

Comment 50

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #49)
> Can't we make cloneInto smart enough to redirect it to the content window if
> we pass the sandbox global?

I could but it would be horrible. Let's say you have two sandboxes and you want
to clone something from the one to the other. But since the target sandbox has
a window hooked into its proto chain, all of a sudden it ends up in the scope
of that window instead...

> The sandbox global is the window object. It has all window properties. I
> think it is only different in our mind because of the internal of xpconnect.

No. If they were then var foo = "bar" defined on one side would be visible from the
other. It is not. Even the base classes are different. Sandbox's Function is not
the same as Function of the window. And now even their principal is different. They
are different scopes. They are connected sure, but not the same object. Like any
object is not equal with its prototype either.

> But I'm not sure it is considered as much different for userscript
> developers.
> The only justification I see here is related to the internal implementation
> of your stuff.

It's not about the internal implementation details. It's the concept. If we
want the two objects to be equal, then content script should be just a string
we evaluate in the context of the content window. Without creating any sandbox
with extended capabilities we want to hide from the content (port object in this case)

> 
> But behind this very precise detail, you have to figure out the big picture.
> That's the most important.
> I would suggest you to take a look at greasemonkey. They are using nsEP! And
> they have a whole bunch of userscript in the wild. I'm pretty sure 90% of
> them are using nsEP flawlessly.
> https://github.com/greasemonkey/greasemonkey/blob/master/components/
> greasemonkey.js#L71

I'm very aware of this since we suggested them to do the same as I'm trying to
do here to fix the very same security leak. If they use the same trick they will
face the same problem probably. Are they?

Comment 51

[Gabor Krizsanits (INACTIVE)]

Ok, to be a bit more constructive how about this. For the SDK I can create a wrapper function around the API that does what you suggest. So it checks if the argument is the |this| itself and if so it replaces it with the actual window. It's a hack on top of a hack, and I'm really against it. Side effect would be if someone wants to clone an object in-place intentionally (so back into it's own scope) he would get an object from the windows scope unexpectedly. I'm against this, but this could work, if people really want to keep the old behavior.

What do people think? (I assume Alex votes on keeping the window getter magic...)

Comment 52

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #51)
> Ok, to be a bit more constructive how about this. For the SDK I can create a
> wrapper function around the API that does what you suggest. So it checks if
> the argument is the |this| itself and if so it replaces it with the actual
> window. It's a hack on top of a hack, and I'm really against it. Side effect
> would be if someone wants to clone an object in-place intentionally (so back
> into it's own scope) he would get an object from the windows scope
> unexpectedly. I'm against this, but this could work, if people really want
> to keep the old behavior.
> 
> What do people think? (I assume Alex votes on keeping the window getter
> magic...)

I vote against it, and agree wholeheartedly with comment 50.

Comment 53

[Alexandre Poirot [:ochameau]]

Greasemonkey isn't doing this so that various frameworks fail to load, whereas they currently work in SDK. But that doesn't prevent greasemonkey developers from using these frameworks (like prototype.js, mootools):
http://jimbojw.com/wiki/index.php?title=Using_Prototype_and_Scriptaculous_with_Greasemonkey
The workaround is to evaluate them in the page and use the framework through unsafeWindow.
Then it if up to you to decide who and how you workaround.

Comment 54

[Gabor Krizsanits (INACTIVE)]

remote:  https://hg.mozilla.org/integration/mozilla-inbound/rev/22feaebc0b2c

Comment 55

[Gabor Krizsanits (INACTIVE)]

Attachment: Using nsEP for content-scripts. v3

updated version of the patch for posterity (using unsafeWindow in the test)

Comment 56

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8400020 [details] [diff] [review]
part2: Stop overwriting window getter in content-script sandboxes. v1

Review of attachment 8400020 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs a bit more work. We need a different bug for this, as it might break popular libraries for content-script which seems to be unacceptable.

Comment 57

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8403889 [details] [diff] [review]
Using nsEP for content-scripts. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Architectural bug in the SDK
User impact if declined: Currently every add-on that calls into a content function, or exposes an object from content-script to content can leak out the port object. With that, arbitrary web content can send messages to the system side of the add-on, possible tricking it. There can be some add-ons that can be exploited this way badly.

Testing completed (on m-c, etc.): passed try tests, now in inbound, the patch has tests

Risk to taking this patch (and alternatives if risky): It will break _some_ add-ons, but it is well communicated to add-on developers what to do. There is a new API for them as well and they can get back to the old (leaky...) behavior temporarily with a manifest flag in their add-on. That would at least narrow the scope of the add-ons that might need a more through check, weather this leak can be exploited or not. Also we can help them using the new API, as this flag will be deprecated soon. Finally, the main reason to uplift is: https://bugzilla.mozilla.org/show_bug.cgi?id=976087#c41

String or IDL/UUID changes made by this patch: none

Comment 58

[Gabor Krizsanits (INACTIVE)]

We have a problem.

https://tbpl.mozilla.org/php/getParsedLog.php?id=37502152&tree=Mozilla-Inbound

We got a scary assertion, but only on windows8 debug. Sigh... just what I needed... 

Bobby, do you have any idea what this can be? I'm quite clueless.

Comment 59

[Gabor Krizsanits (INACTIVE)]

How can this code be called? Maybe we create a sandbox with nsEP instead of system, because I do:

let isSystemPrincipal = secMan.isSystemPrincipal(window.document.nodePrincipal);

check, and window.document.nodePrincipal, because of the redirection is still about:blank temporarily when we attach the sandbox (which would be a bug). Does that make any sense? Can we create an nsEP from that or would that fail?

Even if that is the case, I still don't know how could this code be called at all. XBL scopes should not be created for sandboxes right?

Comment 60

[Carsten Book [:Tomcat]]

hey gabor, sorry had to backout this change for the error gabor mentioned in comment #58 since this resulted in a permanent error

Comment 61

[Bobby Holley (:bholley)]

Ok. So this basically means that we're creating window with an nsExpandedPrincipal. We should really assert against that, which will catch any potential issues earlier. We just happen to be catching it here because we're creating an in-content XBL binding for a window that has an nsEP. So gabor, the first thing that we should do here is to add a patch (underneath yours) that asserts against creating a DOMWindow with an nsEP.

The relevant test is here:

mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-page-worker.js?force=1#316

What's probably happening here is that the navigation happens over Xrays, which means that the data:uri inherits its caller's principal. I'm not totally sure how to fix this. Boris, do you have any thoughts?

Comment 62

[Boris Zbarsky [:bzbarsky]]

We should probably change the block at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=5fb973d5e276#1551 and the corresponding comments above to exclude expanded principals too.

Comment 63

[Bobby Holley (:bholley)]

Ok. So gabor, I think we need two patches:
(1) Assert against creating DOMWindows with nsEP
(2) comment 62

Comment 64

[Bobby Holley (:bholley)]

Talking to Boris, I think we should make (1) a MOZ_RELEASE_ASSERT.

Comment 65

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by, will wait for landing on central here before uplift approval consideration.

Comment 66

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=09b216fc84c1

Comment 67

[Gabor Krizsanits (INACTIVE)]

Attachment: assert for nsEP in DOMWindow. v1

I'm not sure I understand your request correctly, is this the place you wanted me to put that assertion?

Comment 68

[Gabor Krizsanits (INACTIVE)]

Attachment: Excluding nsEP from principal inheriting. v1

This seems to fix the problem indeed.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404831 [details] [diff] [review]
Excluding nsEP from principal inheriting. v1

You need to fix the second part of the comment too.

r=me with that

Comment 70

[Bobby Holley (:bholley)]

Comment on attachment 8404828 [details] [diff] [review]
assert for nsEP in DOMWindow. v1

Review of attachment 8404828 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with that

::: dom/base/nsGlobalWindow.cpp
@@ +2217,5 @@
>    NS_PRECONDITION(mDocumentPrincipal == nullptr,
>                    "mDocumentPrincipal prematurely set!");
>    MOZ_ASSERT(aDocument);
> +  // DOMWindow with nsEP is not supported, we have to make sure
> +  // no one creates one accidentally.

Let's put it at the top of CreateNativeGlobalForInner so that we don't call it unnecessarily during bfcache navigation.

@@ +2219,5 @@
>    MOZ_ASSERT(aDocument);
> +  // DOMWindow with nsEP is not supported, we have to make sure
> +  // no one creates one accidentally.
> +  nsCOMPtr<nsIExpandedPrincipal> nsEP = do_QueryInterface(aDocument->NodePrincipal());
> +  MOZ_RELEASE_ASSERT(!nsEP, "DOMWindow with nsEP is not supported.");

nix the period

Comment 71

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f10a9050474
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47b01425f40c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b816eb9618bb

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f10a9050474
https://hg.mozilla.org/mozilla-central/rev/47b01425f40c
https://hg.mozilla.org/mozilla-central/rev/b816eb9618bb

Comment 73

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8404828 [details] [diff] [review]
assert for nsEP in DOMWindow. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

User impact if declined: Without the main patch, it is already possible to produce the same issue we run into in the failing test. Redirection from a content-script with nsEP. That could run content code with expanded principal which could be potentially exploited to access to XBL scope. In any similar situation this patch choose to crash instead.

Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): I don't think it's risky at all, it's just an extra security barrier. With the excluding nsEP patch this should never happen, but better safe than sorry.

String or IDL/UUID changes made by this patch: none

Comment 74

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8404831 [details] [diff] [review]
Excluding nsEP from principal inheriting. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

User impact if declined: Redirection from a scope that has expanded principal might end up inheriting the expanded principal to the new document. This is a security leak.

Testing completed (on m-c, etc.): m-c

Risk to taking this patch (and alternatives if risky): I cannot think of any possible risk.

String or IDL/UUID changes made by this patch: none

Comment 75

[Al Billings [:abillings - ex-MoCo]]

Do we need to take this on ESR24?

Comment 76

[Gabor Krizsanits (INACTIVE)]

(In reply to Al Billings [:abillings] from comment #75)
> Do we need to take this on ESR24?

No, that would not work.

Comment 77

[Gabor Krizsanits (INACTIVE)]

I was about to push it to aurora but then it hit me, that because of bug 980023 that would break localStorage for addons. Bobby, can you take a look at it if it's safe to uplift that patch? I don't see a reason why not, but just in case. Meanwhile I set an uplift request on it.

Comment 78

[Gabor Krizsanits (INACTIVE)]

Eh, that one is depending on bug 946815... should we uplift that too?

Comment 79

[Gabor Krizsanits (INACTIVE)]

Never mind... I realized that those patches already made into ff30 so my worry was just false alarm.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/aa7423a1d83c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/902765e9c65a
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2917948f7214

Comment 80

[Gabor Krizsanits (INACTIVE)]

Clearing need info, that discussion should continue in another bug.

Comment 81

[u279076]

FYI, this has introduced a regression into Firefox 31 (bug 996069).

Comment 82

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] due to lack of test case. I see some code in comment 0 but unsure how to easily verify what we expect, pre- or post-fix. If anyone has suggestions, feel free to offer them and we'll do it, but for now, we won't verify for Fx30.

Comment 83

[Bobby Holley (:bholley)]

I'm pretty concerned about writing this up as a security advisory. Don't some old addons still run with packaged versions of the SDK?

Comment 84

[Dave Townsend [:mossop]]

-> Jeff

Comment 85

[Al Billings [:abillings - ex-MoCo]]

We've pulled the advisory on talking to Bobby.

Comment 86

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Bobby Holley (:bholley) from comment #83)
> I'm pretty concerned about writing this up as a security advisory. Don't
> some old addons still run with packaged versions of the SDK?

This isn't a problem because:

* add-ons with embedded sdk versions prior to 1.14 aren't compatible with Firefox since IIRC Fx 26
* add-ons packaged with 1.14 or greater will *always* load the SDK version provided by Firefox

Comment 87

[Bobby Holley (:bholley)]

Ok. Then let's just embargo until esr24 EOL.

Comment 88

[Justin Wood (:Callek)]

aa7423a1d83c transplanted to https://hg.mozilla.org/releases/mozilla-release/rev/af599a9ea229
902765e9c65a transplanted to https://hg.mozilla.org/releases/mozilla-release/rev/c37470bad629
2917948f7214 transplanted to https://hg.mozilla.org/releases/mozilla-release/rev/687cb2bfc9f0