Closed Bug 727146 Opened 11 years ago Closed 11 years ago

deleteHistory should clear private data from the database

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox13 verified, blocking-fennec1.0 beta+, fennec13+)

VERIFIED FIXED
Firefox 13
Tracking Status
firefox13 --- verified
blocking-fennec1.0 --- beta+
fennec 13+ ---

People

(Reporter: wesj, Assigned: rnewman)

Details

Attachments

(2 files)

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).
tracking-fennec: --- → ?
tracking-fennec: ? → 13+
Assignee: nobody → lucasr.at.mozilla
Attachment #600354 - Flags: feedback?(wjohnston)
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
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?
Attachment #600354 - Flags: review?(rnewman) → review-
Here's how it would look in the CP…

I couldn't resist fixing some redundant `else` clauses while I was in there. Sorry.
Attachment #600617 - Flags: review?(lucasr.at.mozilla)
Attachment #600617 - Flags: feedback?(wjohnston)
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().
Attachment #600617 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #600617 - Flags: feedback?(wjohnston) → feedback+
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.)
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
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!
Assignee: lucasr.at.mozilla → rnewman
https://hg.mozilla.org/mozilla-central/rev/aa734c1e35ab
https://hg.mozilla.org/mozilla-central/rev/24f54e9d5227
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
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
Status: RESOLVED → VERIFIED
Comment on attachment 600354 [details] [diff] [review]
Ensure private data is removed when history is cleared

Clearing request
Attachment #600354 - Flags: feedback?(wjohnston)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.