Closed Bug 1468968 Opened Last year Closed Last year

Firefox retains favicons with their respective urls after supposedly clearing history

Categories

(Toolkit :: Places, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox62 --- verified
firefox63 --- verified

People

(Reporter: lokyfa, Assigned: mak)

References

(Depends on 1 open bug)

Details

(Keywords: privacy, Whiteboard: [fxsearch])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0.3 Safari/604.5.6

Steps to reproduce:

Selected "clear all history" in the "Everything" time range with all the options checked.


Actual results:

Some visited URLs were retained in the favicons.sqlite database in the user profile folder.


Expected results:

No visited URLs should have appeared in any persistent data managed by firefox
Summary: Firefox retains faviocons with their respective urls after supposedly clearing history → Firefox retains favicons with their respective urls after supposedly clearing history
The URLs in favicons.sqlite do not come from bookmarks, all bookmarks were deleted prior to clearing of the history.
Hi John W,
Thank you for your contribution to our community.

I was able to reproduce this bug on latest Nightly by following listed steps in the bug.


Build ID 	20180619220118
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
Component: General → Data Sanitization
Priority: -- → P3
Product: Firefox → Toolkit
Group: firefox-core-security
Rob, since you're working on bug 1448305 and bug 1470174, I thought you might be the better person to assess this. Can you confirm this? Is this something we should keep hidden for now?
Flags: needinfo?(rob)
I cannot reproduce this issue; I tried Firefox stable and Firefox Nightly (62.0a1 buildID 20180621220127).

I followed the following steps:

1. Start Firefox with a new profile.
2. Confirm that initially there are no new favicons:
   $ sqlite3 favicons.sqlite .dump | grep stackoverflow
   # No output
3. Visit stackoverflow.com
4. Confirm that stackoverflow appears in the result:
   $ sqlite3 favicons.sqlite .dump | grep stackoverflow
   # Some output
5. Use "clear all history", "Everything" and confirm.
6. Confirm that stackoverflow is gone from the database:
   $ sqlite3 favicons.sqlite .dump | grep stackoverflow
   # No output

Please provide the exact steps to reproduce the issue.

Could you dump the contents of your favicons.sqlite, e.g. with:
 sqlite3 favicons.sqlite .dump
and show the unexpected results? You can redact the URLs and content. I'm only interested in the database table structure.

(In reply to Johann Hofmann [:johannh] from comment #3)
> I thought you might be the better person to assess this.

I'm not an expert in this area.
Flags: needinfo?(rob)
bookmarks don't matter much, we remove from the favicons database all the urls that don't appear in moz_places. If for some reason there's an orphan url in moz_places (orphan keywords or tags may do that), it may not be removed, but this likely happens if the database had some coherency issue.

If we (either me or Mark Banner) could gather STR or receive a copy of places.sqlite, along with the urls of the orphan pages through mail, we'd have a look at those.
Even more details from comment 2 would be great, it just states it was reproduced, but without any details for me to do the same.

Note that things changed a bit from 57 on, so I'd like to understand if you're really using version 57 and if upgrading solves your problem.
Just reproduced again with 57.0 with https://tickets.amtrak.com (but strangely not with https://www.amtrak.com - to get to the tickets page try booking a trip somewhere on https://amtrak.com)

Closed FF after clearing history to make sure it's not a issue with reading the sqlite file before the process closed it.

Notes:
The rows are retained in moz_icons table but not in moz_pages_w_icons

Steps to reproduce:

1. delete from moz_icons;
2. verify nothing is in moz_icons via sqlite3 favicons.sqlite .dump 
3. visit https://www.amtrak.com/home
4. check moz_icons for a favicon from https://www.amtrak.com/etc/designs/dotcom-assets/images/favicon.ico
5. close tab, clear history, close FF.
6. sqlite3 favicons.sqlite .dump shows an empty db !


Repeat the above but this time try to book a trip which will get you to https://tickets.amtrak.com/itd/amtrak

https://tickets.amtrak.com/favicon.ico is retained in moz_icons after clearing history and closing FF.
ah, that is a root domain icon, it's not associated with a specific page (thus no entry in icons_to_pages), but it's associated with an origin, and we remove them when the whole origin goes away. The origin stays provided there's at least one entry for that origin in moz_places (any other visit or bookmark to that origin).

Please test with the most recent version though, 57 may have bugs we fixed in the meanwhile.
anyway, it's worth investigating this in more detail, check if we are not cleaning up in some case and in any case add a maintenance task to remove orphans.

I'm not sure why this bug is hidden though, it's privacy related, but not particularly malicious, if a user has access to your filesystem you have bigger concerns than a url in the favicons db.
Can we open it?
Component: Data Sanitization → Places
Keywords: privacy
Priority: P3 → P1
Whiteboard: [fxsearch]
I can confirm the issue with comment 6 in Firefox 57, 58, 59, 60 (latest release).
It appears to be fixed in Firefox 61 (Beta) and 62 (Nightly).

Starting from Firefox 61, the moz_icons database doesn't contain https://tickets.amtrak.com/favicon.ico , even before cleaning the private data.
Reproduced with 60.0.2 (per the procedure above)
Component: Places → Data Sanitization
Keywords: privacy
Priority: P1 → P3
Whiteboard: [fxsearch]
err, collision
Component: Data Sanitization → Places
Keywords: privacy
Priority: P3 → P1
Whiteboard: fxsearch
Group: firefox-core-security
Whiteboard: fxsearch → [fxsearch]
I verified and I can't reproduce as well, we remove icons through a trigger. I tried to look at patches that made 61, but it's unclear what may have fixed it, we had a lot of changes in 61.
Anyway, 61 is the current release, and it's not worth investigating further for now.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WORKSFORME
Have you tried reproducing this on 59 and then upgrading to 61 to see if clearing the cache on 61 removes the orphaned favicons ? If favicon removal is based on triggers from places they will not fire because the original places have long been deleted so upgrading to 61 will still have users carrying history data they believe to have been erased.
Following the suggestion from comment 13, I first followed the steps from comment 6.
Step 1-4 was executed in Firefox 60 (old stable).
Step 5 was executed in Firefox 62 (old Nightly).

It appears that although the entries are not created any more (at least not with the above reproduction steps), any previously created database entries are not removed either. This is a privacy issue.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
A similar issue seems to exist with dirs of the form storage/default/<origin>/idb persisting with sqlite files in them after history clear. 
This appears to have been resolved between 59 and 60 but might suffer from the same problem of orphaned entries.

Given how fragmented the persistent storage of FF is there should probably be a test that performs history/cache clear after running a typical workload and then scans the data dir for urls on sqlite, json and ls/grep levels as well as any sqlite tables with rows in them  - anything that is not explicitly whitelisted should fail the test.
(In reply to unfavicon from comment #15)
> A similar issue seems to exist with dirs of the form
> storage/default/<origin>/idb persisting with sqlite files in them after
> history clear. 

please report that bug in a more appropriate component, nobody working on indexeddb would notice this comment here.

> Given how fragmented the persistent storage of FF is there should probably
> be a test that performs history/cache clear after running a typical workload
> and then scans the data dir for urls on sqlite, json and ls/grep levels as
> well as any sqlite tables with rows in them  - anything that is not
> explicitly whitelisted should fail the test.

It's a good idea, but I fear with the current resources it will be hard to do in practice and since there are many ways to remove information, it will still not catch all the edge cases. The removal tests we add for each API should be enough honestly, if they are not we should improve those.

(In reply to unfavicon from comment #13)
> Have you tried reproducing this on 59 and then upgrading to 61 to see if
> clearing the cache on 61 removes the orphaned favicons ?

Fair point, I though we had a maintenance task doing that, but we don't, as well as expiration skips root icons.
Not sure what we can do for 61, we can't surely do a migration or anything like that, we could maybe do a post-facto cleanup on idle maintenance or on the first shutdown.

I'll look into that.
Assignee: nobody → mak77
Status: REOPENED → ASSIGNED
Thanks, I'm not familiar with FF dev structure so I've no idea who the relevant people are or even wether that's an idb or a toolkit responsibility, also I no longer have  a FF 59 installation to do a repro on this.

Re. the tests - I don't know what exists currently but this feels like an important enough a concern to take a preventive approach on, a test that checks that something that was supposed to be deleted was in fact deleted has value as a unit test but for purposes of validation there should be something that explicitly tries to justify every persistent artifact that survives a clear - weird edge cases are exactly what such a test *will* catch, the downside is of course lot of initial noise from positives which may or may not be false and may or may not will need to be whitelisted but I'd be surprised if there wasn't an eventual positive tradeoff to be had with such a framework.
Comment on attachment 8991926 [details]
Bug 1468968 - Restrict the concept of root icons and remove orphans, to avoid possible privacy leaks. r=standard8

Mike de Boer [:mikedeboer] has approved the revision.

https://phabricator.services.mozilla.com/D2122
Attachment #8991926 - Flags: review+
Comment on attachment 8991926 [details]
Bug 1468968 - Restrict the concept of root icons and remove orphans, to avoid possible privacy leaks. r=standard8

Mark Banner (:standard8) has approved the revision.

https://phabricator.services.mozilla.com/D2122
Attachment #8991926 - Flags: review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/946d21862f86
Restrict the concept of root icons and remove orphans, to avoid possible privacy leaks. r=Standard8,mikedeboer
https://hg.mozilla.org/mozilla-central/rev/946d21862f86
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8991926 [details]
Bug 1468968 - Restrict the concept of root icons and remove orphans, to avoid possible privacy leaks. r=standard8

Approval Request Comment
[Feature/Bug causing the regression]: not a recent regression, it's a bug in the new favicons implementation that landed in Firefox 55
[User impact if declined]: A few favicons may be left behind when clearing history, causing possible privacy leaks
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no, the automated test should suffice. Anyway it should be enough to visit a page having a /favicon.ico on a different host than the page itself
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8991926 - Flags: approval-mozilla-beta?
ehr, I missed a couple fields:
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's adding a couple queries to clean up a few icons based on strict conditions, won't affect other data
[String changes made/needed]: none
Comment on attachment 8991926 [details]
Bug 1468968 - Restrict the concept of root icons and remove orphans, to avoid possible privacy leaks. r=standard8

Icon fix, adds new tests, let's uplift for beta 13.
Attachment #8991926 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Let's try and verify it based on comment#23, if we can't it's ok since it has automated tests as well.
Flags: qe-verify+
I managed to reproduce the issue on Firefox 62.0a1(2018-06-12), under Ubuntu 16.04x64 using STR from Comment 6.
The issue is no longer reproducible on Firefox 62.0b12, or on Firefox 63.0a1(2018-07-30) using the same STR from Comment 6.
The tests were performed under Ubuntu 16.04x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1514945
You need to log in before you can comment on or make changes to this bug.