822948 - Don't capture thumbnails when 'Cache-Control: no-store' is given in a meta tag (instead of a HTTP header)

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Dave Garrett]

I was not aware that there were already supposed to be measures in place to prevent the caching of a thumbnail for my bank account page when I filed bug 822516. I initially just filed it because I noticed a thumbnail of a logged-in Bugzilla screenshot and later tested with my bank. Apparently it's not supposed to be saving a screenshot of the latter...

I've filed this with the security flag set out of paranoia, but I don't think I'll be posting anything too private.

Let me start with the caveat that I think I've used Live HTTP Headers maybe once before and I'm not exactly an expert with it.

The testing here is now with the latest Firefox nightly on Linux in a new profile.

Build identifier: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121218 Firefox/20.0

Starting from a new profile, going straight to pncbank.com and logging in, I can then check the thumbnails dir for that profile and see a couple thumbs of the front page with my username half typed in and another thumb of my logged in account page. If I start again with a fresh profile but first go to about:config and set browser.cache.disk_cache_ssl to false, I only get the first two screenshots not the latter. That pref is off by default, however, so in the normal case I get a thumbnail for my logged in account. Even with it on it is caching two (?) thumbnails for an HTTPS page as well. I've also got two thumbnails for the first-run page for some reason.

Digging into the headers returned for the account page I see the following headers, with a few parts redacted:

------------------------------------------------------------
https://www.onlinebanking.pnc.com/alservlet/OnlineBankingServlet

POST /alservlet/OnlineBankingServlet HTTP/1.1
Host: www.onlinebanking.pnc.com
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121218 Firefox/20.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: https://www.pnc.com/webapp/unsec/Homepage.do?siteArea=/PNCCorp/PNC/Home/Personal
Connection: keep-alive
Content-Type: application/x-www-form-urlencoded
Content-Length: 83
hiddenAcctTypeLetter=<REDACTED>

HTTP/1.1 200 OK
Date: Wed, 19 Dec 2012 03:43:14 GMT
Set-Cookie: TLTSID=<REDACTED>; Path=/; Domain=.pnc.com
Set-Cookie: TLTUID=<REDACTED>; Path=/; Domain=.pnc.com; Expires=Wed, 19-12-2022 03:43:14 GMT
Set-Cookie: JSESSIONID=<REDACTED>; Path=/
Set-Cookie: JSESSIONID=<REDACTED>; Path=/
Set-Cookie: <REDACTED>;path=/;secure
Set-Cookie: <REDACTED>;path=/;secure
X-HP-CAM-COLOR: V=1;ServerAddr=<REDACTED>
ntCoent-Length: 4786
Expires: Thu, 01 Dec 1994 16:00:00 GMT
Cache-Control: no-cache="set-cookie, set-cookie2"
Keep-Alive: timeout=60, max=277
Connection: Keep-Alive
Content-Type: text/html;charset=ISO-8859-1
Content-Language: en-US
Content-Encoding: gzip
Content-Length: 1645
------------------------------------------------------------

------------------------------------------------------------
https://www.onlinebanking.pnc.com/alservlet/MyAccountsServlet

GET /alservlet/MyAccountsServlet HTTP/1.1
Host: www.onlinebanking.pnc.com
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121218 Firefox/20.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: <REDACTED>
Cookie: <REDACTED>
Connection: keep-alive

HTTP/1.1 200 OK
Date: Wed, 19 Dec 2012 03:43:22 GMT
X-HP-CAM-COLOR: V=1;ServerAddr=<REDACTED>
Pragma: no-cache
Expires: Tue, 04 Dec 1993 21:29:02 GMT
Cache-Control: no-cache, max-age=0, s-maxage=0, must-revalidate, proxy-revalidate, no-store, private
Keep-Alive: timeout=60, max=260
Connection: Keep-Alive
Content-Type: text/html;charset=ISO-8859-1
Content-Language: en-US
Content-Encoding: gzip
Transfer-Encoding: chunked

------------------------------------------------------------

This URL is the one displayed in the location bar:
https://www.onlinebanking.pnc.com/alservlet/OnlineBankingServlet

This URL is a frame inside that with the actual content:
https://www.onlinebanking.pnc.com/alservlet/MyAccountsServlet

If I click "Account Activity" that switches the page in the frame to the page for that and the screenshot is updated to show my transaction history. If I then right-click on the "Summary" link (which points back to initial page) and open that in a new tab, I've now got the "MyAccountsServlet" URL open without the frame and no new screenshot is created for it. My initial guess is that the first page's header is not enough to make Firefox not save a thumbnail so it saves the whole thing even though the page in the frame is set not to be cached.

Comment 1

[Dave Garrett]

A few bits of additional information:

The "OnlineBankingServlet" page creates its frameset and frame DOM elements via document.writeln().

While I don't see a "no-store" in the header for the "OnlineBankingServlet" page, it does have this in its markup:
<meta http-equiv="Pragma" content="no-cache" />
<meta http-equiv="Cache-Control" content="no-cache, max-age=0, must-revalidate, no-store" />

Comment 2

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

I think this is a dupe of 754608, that we may need to reopen

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Let's not re-open that bug, it's full of noise already. We should file a follow-up (or just handle it here) because the dynamically written page seems to be a special case. Looks like we can't use nsIHttpChannel.isNoStoreResponse() because that property obviously hasn't been there when the channel was created.

Not sure how we can get access to those meta tags' values from JS after a page load. The only way I found is this:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/PlacesUIUtils.jsm#469

If places does it like this I think that there's probably no other way to do it. Would be a simple fix I think. And we need a test.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

But wait, there's more! I just found a better way:

nsDOMWindowUtils::GetDocumentMetadata(const nsAString& aName, nsAString& aValue);

That should be straightforward to use.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: honor 'Cache-Control: no-store' given in a meta tag

Umm, who might want to review this? Gavin? Feel free to pass it on :)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

BTW, thank you for reporting, Dave. Nice investigation on your side - it really helped writing a fix for this quickly.

Comment 7

[Dave Garrett]

Correct me if I'm wrong, but there are two issues to fix here. First, it needs to check for a "no-store" meta tag, which is what the patch does. That would fix this case, but shouldn't it also be checking for "no-store" in any content in the page, in a frame, image, or whatever, even if the page itself doesn't have "no-store"? Not storing to disk something in a page would mean that even if the page were cached to disk, the sensitive bit wouldn't be. In the case of a thumbnail the sensitive bit gets flattened in, so simply "no-store" on just the part not to be stored isn't enough. Specifically, I think my case should be fixed even if the main page didn't have it in a meta tag (or header); just the "no-store" inside the frame.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Yes, you're right about that. We should apply the same rules to subframes. I'm going to file a follow-up for this.

Comment 9

[Dave Garrett]

I didn't end up really needing this bug hidden, I think. Someone might as well unhide it.

Comment 10

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

(In reply to Dave Garrett from comment #7)
> Correct me if I'm wrong, but there are two issues to fix here. First, it
> needs to check for a "no-store" meta tag, which is what the patch does.

We don't support Cache-Control in <meta> tags. See bug 202896.

Unless the HTML5 spec says that browsers are required to support Cache-Control directive in <meta>, I don't think we should bother with it, as it's non-standard.

Comment 11

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

(In reply to Brian Smith (:bsmith) from comment #10)
> We don't support Cache-Control in <meta> tags. See bug 202896.
> 
> Unless the HTML5 spec says that browsers are required to support
> Cache-Control directive in <meta>, I don't think we should bother with it,
> as it's non-standard.

Clearly some sites expect it to have an effect. "Supporting" it (at least in this particular case) results in a better experience for users (their sensitive bank account page is not thumbnailed), so I'm not sure "it's not standard" is strong enough justification to WONTFIX this. But maybe there are downsides to landing a patch like this one that I am not seeing.

Comment 12

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> Clearly some sites expect it to have an effect. "Supporting" it (at least in
> this particular case) results in a better experience for users (their
> sensitive bank account page is not thumbnailed), so I'm not sure "it's not
> standard" is strong enough justification to WONTFIX this.

Then why doesn't this patch handle <meta content="Firefox, please don't create a thumbnail for this page">? :)

> But maybe there
> are downsides to landing a patch like this one that I am not seeing.

I agree that it is problematic that the site expects all browsers to honor Cache-Control in <meta> like IE does, but IMO the problem is with the site, not with the non-IE browsers (At least Chrome agrees with us, according to chromium issue 2763). 

It would be strange and confusing that we wouldn't save the thumbnail but we would save the actual sensitive page in the cache; doing that would give users of websites using Cache-Control in <meta> a false sense of security. Conversely, our current behavior makes the website's serious bug more obvious. If I were a developer (or user) of said website I might notice the bad thumbnail and then realize that the site is making a bogus assumption in general about how browsers handle Cache-Control in <meta>. Potentially then the site can be corrected.

Comment 13

[Dave Garrett]

This thumbnail is created because of Firefox not honoring the no-store meta tag AND not honoring the no-store header in the frame. Fixing either one should fix this particular case. I would think the latter is more important but the former is probably the quicker fix. Technically speaking, there's not much of a reason for this page with the no-store meta tag to not be stored in the disk cache; it's just a frameset and some scripting.

Comment 14

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

(In reply to Brian Smith (:bsmith) from comment #12)
> Then why doesn't this patch handle <meta content="Firefox, please don't
> create a thumbnail for this page">? :)

We don't have any evidence of sites expecting that to work (and it doesn't work in other browsers).

> It would be strange and confusing that we wouldn't save the thumbnail but we
> would save the actual sensitive page in the cache; doing that would give
> users of websites using Cache-Control in <meta> a false sense of security.

Perhaps, but there is a practical distinction between "caching sensitive pages" and "caching and thumbnailing sensitive pages", so the sense of security may not be entirely false. Thumbnails are much easier to access for a non-technical user than the cache is. If you treat "security" as a binary state, both are "insecure". But in practice, it may be that fewer users will run into trouble if we avoid thumbnailing even when we cache.

But now we're mostly debating the merits of fixing bug 202896. I agree that it would be optimal if our treatment of the disk cache and the thumbnail store were consistent. But, assuming that there are significant benefits to obeying this meta tag value (which I don't think we've established quite yet), I think it is possible for those benefits to outweigh the downsides to being inconsistent.

Comment 15

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> (In reply to Brian Smith (:bsmith) from comment #12)
> > Then why doesn't this patch handle <meta content="Firefox, please don't
> > create a thumbnail for this page">? :)
> 
> We don't have any evidence of sites expecting that to work (and it doesn't
> work in other browsers).

I know. I should have put more emphasis on the smiley.

> But now we're mostly debating the merits of fixing bug 202896. I agree that
> it would be optimal if our treatment of the disk cache and the thumbnail
> store were consistent. But, assuming that there are significant benefits to
> obeying this meta tag value (which I don't think we've established quite
> yet), I think it is possible for those benefits to outweigh the downsides to
> being inconsistent.

I'm pretty sure that we won't fix bug 202896 this year, if ever. It's be a low-benefit, high-cost bug to fix. (In particular, the extra complication and potential negative performance implications of the HTTP cache interfacing with the HTML parser seem hard to justify considering the history that Firefox and Chrome have of not supporting the feature.)

> But in practice, it may be that fewer users will run into trouble
> if we avoid thumbnailing even when we cache.

I still like my reasoning from my previous comment better for not fixing this, but it's a judgement call, so I'll leave it to the peers of this module.

I field bug 823829 for the more serious bug where we don't honor the Cache-Control: no-store directive for sub-requests of the page being thumbnailed.

Comment 16

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

Comment on attachment 693909 [details] [diff] [review]
honor 'Cache-Control: no-store' given in a meta tag

Given comment 13, it's not clear that this will have a material impact in practice, and so it's probably not worth being inconsistent with the disk cache's behavior. We can revisit if we have additional evidence that this would help, once bug 823829 is fixed.

Comment 17

[Dave Garrett]

My bug's been hijacked. The issue I reported in comment 0 is now apparently moved to bug 823829 and the other issue of meta tags apparently got this WONTFIXed?  :/

Comment 18

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

(In reply to Dave Garrett from comment #17)
> My bug's been hijacked. The issue I reported in comment 0 is now apparently
> moved to bug 823829 and the other issue of meta tags apparently got this
> WONTFIXed?  :/

Sorry Dave. I filed bug 823829 because the summary of this bug and the discussion of the patch were specific to the meta tag case. I added your very useful comment 0 to the new bug.

Comment 19

[Dave Garrett]

(In reply to Brian Smith (:bsmith) from comment #18)
> I filed bug 823829 because the summary of this bug and the
> discussion of the patch were specific to the meta tag case

The summary was hijacked to focus on the second issue derived from comment 1.

You know what... I'm taking a stand and de-hijacking the bug; I'm changing it back to focus on my initial diagnosis in the last sentence of comment 0. We don't need to duplicate this bug just because one patch was rejected. :S

Comment 21

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

I'm sorry if the "hijacking" offended you, Dave. It's really not meant as a slight against you at all, we definitely appreciate you reporting the issue and providing detailed comments.

Tim morphed the bug because he saw two separate issues to be fixed, and thus we needed two separate bugs to track that (he mentioned he'd file a followup in comment 8, but Brian ended up handling that). In retrospect, that turned out to be wrong, but the damage is already done.

The focus really needs to be on getting the bug fixed, not about the Bugzilla mechanics involved. It's much easier to do that in the new bug, where this old patch and irrelevant discussion isn't confusing things. It's also important that we track the decision to WONTFIX this bug as previously summarized. "Duplicating the bug" puts us in a much better bug tracking spot, so I'm going to re-do that - hope you understand.

Comment 22

[Dave Garrett]

It's not that it's offensive, it's just frustrating to spend a fair bit of time figuring out how to use Live HTTP Headers (why is there no search feature and only a filter for new headers, thus making you have to retest multiple times to narrow down what you care about in the din of headers?) testing, cleaning up the info to post so I don't post my bank login cookie publicly, and diagnosing the problem, just to get into a logistical oddity about one way to fix the issue? I've seen plenty of other bugs get hijacked in some way before (sometimes acknowledged as such) and it's annoying every time. I know that I shouldn't take it personally but it nonetheless bugs me a bit more than it should...

:sigh:

As to the topic that was WONTFIXed, I tend to agree. It would be better to just standardize the meta tags so they would consistently work everywhere to provide a way to control this sort of thing without server configuration. That seems useful to me.