Last Comment Bug 750859 - Gut unused CAPS code
: Gut unused CAPS code
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 789327
Blocks: deadcode 546848 750661
  Show dependency treegraph
 
Reported: 2012-05-01 13:16 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2013-01-16 09:25 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1 (9.11 KB, patch)
2012-05-01 15:27 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2 - Kill the CAPS confirm dialog. v1 (6.28 KB, patch)
2012-05-01 15:27 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
martijn.martijn: feedback+
jruderman: feedback+
Details | Diff | Splinter Review
Part 3 - Remove (most of) SetCanEnableCapability. v1 (15.38 KB, patch)
2012-05-01 15:27 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4 - Remove {Disable,Revert}Capability. v1 (9.51 KB, patch)
2012-05-01 15:28 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 13:16:19 PDT
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.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 15:27:15 PDT
Created attachment 620098 [details] [diff] [review]
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 15:27:31 PDT
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.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 15:27:46 PDT
Created attachment 620100 [details] [diff] [review]
Part 3 - Remove (most of) SetCanEnableCapability. v1
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 15:28:02 PDT
Created attachment 620101 [details] [diff] [review]
Part 4 - Remove {Disable,Revert}Capability. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-05-01 15:32:40 PDT
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.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:17:54 PDT
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.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:20:33 PDT
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.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:20:49 PDT
Comment on attachment 620098 [details] [diff] [review]
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1

r=me
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:23:48 PDT
Comment on attachment 620100 [details] [diff] [review]
Part 3 - Remove (most of) SetCanEnableCapability. v1

r=me, but this needs iid changes.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:25:21 PDT
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....
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-05-01 18:25:55 PDT
Comment on attachment 620101 [details] [diff] [review]
Part 4 - Remove {Disable,Revert}Capability. v1

This seems fine.
Comment 12 Jesse Ruderman 2012-05-01 18:32:37 PDT
Comment on attachment 620099 [details] [diff] [review]
Part 2 - Kill the CAPS confirm dialog. v1

I stopped using enablePrivilege a long time ago :)
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-05-02 01:35:50 PDT
(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.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-05-02 01:40:28 PDT
(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 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-02 01:44:58 PDT
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.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-05-02 01:49:02 PDT
(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.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-05-02 01:51:18 PDT
(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?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 07:04:42 PDT
> 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 19 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 14:47:55 PDT
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
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 14:48:37 PDT
We should track this; I fully expect people were using this stuff....
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-05-02 15:02:45 PDT
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 :-)
Comment 22 :Ehsan Akhgari 2012-05-02 16:26:40 PDT
Thanks Bobby!
Comment 24 :Ehsan Akhgari 2012-05-02 22:15:20 PDT
This regressed Dromaeo for some reason...
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2012-05-03 00:51:06 PDT
(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?
Comment 26 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-05 05:37:36 PDT
Everything still seems to work when using enablePrivilege from a local file.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2012-05-05 08:59:46 PDT
(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?
Comment 28 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-05 09:02:28 PDT
Yeah, I was. Glad it got removed!
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2012-05-05 09:09:54 PDT
(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.
Comment 30 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-05 09:29:45 PDT
Perhaps, because I allowed it in an older build without this fix?
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2012-05-05 09:48:34 PDT
(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.
Comment 32 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-05 09:54:00 PDT
Yes, with a new profile, it is denied.
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2012-09-10 11:03:56 PDT
The fact that this didn't get dev-doc-needed and relnote keywords is epic fail.  :(
Comment 34 Jesse Ruderman 2012-09-10 12:04:56 PDT
It's too late for relnote, but it's not too late for docs.  What needs documenting (or un-documenting)?
Comment 35 Bobby Holley (:bholley) (busy with Stylo) 2012-09-10 12:06:39 PDT
(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 Jesse Ruderman 2012-09-10 12:09:35 PDT
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 Jesse Ruderman 2012-09-10 12:37:57 PDT
Also filed bug 790023 to update other MDN docs. And shaved some yaks.

Note You need to log in before you can comment on or make changes to this bug.