Remove nsISecurityCheckedComponent

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug, 4 keywords)

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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

7 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.
(Assignee)

Comment 3

7 years ago
(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__.
(Assignee)

Comment 4

6 years ago
We really don't want to do anything here until Components-in-content is no longer behind a pref.
Depends on: 856424
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 951948
(Assignee)

Updated

5 years ago
Blocks: 957688
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 9

5 years ago
This should have been in bug 951948 bug I missed it.
Attachment #8360815 - Flags: review?(mrbkap)
(Assignee)

Comment 14

5 years ago
Attachment #8360820 - Flags: review?(mrbkap)
(Assignee)

Comment 15

5 years ago
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+
(Assignee)

Comment 17

5 years ago
(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.
Attachment #8360815 - Flags: review?(mrbkap) → review+
Attachment #8360816 - Flags: review?(mrbkap) → review+
Attachment #8360818 - Flags: review?(mrbkap) → review+
Attachment #8360819 - Flags: review?(mrbkap) → review+
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+
(Assignee)

Comment 19

5 years ago
Thanks for the reviews.

Final all-platform try push:

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

Comment 21

5 years ago
Followup bustage fix for the XPIDL handlification:

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

Updated

5 years ago
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).

Comment 25

5 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?
(Assignee)

Comment 27

5 years ago
(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

5 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.
(Assignee)

Comment 30

5 years ago
(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
You need to log in before you can comment on or make changes to this bug.