531801 - Should cache SSL content to disk even without Cache-Control: public

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Currently, Firefox only caches SSL content to disk if it was sent with Cache-Control: public. On the other hand, IE and Chrome cache SSL content even without that header. And this header was not designed to affect client caches (only proxies).

The reason why I implemented it this way was to avoid caching secure content to disk without an explicit hint that this is OK to do so, but considering that especially IE doesn't require this header, it seems OK to remove this requirement.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part 1: set the pref to cache all SSL content [Checkin: Comment 8]

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part 2: remove the code for cache-control public [Checkin: Comment 8]

This effectively reverts most of the patch for bug 345181.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'd really like Jesse and Daniel to give this a once-over...

Comment 4

[Jesse Ruderman]

Comment 0 sounds good to me.  I don't like unnecessary differences between http and https, because they can make https a disadvantage.  (I care less about differences that are just defaults, though.)

But I don't understand why you're flipping the pref to true *and* removing code that's needed in the case where the pref is set to false.  If we don't want to support having the pref set to false, we should remove the pref.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

We'd still support setting the pref set to false, it'd just mean we never cache SSL content to disk (like we used to).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 424998 [details] [diff] [review]
part 1: set the pref to cache all SSL content
[Checkin: Comment 8]

r=bzbarsky

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425001 [details] [diff] [review]
part 2: remove the code for cache-control public
[Checkin: Comment 8]

r=bzbarsky

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

thanks, checked in

http://hg.mozilla.org/mozilla-central/rev/51f9d293c939
http://hg.mozilla.org/mozilla-central/rev/c4a6cd1c88ec

Comment 9

[Alfred Kayser]

Should this:
// Only cache SSL content on disk if the pref is set
if (!gHttpHandler->IsPersistentHttpsCachingEnabled())
not also check whether is mConnectionInfo->UsingSSL()?
Now when 'HttpsCachingEnabled' is false, everything not cached?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: add missing UsingSSL() check

Oops. Excellent catch.

Comment 12

[kitchin]

This bug should not be RESOLVED FIXED.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm actually going to reopen this until the patch from comment 10 lands.  That landing needs to block 1.9.3.

Comment 14

[Sven Grull]

This patch seems to have caused bug 547537 where having set browser.cache.disk_cache_ssl to false also prevented http files from being written to disk cache.

Comment 15

[Sven Grull]

Contrary to SeaMonkey trunk Firefox trunk can be run with browser.cache.disk_cache_ssl set to false without problems.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> This patch seems to have caused bug 547537

Yes, because the patch from comment 10 did not land...

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Since the tree's closed right now, marking checkin-needed so that someone else can get to this before I can.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/a6ff3b5d364b

Possible to write tests for it?

Comment 19

[Rick Alther]

I know I'm late to the party here, but I have a few questions regarding this feature.  My concern is that there appears to be no discussion from a security point of view on whether this is a good idea or not.  The argument is simply that IE and Chrome do it, so why don't we?

I realize a bulk of SSL traffic can be cached without any security issues and would lead to a nice performance improvement for SSL sites.  However, there is clearly some SSL content that contains private or sensitive information.  If that is cached and some virus, malware, etc. reads the cache, we open the users to possible data theft of their sensitive information.

Will the SSL cache be encrypted?  Are we relying on the Cache-Control (no-cache/no-store) HTTP header to prohibit caching of some SSL data?

Comment 20

[Jesse Ruderman]

See comment 4 and my comments in bug 424872.  I'm on MoCo's security team :)

Among sites that don't use cache-control:no-store, the correlation between "SSL" and "sensitive" is very low.  Furthermore, protecting against future malware infections doesn't buy you much; the malware can just wait for you to visit the sensitive site again once you're infected.

Penalizing sites' performance for using SSL has had a real effect on the overall security of the internet (see e.g. bug 358384).

Comment 21

[Rick Alther]

Thanks for the info Jesse.  As long as any no-store/no-cache/private data is not cached, I'm comfortable with caching the SSL content.  That puts the decision into the content provider's hands - which is best as they know the sensitivity of the data.  Just for FYI, I'm really thinking about this from a corporate security perspective, which can be quite strict.

Comment 22

[Jesse Ruderman]

I think it's only "cache-control: no-store" that's not cached at all, fwiw.

Comment 23

[Rick Alther]

You're correct - only no-store content should not be stored at the user agent.  Thanks for the clarification.

Comment 24

[Mikko Rantalainen]

Have I understood the current implementation correctly? Please, tell me if one or more of the following statements are incorrect:

- Allow writing cached copy to disk unless header "Cache-Control" contains "no-store". No exceptions (e.g. HTTPS).

- For example, following may be cached on disk, usable for 1h (still available after browser restart):
Cache-Control: private, max-age=3600, s-maxage=0

- For example, following may be cached on disk (to be user for e.g. Save As feature), will not be used to satisfy any subsequent request:
Cache-Control: private, no-cache, max-age=0, s-maxage=0

- For example, following may be cached in memory (to be user for e.g. Save As feature), will not be used to satisfy any subsequent request:
Cache-Control: private, no-store, max-age=0, s-maxage=0

Comment 25

[Boris Zbarsky [:bzbarsky]]

I _think_ that all of those statements are correct.