Closed
Bug 1448305
(CVE-2018-12400)
Opened 7 years ago
Closed 7 years ago
Private browsing mode leaks site visits via cached favicons
Categories
(Firefox for Android Graveyard :: Favicon Handling, defect, P1)
Tracking
(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 wontfix, firefox62 wontfix, firefox63 verified)
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox61 | --- | wontfix |
firefox62 | --- | wontfix |
firefox63 | --- | verified |
People
(Reporter: modi.konark, Assigned: robwu)
References
Details
(4 keywords, Whiteboard: --do_not_change--[priority:high][post-critsmash-triage][adv-main63+])
Attachments
(8 files, 2 obsolete files)
49.06 KB,
image/png
|
Details | |
73.52 KB,
image/png
|
Details | |
100.70 KB,
image/png
|
Details | |
8.14 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
11.02 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
1.42 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
4.85 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
3.93 KB,
patch
|
JanH
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101
Steps to reproduce:
1. Open Firefox for Android
2. Open new tab
3. Visit sites:
a. https://xvideos.com
b. http://spiegel.de
4. Close the browser
5. Restart the browser
Actual results:
Domains with touch-icon in HTML seem to leave an entry in `cache/icons` folder.
Furthermore, it also reveals the number of page loads, because new unique page seems to save a new entry for the touch-icon.
As in the screenshot you can see, I visited three different pages on spiegel.de, and it saved touch icon three times.
Screenshots attached.
Expected results:
Expectation is: Private browsing does not leave any footprint on the device about domains visited.
But in this case, domains can be known. Also, I did not check, but the cache file name looks like a hash, did not confirm if it's hash based on a URL or not.
Reporter | ||
Comment 1•7 years ago
|
||
Correction steps to reproduce:
Steps to reproduce:
1. Open Firefox for Android
2. Open new PRIVATE tab
3. Visit sites:
a. https://xvideos.com
b. http://spiegel.de
4. Close the browser
5. Restart the browser
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Comment 3•7 years ago
|
||
Updated•7 years ago
|
Component: General → Favicon Handling
[triage] Critical: PB leaks user data.
Flags: needinfo?(sdaswani)
Priority: -- → P1
David, I'm not sure if this is a platform or Front End issue - NI'ing you in case you know or can check with the team, NI me back if it's the latter.
Flags: needinfo?(sdaswani) → needinfo?(dbolter)
Comment 7•7 years ago
|
||
Thanks Susheel over to James for deeper triage.
Flags: needinfo?(dbolter) → needinfo?(snorp)
Favicon code is firmly front-end stuff on Fennec, back to Susheel!
Flags: needinfo?(snorp) → needinfo?(sdaswani)
Andreas, Barbara - should we fix this? If so, when?
Flags: needinfo?(sdaswani) → needinfo?(bbermes)
Updated•7 years ago
|
Comment 10•7 years ago
|
||
I think this is important to fix for an upcoming release.
Flags: needinfo?(abovens)
Comment 12•7 years ago
|
||
Jan might be interested in this.
Updated•7 years ago
|
Flags: sec-bounty?
Comment 13•7 years ago
|
||
Some notes of what would have to be done after briefly looking at how icons seem to work:
- IconRequests already have an option to completely skip the disk cache [1], although maybe using that might be a little too blunt an instrument? If we want to still allow *reading* from the cache and only block writes of fresh content while in private mode, we'd have to add a new attribute along the same lines and handle it in the IconRequestBuilder and DiskProcessor.
- Review our usage of "Icons.with(...)" [2] and skip the disk cache (at least for writes) where necessary.
At a first glance, the mainly relevant pieces are the Tab (self-evident), TwoLinePageRow (used for display of recently closed tabs, *including in private mode*), TabHistoryItemRow (session history display when long-pressing back/forward button) and TabsLayoutItemView (tabs tray screen) classes.
- Anything that currently uses both skipNetwork() and skipMemory() and therefore relies only on the disk cache (plus generated fallback icons) needs to be reviewed. One example is HomeScreenPrompt and GeckoApplication.createBrowserShortcut. While I think that the former currently isn't used in private browsing, the latter is used for "Add Page Shortcut" in the "Page" menu. If the disk cache is no longer updated in private browsing and usage of the memory cache is prohibited, adding a launcher shortcut while in private browsing may not work properly. All other call sites from [2] need to be checked for this usage pattern, too.
- Looking briefly, anything else is probably okay as it's either helper functions for reader mode (which only resolves the URL, but doesn't store anything [3]), search engine icons, or used (in the home panels or similar displays) for URLs that have already entered history, have been bookmarked or have saved logins, in which case private browsing won't really matter.
- This bit [4] needs moving into the sanitizer, so it is executed for users that are clearing private data on exit as well. This can be done similar to our existing flow for clearing history [5], i.e. add a Sanitize:Icons request to the offlineApps handler in Sanitizer.jsm and then clear icons in Java only in response to that message instead of directly hooking Settings -> Clear private data.
[1] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java#131-136
[2] https://dxr.mozilla.org/mozilla-central/search?q=Icons.with+-path%3Atest&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java#88
[4] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#55
[5] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/modules/Sanitizer.jsm#169
Comment 14•7 years ago
|
||
Sebastian, can you remember why we're also skipping the memory cache when retrieving an icon for adding a home screen shortcut (https://hg.mozilla.org/mozilla-central/annotate/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/promotion/HomeScreenPrompt.java#l138)?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(s.kaspari)
Comment 15•7 years ago
|
||
I think the reason for that was that the in-memory icon is resized to the ideal size for displaying in the UI and for the launcher icon we may want to have a larger version from disk.
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #13)
> Some notes of what would have to be done after briefly looking at how icons
> seem to work:
> - IconRequests already have an option to completely skip the disk cache [1],
> although maybe using that might be a little too blunt an instrument? If we
> want to still allow *reading* from the cache and only block writes of fresh
> content while in private mode, we'd have to add a new attribute along the
> same lines and handle it in the IconRequestBuilder and DiskProcessor.
> - Review our usage of "Icons.with(...)" [2] and skip the disk cache (at
> least for writes) where necessary.
> At a first glance, the mainly relevant pieces are the Tab (self-evident),
> TwoLinePageRow (used for display of recently closed tabs, *including in
> private mode*), TabHistoryItemRow (session history display when
> long-pressing back/forward button) and TabsLayoutItemView (tabs tray screen)
> classes.
I audited the uses of Icons.with in bug 1468116 and reached the same conclusions.
I also noted an icon persistence of AS in PBM: "activitystream (StreamOverridablePageIconLayout.java and TopSitesCard.java ) : It is based on the browsing history, so I am inclined to not change anything for now and keep caching their icons."
> - Anything that currently uses both skipNetwork() and skipMemory() and
> therefore relies only on the disk cache (plus generated fallback icons)
> needs to be reviewed. One example is HomeScreenPrompt and
> GeckoApplication.createBrowserShortcut. While I think that the former
> currently isn't used in private browsing, the latter is used for "Add Page
> Shortcut" in the "Page" menu. If the disk cache is no longer updated in
> private browsing and usage of the memory cache is prohibited, adding a
> launcher shortcut while in private browsing may not work properly. All other
> call sites from [2] need to be checked for this usage pattern, too.
I guess that the network is skipped for shortcuts because of unpredictable latency.
I think that a fall back to the in-memory cache is better than not having anything at all.
> - This bit [4] needs moving into the sanitizer, so it is executed for users
> that are clearing private data on exit as well. This can be done similar to
> our existing flow for clearing history [5], i.e. add a Sanitize:Icons
> request to the offlineApps handler in Sanitizer.jsm and then clear icons in
> Java only in response to that message instead of directly hooking Settings
> -> Clear private data.
Do we need to add any migration logic to sanitize the existing cache? E.g. remove all icons that do not appear in the history or bookmarks.
Assignee: nobody → rob
Status: NEW → ASSIGNED
Comment 18•7 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #17)
> I guess that the network is skipped for shortcuts because of unpredictable
> latency.
Yes, probably. I also see that the network is skipped as well for tabs, about:home, etc. as well, so it might also have been part of the design to only load favicons from the network when actually loading a page, though.
> I think that a fall back to the in-memory cache is better than not having
> anything at all.
I'd agree, plus potentially the new read-only disk cache option if you choose to implement it that way.
> Do we need to add any migration logic to sanitize the existing cache? E.g.
> remove all icons that do not appear in the history or bookmarks.
Hmm, icons are (mostly?) only fetched from the network when actually loading a page (see above), so dropping the whole icon cache would be the easiest route, but also rather annoying to our users.
So yes, if we don't want to rely on natural wastage then that leaves only an explicit sanitization of all unused favicons.
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty-
Keywords: csectype-disclosure
Summary: Private browsing mode leaks in cache folder → Private browsing mode leaks site visits via cached favicons
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Jan Henning (on vacation in June) [:JanH] from comment #13)
> - This bit [4] needs moving into the sanitizer, so it is executed for users
> that are clearing private data on exit as well. This can be done similar to
> our existing flow for clearing history [5], i.e. add a Sanitize:Icons
> request to the offlineApps handler in Sanitizer.jsm and then clear icons in
> Java only in response to that message instead of directly hooking Settings
> -> Clear private data.
>
> [4] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#55
> [5] https://dxr.mozilla.org/mozilla-central/rev/ee6283795f41d97faeaccbe8bd051a36bbe30c64/mobile/android/modules/Sanitizer.jsm#169
I have opened bug 1470174 for this (and not fixing it now because the code is being refactored by bug 1466130).
I will also defer the one-time clean-up logic (i.e. removing icons that potentially originate from private browsing mode), because removing old favicons while preserving favicons from history / bookmarks is complicated, because DiskLruCache.java does not support a way to enumerate keys, and the keys are hashes of the page and favicon URLs. Therefore, to remove unknown icons, something like this would be needed:
1. First we need to determine all bookmarked and history URLs.
2. Then we calculate the key: SHA256("mapping:" + pageUrl)
3. Then we find the iconUrl in the DiskLruCache.
4. If iconUrl exists, calculate the key: SHA256("icon:" + iconUrl)
5. All icons except for those referenced by the above keys should be removed.
There are simpler alternatives such as reducing DISK_CACHE_SIZE to shrink the cache size (it is currently set to 50MB, but in practice my cache is twice as much...):
https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/mobile/android/base/java/org/mozilla/gecko/icons/storage/DiskStorage.java#36-40,70-74
This alternative won't work if the user rarely uses normal browsing mode.
Yet another approach (other than completely nuking the icon cache) is to store icons at a new location (e.g. cache/icons2/) and use the original cache/icons/ in a read-only fashion. Then the directory can be removed after a few releases.
With these parts moved out of the scope of this bug, my patches that fix the actual leak are ready for review.
Assignee | ||
Comment 20•7 years ago
|
||
Test case:
1. Load a new URL in a private tab.
2. Check that the page's icon is not in cache/icons/
Assignee | ||
Comment 21•7 years ago
|
||
Test case:
1. Press triple-dots
2. Long-press the back button (left arrow).
3. Check that there the icon is not saved to the disk.
Assignee | ||
Comment 22•7 years ago
|
||
Test case:
1. Open a private tab (thanks to a previous patch the icon won't be
saved to cache/icons/ ).
2. Tab on the "[1]" to show all private tabs.
3. Check that the icon is not saved to cache/icons/ .
Assignee | ||
Comment 23•7 years ago
|
||
Test case:
1. Open a URL in normal browsing mode (so it appears in the history).
2. Open a private tab, and type the first three letters of the URL's
domain. Now Fennec will show a suggestion bar with the item from
the first step.
3. Check that the icon from step 2 is not stored in cache/icons/.
Assignee | ||
Comment 24•7 years ago
|
||
Try to fetch an icon from the memory cache before definitely falling
back to an auto-generated icon when a page shortcut is created.
Test case:
1. Visit a site with a favicon in a private tab.
(Thanks to a previous patch, the favicon is only stored in memory).
2. Use Triple dots menu > Page > Add Page Shortcut.
3. Confirm that the created shortcut has the same icon as the page.
(Note: the icon will also be saved to the disk cache.
I did not change this behavior since the shortcut is already
stored on the disk anyway).
Comment 25•7 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #19)
> I have opened bug 1470174 for this (and not fixing it now because the code
> is being refactored by bug 1466130).
Could you just CC me there as well?
Assignee | ||
Comment 26•7 years ago
|
||
@Jan
Done. Could you review my patches? Since you've looked into it before, reviewing should be easier compared to selecting another random reviewer (if not, please recommend another fitting reviewer).
I have manually verified that Firefox behaves as expected.
Locally, I even added logging to DiskProcessor.java and IconTask.java to confirm that the flags are propagated as expected.
Comment 27•7 years ago
|
||
Thanks. I'm happy to look at your patches, but I probably won't be able to look at them properly until I return back home the weekend after this one. If you don't want to wait, either ask Sebastian (who originally wrote the icon code), or if he doesn't have time, either, maybe jchen?
Assignee | ||
Comment 28•7 years ago
|
||
Could you review my patches now?
(I don't know how to add r? without triggering an e-mail notification for each, so I'm using ni? instead)
Flags: needinfo?(jh+bugzilla)
Comment 29•7 years ago
|
||
Yes, I'll look at them this weekend.
Comment 30•7 years ago
|
||
Comment on attachment 8986814 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of private tabs
Review of attachment 8986814 [details] [diff] [review]:
-----------------------------------------------------------------
Two small suggestions for the wording of the Javadoc, but otherwise fine. Thanks for taking this bug.
::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
@@ +98,5 @@
> return privileged;
> }
>
> + /**
> + * Is this request initiated from a tab in private browsing mode?
"Has this request been initiated..." sounds a little more natural to me.
::: mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
@@ +61,5 @@
> return this;
> }
>
> + /**
> + * Set the private mode to avoid saving the result to the disk.
Maybe spell it out and say "Set the private mode to true to avoid..."?
Attachment #8986814 -
Flags: review+
Updated•7 years ago
|
Attachment #8986815 -
Flags: review+
Updated•7 years ago
|
Attachment #8986818 -
Flags: review+
Comment 31•7 years ago
|
||
Comment on attachment 8986819 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of private autocompletion results
Review of attachment 8986819 [details] [diff] [review]:
-----------------------------------------------------------------
Unfortunately this doesn't cover "Recently closed" tabs, where TwoLinePageRow isn't used with the private mode theming and therefore mUrl.isPrivateMode() returns false.
This means that with
1. Open > 1 private mode tab.
2. Close some of them again, but leave at least one private mode tab open.
3. Open the Awesomescreen and look at History > Recently closed
the icons of the tabs appearing under "Recently closed" would still end up in the disk cache, wouldn't they?
Attachment #8986819 -
Flags: review-
Updated•7 years ago
|
Attachment #8986820 -
Flags: review+
Updated•7 years ago
|
Flags: needinfo?(jh+bugzilla)
Assignee | ||
Comment 32•7 years ago
|
||
Test case in comment 31,
followed by checking that the closed tab's icon is not in cache/icons/.
Assignee | ||
Comment 33•7 years ago
|
||
Comment on attachment 8991874 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of recently closed private tabs
This fixes the issue from comment 31.
Attachment #8991874 -
Flags: review?(jh+bugzilla)
Comment 34•7 years ago
|
||
Comment on attachment 8991874 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of recently closed private tabs
Review of attachment 8991874 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryItem.java
@@ +88,5 @@
>
> public void bind(ClosedTab closedTab) {
> final TwoLinePageRow childPageRow = (TwoLinePageRow) this.itemView;
> childPageRow.setShowIcons(false);
> + childPageRow.setPrivateMode(closedTab.isPrivate);
Unfortunately this also means that you're enabling the private mode theming, which
a) poses the question whether we actually want to do that for this case, and
b) doesn't really work, as the background isn't controlled by the TwoLinePageRow, so you end up with white-on-white text for the title.
I think you're better off just introducing a dedicated setting for controlling the icon-loading behaviour of a TwoLinePageRow.
Attachment #8991874 -
Flags: review?(jh+bugzilla) → review-
Assignee | ||
Comment 35•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8986819 -
Attachment is obsolete: true
Attachment #8991874 -
Attachment is obsolete: true
Assignee | ||
Comment 36•7 years ago
|
||
Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs
I haven't introduced a new setting; I just checked whether the selected tab is private.
Tested using the steps from comment 23 and comment 31.
Attachment #8992214 -
Flags: review?(jh+bugzilla)
Comment 37•7 years ago
|
||
Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs
Review of attachment 8992214 [details] [diff] [review]:
-----------------------------------------------------------------
Other things are relying on the currently selected tab as well to make that distinction, so yes, doing it that way should be fine here, too.
::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java
@@ +308,5 @@
> + /**
> + * @return true if this view is shown inside a private tab, independent of whether
> + * a private mode theme is applied via <code>setPrivateMode(true)</code>.
> + */
> + private boolean isTabInPrivateMode() {
The Javadoc is appreciated, but maybe name the function itself something like isCurrentTabInPrivateMode() to remove some ambiguity?
Attachment #8992214 -
Flags: review?(jh+bugzilla) → review+
Assignee | ||
Comment 38•7 years ago
|
||
Comment on attachment 8992214 [details] [diff] [review]
Bug 1448305 - Avoid disk cache for icons of TwoLinePageRow in private tabs
Review of attachment 8992214 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java
@@ +308,5 @@
> + /**
> + * @return true if this view is shown inside a private tab, independent of whether
> + * a private mode theme is applied via <code>setPrivateMode(true)</code>.
> + */
> + private boolean isTabInPrivateMode() {
It did initially not occur to me that `setPrivateMode` was only for theming, and that it could have been different from the actual PBM state. I tried to capture this in a comment for future readers.
A different method name would indeed say what the method does, but not why, so I intend to keep the comment and name as is.
Assignee | ||
Comment 39•7 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #37)
> The Javadoc is appreciated, but maybe name the function itself something
> like isCurrentTabInPrivateMode() to remove some ambiguity?
I misread your comment; you were only asking to make the name more verbose, without necessarily removing the comment.
I'll make that change and then try to figure out how to land these patches.
![]() |
||
Comment 40•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a2010067721dc68eef1e71a7d1d6fd9fafcc45
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9034458471c8478c767d7a27ee3cc3b4f76cebe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b6236aaef8c848ce9e2d3f98ea7a22c1a1c26e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee63d789769e77377e6246817f900727b23f1289
https://hg.mozilla.org/integration/mozilla-inbound/rev/37bd009f11f4e05b02a6219a01616331a315efd0
https://hg.mozilla.org/mozilla-central/rev/43a201006772
https://hg.mozilla.org/mozilla-central/rev/c9034458471c
https://hg.mozilla.org/mozilla-central/rev/c5b6236aaef8
https://hg.mozilla.org/mozilla-central/rev/ee63d789769e
https://hg.mozilla.org/mozilla-central/rev/37bd009f11f4
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 41•7 years ago
|
||
I'm assuming this fix can just ride the trains, but feel free to nominate for Beta approval if you feel strongly otherwise.
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
Updated•7 years ago
|
Flags: qe-verify+
Whiteboard: --do_not_change--[priority:high] → --do_not_change--[priority:high][post-critsmash-triage]
Comment 42•7 years ago
|
||
Tested with Nexus 5(Android 6.0.1) and OnePlus 5T (Android 8.1.0) following the steps provided and we couldn't reproduce the issue on the latest Nightly build (63.0a1 - 21/08).
Flags: qe-verify+
Updated•6 years ago
|
Whiteboard: --do_not_change--[priority:high][post-critsmash-triage] → --do_not_change--[priority:high][post-critsmash-triage][adv-main63+]
Updated•6 years ago
|
Alias: CVE-2018-12400
Updated•5 years ago
|
Group: core-security-release
Updated•5 years ago
|
Flags: sec-bounty-hof+
Updated•4 years ago
|
Restrict Comments: true
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
Updated•9 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•