deleteHistory should clear private data from the database

VERIFIED FIXED in Firefox 13



7 years ago
7 years ago


(Reporter: wesj, Assigned: rnewman)


Firefox 13

Firefox Tracking Flags

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



(2 attachments)



7 years ago
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+
Keywords: fennecnative-betablocker


7 years ago
Assignee: nobody →
Created attachment 600354 [details] [diff] [review]
Ensure private data is removed when history is cleared
Attachment #600354 - Flags: review?(rnewman)


7 years ago
Attachment #600354 - Flags: feedback?(wjohnston)
blocking-fennec1.0: --- → beta+

Comment 2

7 years ago
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-

Comment 3

7 years ago
Created attachment 600617 [details] [diff] [review]
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.
Attachment #600617 - Flags: review?(
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/
@@ +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?( → review+

Comment 5

7 years ago
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+

Comment 6

7 years ago
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

7 years ago
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(
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteDatabase.updateWithOnConflict(
E/AndroidRuntime(17763): 	at android.database.sqlite.SQLiteDatabase.update(
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.updateHistory(
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.deleteHistory(
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.deleteInTransaction(
E/AndroidRuntime(17763): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.delete(
E/AndroidRuntime(17763): 	at android.content.ContentProvider$Transport.delete(
E/AndroidRuntime(17763): 	at android.content.ContentResolver.delete(
E/AndroidRuntime(17763): 	at org.mozilla.gecko.db.LocalBrowserDB.clearHistory(
E/AndroidRuntime(17763): 	at org.mozilla.gecko.db.BrowserDB.clearHistory(
E/AndroidRuntime(17763): 	at org.mozilla.gecko.ConfirmPreference$
E/AndroidRuntime(17763): 	at android.os.Handler.handleCallback(
E/AndroidRuntime(17763): 	at android.os.Handler.dispatchMessage(
E/AndroidRuntime(17763): 	at android.os.Looper.loop(
E/AndroidRuntime(17763): 	at
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

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

Thanks for the reviews, chaps!
Assignee: → rnewman
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Verified fixed on:

Firefox 13.0a1 (2012-02-29)

Device: Samsung Galaxy S2
OS: Android 2.3.4
status-firefox13: --- → verified

Comment 11

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

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