750859 - Gut unused CAPS code

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

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)]

Attachment: Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: 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)]

Attachment: Part 3 - Remove (most of) SetCanEnableCapability. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove {Disable,Revert}Capability. v1

Comment 5

[Bobby Holley (:bholley)]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 620098 [details] [diff] [review]
Part 1 - remove unused (or seldom-used) PrivilegeManager functions. v1

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 620101 [details] [diff] [review]
Part 4 - Remove {Disable,Revert}Capability. v1

This seems fine.

Comment 12

[Jesse Ruderman]

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)]

(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)]

(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 (dead)]

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)]

(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)]

(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 [:bzbarsky]]

> 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 [:bzbarsky]]

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 [:bzbarsky]]

We should track this; I fully expect people were using this stuff....

Comment 21

[Bobby Holley (:bholley)]

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

[(no longer active)]

Thanks Bobby!

Comment 23

[(no longer active)]

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

Comment 24

[(no longer active)]

This regressed Dromaeo for some reason...

Comment 25

[Bobby Holley (:bholley)]

(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 (dead)]

Everything still seems to work when using enablePrivilege from a local file.

Comment 27

[Bobby Holley (:bholley)]

(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 (dead)]

Yeah, I was. Glad it got removed!

Comment 29

[Bobby Holley (:bholley)]

(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 (dead)]

Perhaps, because I allowed it in an older build without this fix?

Comment 31

[Bobby Holley (:bholley)]

(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 (dead)]

Yes, with a new profile, it is denied.

Comment 33

[Boris Zbarsky [:bzbarsky]]

The fact that this didn't get dev-doc-needed and relnote keywords is epic fail.  :(

Comment 34

[Jesse Ruderman]

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)]

(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]

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]

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