1019714 - Allowing sideloading of certified apps allows access to certified data.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

The ability to install certified apps allows access to certified apps, even though we prevent debugging. An attacker with access to connect devtools to a phone can do at least the following two things:

1.  set the origin of an app in the manifest, to replace an existing app with an app that reads the data of the certified app. You can even just reinstall gaia apps as privileged, while allows you to debug them.

2. install an app that reads any shared data store (e.g. settings, read the passcode and display it to the screen)

So firstly, we need a patch which prevents origin from being *.gaiamobile.org from be installed or sideloaded. At least then we cant do things like read the email password so easily. I wonder if there is some way you can combine embed-apps permissions to simulate the same thing, but so long as you c

Secondly, we need a better solution for enabling certified app debugging. We have been discussing this today with the devtools team and thinking about an option here. But we will document and circulate for discussion separate to this bug.

Comment 1

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

PS I think this is probably a sec-moderate. You need physical access to an unlocked phone, at which point you have almost full access anyways, but this issue just makes the impact of this scenario slightly worse.

Comment 2

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

We need to fix this sooner rather than later. This will likely block 2.1 due to partner requirements to protect data.

I'm not sure if these needs to be fixed in webapps.jsm or devtools though? Actually I guess we need both to be sure.

Comment 3

[Alexandre Poirot [:ochameau]]

This should be about rejecting certified side loading from devtools codebase.
We can forbid it is devtools.debugger.forbid-certified-apps is true (which is the default).
Today, we forbid debugging them (i.e. having console,inspector,debugger connected to one),
but still can push new ones.

Comment 4

[[:fabrice] Fabrice Desré]

Alex solution looks reasonable to me. Paul, I'm not sure to understand what we gain by prevent apps with a *.gaiamobile.org domain from being installed, unless we have a bug that let us override an already installed app.

Comment 5

[Stephanie Ouillon [:arroway] (needinfo me)]

Fabrice, that's exactly the bug we have: somebody can install her own certified/privileged app having the same origin as an already installed certified app (such as email for example) and accessed its data.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Stephanie Ouillon [:arroway] from comment #5)
> Fabrice, that's exactly the bug we have: somebody can install her own
> certified/privileged app having the same origin as an already installed
> certified app (such as email for example) and accessed its data.

ouch. let's fix that then.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch

This patch should prevent installing certified if devtools.debugger.forbid-certified-apps == true (the default)
Installing privileged will still be accepted no matter what is the value of this pref.

https://tbpl.mozilla.org/?tree=Try&rev=252e7508ce46

Comment 8

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #7)
> Created attachment 8452379 [details] [diff] [review]
> patch
> 
> This patch should prevent installing certified if
> devtools.debugger.forbid-certified-apps == true (the default)
> Installing privileged will still be accepted no matter what is the value of
> this pref.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=252e7508ce46

Don't we just want to forbid existing app to be overwritten?

Comment 9

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #8)
> (In reply to Alexandre Poirot (:ochameau) from comment #7)
> > Created attachment 8452379 [details] [diff] [review]
> > patch
> > 
> > This patch should prevent installing certified if
> > devtools.debugger.forbid-certified-apps == true (the default)
> > Installing privileged will still be accepted no matter what is the value of
> > this pref.
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=252e7508ce46
> 
> Don't we just want to forbid existing app to be overwritten?

Basically - we don't want to allow sideloading of apps for an origin that already exists. But I'm not sure how we know "an origin that already exists" other than blacklisting. I.E  What I was expecting here was forbidding 'sideloading apps which have an origin of "*.gaiamobile.org". 

Or could just ignore the origin manifest parameter if devtools.debugger.forbid-certified-apps flag is true (would this protect privileged apps from a similar attack?)

Comment 10

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

Actually ignore privileged apps, you can debug them anyways.

Comment 11

[Paul Rouget [:paul]]

The App Manager would specify a prefix ("xxx.canBeOverwritten.gaiamobile.org"), and on the phone, we would only allow app to be overwritten if 1) it's a non-certified app, or 2) it has the canBeOverwritten.gaiambileorg prefix.

Of course, if any certified app is installed on the phone with the devtools prefix, it will be overwrittabled.

Comment 12

[Alexandre Poirot [:ochameau]]

Isn't pushing brand new certified apps also an issue?

Otherwise I'm not sure overloading is only an issue for certified?
You could also stole valuable information from hosted apps with no particular privileges.

Comment 13

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #12)
> Isn't pushing brand new certified apps also an issue?

What is "brand new certified apps"> Apps sideloaded by the app manager?

> Otherwise I'm not sure overloading is only an issue for certified?
> You could also stole valuable information from hosted apps with no
> particular privileges.

We can already do it with the devtools.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #13)
> (In reply to Alexandre Poirot (:ochameau) from comment #12)
> > Isn't pushing brand new certified apps also an issue?
> 
> What is "brand new certified apps"> Apps sideloaded by the app manager?

Certified apps being sideloaded that doesn't already exists on the phone.
I just want to confirm that we wont be asked to also prevent installing certified apps at all (without the magic pref)
You don't get access to the storage API data, but you get access to all certified API,
and may be that already too much without the magic pref.

> 
> > Otherwise I'm not sure overloading is only an issue for certified?
> > You could also stole valuable information from hosted apps with no
> > particular privileges.
> 
> We can already do it with the devtools.

We are only talking about limiting actions that we can already do without the pref...

Comment 15

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #14)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #13)
> > (In reply to Alexandre Poirot (:ochameau) from comment #12)
> > > Isn't pushing brand new certified apps also an issue?
> > 
> > What is "brand new certified apps"> Apps sideloaded by the app manager?
> 
> Certified apps being sideloaded that doesn't already exists on the phone.

So why "pushing brand new certified apps [is] also an issue" ?
They have access to many APIs, but can't steal data from other certified apps, can they?

Comment 16

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

Fabrice, do you know the answer to Paul's question in comment 15?

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to J. Ryan Stinnett [:jryans] from comment #16)
> Fabrice, do you know the answer to Paul's question in comment 15?

No I don't have an answer. Paul is right that even a certified app can't eg. access another app indexedDB database. Anyway I'll talk with pauljt face to face this week about that.

Comment 18

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

Okay, we'll wait for a further update from Fabrice or :pauljt then before proceeding.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Paul T: Any updates here. It seems to me like it's fine to allow sideloading of certified apps, as long as we prevent sideloading to overwrite existing certified apps.

Comment 20

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

(In reply to Jonas Sicking (:sicking) from comment #19)
> Paul T: Any updates here. It seems to me like it's fine to allow sideloading
> of certified apps, as long as we prevent sideloading to overwrite existing
> certified apps.

Sorry this fell of my radar. Yes for now I was thinking we could just prevent overwriting apps which have an origin of *.gaiamobile.org. 

If we prevent overwriting of certified apps completely, wont that basically break the feature? I.e. I install certified for testing, and after that I can't change it?

Alternatively, Stephanie and I wrote a proposal to bring back a developer mode [1]. Basically the idea was to give the user a chance to enable developer mode upon first-run, or after a factory reset. If we had this, it might be simpler just to forbid any interactions with certified-apps (debugging/installing) unless developer mode is enabled.


[1] https://docs.google.com/a/mozilla.com/document/d/11Q1_fj2nKciVyG2PdGH_LuiH09BhZJKzFjBuIcaHKqs/edit#heading=h.p6yd2hzg2hfp

Comment 21

[Dietrich Ayala (:dietrich)]

Reviewed in triage - it's not clear why this longstanding issue would block 2.1, so blocking-. Paul, if there's a reason you think this should block 2.1 instead of riding the trains please explain and re-nominate the bug.

Comment 22

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

Not sure I can make this any more plain than comment 5 but I'll try.

Its longstanding since the proper solution is hard. But as a minimum step we really need to block the ability to replace Gaia apps. As it currently stands, if I have your phone I can sideload a version of gaia apps onto it. So its unlikely since the attacker needs access to a physical unlocked device, but it would be trivial with this access to:

- read sensitive data that is usually not available without root access (email password etc)
- backdoor the device by installing a modified version of Gaia that did something malicious (sends all your photos somewhere?)

Does that make it more clear? I wavered on calling this sec-high, but the only thing that stopped me was that you do need physical access to a device without a pass code, so the likelihood is probably low. But the impact is very severe.

Comment 23

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

[Blocking Requested - why for this release]:

Comment 24

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

To summarise, I am nominating this to fix the issue of being able to overwrite Gaia apps. That should be a relatively small and low risk change that brings significant security improvement (namely that you cant sideload an app that can read another apps data).

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

New patch. Instead of rejecting any certified app install,
we now only blacklist install whose origin contains gaiamobile.org.

Note that, there is already a condition that should prevent overloading system apps
when passing the origin as argument of the install request:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#575
But it is defeated if we use another way of overloading the origin:
use `origin` attribute in manifest.webapp.

So that could be another option. Instead of blacklisting by origin,
we could reject overloading non-removable apps... ??

Comment 26

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

> 
> So that could be another option. Instead of blacklisting by origin,
> we could reject overloading non-removable apps... ??

If you can do that, this would be a better approach (and remove the chance that you could somehow bypass the blacklist)

Comment 27

[bhavana bajaj [:bajaj]]

Passing this onto poirot, given I see a patch from him, please re-assign as needed.
Blocking given Paul's recommendation.

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

https://tbpl.mozilla.org/?tree=Try&rev=0255b9ad4a66

Here is a completely new patch, that drastically changes how we limit the features
of the webapps actor. Instead of forbidding access to certified apps while allowing pushing new ones,
we do not allow to do anything related to apps already installed on the phone.
Instead we only allow pushing new apps, that are not already installed.
(i.e. we don't allow overloading apps installed by the user via marketplace or preinstalled apps)
We no longer make a difference between apps being certified or not,
as private data may be stolen from apps installed via the marketplace (and are priviledged or hosted).

It works by flagging apps installed via devtools.
Then we only accept debugging and overloading apps that have the devtools flag.
(here the flag is named 'sideloaded')

Then, as before, if you toggle the certified pref, you can basically do whatever you want.

While working on this patch I also improve the check to prevent overloading non-removable apps.
(It should be now redundant with this new devtools flag, but anyway it was partially rejecting such scenario)

And now I also prevent uninstalling apps that don't have the devtools flag as well!

Comment 29

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

(In reply to Alexandre Poirot [:ochameau] from comment #28)
> Created attachment 8497477 [details] [diff] [review]
> patch v3
> 
> https://tbpl.mozilla.org/?tree=Try&rev=0255b9ad4a66
> 
> Here is a completely new patch, that drastically changes how we limit the
> features
> of the webapps actor. Instead of forbidding access to certified apps while
> allowing pushing new ones,
> we do not allow to do anything related to apps already installed on the
> phone.
> Instead we only allow pushing new apps, that are not already installed.
> (i.e. we don't allow overloading apps installed by the user via marketplace
> or preinstalled apps)
> We no longer make a difference between apps being certified or not,
> as private data may be stolen from apps installed via the marketplace (and
> are priviledged or hosted).
> 
> It works by flagging apps installed via devtools.
> Then we only accept debugging and overloading apps that have the devtools
> flag.
> (here the flag is named 'sideloaded')
> 
> Then, as before, if you toggle the certified pref, you can basically do
> whatever you want.
> 
> While working on this patch I also improve the check to prevent overloading
> non-removable apps.
> (It should be now redundant with this new devtools flag, but anyway it was
> partially rejecting such scenario)
> 
> And now I also prevent uninstalling apps that don't have the devtools flag
> as well!

This sounds like a good approach to me Alex. The only issue that I can think of is related to the embed-apps permission. If you can debug an app that is allowed to embed another app, can you effectively debug any app, regardless of the devtools flag?  I'm not sure, but there really is no reason for any app other than the system app to have the embed-apps permission, so I am raising a separate bug to enforce this restriction.

Comment 30

[Alexandre Poirot [:ochameau]]

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

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

Fabrice, I also flagged you for review as I'm adding a "sideloaded" attribute on internal webapps object.
This attribute has to be stored within the webapps.json file.

Comment 31

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

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

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

::: toolkit/devtools/server/actors/webapps.js
@@ +245,5 @@
>      let self = this;
>  
> +    if (aId in reg.webapps && !reg.webapps[aId].sideloaded &&
> +        !this._areCertifiedAppsAllowed()) {
> +      throw new Error("Can't overloaded apps not being installed by devtools.");

Nit: "Replacing non-sideloaded apps is not permitted."

@@ +388,5 @@
>          let manFile = OS.Path.join(aDir.path, "manifest.webapp");
>          return AppsUtils.loadJSONAsync(manFile);
>        }
>      }
> +    function writeManifest(arg) {

There must be a better parameter name than this...

@@ +513,5 @@
> +          // Prevent overloading system apps
> +          if (id in DOMApplicationRegistry.webapps &&
> +              DOMApplicationRegistry.webapps[id].removable === false &&
> +              !self._areCertifiedAppsAllowed()) {
> +            self._sendError(deferred, "The application " + id + " can't be overriden.");

Nit: overridden

@@ +603,1 @@
>                 message: "The application " + appId + " can't be overriden."

Nit: overridden

@@ +703,4 @@
>      });
>    },
>  
>    _areCertifiedAppsAllowed: function wa__areCertifiedAppsAllowed() {

Should this be renamed since it's no longer about certified apps?  Maybe "_isUnrestrictedAccessAllowed"?

@@ +703,5 @@
>      });
>    },
>  
>    _areCertifiedAppsAllowed: function wa__areCertifiedAppsAllowed() {
>      let pref = "devtools.debugger.forbid-certified-apps";

Is it time to have a new pref?  This one is now completely meaningless...  People would have to set a new pref again though.  Just an idea.  It's quite odd that the pref name has no connection to what it does.

@@ +707,5 @@
>      let pref = "devtools.debugger.forbid-certified-apps";
>      return !Services.prefs.getBoolPref(pref);
>    },
>  
> +  _isAppAllowed: function wa__isAppAllowedForManifest(aApp) {

Fix this function name (or remove it).

Comment 32

[[:fabrice] Fabrice Desré]

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

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

I'm not a big fan of using the "removable" state as an equivalent to "certified", but I may be misunderstanding what you're trying to do here.

::: toolkit/devtools/server/actors/webapps.js
@@ +511,5 @@
>            }
>  
> +          // Prevent overloading system apps
> +          if (id in DOMApplicationRegistry.webapps &&
> +              DOMApplicationRegistry.webapps[id].removable === false &&

that's not checking if the app is certified. Why not use webapps[id].appStatus ?

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

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

Comment 34

[Alexandre Poirot [:ochameau]]

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

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

(In reply to J. Ryan Stinnett [:jryans] from comment #31)
> >      let pref = "devtools.debugger.forbid-certified-apps";
> 
> Is it time to have a new pref?  This one is now completely meaningless... 
> People would have to set a new pref again though.  Just an idea.  It's quite
> odd that the pref name has no connection to what it does.

I don't know. We have to see the benefits of having a better pref name vs asking dev to set a new one and update various scripts to set the new pref.
That would be to do something in a dedicated bug, to sync up with the various scripts/tools setting this pref.

(In reply to Fabrice Desré [:fabrice] from comment #32)
> Comment on attachment 8497477 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8497477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a big fan of using the "removable" state as an equivalent to
> "certified", but I may be misunderstanding what you're trying to do here.

That's not my goal here. The goal of this patch is to restrict devtools
to only push *new* apps and only be able to debug these new apps being sideloaded.
Here, I'm just tweaking webapps actor checks, it shouldn't even be possible
to do anything with non-removable apps as they aren't flagged as "sideloaded".
That's just an extra, explicit check, to match this existing one:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#587
I updated the comment to make it clearer.

Comment 35

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

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

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

This is the permissions patch for bug 1064108, not this bug.

Comment 36

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Oops, here is the right patch for this bug...

With new try, after some test fixes:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=41ff1347e3dd

Comment 37

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Rebased to resolve conflicts.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fd669321cd9d

Comment 39

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

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

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 approval on this when you get a chance.

Comment 41

[Alexandre Poirot [:ochameau]]

Comment on attachment 8511854 [details] [diff] [review]
patch v5

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None, always been there.
User impact if declined: Devtools may open ways to steal user private data.
Testing completed: Limited, only tested locally on simulator and device, but juste landed on m-c.
Risk to taking this patch (and alternatives if risky): may introduce unexpected limitation to debugging apps via devtools
String or UUID changes made by this patch: none

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a0330dc750b1

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/63f5aa98332c

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

Comment 44

[Alexandre Poirot [:ochameau]]

Attachment: 2.1 patch

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0953b46ee9a8

Comment 46

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

We need to document this policy change.  CCed Will tentatively for this.  I'll also email with more details about the change.

Comment 47

[Christiane Ruetten [INACTIVE]]

Sounds like the bug never made it into any release, so setting [b2g-adv-main2.2-].