727146 - deleteHistory should clear private data from the database

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Currently, when the user clears their history we mark things in the database as deleted, but we don't actually remove them until later. That could present a security breach for a user who assumes that clearing the history means its all ready to hand their device over to someone.

Easy fix is just to make sure we clear out any sensitive data from the rows when we delete, but try to leave enough info for sync to determine what was deleted (a guid and maybe a changedtime I assume).

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Ensure private data is removed when history is cleared

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 600354 [details] [diff] [review]
Ensure private data is removed when history is cleared

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

Shouldn't this be in the CP?

Comment 3

[Richard Newman [:rnewman]]

Attachment: Counter-patch. v1

Here's how it would look in the CP…

I couldn't resist fixing some redundant `else` clauses while I was in there. Sorry.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 600617 [details] [diff] [review]
Counter-patch. v1

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

Looks good but split the patch.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1477,5 @@
> +        values.put(History.IS_DELETED, 1);
> +
> +        // Wipe sensitive data.
> +        values.putNull(History.URL);
> +        values.putNull(History.TITLE);

Didn't know about putNull(). Neat.

@@ +1497,2 @@
>  
> +        debug("Marking bookmarks as deleted for URI: " + uri);

This refactoring should go on a separate patch together with the other one in deleteImages().

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 600617 [details] [diff] [review]
Counter-patch. v1

I'd rather see this in here as well! Is there some reason you're not clearing the favicon url or sensitive bookmark data?

Comment 6

[Richard Newman [:rnewman]]

I just preserved the logic from lucasr's patch, wesj. Do we even allow clearing bookmarks?

(Keeping the bookmark data around makes reconciling easier for Sync in that edge case. History is more important from a privacy standpoint, I think.)

Comment 7

[Richard Newman [:rnewman]]

Well, this is interesting.

E/AndroidRuntime(17763): FATAL EXCEPTION: GeckoBackgroundThread
E/AndroidRuntime(17763): android.database.sqlite.SQLiteConstraintException: error code 19: constraint failed
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteStatement.native_execute(Native Method)
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteStatement.executeUpdateDelete(SQLiteStatement.java:92)
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteDatabase.updateWithOnConflict(SQLiteDatabase.java:1810)
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteDatabase.update(SQLiteDatabase.java:1761)
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.updateHistory(BrowserProvider.java:1419)
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.deleteHistory(BrowserProvider.java:1518)
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.deleteInTransaction(BrowserProvider.java:850)
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.delete(BrowserProvider.java:803)
E/AndroidRuntime(17763): 	at android.content.ContentProvider$Transport.delete(ContentProvider.java:213)
E/AndroidRuntime(17763): 	at android.content.ContentResolver.delete(ContentResolver.java:822)
E/AndroidRuntime(17763): 	at org.mozilla.gecko.db.LocalBrowserDB.clearHistory(LocalBrowserDB.java:306)
E/AndroidRuntime(17763): 	at org.mozilla.gecko.db.BrowserDB.clearHistory(BrowserDB.java:144)
E/AndroidRuntime(17763): 	at org.mozilla.gecko.ConfirmPreference$1.run(ConfirmPreference.java:68)
E/AndroidRuntime(17763): 	at android.os.Handler.handleCallback(Handler.java:605)
E/AndroidRuntime(17763): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/AndroidRuntime(17763): 	at android.os.Looper.loop(Looper.java:137)
E/AndroidRuntime(17763): 	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)
W/ActivityManager(  170):   Force finishing activity org.mozilla.fennec_rnewman/org.mozilla.gecko.GeckoPreferences

Presumably one of the fields must be non-null. /me fixes

Comment 8

[Richard Newman [:rnewman]]

Pushed to inbound with nit addressed, creation time zeroed, and URL set to "" instead of NULL.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa734c1e35ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/24f54e9d5227

Thanks for the reviews, chaps!

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/aa734c1e35ab
https://hg.mozilla.org/mozilla-central/rev/24f54e9d5227

Comment 10

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-29)
20120229031108
http://hg.mozilla.org/mozilla-central/rev/30b4f99a137c

--
Device: Samsung Galaxy S2
OS: Android 2.3.4

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 600354 [details] [diff] [review]
Ensure private data is removed when history is cleared

Clearing request