Closed Bug 821211 Opened 7 years ago Closed 7 years ago

Information presented to the user from the mini-manifest is not verified to exactly match the information in the manifest within the JAR file

Categories

(Core Graveyard :: DOM: Apps, defect, P1)

defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

VERIFIED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: briansmith, Assigned: fabrice)

References

Details

Attachments

(1 file)

For example, the mini-manifest might have name: "Gmail", which would cause us to prompt the user asking if she wants to install "Gmail." But the downloaded package might actually be for an app *not* named GMail; if so, then the user did not really consent to install that app.

Consequently, we need to verify that all of the fields of the mini-manifest (or are least the ones we use) are present in the signed manifest file, and that each value of each property in the mini-manifest exactly matches the corresponding value in the signed manifest file.
Depends on: sign-packaged-apps
No longer depends on: 821209
Going to generalize this - this applies also to a non-signed packaged app. When this issue occurs, we probably should throw a DOMError back.
Summary: Information presented to the user from the mini-manifest is not verified to exactly match the information in the signed manifest within the JAR file → Information presented to the user from the mini-manifest is not verified to exactly match the information in the manifest within the JAR file
Assignee: nobody → bsmith
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Attached patch patchSplinter Review
Stealing since Brian is busy with other bugs.

In this patch we check that 2 manifests are similar by making sure that:
- they have the same set of locales
- for each locale and for the default root, that the application name and developer information are the same.
Assignee: bsmith → fabrice
Attachment #693070 - Flags: review?(ferjmoreno)
Blocks: 821174
Comment on attachment 693070 [details] [diff] [review]
patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +284,5 @@
> +      for (let locale in aManifest2.locales) {
> +        locales2.push(locale);
> +      }
> +    }
> +    if (locales1.sort().join() !== locales2.sort().join()) {

If I am not wrong, you can get rid of 'locales1' and 'locales2' and have this condition as:

|if (aManifest1.slice().sort().join() !== aManifest2.slice().sort().join) { ... }|

I didn't test it that way though.
Attachment #693070 - Flags: review?(ferjmoreno) → review+
As the code is right now, it is far from obvious that we're checking all the fields that we actually need to match.

I suggest that after we parse the mini manifest, we strip out every field that isn't checked by this function, and pass the stripped version on to the later layers. The function that does the stripping should be next to the function that does these checks. This would ensure that we never forget to check consistency of any new fields we decide to rely on from the mini manifest.
I though about that during the revision, but after reading again "or at least the ones we use" in the bug description I thought that the attached patch would be enough for v1 since we are only using the fields that are being checked in the patch [1]. In any case, you suggestion sounds reasonable to me.

[1] http://mxr.mozilla.org/gaia/source/apps/system/js/app_install_manager.js#93
There are valid fields that we display from the mini-manifest that are not in the app manifest, like the package size. So I'm not 100% convinced by this approach.
(In reply to Fabrice Desré [:fabrice] from comment #6)
> There are valid fields that we display from the mini-manifest that are not
> in the app manifest, like the package size. So I'm not 100% convinced by
> this approach.

For size, we should just verify that the size of the JAR matches the size in the mini-manifest.

The ultimate goal is to be able to say "it's safe to present the information in the mini-manifest to the user even though it isn't signed because of X and Y." In this case, X is "because we verified that it matches the information in the app manifest signed within the JAR" and Y is "because we verified that the size in the mini-manifest matches the actual size of the JAR."
I don't think we need to verify that the size information is correct at all.

By the time we have downloaded the package, the size information is no longer important and is never going to be exposed to the user. The size information is only used for displaying UI to the user before the download is started.
(In reply to Jonas Sicking (:sicking) from comment #8)
> I don't think we need to verify that the size information is correct at all.
> 
> By the time we have downloaded the package, the size information is no
> longer important and is never going to be exposed to the user. The size
> information is only used for displaying UI to the user before the download
> is started.

We also use the size information to check that the device has enough free space to store the app.
But doesn't that also happen before the download?
Cool, so sounds like there's no need to do any verification of the size property in the mini-manifest.
https://hg.mozilla.org/mozilla-central/rev/ffab6379f313
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
A couple of sanity checks on mistakes on my behalf building packaged apps has definitely revealed that the checks here are indeed working. Marking as verified.
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.