996129 - Personal data exposed in Top Sites thumbnails

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Duncan]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

Using Firefox 27 on my Android mobile device I checked my bank account.  I then logged out of my bank account and closed the browser.  I re-opened the browser some time later and the "Pages Recently Visited" display popped up.  I was looking at a screenshot of my bank transactions and account information as one of the screen grabs!


Actual results:

In the pages visited memory function I saw a screen grab of my bank page displaying my accounts, amounts, numbers and activities.  I was able to do a screen print and capture the data on printed form.


Expected results:

The "pages visited" display should not follow the "log-in" pages when someone goes to a website.  Were this a public system or if someone were simply to acquire the device and open your browser very personal data could be compromised (like my bank accounts and transactions).  

I work for a Penetration Testing organization.  I have brought this to the attention of our CIO/CTO and we may look at possibly creating a technical blog report after some investigating.  Please feel free to check some of our other technical blogs at www.netspi.com.  Obviously, nothing would be made public without notifying you first and following proper procedure.

Thank you,
Paul Duncan

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

There is no need for this to be a security bug.

Comment 2

[Ian Barlow (:ibarlow)]

I believe there are some guidelines that we use on Firefox desktop to avoid this kind of problem. I'll see what I can dig up.

Comment 3

[Wesley Johnston (:wesj)]

Desktop has some thumbnail generation code that I keep wondering if we can share/reuse.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

bug 754608 - " [New Tab Page] shows thumbnails from pages with "Cache-Control: no-store", and HTTPS pages when HTTPS disk caching is disabled"

Which creates holes in the tab thumbnail view.

Comment 5

[Ian Barlow (:ibarlow)]

From Boriss, the designer who worked on this on desktop:

"The way we detected these sites was via "Cache-Control no-store" (Bug 754608 and Bug 756881).  To get around this, we implemented a background service for taking thumbnails of sites when the user wasn't on them and not logged in (Bug 841495).  Of course, the problem with this is that you get a bunch of logins visible on about:newtab, which is both annoying (because it's not useful information) and misleading (because the user may actually be logged into those sites)."

--

Seems like we could take a similar approach on Android as well. Though I might suggest for thumbnails we fall back to the "favicon + dominant colour background" to avoid showing a bunch of login screens as thumbnails.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP

Saving my place. I wanted to see if we could even do this, since we don't decide to even ask for these on the same schedule as desktop. This seems to work though (Facebook and GMail refuse thumbnails). Right now this just refuses to get them at all. Longer term, it will probably need to inform java not to store them (so that we can use them for tabs, but not for top sites? I assume top sites shouldn't show these even if the site is actually open in a background tab?).

We also apparently aren't smart enough to fallback to favicons if the thumbnail fails to load, so we'll tackle that (somewhere...) too.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8416375 [details] [diff] [review]
WIP

Take a look at the way Firefox for Metro made the decision to thumbnail or not:

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#951

Since AndroidBridge::CaptureThumbnail has access to a nsIDocShell, maybe we could try to use docShell.currentDocumentChannel to get the nsIHttpChannel?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This is basically the same as desktop (which is the same as what Metro did above). We still want the thumbnail (I think?) for the tabs tray, even if we shouldn't store it, so I pass a boolean back to Java from the Gecko thumbnail code, which then has to trickle through a bunch of methods in order to get to the tab. If its set to NO_STORE, we intentionally delete any thumbnail that might be currently stored for this url

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8416767 [details] [diff] [review]
Patch v1

Review of attachment 8416767 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: mobile/android/base/db/LocalBrowserDB.java
@@ +840,5 @@
>              BitmapDrawable thumbnail) {
> +
> +        // If a null thumbnail was passed in, delete the stored thumbnail for this url.
> +        if (thumbnail == null) {
> +            cr.delete(mThumbnailsUriWithProfile, Thumbnails.URL + " == ?", new String[] { uri });

This should be a = instead of ==

::: widget/android/AndroidBridge.cpp
@@ +1806,5 @@
>      return NS_OK;
>  }
>  
> +bool
> +AndroidBridge::ShouldStoreThumbnail(nsIDocShell* docshell) {

I would rather make this function static and remove it from the AndroidBridge class. (You'll have to move it up above the CaptureThumbnail code).

Also, rather than having three copies of roughly the same code, can we move this into nsIDocShell? Ok to file a follow-up bug for that. We'll need a copy for B2G as well, it doesn't look like they respect this either.

@@ +1828,5 @@
> +
> +    // Cache-Control: no-store.
> +    bool isNoStoreResponse = false;
> +    httpChannel->IsNoStoreResponse(&isNoStoreResponse);
> +    if (isNoStoreResponse) {

You're not checking the 200-level response code like the JS implementations do. Is that intentional?

@@ +1840,5 @@
> +    if (!NS_SUCCEEDED(rv)) {
> +        return false;
> +    }
> +
> +    // Don't capture HTTPS pages unless the user enabled it.

Append to this comment "or the page has a Cache-Control:public header"

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/58ae3a4765ae

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58ae3a4765ae

Comment 12

[u421692]

Verified this issue on latest Nightly 32.0a1(2014-05-12). I'm gonna answer punctually to the mail regarding this issue.

- Regarding the thumbnail:
I see the favicon of the site visited, centered in the thumbnail with background color(the thumbnail is not gray or empty, please se attached screenshot), see comment 5 (favicon + dominant colour background)

- Have the Cache-Control header set to no-store: 
Verified with Web Console that the header is set to no-store, the screen capture of the page is not saved in the thumbnail.
Site used: https://ib.btrl.ro/BT24/bfo/channel/web/loginframe.jsp  ;  http://softvision.ro/ 
Response Headers: Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0

- Are https and have the Cache-Control header set to anything but public: 
I did not find a site to test(most of the https sites have the Cache-Control header set to no-store)

- The code should also erase already existing thumbnails if we get a no-store response from a site (i.e. we should remove your old thumbnails). 
This works fine I've visited a site before and after this issue has been fixed, and the thumbnail is updated. 

Can I mark this as Verified, or can you please guide me to a site that is https and has the Cache-Control header public/anything but public and does not have the Cache-Control header set to no-store.

Comment 13

[u421692]

Attachment: before_after_thumbnails.jpg

Comment 14

[Wesley Johnston (:wesj)]

I'm not sure of any site either. I'm fine marking this verified.