1274332 - Add testpilot.firefox.com to the xpinstall whitelist

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

+++ This bug was initially created as a clone of Bug #744355 +++

(dug up this bug to clone, I hope it's still relevant)

This is related to https://github.com/mozilla/testpilot/issues/715

Test Pilot recently launched, and for a few reasons we'd like to self-host. This is a project with a lot of user impact, and we'd like to have a seamless install experience.

Comment 1

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Moving this to Firefox :: General, because it looks like the Fx Desktop Team handles the implementation here.

Comment 2

[Justin Dolske [:Dolske]]

Hmm, this isn't a trivial change. As Andy noted on the Github issue, this is part of the defenses against drive-by software installs, and whitelisting TestPilot would mean that site needs to pass a high bar to make sure there's no opportunity for it to be used as a malware installation vector. If the security team signs off on it, I've no objection though.

Comment 3

[Andy McKay]

I think it is just a matter of adding the domain to the whitelist, so I would say its a pretty simple change, but one that shouldn't be take without security input. Of note we might also want to get testpilot access to the new AddonsManager API that we added in bug 1245571, because that gives testpilot access to some cool APIs.

Comment 4

[Daniel Veditz [:dveditz]]

dolske didn't mean the code would be complex, he meant the decision should not be taken lightly.

One big reason not to allow it is precedent: EVERYONE wants to self-host with a nicer experience because they are not evil so why should there be any roadblocks? Is TestPilot enough "a part of Firefox" that we can avoid those pressures and arguments?

If add-on installing is too easy users think they are trivial and safe, they do not feel like "installing software" which is exactly what it is. We allow a nicer experience from AMO because our review process is supposed to uphold the user's expectations about "safe", but it has the unfortunate side-effect of reinforcing the belief that add-ons are something you can try and remove if you don't like it with no harm done. Signing was initially supposed to spread that "safe-ness" to add-ons installed anywhere, but we didn't have the bandwidth to fulfill the review requirements that would take: signing now only means we have a better way to revoke add-ons after they've caused enough damage that word gets back to us. Unfortunately the UI changed based on that initial assumption and the scary (but ignored) warning about installing software being unsafe were removed or toned down. This extra "Allow" step is all we have. 

TestPilot can add itself to the whitelist so that subsequent installs of the individual experiments are nicer. Is that good enough? or is this request about installing the initial TestPilot addon itself?

Another approach would be to send people to the TestPilot page on AMO to install it. Yes, it's an extra step, but it reinforces the teaching "AMO is the safe place to get add-ons".

Or you could cooperate with the AMO folks to create a static page that just triggers the TestPilot install, and see if they could change their X-Frame-Options header for that one page from "DENY" to "ALLOW-FROM https://testpilot.firefox.com".

If you're self-hosting because you don't want to sit in the AMO review queue that's an organizational failure that can be solved -- either we fix our review queues because if it sucks for us it's sucking for everyone, or you set up a relationship so your add-on gets short-cuts to the top of the line.

And finally some technical security worries: once a site is on this whitelist the add-ons themselves can live anywhere (e.g. AMO is on the whitelist but the add-ons come from the CDN). That means if someone can inject HTML/XSS content into your site they can easily fool users into installing malicious stuff. They don't have to get the the malicious add-on itself uploaded to our server, just signed by our no-review service for non-hosted add-ons. Has your site gotten a security review? What coding practices and security defenses are you using? How big/complex is it? mostly static? dynamic, and if so what languages and frameworks are you using?

NB: an attacker doesn't have to get script running on your site so CSP is of limited help here. All they have to do is inject a static link/button that they can get the user to click on.

For website security this might fall under Julien's Cloud Security team since this will effectively be the security equivalent of AMO if you add it to the whitelist.

Comment 5

[Les Orchard [:lorchard]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> dolske didn't mean the code would be complex, he meant the decision should
> not be taken lightly.

Yeah, I think we should all be on the same page about that.

> Is TestPilot enough "a part of Firefox" that we can avoid those
> pressures and arguments?

I don't want to speak for product folks or anyone else, but my understanding is that testpilot.firefox.com wants to grow up to be a core piece of the feature feedback loop in Firefox product development.

> TestPilot can add itself to the whitelist so that subsequent installs of the
> individual experiments are nicer. Is that good enough? or is this request
> about installing the initial TestPilot addon itself?

This is about installing the initial Test Pilot add-on itself. It's a bootstrap: once installed, it manages subsequent experiment installs (and removals) without the warning.

> Another approach would be to send people to the TestPilot page on AMO to
> install it. Yes, it's an extra step, but it reinforces the teaching "AMO is
> the safe place to get add-ons".

There's no AMO page for the current incarnation of Test Pilot - and I don't think we want one.

While the bootstrap & experimental features are currently built as add-ons, I don't think we want to encourage thinking about Test Pilot in terms of add-ons as such. That implementation choice has potential to change and drift away from what AMO aims to support. So, I don't think AMO should be a home or entry point for Test Pilot stuff.

Granted, a potential outcome for a feature going through Test Pilot is to be intentionally spun off as an add-on project - but that's an exit path from Test Pilot.

> Or you could cooperate with the AMO folks to create a static page that just
> triggers the TestPilot install, and see if they could change their
> X-Frame-Options header for that one page from "DENY" to "ALLOW-FROM
> https://testpilot.firefox.com".

This option is mentioned in Issue #715 [4]. Could be that's what we end up doing if we don't pass whitelist muster. Then again, this feels like passing the buck to AMO if we're not capable of being trusted in the whitelist. We should get Test Pilot into proper shape.

[4] https://github.com/mozilla/testpilot/issues/715

> Has your site gotten a security review? What coding practices and security
> defenses are you using? How big/complex is it? mostly static? dynamic, and
> if so what languages and frameworks are you using?
> ...
> For website security this might fall under Julien's Cloud Security team
> since this will effectively be the security equivalent of AMO if you add it
> to the whitelist.

I think this process is underway in bug 1255231 (moz confidential)

Comment 6

[Julien Vehent]

> I think this process is underway in bug 1255231 (moz confidential)

We have done a high level risk assessment but have not performed a security audit of the testpilot website itself. :claudijd started poking at the staging site recently and :psiinon will include it in the automated ZAP vulnerability scans in Q3. Until we get the time to perform a complete audit of testpilot.firefox.com, we should probably err on the conservative side.

Comment 7

[Julien Vehent]

Jonathan finished his pentest and didn't find any major issue in testpilot itself (but we have an ops concern we're working on). From my point of view, we should first implement CSP before giving the site more privileges. The team already has an issue for it: https://github.com/mozilla/testpilot/issues/882

Comment 8

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Thanks Les for responding.

(In reply to Les Orchard [:lorchard] from comment #5)
> I don't want to speak for product folks or anyone else, but my understanding
> is that testpilot.firefox.com wants to grow up to be a core piece of the
> feature feedback loop in Firefox product development.

Just a quick +1 here. Test Pilot has been identified as a core program for the foreseeable future. All experiments deployed through the program are approved by our Mission Control (Director and VP levels).

(In reply to Julien Vehent [:ulfr] from comment #7)
> Jonathan finished his pentest and didn't find any major issue in testpilot
> itself (but we have an ops concern we're working on). From my point of view,
> we should first implement CSP before giving the site more privileges. The
> team already has an issue for it:
> https://github.com/mozilla/testpilot/issues/882

:ulfr - thanks! Are you recommending that once we resolve #882, we should be good for whitelisting?

Comment 9

[Julien Vehent]

> :ulfr - thanks! Are you recommending that once we resolve #882, we should be good for whitelisting?

Agreeing to the whitelisting is the prerogative of the platform security team.

From my point of view, there are two outstanding items to bring testpilot to a security level comparable with AMO:
1. implement CSP (without unsafe-inline or unsafe-eval)
2. move the admin behind the vpn (bug 1258766)

I forgot about #2 in my previous comment. When the two items are resolved, I would consider the platform secure according to cloudservices's standards, but I cannot speak to the eligibility to being whitelisted.

Comment 10

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Julien Vehent [:ulfr] from comment #9)
> From my point of view, there are two outstanding items to bring testpilot to
> a security level comparable with AMO:
> 1. implement CSP (without unsafe-inline or unsafe-eval)
> 2. move the admin behind the vpn (bug 1258766)

Thank you! We are tracking both of these items and hope to have them resolved soon.

> Agreeing to the whitelisting is the prerogative of the platform security
> team.

I'm going to bounce this back to :dveditz to ask if there are any other questions or updates he'd like to see on the site.

Comment 11

[Daniel Veditz [:dveditz]]

I'm unhappy that the revamped install UI no longer shows the URL for the add-on in the confirmation dialog, which means if AMO or testpilot got hacked all users would see is "testpilot.mozilla.org would like to install an add-on: Nifty-Feature" and not that the URL is https://haha.alqaida.com/evil.xpi -- but that's somewhat tangential to the concerns here.

I'm happy to hear the issues in comment 9 are being addressed. Neither is enough to remove all worry, however. CSP (without 'unsafe' loopholes) is a second-line of defense that can prevent injected scripts from running and injected remote loads, but it does not prevent the site security problem that allows for the injection. Static non-remote HTML will render if the site has an injection problem and that could be enough to spoof someone into installing. Users will get the confirmation dialog, but if they got spoofed into clicking there's nothing on that dialog that would make them suspicious (this is the relevance of the first para here, though that would be a slim hope for saving users).

So back to my questions about how the site is coded and whether thought was taken to minimize or eliminate content injection? Is it big and complex, dynamic? or is it small and easy to audit? There's no user-supplied content (unlike AMO) so that helps, and it's hard to imagine there are forms or anything users submit?

Moving the admin console behind the VPN protects it against direct attack, but since the source code is open that doesn't prevent XSS-type attacks against those pages targeted at actual admins who presumably have access behind the VPN. Not a show stopper, just a caution not to let your guard down just because we've strung up some "do not enter" tape.

testpilot is/will be part of the bug bounty program -- they're coming for you!

With that said, it should be fine to add testpilot to the install permission whitelist.

Comment 12

[Wil Clouser [:clouserw]]

Thanks Dan.  An aside, but you might be interested in https://github.com/mozilla/testpilot/issues/988 -- we're considering dropping the database and dynamic part of the site completely and just going to flat files.

Dolske - what are the next steps to this bug?  Also, is this something we can uplift?

Comment 13

[Justin Dolske [:Dolske]]

(In reply to Wil Clouser [:clouserw] from comment #12)

> Dolske - what are the next steps to this bug?  Also, is this something we
> can uplift?

As long as the security folks are ok with the change, I expect this is basically a rubber-stamp review for the actual Firefox change. That is, the main risk here is that testpilot.firefox.com getting popped is a pretty terrible prospect, and so as long as security folks are satisfied with the relevant mitigations I don't see any further issues.

Comment 6 and comment 11 suggest that there's no fundamental security objection, but that that there were a few specific things to address before making this change. I'd suggest following up with Julien and Dan to make sure they're satisfied with the progress.

I think this would be fine for rapid uplift.

Comment 14

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Daniel/Julien - can you please confirm that Test Pilot is cleared for whitelisting? Thanks.

Comment 15

[Daniel Veditz [:dveditz]]

Never got an answer to my questions in comment 11, but the main thrust from looking at github is "written in python using Django". Who is responsible for keeping those libraries up to date, what's the plan for that?

CSP has been added (with a lot of seemingly useless explicit http: entries?) but the bug to move the admin panel off the open internet is still unresolved (bug 1258766).

Comment 16

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Never got an answer to my questions in comment 11, but the main thrust from
> looking at github is "written in python using Django". Who is responsible
> for keeping those libraries up to date, what's the plan for that?
NI :clouserw on this one.

> CSP has been added (with a lot of seemingly useless explicit http: entries?)
> but the bug to move the admin panel off the open internet is still
> unresolved (bug 1258766).
Looking to :ulfr to confirm this is indeed a requirement.

Comment 17

[Julien Vehent]

The admin panel is a concern. Moving it behind VPN is possible, but the database user in the frontend currently needs full access to all tables, negating the benefit of moving the admin role behind a secured location. I'd like to have another chat with the dev team to figure out what can be done there.

Purely from a risk perspective, this is no better nor worse than the current state of AMO, but it's important we do not replicate the bad practices from the past and stop hosting admin panels on the public internet.

Comment 18

[Julien Vehent]

We add a quick chat today with :wclouser and :ckprice about the exposure of /admin/ to the internet. The current thinking is testpilot is likely to be moved to a static site, without admin panel or database, within the next quarter or two. Doing work to secure the admin right now seems unnecessary.

With that cleared out, the testpilot website checks all the boxes to be whitelisted in Firefox.

Comment 19

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Thanks Julien!

:dolske - we've been cleared for take off here! How best to get this updated (and hopefully uplifted)?

Comment 20

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Attachment: Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist

Took a stab at this. My first patch so please bear with me. :dolske r?

Comment 21

[Justin Dolske [:Dolske]]

Hmm, I thought the permissions file was only used for the initial defaults, and that adding a new default for existing profiles needed the foo.whitelist.add.# pref form (PermissionsUtils :: importPrefBranch). But maybe that was just for 3rd party xpiinstall additions? I've not touched this in too long, and bug 1256598 didn't do that.

Matt, is the permissions file change all that's needed now?

Comment 22

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #21)
> Hmm, I thought the permissions file was only used for the initial defaults,
> and that adding a new default for existing profiles needed the
> foo.whitelist.add.# pref form (PermissionsUtils :: importPrefBranch). But
> maybe that was just for 3rd party xpiinstall additions? I've not touched
> this in too long, and bug 1256598 didn't do that.
> 
> Matt, is the permissions file change all that's needed now?

Yes, markh fixed this since the old method had a problem whereby Clear Recent History would nuke xpinstall/UITour permissions. That file is now read on every startup and those permissions are kept in memory and not persisted to the DB unless the defaults from the file are overridden.

Comment 23

[Justin Dolske [:Dolske]]

Comment on attachment 8771110 [details] [diff] [review]
Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist

Review of attachment 8771110 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the /browser change, you'll need review from a Android reviewer for the /mobile change. (Which seems fine to me, but I'd wonder why Android is not using a permissions file to avoid the problem Matt described.)

Congratulations on your first patch. :)

Comment 24

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #23)
> r+ on the /browser change, you'll need review from a Android reviewer for
> the /mobile change. (Which seems fine to me, but I'd wonder why Android is
> not using a permissions file to avoid the problem Matt described.)

That's bug 1072744 btw. and bug 1072748 for TB.

Comment 25

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Hi Kit! Mind giving the /mobile change an r? (/browser was reviewed by dolske).

Comment 26

[John Gruen [:jgruen]]

Stefan, mind giving the /mobile change an r?

Comment 27

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Sebastian, can you take a look at this from the Android perspective?

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8771110 [details] [diff] [review]
Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist

Review of attachment 8771110 [details] [diff] [review]:
-----------------------------------------------------------------

> Sebastian, can you take a look at this from the Android perspective?

LGTM. Although there's no test pilot experiment on Fennec and the website doesn't really interact with Fennec. But I guess it doesn't hurt to add this now anyways.

Comment 29

[John Gruen [:jgruen]]

Hey Dolske, looks like we've got an R+ on Mobile. Next steps?

Comment 30

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Spoke to dolske a bit. Setting as `checkin-needed` and requesting for uplift.

Comment 31

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Comment on attachment 8771110 [details] [diff] [review]
Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist

Approval Request Comment
[Feature/regressing bug #]:

https://github.com/mozilla/testpilot/issues/715

When installing Test Pilot, user sees the red warning.

[User impact if declined]:

We could be losing users who are apprehensive about this warning.

[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 

Low risk, adding an entry into an existing structure.

[String/UUID change made/needed]:

N/A

Comment 32

[Carsten Book [:Tomcat]]

has problems to apply:

patching file browser/app/permissions
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file browser/app/permissions.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 33

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Carsten Book [:Tomcat] from comment #32)
> 1 out of 1 hunks FAILED -- saving rejects to file browser/app/permissions.rej
Thanks! Any way I can see the contents of this file?

Comment 34

[Wil Clouser [:clouserw]]

I also don't see why that patch would conflict.  We're trying to patch http://hg.mozilla.org/firefox/file/tip/browser/app/permissions right?

Comment 35

[Justin Dolske [:Dolske]]

For some reason that file is using DOS line endings, and the patch is against a file with normal (unix) line endings. Cory's editor probably automatically changed it.

We should probably just fix the file, but for now here's an updated patch.

Comment 36

[Justin Dolske [:Dolske]]

Attachment: Same patch w/ matching line endings

Comment 37

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/0b1fc540eace69753abe0f344e061dba3e9df6fc
Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist. r=dolske,s.kaspari

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 8779520 [details] [diff] [review]
Same patch w/ matching line endings

Approval Request Comment
(see comment 31)

Comment 39

[Wil Clouser [:clouserw]]

Comment on attachment 8779520 [details] [diff] [review]
Same patch w/ matching line endings

Approval Request Comment

See comment 31 for details.

Since we're on a fresh new train I'm requesting uplift to beta as well.  We see a large drop off of people clicking the install button and then not installing the add-on (presumably, due to this warning).  Shipping this sooner will get more people using Test Pilot which directly affects top line goals.  Thanks!

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b1fc540eace

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8779520 [details] [diff] [review]
Same patch w/ matching line endings

Whitelisting our own extensions sounds good.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a63354e882d9

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/231dfddf9f25

Comment 44

[Cornel Ionce [:noni] [Hubs QA]]

Test Pilot is now correctly whitelisted and installed without the red warning.

Tested on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 12.04x86 using:
*latest 51.0a1 Nightly, build ID 20160906030431
*latest 500a2 Aurora, build ID 20160906004000
*Fx 49.0 RC, build ID 20160905130425.