Carmen Jimenez Cabezas Reporter
	Description • 11 years ago

STR: 
1. Prepare a build that includes operator variant apps from a store.
2. Don't include the store signing certificate on the phone build.
3. Flash the build and boot the phone.

Expected: 

The apps are installed correctly, since being local installed apps the signing should not be checked

Actual: 
The signature is verified and the apps aren't installed since the verification fails (the signing certificate isn't trusted)

	Jason Smith [:jsmith]
	Comment 1 • 11 years ago

I don't understand why the expected behavior makes sense. Our privileged app design argues for requiring the app to be signed with the correct signature to be installed. Breaking that model means we're opening a security hole in the build process that operators could abuse unknowingly.

	Carmen Jimenez Cabezas Reporter
	Comment 2 • 11 years ago

The expected behavior makes sense because when the operator selects a set of apps to create the build he expects that they will be installed and he doesn't expect that the installation fails for security reasons that are designed for remote download (not for local installation).

There are three ways to solve this:
1.- Modify gecko so the signature isn't verified on a local installation.
2.- Modify the build process to delete the signature from the download packages before the image is created.
3.- Do nothing and to force all app images origin to be a valid origin

From my point of view the more flexible solution is the second one.

	Jason Smith [:jsmith]
	Comment 3 • 11 years ago

(In reply to Carmen Jimenez Cabezas from comment #2)
> The expected behavior makes sense because when the operator selects a set of
> apps to create the build he expects that they will be installed and he
> doesn't expect that the installation fails for security reasons that are
> designed for remote download (not for local installation).

Ah, okay. Makes sense. So this is blocking installing locally installed preinstalled packaged apps from working on 1.2 then right? Was this working on 1.1? If it's a regression, then it should probably be a blocker for 1.2.

	Carmen Jimenez Cabezas Reporter
	Comment 4 • 11 years ago

This functionality didn't exist in 1.1. I wouldn't go as far as calling this blocking since it has an easy work around: don't use signed zips as input for the build. After the patch is written we can always ask for approval to uplift to 1.2

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 5 • 11 years ago

This patch has two effects:

* If a locally installed app (single variant operator app currently) is signed with a expired certificate, consider the file as signed correctly.
* If a locally installed app is signed but the signature validation fails for some other reason, consider it *not* signed, and apply the rest of the validations as usual (so if the file must be signed it will fail and throw still, but if it's from an origin that doesn't force signing, it will be installed correctly).

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 6 • 11 years ago

Requesting koi because without this either the phones come with a valid current date, or all the single variant apps which are downloaded from the Mozilla store will not be installed correctly (since the certificate validation will fail).

I didn't take Carmen's second approach (removing the signature) since the origin forces us to be signed.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 7 • 11 years ago

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #5)
> This patch has two effects:
> 
> * If a locally installed app (single variant operator app currently) is
> signed with a expired certificate, consider the file as signed correctly.

This is not something we should do. Is there another solution to the problem this solves? (Indeed, what is that problem?)

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 8 • 11 years ago

The problem is that when a device boots for the first time (newly taken out of the box) it usually has a date set on the dark ages (1970? 1980 if you're lucky). And the single variant operator apps (which are pre-copied on the device) are installed as soon as the user enters his SIM, which is two or three screens before he can even try to set the date. So the app installation launches before the correct date can be set, and then it fails because... well, the store certificate wasn't valid on the 70s.

So what I did was to relax the validation *only* when the app being installed is being installed from a file:// URL. In that case, and if the error that the signature validation gives is certificate expired, I consider the signature to be valid.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 9 • 11 years ago

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #8)
> The problem is that when a device boots for the first time (newly taken out
> of the box) it usually has a date set on the dark ages (1970? 1980 if you're
> lucky). And the single variant operator apps (which are pre-copied on the
> device) are installed as soon as the user enters his SIM, which is two or
> three screens before he can even try to set the date. So the app
> installation launches before the correct date can be set, and then it fails
> because... well, the store certificate wasn't valid on the 70s.

Is it a requirement that these apps be installed before the user gets to the screen that sets the date?
Since the user just inserted a SIM card, can they get the date from the network before proceeding?

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 10 • 11 years ago

(In reply to David Keeler (:keeler) from comment #9)
> (In reply to Antonio Manuel Amaya Calvo (:amac) from comment #8)
> > The problem is that when a device boots for the first time (newly taken out
> > of the box) it usually has a date set on the dark ages (1970? 1980 if you're
> > lucky). And the single variant operator apps (which are pre-copied on the
> > device) are installed as soon as the user enters his SIM, which is two or
> > three screens before he can even try to set the date. So the app
> > installation launches before the correct date can be set, and then it fails
> > because... well, the store certificate wasn't valid on the 70s.
> 
> Is it a requirement that these apps be installed before the user gets to the
> screen that sets the date?
> Since the user just inserted a SIM card, can they get the date from the
> network before proceeding?

The installation is launched as soon as we can get a MCC/MNC (which if the SIM isn't PIN protected can even be before the FTU is even shown). At this moment there's no way to know from the installation code if a date has been set or not (not to mention that maybe the user likes thinking he lives in the 80s and the pre-distributed apps still should be installed correctly). As for getting the date from the network (I'm assuming the mobile network here) apparently not all networks broadcast that information.

	Jason Smith [:jsmith]
	Comment 11 • 11 years ago

Paul - Can you weigh in on whether we should do this or not? I'm concerned this could open an unexpected hole in our privileged apps security model.

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 12 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #11)
> Paul - Can you weigh in on whether we should do this or not? I'm concerned
> this could open an unexpected hole in our privileged apps security model.

Just to be clear:

* This patch applies only to applications that are installed locally (that is, that are installed from a file:// URI). If the application is installed from a remote URI, the complete signature validation, including the certificate expiration time, is applied.

* Only Gecko itself can install apps from a file:// URI

* From that point of view, the apps this change affect are equivalent to certified apps (since they have to come with the phone, and their installation is launched only from Gecko itself

* Privilege security level < Certified security level, and certified apps are not even signed. We trust them just because they're already on the phone (same as the apps this change affects).

* There's currently an option to side-load apps on the phone (using the emulator). Sideloading allows loading external apps (with an a priori lower trust level) and the sideloaded apps can be privileged. Again, no signature is even required for sideloading privileged apps.

The reason why certified apps do not require a signature is because the trust level of having the app predistributed with the phone is much higher than the trust level of any externally downloaded app, signed or not. And the apps whose signature validation I've relaxed with this patch are also predistributed with the phone (they're just not installed so we can choose at runtime which ones to install).

Taking all of the previous into account I fail to see how this can open a hole on the security model (which was/is designed to establish some measure of trust on apps that are downloaded from an external source).

	[:fabrice] Fabrice Desré
	Comment 13 • 11 years ago

Comment on attachment 8334718 [details] [diff] [review]
V1. Relax the signature validation for locally installed apps

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

Fine for me. Please land once the sec guys come to an agreement ;)

::: dom/apps/src/Webapps.jsm
@@ +8,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  
> +// Possible errors thrown by the signature verifier

nit: full stop at the end of the comment.

@@ +17,5 @@
> +const SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE = (SEC_ERROR_BASE + 30);
> +const SEC_ERROR_UNTRUSTED_CERT = (SEC_ERROR_BASE + 21);
> +const SEC_ERROR_INADEQUATE_KEY_USAGE = (SEC_ERROR_BASE + 90);
> +const SEC_ERROR_EXPIRED_CERTIFICATE = (SEC_ERROR_BASE + 11);
> +const SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED = (SEC_ERROR_BASE + 176);

We only use SEC_ERROR_EXPIRED_CERTIFICATE. If there are no plans to need the other ones, remove them for now.

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 14 • 11 years ago

Thanks for the review, Fabrice. I'll upload a new version tomorrow.

Regarding the sec guys agreement :P, I forgot to say that on top of everything I said before, the signing certificate is a pinned certificate... which either its valid or would have been replaced by a new one.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 15 • 11 years ago

Comment on attachment 8334718 [details] [diff] [review]
V1. Relax the signature validation for locally installed apps

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

::: dom/apps/src/Webapps.jsm
@@ +2660,4 @@
>                    throw "INVALID_SIGNATURE";
>                  } else {
> +                  // If it's a localFileInstall and the validation failed
> +                  // because of a expired certificate, just assume it was valid.

s/valid./valid and that the error occurred because the system time has not been set yet./

It would be good to have an additional check that the current time is less than the build time.

Please add a comment to where localFileInstall is set indicating that this variable controls whether expired certificates are accepted for the file.

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 16 • 11 years ago

As Brian suggested, I also added an extra check to only ignore the validation error if the current system date is earlier than the build date. I'm rerequesting review since actually the modification is bigger than the original patch was :).

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 17 • 11 years ago

As per comment 12 this sounds OK to me. This sounds OK to me, but this is the first I have heard of operator variant apps, so I'm am taking the comments above to be accurate. (NB I'd queued a secreview for 893800). In any case I can't think of any risks here that would stop us going ahead with this approach.

	Jason Smith [:jsmith]
	Comment 18 • 11 years ago

The component was right before - this is a DOM: Apps change. Moving it back to DOM: Apps.

Per comment 17 - this sounds like it's a requirement for locally installed packaged apps (e.g. Movistar apps), so blocking on this.

	[:fabrice] Fabrice Desré
	Comment 19 • 11 years ago

Comment on attachment 8336863 [details] [diff] [review]
V2. Including review comments

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

r=me with nits fixed.

::: dom/apps/src/Webapps.jsm
@@ +15,5 @@
> +
> +// We need this to decide if we should accept or not files signed with expired
> +// certificates.
> +function buildIDToTime() {
> +  var platformBuildID =

nit: s/var/let

@@ +17,5 @@
> +// certificates.
> +function buildIDToTime() {
> +  var platformBuildID =
> +    Components.classes["@mozilla.org/xre/app-info;1"]
> +      .getService(Components.interfaces.nsIXULAppInfo).platformBuildID;

Use Cc["..."]
      .getService(Ci.nsIXULAppInfo).platformBuildID;

@@ +18,5 @@
> +function buildIDToTime() {
> +  var platformBuildID =
> +    Components.classes["@mozilla.org/xre/app-info;1"]
> +      .getService(Components.interfaces.nsIXULAppInfo).platformBuildID;
> +  var platformBuildIDDate = new Date();

s/var/let

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 20 • 11 years ago

Thanks for the review, Fabrice.

Waiting on Brian's review (and for the patch in Bug 910815 to reach Mozilla Central) before uploading a revised version.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 21 • 11 years ago

Comment on attachment 8336863 [details] [diff] [review]
V2. Including review comments

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

Thanks for making those changes.

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 22 • 11 years ago

r=fabrice, r=bsmith

This is the reviewed version, including the nit fixes, rebased for M-C (since there was a huge Webapps.jsm patch landed recently).

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 23 • 11 years ago

Try run at https://tbpl.mozilla.org/?tree=Try&rev=7d2d6d1789ad

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 24 • 11 years ago

r=fabrice, r=bsmith

And this is the B2G26 version (which is different since Webapps.jsm change hasn't been uplifted there).

	Antonio Manuel Amaya Calvo (:amac) Assignee
	Comment 25 • 11 years ago

Try run looks good, requesting checkin

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/808b91cff779

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6d7f6f44c680

	rafael.marquez
	Comment 29 • 11 years ago

Verified:
Device -> Unagi
Branch -> v1.2
Gecko -> 4df52ee
Gaia -> 8d762f3