deleteHistory should clear private data from the database

VERIFIED FIXED in Firefox 13

Status

()

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: wesj, Assigned: rnewman)

Tracking

unspecified
Firefox 13
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

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

Updated

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

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-
(Assignee)

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?(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+
(Reporter)

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+
(Assignee)

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.)
(Assignee)

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(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
(Assignee)

Comment 8

7 years ago
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
Last Resolved: 7 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
status-firefox13: --- → verified
(Reporter)

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.