Remove nsISecurityCheckedComponent

RESOLVED FIXED in mozilla29

Status

()

Core
Security: CAPS
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug, 4 keywords)

unspecified
mozilla29
addon-compat, compat, dev-doc-needed, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

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.
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

5 years ago
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.
(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__.
We really don't want to do anything here until Components-in-content is no longer behind a pref.
Depends on: 856424
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.
Depends on: 913734
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
(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.
Depends on: 951948
Blocks: 957688
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
Created attachment 8360815 [details] [diff] [review]
Part 1 - Remove nsISecurityCheckedComponent IID push from Components.interfaces{,ById}. v1

This should have been in bug 951948 bug I missed it.
Attachment #8360815 - Flags: review?(mrbkap)
Created attachment 8360816 [details] [diff] [review]
Part 2 - Remove custom support for nsISecurityCheckedComponent in nsXPCWrappedJS. v1
Attachment #8360816 - Flags: review?(mrbkap)
Created attachment 8360817 [details] [diff] [review]
Part 3 - Make window.controllers [ChromeOnly], and remove nsISecurityCheckedComponent from nsXULControllers. v1
Attachment #8360817 - Flags: review?(mrbkap)
Created attachment 8360818 [details] [diff] [review]
Part 4 - Remove checks for nsISecurityCheckedComponent in caps. v1
Attachment #8360818 - Flags: review?(mrbkap)
Created attachment 8360819 [details] [diff] [review]
Part 5 - Remove nsISecurityCheckedComponent interface. v1
Attachment #8360819 - Flags: review?(mrbkap)
Created attachment 8360820 [details] [diff] [review]
Part 6 - Remove CheckXPCPermissions. v1
Attachment #8360820 - Flags: review?(mrbkap)
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.
Attachment #8360817 - Flags: review?(mrbkap) → review?(bzbarsky)
Blocks: 960392
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
Attachment #8360817 - Flags: review?(bzbarsky) → review+
(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.

Updated

4 years ago
Attachment #8360815 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8360816 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8360818 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8360819 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8360820 - Flags: superreview+
Comment on attachment 8360820 [details] [diff] [review]
Part 6 - Remove CheckXPCPermissions. v1

Oops.
Attachment #8360820 - Flags: superreview+
Attachment #8360820 - Flags: review?(mrbkap)
Attachment #8360820 - Flags: review+
Thanks for the reviews.

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=7a46f1e200b8
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
Followup bustage fix for the XPIDL handlification:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a3ebc36117
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 965715
Keywords: compat, dev-doc-needed
Blocks: 965921
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.
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).
Depends on: 1005806

Comment 25

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

3 years ago
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.
(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. ;-)
Added this to the site compat doc for reference:
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#DOM
Keywords: site-compat
OS: Mac OS X → All
Hardware: x86 → All
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

Updated

3 years ago
Depends on: 1010137

Updated

3 years ago
Depends on: 1010340

Updated

3 years ago
Depends on: 1010311

Updated

3 years ago
Depends on: 1010577

Updated

3 years ago
No longer depends on: 1010340
You need to log in before you can comment on or make changes to this bug.