1359837 - If xpinstall.enabled is false and locked, pages should not be able to use mozAddonManager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Kaply [:mkaply]]

If xpinstall.enabled is set to false and locked, you can still install test pilot:

https://testpilot.firefox.com/

You should not be able to.

I imagine you could also use the discovery pane, but we remove the discovery pane if xpinstall.enabled is false, so I couldn't test that.

Comment 1

[Chuck Harmston [:chuck]]

I'm going to mark this as a security bug, just in case. CC dveditz to make that call.

Comment 2

[Robert Helmer [:rhelmer]]

(In reply to Mike Kaply [:mkaply] from comment #0)
> If xpinstall.enabled is set to false and locked, you can still install test
> pilot:
> 
> https://testpilot.firefox.com/
> 
> You should not be able to.
> 
> I imagine you could also use the discovery pane, but we remove the discovery
> pane if xpinstall.enabled is false, so I couldn't test that.

You can load it directly, for instance:
https://discovery.addons.mozilla.org/en-US/firefox/discovery/pane/55.0a1/Darwin/normal

It does indeed not have any effect - I think you're correct and this pref should be honored for mozAddonManager.

I don't think this needs to be a security bug - having trouble thinking how this could be exploited. It's certainly not something the folks setting this pref probably want though.

Comment 3

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: We'll need this on the ESR if possible.

There appears to be no way to remove this programmatically by overriding the page via a frame script.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Bug 1359837 - Block mozAddonsManager if xpinstall.enabled is false/locked.

Review commit: https://reviewboard.mozilla.org/r/134778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134778/

Comment 5

[Dave Townsend [:mossop]]

How does the test pilot page react when this patch is in and the setting set?

Comment 6

[Mike Kaply [:mkaply]]

Even though there is Javascript in the code that checks for mozAddonManager, it seems to assume it is going to have it.

It still shows the Install button and when you click it, it changes to Installing and then you get the notification that the install was blocked.

So not perfect, but it doesn't do anything really strange. Ideally it should be smart enough to not show the install button.

Comment 7

[Chuck Harmston [:chuck]]

Ref: https://github.com/mozilla/testpilot/issues/2364

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8862897 [details]
Bug 1359837 - Block mozAddonsManager if xpinstall.enabled is false/locked.

https://reviewboard.mozilla.org/r/134778/#review138160

r+ for the code but please check that the test pilot site and the discovery page do something sane before landing this

Comment 9

[Mike Kaply [:mkaply]]

Here's code you can use on the Chrome scratchpad to test this

var prefs = Services.prefs.getDefaultBranch(null); 
prefs.setBoolPref("xpinstall.enabled", false);
prefs.lockPref("xpinstall.enabled");

Comment 10

[Andy McKay]

cc'ing Stuart. If this about:config pref is set, mozAddonsManager will not be present. So we might want to ensure we've got a fallback for that in the future. I think right now it falls back to just linking to the .xpi and the normal installation flow.

Comment 11

[:shell escalante]

just making sure this is good to land and you don't need anything :)

Comment 12

[Stuart Colville [:muffinresearch]]

(In reply to Andy McKay [:andym] from comment #10)
> cc'ing Stuart. If this about:config pref is set, mozAddonsManager will not
> be present. So we might want to ensure we've got a fallback for that in the
> future. I think right now it falls back to just linking to the .xpi and the
> normal installation flow.

Yep we have a fallback in place now if mozAddonmanager isn't available so this shouldn't require special handling at the amo end of things.

Comment 13

[Mike Kaply [:mkaply]]

Here's a video of the experience on the test pilot site.

https://youtu.be/MCCWyQ4O4N0

I think the way it works today is fine (no need to update the site)

Comment 14

[Mike Kaply [:mkaply]]

Video in comment 13 is for you, Dave, to make sure you are OK with it.

Comment 15

[Chuck Harmston [:chuck]]

+ni John Gruen to review that experience. Any changes to it are likely on the Test Pilot side, though.

Comment 16

[John Gruen [:jgruen]]

thanks :chuck. will flag this for the UX team in our meeting tomorrow; i bet they can crank out a fix pretty fast. I'll leave the NI open until we resolve in GH

Comment 17

[Dave Townsend [:mossop]]

The user gets a message saying what happens. This seems fine to me.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4639f041d809
Block mozAddonsManager if xpinstall.enabled is false/locked. r=mossop

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4639f041d809

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request Beta and ESR52 approval on this when you get a chance. Also, I don't suppose we could write a test for this, could we?

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8862897 [details]
Bug 1359837 - Block mozAddonsManager if xpinstall.enabled is false/locked.

Approval Request Comment
[Feature/Bug causing the regression]: Addon installs from test pilot
[User impact if declined]: Admins can't prevent installs from test pilot
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Using snippet in bug, lock preferences. Verify install doesn't work.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low.
[Why is the change risky/not risky?]: Very specialized in what it does.
[String changes made/needed]:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Admin can't prevent addon installs
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Mike Kaply [:mkaply]]

As far as a test goes, I'm investigating.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8862897 [details]
Bug 1359837 - Block mozAddonsManager if xpinstall.enabled is false/locked.

respect xpinstall.enabled locking for addons manager, beta54+

Should be in 54.0b13

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5e1be51863d

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8862897 [details]
Bug 1359837 - Block mozAddonsManager if xpinstall.enabled is false/locked.

ESR admins should be able to prevent installs from Test Pilot, ESR52+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3669ab00f59d

Comment 27

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

I encounter some problems verifying this issue:

It seems that the issue mentioned in comment 0 is not fixed on FX55 because it can be reproduced by following the steps mentioned in comment 9 on Firefox 55.0a1 (Build Id:20170606030207) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.

This issue is not fixed on Firefox 54.0 (Build Id:20170605134926) on macOS 10.11.6 (It seems that it is fixed on Firefox 54.0 usin Windows 10 64bit and Ubuntu 16.04 64bit).

Can you take a look into this?

Thanks!

Comment 28

[Mike Kaply [:mkaply]]

So it looks like the behavior is different on the Test Pilot site, but in my testing, the installs are still blocked.

If you click the intial button, it says test pilot is installed but it is not. If you then click on an experiment and then click the install button, you get a message that the install was blocked.

Is this what you are seeing?

Comment 29

[Emil Ghitta, QA [:emilghitta]]

I can see the following:

On Firefox 56.0a1(BuildId:20170619030208):

After clicking the button to install the Test Pilot, the installation process is successful and the add-on is actually installed. 
Also this behavior is the same with the experiments (they are successfully installed and added to Firefox)

On Firefox 55.0b3(BuildId:20170619071723):
Test pilot is not installed and the message that the install was blocked is displayed (On Windows 10 64bit and Ubuntu 16.04 64bit).

Test pilot is installed and added to Firefox on macOS 10.11.6.

Here is a video of this issue on 56.0a1 (https://drive.google.com/open?id=0B94iuIVMr-TEQVBOYnBUcWZVejg).

Thanks!

Comment 30

[Emil Ghitta, QA [:emilghitta]]

Hi Mike!

Any thoughts regarding comment 29 ?

Thanks!

Comment 31

[Mike Kaply [:mkaply]]

On Firefox 56, prefs->PrefIsLocked("xpinstall.enabled", &isLocked); is coming back as false.

This seems like a pretty serious regression. I'm investigating.

Comment 32

[Mike Kaply [:mkaply]]

This is e10s related. If e10s is enabled, the check for lockpref fails. There's a comment here:

http://searchfox.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#613

That says:

ENSURE_MAIN_PROCESS("Cannot check PrefIsLocked from content process:", aPrefName);

but I don't get this error anywhere. And when I remove that line, it still doesn't work.

Comment 33

[Mike Kaply [:mkaply]]

I verified this is working with 1394578 in. Please retest on nightly

Comment 34

[Emil Ghitta, QA [:emilghitta]]

I can confirm that this issue is verified fixed using Firefox 59.0a1 (BuildId:20171205100345) on Windows 10 64bit, macOS 10.11 and Ubuntu 16.06 64bit.