1064108 - Prevent side-loading of apps with engineering mode permission

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Engineering mode (bug 997564) allows partners to add APIs to gecko for engineering purposes (debug etc). They will install their own apps to use these permissions. Since this API can be very dangerous (equivalent to root level code execution) and there is no use case for thrid-party apps having these we should prevents apps getting these permissions at all if the phone is not already rooted. 

IE I would like the protect against the risk that an attacker with access to the phone could elevate privileges by side-loading an app with the engineering mode permission. I imagine that we can add a check to the devtools actor responsible for installing apps to ensure that engineering-mode permission is ONLY allowed when the devtools.debugger.forbid-certified-apps is set to false (i.e. the phone is already in debug mode).

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

Here is a patch to do that.
It is based on top of bug 1019714,
not that it is based on it but modifies same code.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: patch

Ok, so similar patch but with a blacklist in a pref.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c46258e9e119

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Rebased patch.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8509017 [details] [diff] [review]
patch v3

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

Paul, nothing changed? are we still on the same page with this bug and bug 1019714?
This will significantly change the restrictions of the webapps actor, hopefully it becomes safe now.
And we don't want to change it multiple times over releases.
The discussion with jan about unrestricted mode doesn't conflict with these tweaks
as it might only open up new possibilities, so it will be easier to explain if we end up changing that again.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8509017 [details] [diff] [review]
patch v3

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

::: modules/libpref/init/all.js
@@ +738,5 @@
>  pref("devtools.debugger.prompt-connection", true);
>  // Block tools from seeing / interacting with certified apps
>  pref("devtools.debugger.forbid-certified-apps", true);
> +// List of permissions that a sideloaded app can't ask for
> +pref("devtools.forbidden-permissions", "embed-apps,engineering-mode,embed-widgets");

Let's use a more specific name, like "devtools.apps.forbidden-permissions", since the permissions are really about apps, not DevTools.

::: toolkit/devtools/server/actors/webapps.js
@@ +493,5 @@
>              return;
>            }
>  
> +          // Completely forbid pushing apps asking for unsafe permissions
> +          if ("permissions" in manifest) {

Should the unsafe permissions be allowed if were are already in "unrestricted" DevTools mode?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8509017 [details] [diff] [review]
patch v3

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

r+ assuming you tweak the pref name.

::: toolkit/devtools/server/actors/webapps.js
@@ +493,5 @@
>              return;
>            }
>  
> +          // Completely forbid pushing apps asking for unsafe permissions
> +          if ("permissions" in manifest) {

As mentioned on IRC, a future "OS developer mode" could set this pref to an empty string to disable this protection.

That seems more flexible than tying everything into this one global "unrestricted" pref as we've been doing so far.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

New pref name.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08e2ccf37eda

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8509593 [details] [diff] [review]
patch v4

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

Looks good to me.

Comment 10

[Carsten Book [:Tomcat]]

Hi, seems the patch didn't apply cleanly:

patching file toolkit/devtools/apps/tests/unit/test_webappsActor.js
Hunk #1 FAILED at 276
1 out of 1 hunks FAILED -- saving rejects to file toolkit/devtools/apps/tests/unit/test_webappsActor.js.rej
patching file toolkit/devtools/server/actors/webapps.js
Hunk #2 FAILED at 487
1 out of 2 hunks FAILED -- saving rejects to file toolkit/devtools/server/actors/webapps.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1064108---Prevent-sideloading-apps-with-too-pe.patch

could you take a look and maybe rebase? Thanks!

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Carsten Book [:Tomcat] from comment #10)
> Hi, seems the patch didn't apply cleanly:
> 
> could you take a look and maybe rebase? Thanks!

Here is a rebase on today's m-c.
Note that this patch is made on top of bug 1019714 one.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: 2.1 patch

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

Rebased today, with a new try, just in case:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2f03bda1d585

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/fbb1eaf088b1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbb1eaf088b1

Comment 17

[Alexandre Poirot [:ochameau]]

Paul, I imagine we want to also uplift this patch, since we uplifted bug 1019714?
Would there be anything else to fix/uplift with this change?

Comment 18

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Yes this should be uplifted since engineering mode is 2.1.
I'm not aware of anything apart from 1019714 (but I will comment separately in that bug to make sure i understand it though).

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8516697 [details] [diff] [review]
2.1 patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  bug 1019714 changed the security pattern used for devtools and offered new ways to get sensible permissions. This patch prevent accessing them.
User impact if declined: 
  Unsafety introduced by devtools
Testing completed: 
  Branch specific patch, launched tests locally and patch baked on master for a bit.
Risk to taking this patch (and alternatives if risky): 
  Limited to devtools codepath.
String or UUID changes made by this patch:
  None

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 8516697 [details] [diff] [review]
2.1 patch

Approving given Paul's feedback in comment #18. Please flag QA if you need any specific testing once this lands on 2.1

Comment 22

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

b2g34_v2_1: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/75880d8b6628

Comment 23

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

Backed out from b2g34 in https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/622f480eee8b for m-oth orange:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=40168&repo=mozilla-b2g34_v2_1

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: 2.1 patch with working mochitest-chrome

Sorry Wes, I thought I tested locally, but it appears I didn't.
Here is a try run this time:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4146208c1b9e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3d7442bb08ff