794943 - Remove nsISecurityCheckedComponent

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

This is a bunch of old caps garbage, and needs to go away. When we stop using it for Components defense-in-depth (which I'll be comfortable doing once bug 790732 lands), we can rip it out, modulo addon usage.

I checked the addons on MXR, and there are some scattered uses of this interface. But most of them don't actually do anything real. Here's the list:

386: commented out
1833: uses it
2032: no-op use
3829: commented out
5082: uses it
6665: uses it
8966: commented out
181056: no-op usage
207634: commented out
222515: no-op usage
235854: Just throws if QIed to that
283739: uses it
300776: uses it
337278: just throws if QIed to that
337326: just throws if QIed to that
356737: just forwards. I think it can go away?
358590: just throws if QIed to that
382841: just throws if QIed to that

So in total, there are 5 addons that use it. I think those addons should probably move to __exposedProps__. But keep in mind that this is somewhat apples/oranges here. nsISecurityCheckedComponent allows people to expose XPCOM components with security checks, whereas __exposedProps__ is for vanilla JS objects. People really shouldn't be exposing XPCOM components to content JS anyway. If they need to convert, they should make a separate JS object (with __exposedProps__) that exposes the relevant API.

Comment 1

[Bobby Holley (:bholley)]

Blake (or anyone else): a number of those addons implement nsISecurityCheckedComponent but just return "AllAccess" for all the policy enforcement traps. That's the same as not implementing it at all, right? Is there anything special and non-obvious that implementing the interface gives you?

Comment 2

[Benjamin Smedberg]

It is not the same. Normally attaching an XPCOM object to the DOM if the object doesn't implement DOM classinfo means that content may not touch the object at all. Implementing nsISecurityCheckedComponent and returning allaccess for the methods means that content can touch the XPCOM object.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> It is not the same. Normally attaching an XPCOM object to the DOM if the
> object doesn't implement DOM classinfo means that content may not touch the
> object at all. Implementing nsISecurityCheckedComponent and returning
> allaccess for the methods means that content can touch the XPCOM object.

Ah, in that case, the three addons I described as having "no-op usage" will need to be tweaked so that they expose a JS object interface to content with __exposedProps__.

Comment 4

[Bobby Holley (:bholley)]

We really don't want to do anything here until Components-in-content is no longer behind a pref.

Comment 5

[Bobby Holley (:bholley)]

Ok, I think we can do this now. Here are the steps:

1 - Migrate anyone using nsISecurityCheckedComponent (especially the Components object) to using the appropriate nsIClassInfo bit to prevent the object from being created in content scopes.

2 - Stop checking for nsISecurityCheckedComponent in CAPS.

3 - Hoist the CanCreateWrapper check directly into XPConnect.

4 - Do a just-in-case MOZ_CRASH if we somehow reach the nsXPCComponents_Classes constructor in unprivileged code.

5 - Remove the Components wrapper.

Comment 6

[Tom S]

I worked a bit on this. Still need to look at the ClassInfo flags. Some of these checks seem unnecessary to me, because Components should be exposed to content at all, right? https://tbpl.mozilla.org/?tree=Try&rev=f2f67833f36d

Comment 7

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #6)
> I worked a bit on this. Still need to look at the ClassInfo flags. Some of
> these checks seem unnecessary to me, because Components should be exposed to
> content at all, right?

No, it's still exposed to XBL scopes, which run with nsEP. I don't know the status of whether we allow reflector creation for non-DOM XPCWNs in nsEP scopes or not. We may need to add another nsIClassInfo bit called ChromeOnly which we can set for nsXPCComponents_Utils and nsXPCComponents_Classes and whatnot.

Also, I think we should wait until my patches in bug 913734 are landed before tackling this.

Comment 8

[Bobby Holley (:bholley)]

I've got patches for this. It's going to break the addons that are using nsISecurityCheckedComponent, but as-noted above it's only ~10 of them, many of which are cribbing the same code.

https://tbpl.mozilla.org/?tree=Try&rev=78824c372bc4

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove nsISecurityCheckedComponent IID push from Components.interfaces{,ById}. v1

This should have been in bug 951948 bug I missed it.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove custom support for nsISecurityCheckedComponent in nsXPCWrappedJS. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make window.controllers [ChromeOnly], and remove nsISecurityCheckedComponent from nsXULControllers. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove checks for nsISecurityCheckedComponent in caps. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove nsISecurityCheckedComponent interface. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove CheckXPCPermissions. v1

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8360817 [details] [diff] [review]
Part 3 - Make window.controllers [ChromeOnly], and remove nsISecurityCheckedComponent from nsXULControllers. v1

Actually, Boris and I were already talking about this, so I'll flag him for review on this patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8360817 [details] [diff] [review]
Part 3 - Make window.controllers [ChromeOnly], and remove nsISecurityCheckedComponent from nsXULControllers. v1

Do we need to check that we have the system principal?  Or do the cross-origin Xrays we have for pages pointing at each other not end up in that code in resolveNativeProperty?

r=me modulo that

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 8360817 [details] [diff] [review]
> Part 3 - Make window.controllers [ChromeOnly], and remove
> nsISecurityCheckedComponent from nsXULControllers. v1
> 
> Do we need to check that we have the system principal?  Or do the
> cross-origin Xrays we have for pages pointing at each other not end up in
> that code in resolveNativeProperty?

Yeah that's a good point. It's not a problem for cross-origin Xrays (since the property would be filtered out before it was accessed), but it would show up for same-origin unprivileged Xrays in Sandboxes (even though resolving the property would fail, because we wouldn't be able to create the instance in an unprivileged scope).

I added an AccessCheck::isChrome(wrapper) check just to be safe.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8360820 [details] [diff] [review]
Part 6 - Remove CheckXPCPermissions. v1

Oops.

Comment 19

[Bobby Holley (:bholley)]

Thanks for the reviews.

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=7a46f1e200b8

Comment 20

[Bobby Holley (:bholley)]

This looks green, modulo the failures due to bug 859751 (which landed on central for some reason and was backed out when it went orange).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/051af1936834
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56471970fe89
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49e949b48381
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c06f2a97115
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5c0de7c8c5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5379a6ae2dcc

Comment 21

[Bobby Holley (:bholley)]

Followup bustage fix for the XPIDL handlification:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a3ebc36117

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/051af1936834
https://hg.mozilla.org/mozilla-central/rev/56471970fe89
https://hg.mozilla.org/mozilla-central/rev/49e949b48381
https://hg.mozilla.org/mozilla-central/rev/8c06f2a97115
https://hg.mozilla.org/mozilla-central/rev/8b5c0de7c8c5
https://hg.mozilla.org/mozilla-central/rev/5379a6ae2dcc
https://hg.mozilla.org/mozilla-central/rev/13a3ebc36117

Comment 23

[Mike Kaply [:mkaply]]

ARGH. Why wasn't this stuff documented on https://developer.mozilla.org/en-US/Firefox/Releases/29?

Keywords aren't terribly useful if no one does anything with them.

Comment 24

[Mike Kaply [:mkaply]]

Can someone please explain to me how I create an XPCOM Components via Javascript global constructor and allow it to be instantiated in a web page?

See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1005700

Also, checking against AMO was probably not the right thing in this case. This is the kind of API that used by non AMO add-ons to allow things in web pages that normally aren't needed (which is the use case here because CAPS was removed).

Comment 25

[jordan.williams]

love a good "if it's not broke, fix the $hit out of it" situation, but gotta say this one is bumming me out. Maybe addons didn't use it but I sure did; the CAPs at least. Being able to just set a pref to disallow window.alert and window.prompt access was sweet. Now I'm not sure what I'm supposed to do...

Since I'm sure I can't be the only one in the world with this question, might as well ask here, what exactly am I supposed to do to block access to those global functions? Create a global content observer and overwrite with empty functions?

Comment 26

[Mike Kaply [:mkaply]]

(In reply to jordan.williams from comment #25)
> love a good "if it's not broke, fix the $hit out of it" situation, but gotta
> say this one is bumming me out. Maybe addons didn't use it but I sure did;
> the CAPs at least. Being able to just set a pref to disallow window.alert
> and window.prompt access was sweet. Now I'm not sure what I'm supposed to
> do...
> 
> Since I'm sure I can't be the only one in the world with this question,
> might as well ask here, what exactly am I supposed to do to block access to
> those global functions? Create a global content observer and overwrite with
> empty functions?

Can you be more specific as to exactly what were doing and what you need?

Comment 27

[Bobby Holley (:bholley)]

(In reply to jordan.williams from comment #25)
> Since I'm sure I can't be the only one in the world with this question,
> might as well ask here, what exactly am I supposed to do to block access to
> those global functions? Create a global content observer and overwrite with
> empty functions?

A greasmonkey script is probably the simplest way to do this. Should just be a 1-liner.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> and allow it to be instantiated in a web page

Just drop the nsISecurityCheckedComponent bit.  It's not needed in the addon in that bug.

> Being able to just set a pref to disallow window.alert and window.prompt access was sweet.

The CAPS way of doing that wasn't going to work with WebIDL bindings no matter what, for what it's worth.  It was just way too slow.

Comment 29

[jordan.williams]

wow, props for quick responses. 

(In reply to Mike Kaply (:mkaply) from comment #26)
> Can you be more specific as to exactly what were doing and what you need?
I was setting a preference, say setPreference("capability.policy.default.Window.alert", "noAccess"). I have some automation running and I can't allow content scripts to alert/prompt/confirm/print.

(In reply to Bobby Holley (:bholley) from comment #27)
> A greasmonkey script is probably the simplest way to do this. Should just be
> a 1-liner.
I'm more of a 'do it yourself' kind of guy. Not that GM isn't a heck of a convenient tool, but the fewer points of failure the better, so I try to run trim. I just wanted to verify that I wasn't missing out on some equivalent replacement functionality elsewhere (within FF/Gecko).

I'm also kind of lamenting the fact that CAPs is gone, or at least that level of control being so easy. I don't suppose there are CSP strings like "default-alert 'self'"? yeah I know, but there should be.

Comment 30

[Bobby Holley (:bholley)]

(In reply to jordan.williams from comment #29)
> (In reply to Bobby Holley (:bholley) from comment #27)
> > A greasmonkey script is probably the simplest way to do this. Should just be
> > a 1-liner.
> I'm more of a 'do it yourself' kind of guy. Not that GM isn't a heck of a
> convenient tool, but the fewer points of failure the better, so I try to run
> trim. I just wanted to verify that I wasn't missing out on some equivalent
> replacement functionality elsewhere (within FF/Gecko).

FWIW, GreaseMonkey is way better and more flexible for this kind of DIY stuff than the crusty old CAPS code ever was. ;-)

Comment 31

[Kohei Yoshino]

Added this to the site compat doc for reference:
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#DOM

Comment 32

[Mike Kaply [:mkaply]]

I've also added it to Firefox 29 for developers.

We'll need to update this page as well:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISecurityCheckedComponent