Closed Bug 926219 Opened 6 years ago Closed 6 years ago

Signed operator variant apps are not installed if the signing certificate isn't installed on the phone

Categories

(Core Graveyard :: DOM: Apps, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

VERIFIED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: macajc, Assigned: amac)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

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)
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.
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.
Blocks: 893800
(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.
Whiteboard: [systemsfe]
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
Assignee: nobody → amac
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).
Attachment #8334718 - Flags: review?(fabrice)
Attachment #8334718 - Flags: review?(brian)
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.
blocking-b2g: --- → koi?
(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?)
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.
(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?
(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.
Status: NEW → ASSIGNED
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.
Flags: needinfo?(ptheriault)
(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).
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.
Attachment #8334718 - Flags: review?(fabrice) → review+
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.
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.
Attachment #8334718 - Flags: review?(brian) → review+
Attached patch V2. Including review comments (obsolete) — Splinter Review
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 :).
Attachment #8336863 - Flags: review?(fabrice)
Attachment #8336863 - Flags: review?(brian)
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.
Flags: needinfo?(ptheriault)
Component: DOM: Apps → General
Product: Core → Firefox OS
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.
blocking-b2g: koi? → koi+
Component: General → DOM: Apps
Product: Firefox OS → Core
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
Attachment #8336863 - Flags: review?(fabrice) → review+
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.
QA Contact: rafael.marquez
Comment on attachment 8336863 [details] [diff] [review]
V2. Including review comments

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

Thanks for making those changes.
Attachment #8336863 - Flags: review?(brian) → review+
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).
Attachment #8334718 - Attachment is obsolete: true
Attachment #8336863 - Attachment is obsolete: true
Attachment #8339197 - Flags: review+
r=fabrice, r=bsmith

And this is the B2G26 version (which is different since Webapps.jsm change hasn't been uplifted there).
Attachment #8339199 - Flags: review+
Try run looks good, requesting checkin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/808b91cff779
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: verifyme
Verified:
Device -> Unagi
Branch -> v1.2
Gecko -> 4df52ee
Gaia -> 8d762f3
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.