1150199 - Allow Langpacks to be signed and un-privileged

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Staś Małolepszy :stas]

I'm filing this in the Marketplace product but this might also be a Gecko bug.

Despite bug 1122277, it's not possible to install langpacks unless the following pref is set:

  user_pref("dom.apps.allow_unsigned_langpacks", true);

Could it be that we're not actually signing langpacks?

Comment 1

[[:fabrice] Fabrice Desré]

I downloaded the first langpack from the answer of https://marketplace.firefox.com/api/v2/langpacks/?dev=firefoxos&fxos_version=3.0 and it seems signed.
I'm investigating further.

Comment 2

[Staś Małolepszy :stas]

Thanks, Fabrice.  The comment in line 506 and the code in http://mxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#514 seem to imply that Gecko expects langpacks to be privileged, while they shouldn't.  They're regular un-privileged apps with signed packages.  Could this be related?

Comment 3

[[:fabrice] Fabrice Desré]

Yes, this is it. You need to set the type to "privileged" since we don't really support unprivileged signed apps.
How bad is that to do this change on your side?

Comment 4

[Staś Małolepszy :stas]

We wanted to default to the more open setting and since langpacks don't use any APIs, un-privileged seemed good.  I recall Mat's first implementation of the Marketplace's langpack chacker actually required langpacks to be privileged but I asked him to change that.

If we were to require langpacks to be privileged, we'll need to:

 - change the langpack builder to add type: privileged to langpacks' manifests,
 - change the langpack checker to require submitted langpacks to be privileged,
 - re-upload existing (testing) langpacks (no biggie).

Before we move on, can we make sure this is the only way?  I really appreciated the fact that we managed to keep langpacks un-privileged and changing that would seem like a step backwards.

Also NI'ing Freddy in case this has security implications.  Privileged langpacks could in theory access some device APIs, but then, they don't have any code in them, so maybe that's fine.  Especially given that they are already run in the context of other apps which are often certified.

Comment 5

[[:fabrice] Fabrice Desré]

I'm sorry for the misunderstanding there. Until now we always considered that signed === privileged.

We could special case apps with a langpack role to need a signature even if they are not privileged. I'm a bit nervous doing that so late in the 2.2 cycle but that's possible.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: unprivileged-langpacks.patch

That passes tests.
Stas, can you verify with a langpack from the marketplace?

Comment 7

[Frederik Braun [:freddy]]

The special case that Fabrice suggests in comment 5 is indeed the desired path:
We'd prefer Langpacks to remain unprivileged, even if they do not contain code.

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8587048 [details] [diff] [review]
unprivileged-langpacks.patch

I tested with

  user_pref("dom.apps.allow_unsigned_langpacks", false);

and it works fine.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8587048 [details] [diff] [review]
unprivileged-langpacks.patch

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

::: dom/apps/Webapps.jsm
@@ +3900,5 @@
> +      allowUnsignedLangpack =
> +        Services.prefs.getBoolPref("dom.apps.allow_unsigned_langpacks");
> +    } catch(e) {}
> +    let isLangPack = newManifest.role === "langpack" &&
> +                     (aIsSigned || allowUnsignedLangpack);

nit: maybe we can avoid getting the pref if status <= maxStatus

Comment 10

[[:fabrice] Fabrice Desré]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57af1d6f7597

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8587048 [details] [diff] [review]
unprivileged-langpacks.patch

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 #): Mis-interpretation of requirement when doing the initial implementation.
User impact if declined: Can't install langpacks from the marketplace.
Testing completed: yes, manual with existing marketplace packages.
Risk to taking this patch (and alternatives if risky): low risk, no alternative.
String or UUID changes made by this patch: none.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7b1ee4c639

Comment 14

[Staś Małolepszy :stas]

Thanks, Fabrice, for fixing this so quickly!