775327 - App ID generation and storage is unsafe

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

We store lots of security-sensitive information that is keyed at least partially on the App ID. Therefore, it is essential to ensure that we never accidentally re-use an App ID unless we've deleted all the security-sensitive information that references that App ID.

Currently, we generate an App ID for an app by looking at the last App ID we know of (n), and then making the new App ID n+1.

But, it is possible that wherever we store the App ID sequence will not get synced to disk before other databases that reference that App ID. In That case, the App ID sequence/database will hold a lower App ID than what is currently being used elsewhere; then, the next time we install an app, that app will use that already-used-for-a-different-purpose App ID.

We need to ensure that we sync the App ID sequence to disk before we reference it in any other databases (e.g. permission manager), and we need to ensure we that the sequence is always monotonically increasing.

An alternative would be to always generate 128-bit random App IDs using a secure random number generator, to avoid the possibility of such collisions. However, there is a concern that having an 128-bit App ID would be inconvenient because it is much more difficult to pass 128-bit integers around than 32-bit or 64-bit integers.

Comment 1

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

Fabrice: What would the implementation plan be here? Are we currently storing the set of installed apps in a sqlite database which we could switch to using an AUTOINCREMENT, or are we using a flat-file or some such?

Comment 2

[[:fabrice] Fabrice Desré]

We're using a flat file, and I'd like that to move to something else (either sqlite or indexedDb) eventually.

We keep the structure in memory and write it asynchronously to disk whenever there's a change (installing or uninstalling an app) so I'm not sure we can hit a situation where the max AppID in the webapps registry can be lower than the ones used elsewhere.

The issue that can arise is when deleting the app with the highest appId, the number will be reused by the next application installed. So we should take care of cleaning all the relevant jars keyed on an appId when uninstalling an app.

Comment 3

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

I'd really like to avoid reusing app-ids. I agree that we should clear data as best we can on uninstall, but if we crash at the wrong time, I don't think we can guarantee that data gets flushed. Especially once taking various IO caches into account.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch ensure that we don't reuse the same localId twice for non-preinstalled apps, by storing the current mzx id in a pref.

Comment 5

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

Comment on attachment 658563 [details] [diff] [review]
patch

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

brilliant!

Comment 6

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb7e34d6b03

Comment 7

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

:( I am sad that Jonas r+d this patch even though he knows better. :( 

It would be better to store the max ID in some kind of transactional database like IndexedDB or SQLite. Consider the case where the phone powers off (e.g. runs out of battery) before the operating system has flushed the prefs file to disk. It would be the same kind of problem that we're trying to fix here. See comment 3.

What Fabrice's patch does is somewhat of an improvement so we don't need to back it out, but we should completely fix the bug by addressing the crash issue.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Brian Smith (:bsmith) from comment #7)
> :( I am sad that Jonas r+d this patch even though he knows better. :( 
> 
> It would be better to store the max ID in some kind of transactional
> database like IndexedDB or SQLite. Consider the case where the phone powers
> off (e.g. runs out of battery) before the operating system has flushed the
> prefs file to disk. It would be the same kind of problem that we're trying
> to fix here. See comment 3.
> 
> What Fabrice's patch does is somewhat of an improvement so we don't need to
> back it out, but we should completely fix the bug by addressing the crash
> issue.

Can you file a followup bug for this?

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cb7e34d6b03