788909 - App ID generation and storage is unsafe in the face of crashes

Status: RESOLVED WONTFIX

(Core :: Security, defect)

Description

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

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

> 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 
> Bug 775327 comment 3.

Comment 1

[Andrew Overholt [:overholt]]

If we get this wrong, it can lead to privacy issues.

Mounir, is this something you can do?

Comment 2

[[:fabrice] Fabrice Desré]

Sorry I was muted while we discussed it. The risk here is to reuse the same appId for two different apps. Note that we could mitigate that at startup by checking that the maxLocalId pref is actually higher than the highest appId used by currently installed apps.

Comment 3

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

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Sorry I was muted while we discussed it. The risk here is to reuse the same
> appId for two different apps. Note that we could mitigate that at startup by
> checking that the maxLocalId pref is actually higher than the highest appId
> used by currently installed apps.

The original bug was that we were using the (highest appId + 1) of the currently-installed app, which causes problems when we've uninstalled an app. If we do what is suggested in #2, we will still be subject to the original bug during the reboot after we crash:

* The pref's value is 6.
* We install an app, which will have appID 6.
* The pref's value becomes 7 in memory.
* We write a bunch of data with appID 6
* That data gets synced to disk
* We uninstall that app
* The max appID in the app file becomes 6 in memory.
* The app file gets synced to disk, so it is 6 on disk too.
* Phone crashes before the prefs file gets synced to disk, so the prefs file still contains maxLocalId=6.
* We reboot and verify that 6 > 5 so everything looks fine, even though it's not.

Comment 4

[Lucas Adamski [:ladamski]]

How about we just don't ever decrement the appID?  You can hate me in the event someone ever goes through MAXINT+1 app installs.

Otherwise, this bug sucks but I'm not sure given the straw we have to fit through, that its really a basecamp blocker.  At least I haven't seen traction enough to suggest that's the case.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Lucas Adamski from comment #4)
> How about we just don't ever decrement the appID?  You can hate me in the
> event someone ever goes through MAXINT+1 app installs.

This is what we do currently, but the bug is about the storage we use to store this ID (a pref).

Comment 6

[Andrey Bienkowski]

https://bugzilla.mozilla.org/show_bug.cgi?id=1548706

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.