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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Thomas]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120509070325

Steps to reproduce:

When you open a new tab, clicking the +, a new tab will open with a blank page. Click on the grid in the upper right hand corner and a 3x3 page preview appears.


Actual results:

I have found that there is too much information available to people when using this option. For instance, I filled out a form on the computer, and in the preview screen it showed my completed form, including my full name, address, phone number, etc.



Expected results:

This feature is too risky as is. Even if there is an option to shut this off, someone with a little know-how could just turn it back on and wait to retrieve someones information on a public computer.

Comment 1

[Thomas]

You may have to restart your computer to get your immediate History to appear in these previews. After a restart I am able to read emails(standard 10pt font), see my twitter page and facebook page(Shows my homepage, but if i click on it, promts me to login) see my paypal account, and see my credit card account in the previews.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree that we should try to creating thumbnails of pages that have sensitive information on them. Unfortunately, I do not know how we could do that in all cases. One heuristic would be to avoid creating thumbnails for pages that contain any Cache-control:no-store content, because Cache-control-:no-store is most typically used to avoid persistent storage of pages with very sensitive information along the lines of the things that the user mentioned (e.g. credit card numbers).

A more extreme possible solution may be to always use a capture of the home page (scheme://hostname:port/) of the site, retrieved without any HTTP authentication credentials or cookies. This should prevent almost any sensitive data from being shown. But, it would often mean that the page that the user ends up at does not look like the thumbnail; on the other hand, that is often already the case now.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #2)
> I agree that we should try to creating thumbnails of pages that have
> information on them.

We should try to AVOID creating thumbnails of pages that have sensitive information on them.

Comment 4

[Ian Melven :imelven]

(In reply to Brian Smith (:bsmith) from comment #2)
One heuristic would be to avoid creating thumbnails for
> pages that contain any Cache-control:no-store content, because
> Cache-control-:no-store is most typically used to avoid persistent storage
> of pages with very sensitive information along the lines of the things that
> the user mentioned (e.g. credit card numbers).

in reference, see Bug 627239 - Don't store thumbnails for cache:control:no-store pages

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - don't capture thumbnails for privacy-sensitive pages

This patch makes sure that we don't capture thumbnails for privacy-sensitive pages anymore. This is exact the same implementation that Panorama used before and is also what nsHttpChannel does:

http://mxr.mozilla.org/firefox/source/netwerk/protocol/http/src/nsHttpChannel.cpp#2136

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - remove getXULDocument() from head.js and use the browser's XUL document

For some reason, suddenly I can't run thumbnail tests locally anymore. To fix this I removed the getXULDocument() method that used the hiddenWindow and just use the main browser window to create temporary elements that aren't appended into DOM. That's much easier and makes the test suite run locally again.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - add regression tests

This patch adds regression tests to make sure we don't capture thumbnails of privacy-sensitive pages.

Comment 8

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Since there is an open privacy review of this feature we may want to ensure we document this as part of that:
https://wiki.mozilla.org/Privacy/Reviews/New_Tab

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 623648 [details] [diff] [review]
Part 2 - remove getXULDocument() from head.js and use the browser's XUL document

>+  let doc = gBrowser.ownerDocument;
>+  let htmlns = "http://www.w3.org/1999/xhtml";
>+  let img = doc.createElementNS(htmlns, "img");
>+  img.setAttribute("src", thumb);

Just use 'document' instead of doc / gBrowser.ownerDocument.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 623647 [details] [diff] [review]
Part 1 -  don't capture thumbnails for privacy-sensitive pages

>+    } catch (e) { /* Not a HTTP channel. */ }

"Not an HTTP channel"

>+        try {
>+          let cacheControl = httpChannel.getResponseHeader("Cache-Control");
>+          if (cacheControl && !(/public/i).test(cacheControlHeader))
>+            return false;
>+        } catch (e) { /* No Cache-Control header set. */ }

This throws without a Cache-Control header? If so, why null-check cacheControl?
Also note that you're testing cacheControlHeader (which is undefined) rather than cacheControl.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 2 - remove getXULDocument() from head.js and use the browser's XUL document

(In reply to Dão Gottwald [:dao] from comment #9)
> >+  let doc = gBrowser.ownerDocument;
> >+  let htmlns = "http://www.w3.org/1999/xhtml";
> >+  let img = doc.createElementNS(htmlns, "img");
> >+  img.setAttribute("src", thumb);
> 
> Just use 'document' instead of doc / gBrowser.ownerDocument.

Fixed.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - don't capture thumbnails for privacy-sensitive pages

(In reply to Dão Gottwald [:dao] from comment #10)
> >+    } catch (e) { /* Not a HTTP channel. */ }
> 
> "Not an HTTP channel"

Fixed.

> >+        try {
> >+          let cacheControl = httpChannel.getResponseHeader("Cache-Control");
> >+          if (cacheControl && !(/public/i).test(cacheControlHeader))
> >+            return false;
> >+        } catch (e) { /* No Cache-Control header set. */ }
> 
> This throws without a Cache-Control header? If so, why null-check
> cacheControl?

Good question. That's unnecessary, removed.

> Also note that you're testing cacheControlHeader (which is undefined) rather
> than cacheControl.

Fixed that locally already but forgot the update the patch here, sorry.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - add regression tests

Rebased.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 623767 [details] [diff] [review]
Part 1 -  don't capture thumbnails for privacy-sensitive pages

>+      // Deny storage if we're viewing a HTTPS page with a
>+      // 'Cache-Control' header having a value that is not 'public'.
>+      if (uri.schemeIs("https") && !this._enablePersistentHttpsCaching) {
>+        try {
>+          let cacheControl = httpChannel.getResponseHeader("Cache-Control");
>+          if (!(/public/i).test(cacheControl))
>+            return false;
>+        } catch (e) { /* No Cache-Control header set. */ }
>+      }
>     }
>+
>+    return true;

The logic here seems to clash with bug 531801.

(In reply to Tim Taubert [:ttaubert] from comment #5)
> This is exact the same implementation that Panorama used
> before and is also what nsHttpChannel does:
> 
> http://mxr.mozilla.org/firefox/source/netwerk/protocol/http/src/
> nsHttpChannel.cpp#2136

This is the Firefox 3 code base.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Tim Taubert [:ttaubert] from comment #5)
> > http://mxr.mozilla.org/firefox/source/netwerk/protocol/http/src/
> > nsHttpChannel.cpp#2136
> 
> This is the Firefox 3 code base.

Oops, this is the right one:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3178

And indeed, this reflects the changes from bug 531801. Will update the patch. Thank you for finding this.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 1 - don't capture thumbnails for privacy-sensitive pages

Updated the patch to not capture HTTPS pages by default.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Part 3 - add regression tests

Updated tests to reflect changes in part 1.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 623786 [details] [diff] [review]
Part 1 -  don't capture thumbnails for privacy-sensitive pages

please rename _enablePersistentHttpsCaching to _sslDiskCacheEnabled

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 623788 [details] [diff] [review]
Part 3 - add regression tests

Please get somebody else (e.g. dietrich, who I think reviewed the head.js glue) to review those tests using generators.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a9e0c4e24451
https://hg.mozilla.org/integration/fx-team/rev/fa73740859cf
https://hg.mozilla.org/integration/fx-team/rev/b1292af5ecd3

Comment 22

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/a9e0c4e24451

https://hg.mozilla.org/mozilla-central/rev/fa73740859cf

https://hg.mozilla.org/mozilla-central/rev/b1292af5ecd3

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Nit: the caching of HTTPS pages is enabled by default, so the comments saying "don't cache thumbnails of HTTPS unless the user explicitly enabled it" are misleading.

More seriously, I do not think the patches in this bug do a lot to address the bug reporter's concerns. It does make sense to honor no-store but it seems like that is a very small part of solving this bug.

Consider a modern web application where it will be pretty normal for the application itself to be cacheable, and then load in sensitive (no-store) content via subresources and/or via XHR. For example, you can imagine an HTML 5 e-banking application that uses AppCache for the application HTML+JS+CSS, and then loads bank account information via XHR, and even does login/logout via XHR. Presumably, if we really think "no-store" is the way to prevent thumbnails of sensitive information, then it needs to take into account every load done on the page; that is, if you couldn't, in theory, reconstruct the page by re-loading everything exclusively from the cache, then for a no-store-based solution, you shouldn't save the thumbnail. On the other hand, if somebody using the computer could just go to the URL of the page that was captured and see the same information (e.g. because of saved cookies), then "honoring" no-store isn't much of a security benefit.

Also, we have non-HTTP data loads, WebSockets in particular, that don't have a way to say "no-store" that can and will be used to load sensitive information.

It seems very difficult to actually solve the bug reporter's problems, in general. However, I think there are several additional mitigations that could be taken:

1. The bug reporter said "I filled out a form on the computer, and in the preview screen it showed my completed form, including my full name, address, phone number, etc." It seems to me that a good heuristic to prevent most instances of this would be to avoid including form fields in the screenshot.

2. Like I mentioned above, if we care about no-store, we should take all the no-store headers into account.

3. The bug reporter mentioned the idea of using a "public computer" or more generally a shared computer. Because all solutions for this are imperfect, it does seem reasonable to enable the computer administrator to disable thumbnails completely and permanently in a way that users cannot enable it. I think there might be an option to do so, but it isn't clear at all how to do so. (More generally, I think it would be beneficial if we had a "public kiosk" installation mode that made it really easy to flip all the relevant settings at once.) I think we are going to get lots of bug reports like this, for this feature and others, and it would be good to have a clear description of our design criteria for these features regarding the privacy issues for public/shared computers, so we don't have to rehash these issues over and over again.

4. Like I mentioned above, a very thorough way of solving these issues would be to save a thumbnail of the page that was retrieved without any HTTP cookies or HTTP auth credentials sent in the requests (for the page, and for any sub-resources).

5. Because the above mechanisms are far from perfect, and because we cannot detect all problematic cases, and because of WebSockets and other upcoming non-HTTP transport protocols, we should have some mechanism for a page to indicate that the current page (and/or some parts of the page; e.g. using CSS @media thumbnail or some such thing) should not be saved as part of the thumbnail, independent of cache-control:no-store.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Those are all valid points, but now that we've used this bug to track the landing of a patch, I don't think we should re-use it to track the larger issue, despite that larger issue being what the bug was originally filed about. Can you create a new bug and copy your comment there?

Comment 25

[Jim Jeffery not reading bug-mail 1/2/11]

Can or should this be approved for Firefox 13 that was released today?  Testing finds that www.distrowatch.com which does use 'cache-control: no-store' is showing a thumbnail on the New Tab Page, thus opening up a privacy/security issue in 13.0.

Comment 27

[Patrick Thompson]

The is a serious security bug and warrants an immediate patch to Version 13.0 rather than waiting until version 15.0. Here's a reproducible scenario as to why I believe a patch is warranted:

1. I logged onto my financial institution and then cycled through the new tab thumbnails until my banking site displayed. It displayed a thumbnail of my logged in accounts, just like the open tab page.

2. I then closed the new tab, logged off the financial institution site, closed its tab page and exited the browser.

3. I re-opened the browser, opened a new tab and, Voila!, there was the thumbnail of my previously logged in account of my banking institution. Of course, with I clicked that page to open it in a tab, it re-loadeded it from the site and showed me the login screen.

Comment 28

[Daniel]

Just as an FYI, this is also occurring in SeaMonkey 2.9.1, so, hopefully, the SM devs action the FF solution as well

Comment 31

[(mostly gone) XtC4UaLL [:xtc4uall]]

Is it feasible/justifiable to get this Mitigation into 14 and maybe the already planned 13.0.2 Chemspill regarding the recent Press Coverages?

Comment 32

[Alex Keybl [:akeybl]]

Tracking for FF14 in case we end up in a position to take this fix in the release. Are we ready to uplift to FF15.

Is this safe to uplift to Aurora already at this point?

Comment 33

[Dão Gottwald [:dao]]

This already landed in time for Firefox 15.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alex Keybl [:akeybl] from comment #32)
> Tracking for FF14 in case we end up in a position to take this fix in the
> release.

All the patches apply cleanly on beta. Tests are passing and looks like this would not be too much of a risk to take if we decide to.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Great! Can you prepare a single rollup patch that applies to beta, and request approval?

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new tab page feature
User impact if declined: private data may be exposed
Testing completed (on m-c, etc.): Landed for Fx 15, multiple weeks without issues
Risk to taking this patch (and alternatives if risky): Quite low risk
String or UUID changes made by this patch: None.

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 637538 [details] [diff] [review]
patch for beta

[Triage comment]
Please go ahead and land to mozilla-beta, before Tuesday at noon PT would be best so it goes into the next beta.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f3f210a275b

Comment 40

[Petes]

This still is a terrible idea, "New Tab Page" should be more like a speed dial (with customisable thumbnails), not like a history tool, otherwise it WILL end up compromising people's privacy somehow. 

Take Opera Mobile as an example to follow, you get the 3x3 page but each cell has a + sign in order for you to configure your favorite site's homepage or link.

Yes you could pin the pages in FF13 but if you unpin them the thumbnails will show up again. 

There's no such a need to make it work as history. Even tough I can see the functionality, you'll also get a lot of useless thumbnail results (many may even appear to be there randomly). To be sure of the speed dial behavior you should go with the Opera Mobile approach, that way you know exactly what will show up each time (and even the position of the cells), it also helps people to get familiar with the feature.

There is no code you can write to "patch" this situation, since different people consider privacy as they may. There are also many websites in the world with different technologies and many countries with different policies/laws (where the simple visit a to a site like Facebook or Twitter may be illegal).

I can see how this will result in more bug reports or complaints, which will only be fixable with some sort of site-specific filter, which can't be perfect due to people's different approaches to privacy.

Thanks.

Author of duplicate 771389

Comment 41

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Marking as verified in Firefox 14 Beta 12. Mac Os 10.6, Ubuntu 12.04 and Windows 7.
Private information (mail content, forms, any information for pages which require log in) is no longer displayed in thumbnails - no thumbnail for that specific site entry in new tab.

Comment 42

[Csaba Kozák [:WonderCsabo]]

Is there a way to re-enable the thumbnails for these sites? For example via a variable in about:config?

Comment 44

[UK92]

(In reply to Csaba Kozák [:WonderCsabo] from comment #42)
> Is there a way to re-enable the thumbnails for these sites? For example via
> a variable in about:config?

Option via about:config would be needed ... it sucks, that half of the "top sites" do not have thumbnails...

Comment 45

[Daniel "warmth" Delgado]

I can confirm that this has been fixed for the latest Nightly version. :D