The default bug view has changed. See this FAQ.

Gut unused CAPS code

RESOLVED FIXED in Firefox 15

Status

()

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

People

(Reporter: bholley, Assigned: bholley)

Tracking

({addon-compat, dev-doc-needed})

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

Firefox Tracking Flags

(firefox15 fixed)

Details

Attachments

(4 attachments)

This needs to happen anyway, and will just make our lives easier later. I'm doing it now because we're hitting the Win PGO wall again and we need to land some code reductions.

Updated

5 years ago
Blocks: 750661
Created attachment 620098 [details] [diff] [review]
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1
Attachment #620098 - Flags: review?(bzbarsky)
Created attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

This will break addons using enablePrivilege, but that's going away too. We've been warning for many releases now, so it's time to bite the bullet.
Attachment #620099 - Flags: review?(bzbarsky)
Created attachment 620100 [details] [diff] [review]
Part 3 - Remove (most of) SetCanEnableCapability. v1
Attachment #620100 - Flags: review?(bzbarsky)
Created attachment 620101 [details] [diff] [review]
Part 4 - Remove {Disable,Revert}Capability. v1
Attachment #620101 - Flags: review?(bzbarsky)
pushed this all to try: https://tbpl.mozilla.org/?tree=Try&rev=1b5599ad9efd

There's a whole bunch more code here that's _almost_ useless, but it's too scary to touch until we can rip out the privilege manager stuff entirely. We're closing in on it, but we're not there yet.
Keywords: addon-compat
So the more I think about this, the more it seems like we should disable this stuff without removing the code and see whether enough stuff breaks compat-wise that we have to reenable it (which is a lot easier than reinstating code).

And yes, we've been warning about it.  I'm not sure people listen to warnings much.
Do we have hard data on usage or not-usage of the functions in part 1?  Though I suspect you're right that they're rare.
Blocks: 546848
Comment on attachment 620098 [details] [diff] [review]
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1

r=me
Attachment #620098 - Flags: review?(bzbarsky) → review+
Comment on attachment 620100 [details] [diff] [review]
Part 3 - Remove (most of) SetCanEnableCapability. v1

r=me, but this needs iid changes.
Attachment #620100 - Flags: review?(bzbarsky) → review+
Comment on attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

I'm pretty sure this will break some of the QA stuff Martijn and Jesse are doing....
Attachment #620099 - Flags: feedback?(martijn.martijn)
Attachment #620099 - Flags: feedback?(jruderman)
Comment on attachment 620101 [details] [diff] [review]
Part 4 - Remove {Disable,Revert}Capability. v1

This seems fine.
Attachment #620101 - Flags: review?(bzbarsky) → review+

Comment 12

5 years ago
Comment on attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

I stopped using enablePrivilege a long time ago :)
Attachment #620099 - Flags: feedback?(jruderman) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #7)
> Do we have hard data on usage or not-usage of the functions in part 1? 
> Though I suspect you're right that they're rare.

None of them are used in AMO. disablePrivilege is used in one place, but it's commented out. :-)

On the other hand, there are 181 addons using enablePrivilege.
(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 620100 [details] [diff] [review]
> Part 3 - Remove (most of) SetCanEnableCapability. v1
> 
> r=me, but this needs iid changes.

I revved the IID in that patch, no? Or are you referring to something else?
Comment on attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

I guess I would need to use a different way to run privileged script inside content. The SpecialPowers way sort of does that, although it's a major pain to use.
Anyway, I don't do a lot of fuzzing anymore.
Attachment #620099 - Flags: feedback?(martijn.martijn) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #6)
> So the more I think about this, the more it seems like we should disable
> this stuff without removing the code and see whether enough stuff breaks
> compat-wise that we have to reenable it (which is a lot easier than
> reinstating code).


TBH, I was thinking of this patch as just that. We're just killing the confirm dialog, not the entire privilege manager, so people can still set the pref and have their stuff whitelisted for another 6 weeks or so. This patch is trivial to revert IMO (since this code doesn't really change much), and it's much smaller than the eventual "rip-out-the-privilege-manager" patch.

We could indeed just disable this code without removing it, but I'm not sure that would help much. It wouldn't receive any more continued testing by remaining in the tree (modulo things that the compiler can detect), and we're trying to cut fat to help on the WinPGO front (note that this patch also allows us to remove SavePrincipal in the ensuing patch).

More to the point, I think the threshold of backpedaling on enablePrivilege needs to be pretty darn high, at which point I don't think the effort of backing out this patch is all that significant.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #15)
> Comment on attachment 620099 [details] [diff] [review]
> Part 2 - Kill the CAPS confirm dialog. v1
> 
> I guess I would need to use a different way to run privileged script inside
> content. The SpecialPowers way sort of does that, although it's a major pain
> to use.

A few months ago I landed a really convenient API for doing arbitrary stuff. See bug 750638 comment 0, about half-way through for a description of how to use it (the SpecialPowers.wrap stuff).

Does that make stuff easier for you?
> I revved the IID in that patch, no?

Yes, but you changed two interfaces.  Rev nsIScriptSecurityManager too.

Also, part 4 needs IID changes too, in theory.
Comment on attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

Please file a followup bug to look into removing the strings from the stringbundle?

r=me
Attachment #620099 - Flags: review?(bzbarsky) → review+
We should track this; I fully expect people were using this stuff....
tracking-firefox15: --- → ?
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2ce6373e5e04
http://hg.mozilla.org/integration/mozilla-inbound/rev/05f7445feda3
http://hg.mozilla.org/integration/mozilla-inbound/rev/2a59d26bc6c7
http://hg.mozilla.org/integration/mozilla-inbound/rev/e0d9d5a0987b

Heading to bed now, but I wanted to get this in to help out on this stuff. If it goes red/orange, you know what to do :-)
Target Milestone: --- → mozilla15
Thanks Bobby!
https://hg.mozilla.org/mozilla-central/rev/2ce6373e5e04
https://hg.mozilla.org/mozilla-central/rev/05f7445feda3
https://hg.mozilla.org/mozilla-central/rev/2a59d26bc6c7
https://hg.mozilla.org/mozilla-central/rev/e0d9d5a0987b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
This regressed Dromaeo for some reason...
(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> This regressed Dromaeo for some reason...

That doesn't make a whole lot of sense, given what the patches are. You sure it wasn't one of the nearby libxul patches?
Everything still seems to work when using enablePrivilege from a local file.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #26)
> Everything still seems to work when using enablePrivilege from a local file.

Yeah, I think that was always allowed. This change just killed the security prompt for using enablePrivilege on an untrusted site.

You weren't seeing the prompt before, right?
Yeah, I was. Glad it got removed!
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #28)
> Yeah, I was. Glad it got removed!

Oh, hm. In places were we were prompting before, we now unconditionally deny. Are you sure your call is still succeeding? If it is, we need to look into this ASAP.
Perhaps, because I allowed it in an older build without this fix?
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #30)
> Perhaps, because I allowed it in an older build without this fix?

Yeah, it's possible it was remembered in your profile. Can you try again with a clean profile to make sure the access is denied where you formerly had a confirm dialog? I'd like to be 100% sure about this.
Yes, with a new profile, it is denied.
status-firefox15: --- → fixed
tracking-firefox15: ? → ---
The fact that this didn't get dev-doc-needed and relnote keywords is epic fail.  :(

Comment 34

5 years ago
It's too late for relnote, but it's not too late for docs.  What needs documenting (or un-documenting)?
Keywords: dev-doc-needed
(In reply to Jesse Ruderman from comment #34)
> It's too late for relnote, but it's not too late for docs.  What needs
> documenting (or un-documenting)?

The fact that the enablePrivilege confirm dialog went away (enablePrivilege goes away entirely in bug 757046).

Comment 36

5 years ago
I updated https://developer.mozilla.org/en-US/docs/Bypassing_Security_Restrictions_and_Signing_Code and filed bug 787269 to torch the old docs.

Comment 37

5 years ago
Also filed bug 790023 to update other MDN docs. And shaved some yaks.

Updated

4 years ago
Depends on: 789327
You need to log in before you can comment on or make changes to this bug.