814131 - Include a per-app unique identifier and monotonically-increasing version number in every packaged app

Status: RESOLVED FIXED

(Marketplace Graveyard :: Security, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #791741 +++

For updates, the important things to know are:
1. Should I download a new JAR?
2. Is the new JAR the "same app" as the app I am trying to update?
3. Is the new JAR a *newer* version of that app?
4. Is the server that is serving the JAR giving us the information that the marketplace app/website expects it to?

This bug is about #2 and #3. #2 is important so that nobody can trick the web server into asking the user to replace one app with another (unrelated) app that the marketplace has signed. #3 is important to prevent downgrade attacks. For #2 an #3, we need to have some way, given two JAR files, and no other information, to decide whether those two JAR files are the same app and that one is newer than the other. It seems like these two pieces of information are currently missing.

I suggest we add these items to either the app manifest, or in its own entry under META-INF/ in the JAR.

Comment 1

[Jason Smith [:jsmith]]

+Jonas for input whether we should add entries to the manifest

Comment 2

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

I think we should add two properties for this, something like "app identifier" and "app version". But we should make it clear that these properties are *just* an store-internal identifier and store-internal version.

So I suggest we name them something like:

store-app-id: "...",
store-app-version: 5,

We should not allow people to submit manifests to the store which have these properties. I.e. we should not modify the manifest validator to require or permit these properties.

The identifier can just be any string that we're going to compare in a case sensitive manner. So the store can use any internal id that it wants, as long as it never changes. So using the app name might not be a good idea if we ever decide that renaming apps should be permitted.

The version can just be an integer or a float. It should likely not be the developer-provided version identifier since that can be things like "1.10" where that is a later version than "1.2". So we need to generate some internal revision that we stick in here.

Comment 3

[Andrew Overholt [:overholt]]

Who is going to do this?  Brian, are you?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This is the server-side counterpart to bug 814136. The marketplace team should own this and I will work with them on it.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Brian Smith (:bsmith) from comment #4)
> This is the server-side counterpart to bug 814136. The marketplace team
> should own this and I will work with them on it.

Hmm...but Jonas's comment 2 implies there's work here on the client side in the mini-manifest, no?

Comment 6

[Rob Hudson [:robhudson]]

There was a suggestion to store these in the META-INF/ rather than the manifest.webapp inside the package.

What about also including the mini-manifest URL from which the store could be derived? Having the mini-manifest URL here could also potentially solve the problem of pre-loaded 3rd party apps knowing where their update URL is... not exactly sure how FxOS stores this but if it doesn't exist this could be a fallback check.

E.g.:
$ cat META-INF/ids.json
{
  "manifest": "https://marketplace.firefox.com/app/cut-the-rope/manifest.webapp",
  "id": "{717bdfe2-3550-4ff7-90a5-eab984ab8bed}",
  "version": 42
}

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Smith [:jsmith] from comment #5)
> Hmm...but Jonas's comment 2 implies there's work here on the client side in
> the mini-manifest, no?

All the client-side work is bug 814136. Basically, the marketplace server needs to be modified to stuff this information into the JARs (this bug), and the client needs to be modified to verify that data (bug 814136). That's why I originally filed two bugs, and why I originally put this in the Marketplace product.

There's no client-side work blocking the marketplace team from embedding this information into the JARs. Basically, what's blocking this is the absence of a spec.

(In reply to Rob Hudson [:robhudson] from comment #6)
> There was a suggestion to store these in the META-INF/ rather than the
> manifest.webapp inside the package.

Yes, I think that is better to put all the marketplace-generated metadata into META-INF instead of modifying the manifest.webapp that the author submitted. This way, we minimize the likelihood of us modifying manifest.webapp in a way that would make it work differently from an (unpackaged) unsigned app that the author was testing.

> What about also including the mini-manifest URL from which the store could
> be derived? Having the mini-manifest URL here could also potentially solve
> the problem of pre-loaded 3rd party apps knowing where their update URL
> is... not exactly sure how FxOS stores this but if it doesn't exist this
> could be a fallback check.

The mini-manifest URL is already stored...somewhere. I don't think we should invent a second way of storing it. Instead, when we preload apps, we should preload that (IndexedDB?) database as though the user manually installed them. Consequently, I think we should avoid adding the manifest path to this new file.

> E.g.:
> $ cat META-INF/ids.json
> {
>   "manifest":
> "https://marketplace.firefox.com/app/cut-the-rope/manifest.webapp",
>   "id": "{717bdfe2-3550-4ff7-90a5-eab984ab8bed}",
>   "version": 42
> }

Besides "manifest", I think that is very reasonable. You can avoid some of the quoting and whatnot:

{ id: "717bdfe2-3550-4ff7-90a5-eab984ab8bed"
, version: 42
}

The client would treat (app signer root cert, id) as the unique ID for the JAR.

Comment 8

[Rob Hudson [:robhudson]]

I realized this morning that verifying this client side will break our current implementation of blocklisting, which was pushed to Marketplace as a simple workaround because client-side didn't have time to implement a true blocklisting feature.

Should we consider what can be done client-side to reduce extra work that will eventually get thrown with "blocklisting v2"? If there's still no time client-side, I'll file bugs to fix blocklisting to support these, but the simple server-side workaround will no longer be so simple.

Comment 9

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

(In reply to Rob Hudson [:robhudson] from comment #6)
> E.g.:
> $ cat META-INF/ids.json
> {
>   "manifest":
> "https://marketplace.firefox.com/app/cut-the-rope/manifest.webapp",
>   "id": "{717bdfe2-3550-4ff7-90a5-eab984ab8bed}",
>   "version": 42
> }

I agree, this looks good. Except that I don't understand the purpose of the "manifest" property.

I suspect that the quotes around the attribute names are needed since otherwise it isn't valid JSON :(

The client will treat the contents of the "id" attribute as an opaque string so you can put whatever you want there. As long as it is unique. So whether you have the curly braces there or not is entirely up to you.

Comment 10

[Rob Hudson [:robhudson]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> I agree, this looks good. Except that I don't understand the purpose of the
> "manifest" property.

That was just me wondering if we should add either a way to derive the store a package came from or the update URL for the package (if, somehow, you had a package without knowing it to begin with, e.g., pre-loaded 3rd-party apps?)

> The client will treat the contents of the "id" attribute as an opaque string
> so you can put whatever you want there. As long as it is unique. So whether
> you have the curly braces there or not is entirely up to you.

Yes. I was mostly following what we do for add-ons.

Question regarding the app IDs... I know these are also currently generated on device. If marketplace supplies app IDs, would the device use these, or would the device also generate its own app IDs? And does it matter one way or the other?

Comment 11

[Rob Hudson [:robhudson]]

(In reply to Rob Hudson [:robhudson] from comment #8)
> I realized this morning that verifying this client side will break our
> current implementation of blocklisting, which was pushed to Marketplace as a
> simple workaround because client-side didn't have time to implement a true
> blocklisting feature.
> 
> Should we consider what can be done client-side to reduce extra work that
> will eventually get thrown with "blocklisting v2"? If there's still no time
> client-side, I'll file bugs to fix blocklisting to support these, but the
> simple server-side workaround will no longer be so simple.

Note: If we can decide on a pre-determined app ID that the blocklisted app uses and conditionally check app ID and version ID on update if the app ID isn't the blocklisted app ID, this makes the server side changes very minimal.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Rob Hudson [:robhudson] from comment #8)
> I realized this morning that verifying this client side will break our
> current implementation of blocklisting, which was pushed to Marketplace as a
> simple workaround because client-side didn't have time to implement a true
> blocklisting feature.

This was discussed in the email thread. The currently-designed blocklisting mechanism will work just fine as long as treat the dummy app used for blocklisting exactly like a new version of the blocklisted app; i.e. sign it with the same META-INF/ids.json that the blocklisted app has.

I believe you'll have to do that anyway, because an app can get un-blocklisted after it's been blocklisted, so we still have to check for updates to blocklisted apps, and we still have to do the "same app" and "higher version" checks during that update.

> Should we consider what can be done client-side to reduce extra work that
> will eventually get thrown with "blocklisting v2"? If there's still no time
> client-side, I'll file bugs to fix blocklisting to support these, but the
> simple server-side workaround will no longer be so simple.

Please file a bug (if there's not one already) and nominate it for blocking-basecamp. Of course it would be great to have better blocklisting but I don't know if we can get it done in time.

Comment 13

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

(In reply to Rob Hudson [:robhudson] from comment #10)
> Question regarding the app IDs... I know these are also currently generated
> on device. If marketplace supplies app IDs, would the device use these, or
> would the device also generate its own app IDs? And does it matter one way
> or the other?

We actually currently generate two IDs on the device for packaged apps, and one ID for hosted apps.

I think we'd need to keep the IDs that we're currently using. One of the IDs (the one generated for both hosted and packaged apps) needs to be a simple 32bit integer, so obviously not something that we can keep unique across anything more than a single device.

The other ID that we generate for packaged apps needs to be unique, which we can't guarantee that the ID we get from the store is since there can be multiple stores involved. Including malicious stores.

Additionally non-signed packaged apps from other stores will not have the type of store-ID that we're talking about here.


We should definitely look at coming up with some type of identity that can be shared across store and device, but I don't think we'll have time for that in v1.

Comment 14

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

(In reply to Rob Hudson [:robhudson] from comment #11)
> Note: If we can decide on a pre-determined app ID that the blocklisted app
> uses and conditionally check app ID and version ID on update if the app ID
> isn't the blocklisted app ID, this makes the server side changes very
> minimal.

Unfortunately I think this would require too big changes client-side. It would also mean that someone hacking the app store could blacklist any app (which might not be a big deal).

Is it not an option to recreate and resign a blocklist app package any time we need to blocklist an app?

Comment 15

[Rob Hudson [:robhudson]]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Is it not an option to recreate and resign a blocklist app package any time
> we need to blocklist an app?

It just means some extra work server-side that I thought it wouldn't hurt to ask first. I'll file bugs for the things that need to happen server-side so we can track it. Thanks.

Comment 16

[Rob Hudson [:robhudson]]

Digging into this a little raises a few questions...

The most efficient place to put this is with the signing since we're opening the zip file, adding stuff to it, and doing a copy by writing into a new zip file. If I do it while adding the signatures this is a couple line code change.

This will work great for approved apps, but I'm wondering if it will work for developers or apps being reviewed by reviewers.

Do we need to add this "ids.json" file for non-approved apps? Does dev_mode disable this check?

For reviewers, if we sign with a reviewer cert, the above solution should work the same. I forget if reviewers are expected to load a cert via adb or enable dev_mode?

For developers, e.g. local testing, if dev_mode does not disable this check perhaps the developer could use an integer timestamp for a version. But this seems outside the scope of what the Marketplace is doing but is nice to think about how developers will test their apps on device prior to submitting to the Marketplace.

Comment 17

[Rob Hudson [:robhudson]]

(In reply to Rob Hudson [:robhudson] from comment #16)
> Digging into this a little raises a few questions...
> 
> The most efficient place to put this is with the signing since we're opening
> the zip file, adding stuff to it, and doing a copy by writing into a new zip
> file. If I do it while adding the signatures this is a couple line code
> change.
> 
> This will work great for approved apps, but I'm wondering if it will work
> for developers or apps being reviewed by reviewers.
> 
> Do we need to add this "ids.json" file for non-approved apps? Does dev_mode
> disable this check?
> 
> For reviewers, if we sign with a reviewer cert, the above solution should
> work the same. I forget if reviewers are expected to load a cert via adb or
> enable dev_mode?
> 
> For developers, e.g. local testing, if dev_mode does not disable this check
> perhaps the developer could use an integer timestamp for a version. But this
> seems outside the scope of what the Marketplace is doing but is nice to
> think about how developers will test their apps on device prior to
> submitting to the Marketplace.

Since we need ids.json to be signed, ignore everything in comment 16. I'll add it earlier in the process, soon after upload.

Comment 18

[Rob Hudson [:robhudson]]

First commit... updating all apps to have a UUID we can use:
https://github.com/mozilla/zamboni/commit/def797b

Comment 19

[Rob Hudson [:robhudson]]

(In reply to Rob Hudson [:robhudson] from comment #18)
> First commit... updating all apps to have a UUID we can use:
> https://github.com/mozilla/zamboni/commit/def797b

That commit was reverted in favor of this one:
https://github.com/mozilla/zamboni/commit/b87ccf4

Comment 20

[Rob Hudson [:robhudson]]

2nd part:
https://github.com/mozilla/zamboni/commit/95d40e4

Comment 21

[Rob Hudson [:robhudson]]

This can be QA'd by submitting a packaged app with no META-INF/ in it (also see bug 819173). As a reviewer you could download the package and open it and find the META-INF/ids.json file. Or approve it and download the signed zip with the same file in it.