788653 - Make enablePrivilege pref name more dire

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

dbaron reports in bug 757046 comment 55 that app developers are suggesting that users flip the enablePrivilege pref to make their stuff continue to work. This is heinous, because it allows any website anywhere can invoke enablePrivilege and pwn the user's computer.

Per bz's suggestion, I'm in favor of:

security.enablePrivilege.turn_off_all_security_so_that_viruses_can_take_over_this_computer

Does everyone agree? If so, the first step is for Joel to stage this to talos (alongside the other old pref). Once it hits, we can do the rename on m-c and m-a.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

+1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, I don't think we need the "enablePrivilege" part in the pref name.  Gets the rest to a more-likely-to-be-read place, and might make it less likely that people will figure out what the pref does?  ;)

Comment 3

[:Gijs (out for now; he/him)]

Stupid question time: why not go with bz's other suggestion of #ifdef'ing the pref's support? Because people might use non-test builds to run the tests? Or some other reason?

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Gijs Kruitbosch from comment #3)
> Stupid question time: why not go with bz's other suggestion of #ifdef'ing
> the pref's support? Because people might use non-test builds to run the
> tests? Or some other reason?

Egh, missed bug 757046 comment 57. Nevermind!

Comment 5

[Andrew McCreight [:mccr8]]

I wonder if adding telemetry for if this pref is set would be useful.  Though maybe having public stats about how many people are vulnerable is not a good idea.

Comment 6

[Boris Zbarsky [:bzbarsky]]

One other thought.  If we want to make it hard to convince users to add this pref, and if everything working with this pref correctly does UTF-8, we could do two things to make it hard for users to add:

1)  Replace some letter with homographs (e.g. replace Latin 'e' with Cyrillic 'е') to make
    it harder to type the pref name.
2)  Add some zero-width spaces to the pref name at start and end of the pref name to make
    it hard to double-click and copy/paste.

Thoughts?

Comment 7

[Bobby Holley (:bholley)]

Clever idea, Boris!

The affected files are:

nsSecurityManagerFactory.cpp (presumably the pref system is UTF8-friendly?)
automation.py.in
js/src/tests/user.js
whatever talos uses

Joel, do you think we're good with unicode there? If so, we can do something like the following:

security.turn_off_all_sеcurity_so_that_viruses_can_take_over_this_computer

Joel, given that you have access to all the relevant bits (talos etc), is this something you can own?

Comment 8

[Hans Hokke]

(In reply to Boris Zbarsky (:bz) from comment #6)
> 1)  Replace some letter with homographs (e.g. replace Latin 'e' with
> Cyrillic 'е') to make it harder to type the pref name.
That would pose no problem for russian mobsters.
A more alarming name signifying the impact it has on the setup, and not merely a reference the single usecase it was originally intended for may always be better. "Enable tests" does clearly not describe the scope of a setting that opens the computer to several kinds attacks.

How long is this setting gonna be available?

A setting that users have to flip will not be really interesting to a rational virus maker, since only a very limited number of users will have this setting actually flipped. Unless some popular trusted provider asks for it. For a malicious content provider it may be tedious to get the user to flip the setting to facilitate his attack. It will be much easier to provide a malicious extension or an executable. 
So this item may not have a great impact in practice. 

Let's change the name to something more descriptive of the impact and not make too much drama about it.
Something in the line of enableUnsafeContent would be scary enough to me.

Comment 9

[Hans Hokke]

(Sorry I said a few beside the point things in comment 8. I would love to revoke or heavily edit it but that does not seem possible on this forum.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That would pose no problem for russian mobsters.

The point is to make it hard to impossible for users to follow the instruction to add the pref name.  In particular, because they won't realize they need the 'е' as opposed to the 'e'.

> How long is this setting gonna be available?

For as short a time as we can manage.  How long that is depends on how fast we can update our test harnesses.

> will not be really interesting to a rational virus maker

That depends on how many "well-meaning" app authors ask their users to flip the pref.  My goal in this bug is to make it hard for users to want to comply with such a request, and then further to make it hard to comply even if they want to.

> Something in the line of enableUnsafeContent would be scary enough to me.

Frankly, you're not the target audience we're trying to protect here.  You know too much.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but that does not seem possible on this forum

That's because this is a bug database, not a forum.  Different constraints (and different rules!) apply.

Comment 12

[Daniel Veditz [:dveditz]]

Yes, please make the name more dire.

I expect the most common cases for using this pref would be people trying to get their internal apps to work again. They'll probably get the name from some help forum and simply cut/paste -- I don't think a homoglyph helps that case very much in practice. The other place this will come up is in a social engineering attack, again going to be cut/paste.

The zero-width spaces idea is interesting and might work, but might just piss people off and not be much more effective than the dire name alone.

Apart from the name, though, a global foot-gun is terrible. From the start of this particular bug (bug 757046 comment 55) we know at least some users ARE going to use the pref. The dire name is intended to scare most of them away, but until the functionality is gone entirely (oh happy day!) it's going to get used. It's probably worth the effort to make this a per-domain pref.

  security.turn_off_all_sеcurity_and_allow_viruses_from.<prepath>

Where we can pretty easily/efficiently construct the prefname from the principal's URI without needing any complex CAPS code. I chose prePath over plain host so that testers could make file: work, but <scheme> "_" <host> would also work.

Downsides: 
 * more work for us enumerating our test hosts to keep our tests running
 * although this _will_ make users safer, the fact that it constrains
   the evil somewhat might let some site admins convince themselves
   they don't have a problem since of course /their/ site would never
   host malicious code and would never be MITM'd

Anecdotally, I'd also like to note that when browsing with the old signed.applets.codebase_principal_support pref enabled I would occasionally run across public production sites trying to use enablePrivilege. Since they weren't signed pages the average user never saw a prompt and wasn't affected, and because I'm not an idiot I wasn't affected. But if we have a global pref users who enable it will end up with this code trying to run and it's out there. Not sure why, I suspect it was debug code the web site's developers used and was probably merely insecure rather than malicious.

Comment 13

[Daniel Veditz [:dveditz]]

There may not be enough benefit. HTTP sites can be MITM'd so allowing it from domain-X doesn't mean only domain-X (which you presumably trust) will be doing this. Likewise more than half of all sites have an XSS flaw somewhere, so if you flip the pref for only that site you're still unsafe. An origin limitation to the pref does limit the scope of the problem, but not as much as users might think.

How long will we be supporting this pref? I hear "as short as possible", but it's taken so long to get this far that I bet it's not really all that short.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #12)
> Apart from the name, though, a global foot-gun is terrible. From the start
> of this particular bug (bug 757046 comment 55) we know at least some users
> ARE going to use the pref.

Not necessarily. I think the number of admins who would suggest flipping the pref will drop precipitously if they realize the implications. For example, from the sound of comment 8, Hans didn't previously grok the security implications of flipping the pref.

(In reply to Daniel Veditz [:dveditz] from comment #13)
> How long will we be supporting this pref? I hear "as short as possible", but
> it's taken so long to get this far that I bet it's not really all that short.

The pref is entirely unsupported in the sense that its semantics are constrained only by what keeps the tree green. I've already been ripping out lots of UniversalXPConnect checks throughout the DOM, and converting the resulting orange tests to SpecialPowers. So it's likely that intranet sites will break anyway, even if people flip the pref and manage to avoid loading any nefarious web content.

That being said, the long tail is quite long (about 700 tests at the moment), and I certainly wouldn't be quoted saying "as short as possible", because I don't think that aligns with the priority this issue has been given across the org. I'll continue to steadily remove enablePrivilege from tests whenever it stands in the way of me making (the release configuration of) Gecko more secure. But it's only ever going to go away when we do a more coordinated assault.

Also note that throwing interns/contractors/new-hires at the problem is unlikely to succeed, because ~30% of the enablePrivilege tests require a nontrivial understanding of Gecko to convert to SpecialPowers. Though it might be enough to reduce the problem to a more approachable scale.

Comment 15

[Bobby Holley (:bholley)]

(unassigning myself here to reflect the blockage on Talos and the fact that I'm not actively working on this)

Another roadblock we could implement would be to rename the call (enablePrivilege2 or something) so that old uses of enablePrivilege don't work out of the box either. Like everything, this involves some amount of non-trivial but non-insurmountable engineering effort (adding support for the new function, changing our test infrastructure, and then removing support for the old one).

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

For talos, which pref would be preferred to use?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Joel Maher (:jmaher) from comment #16)
> For talos, which pref would be preferred to use?

dveditz, can you (or someone in sec) make the final call here?

Comment 18

[Andrew McCreight [:mccr8]]

For some comparison, Chrome apparently has a command line option "--disable-web-security", that disables same-origin checks, for use testing:

http://stackoverflow.com/questions/3102819/chrome-disable-same-origin-policy

Apparently it pops up a little warning box, though one that isn't particularly scary:

https://productforums.google.com/forum/?fromgroups=#!topic/chrome/r-QGNb0MACo

Comment 19

[Bobby Holley (:bholley)]

Another thought - we could make all the enablePrivilege tests debug-only. It'd certainly lose us some test coverage, but it might be acceptable. I'm not the one to make that call though.

Comment 20

[Bobby Holley (:bholley)]

dveditz - still waiting for a call on the name here.

Comment 21

[Bobby Holley (:bholley)]

dveditz, can you comment? This is really a 10-second thing.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't really meet criteria for tracking, when a decision is made/tested and a patch is reviewed please go ahead with nominating for uplift so we can consider based on risk assessment.

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to Bobby Holley (:bholley) from comment #20)
> dveditz - still waiting for a call on the name here.

Still waiting on a response to my per-origin counter-proposal in comment 12

If completely ignoring it means everyone thinks the gain is not worth the effort and is hoping I'll forget about it (it would increase the amount of work required to get the tests running) then I guess

security.turn_off_all_sеcurity_so_that_viruses_can_take_over_this_computer

is fine, or 

security.test_setting_allows_malware_to_take_over_this_computer

Comment 24

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> (In reply to Bobby Holley (:bholley) from comment #20)
> > dveditz - still waiting for a call on the name here.
> 
> Still waiting on a response to my per-origin counter-proposal in comment 12
> 
> If completely ignoring it means everyone thinks the gain is not worth the
> effort and is hoping I'll forget about it

FWIW, that's not really what happened here. You and I discussed this on IRC at the beginning of September, and I said that I didn't have the cycles to drive something like that, but could provide support if sec-eng wanted to muster up a developer to make this happen.

I'm still not convinced it's a great idea though, because it gives intranet admins an option that's probably acceptable to them but IMO entirely unacceptable to us. But IMO our behavior here is ultimately the sec team's call.

Now that we've got the pref name, flagging joel to make the Talos changes.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add long unsecure pref to talos (1.0)

Lets add this single pref to talos.  We already have another pref:
'security.enablePrivilege.enable_for_tests': True

I assume we need both for the ideal situation here.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

landed the talos bits:
https://bugzilla.mozilla.org/show_bug.cgi?id=788653

Will do a deployment by end of week.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Joel Maher (:jmaher) from comment #26)
> landed the talos bits:
> https://bugzilla.mozilla.org/show_bug.cgi?id=788653
> 
> Will do a deployment by end of week.

Thanks, joel.

I'm pretty swamped with security stuff right now. Would you be able to handle the rest of the bits as well? It just involves changing the string in the three places you see here:

http://mxr.mozilla.org/mozilla-central/search?string=enable_for_tests

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix this for unittests (1.0)

this takes care of the 3 spots in m-c where we have the old pref.

Comment 29

[Bobby Holley (:bholley)]

Oh, I didn't realize we were doing the cyrillic thing.

Boris, can we stick a cyrillic character in a c++ string literal?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

actually this is just plain old ascii.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Boris, can we stick a cyrillic character in a c++ string literal?

C++ strings are just bytes, so I think so, yes.  I guess just sticking the UTF-8 bytes for it in there should work.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix this for unittests with all ascii (2.0)

doing a hg diff showed me the non ascii character, I have adjusted the patch to be ascii only.

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to Bobby Holley (:bholley) from comment #29)
> Oh, I didn't realize we were doing the cyrillic thing.

Sorry, didn't notice I copy/pasted that! The cyrillic character does not add any meaningful protection given people who are going to persist despite the dire pref name.

Of course that assumes the potential victims can read English which is a bad assumption globally. Wonder if we want telemetry on this pref to alert us should it be abused?

Comment 34

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #33)
> Of course that assumes the potential victims can read English which is a bad
> assumption globally. Wonder if we want telemetry on this pref to alert us
> should it be abused?

We already have it: bug 788704. ;-)

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 672457 [details] [diff] [review]
fix this for unittests with all ascii (2.0)

Looks great! r=bholley

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17257a8c6300

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/17257a8c6300

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is now present in our Mozmill test suites and checked across platforms and locales for Firefox >=19.