+++ 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).

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.

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

Rebased patch.

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 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 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.

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

Comment on attachment 8509593 [details] [diff] [review] patch v4 Review of attachment 8509593 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

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!

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

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

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?

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

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

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

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