Last Comment Bug 531801 - Should cache SSL content to disk even without Cache-Control: public
: Should cache SSL content to disk even without Cache-Control: public
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla1.9.3a3
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
: Patrick McManus [:mcmanus]
Mentors:
: 309368 (view as bug list)
Depends on: CVE-2011-0082 547537
Blocks: 810085
  Show dependency treegraph
 
Reported: 2009-11-30 04:04 PST by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2012-11-08 15:00 PST (History)
13 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
part 1: set the pref to cache all SSL content [Checkin: Comment 8] (941 bytes, patch)
2010-02-03 08:30 PST, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
Details | Diff | Splinter Review
part 2: remove the code for cache-control public [Checkin: Comment 8] (6.40 KB, patch)
2010-02-03 08:45 PST, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
Details | Diff | Splinter Review
add missing UsingSSL() check (1022 bytes, patch)
2010-02-14 08:15 PST, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2009-11-30 04:04:42 PST
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) 2010-02-03 08:30:51 PST
Created attachment 424998 [details] [diff] [review]
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) 2010-02-03 08:45:17 PST
Created attachment 425001 [details] [diff] [review]
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 [:bz] (still a bit busy) 2010-02-03 09:05:22 PST
I'd really like Jesse and Daniel to give this a once-over...
Comment 4 Jesse Ruderman 2010-02-03 12:05:37 PST
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) 2010-02-03 12:58:02 PST
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 [:bz] (still a bit busy) 2010-02-11 19:21:39 PST
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 [:bz] (still a bit busy) 2010-02-11 19:57:56 PST
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) 2010-02-12 09:15:28 PST
thanks, checked in

http://hg.mozilla.org/mozilla-central/rev/51f9d293c939
http://hg.mozilla.org/mozilla-central/rev/c4a6cd1c88ec
Comment 9 Alfred Kayser 2010-02-14 01:41:21 PST
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) 2010-02-14 08:15:07 PST
Created attachment 426899 [details] [diff] [review]
add missing UsingSSL() check

Oops. Excellent catch.
Comment 11 Steffen Wilberg 2010-02-15 13:29:18 PST
*** Bug 309368 has been marked as a duplicate of this bug. ***
Comment 12 kitchin 2010-02-26 18:03:11 PST
This bug should not be RESOLVED FIXED.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2010-02-26 18:09:01 PST
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 2010-03-06 04:31:04 PST
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 2010-03-06 04:42:50 PST
Contrary to SeaMonkey trunk Firefox trunk can be run with browser.cache.disk_cache_ssl set to false without problems.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2010-03-07 21:25:07 PST
> 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) 2010-03-08 14:09:08 PST
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 [:bz] (still a bit busy) 2010-03-08 16:52:31 PST
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/a6ff3b5d364b

Possible to write tests for it?
Comment 19 Rick Alther 2010-10-12 23:13:26 PDT
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 2010-10-13 01:12:16 PDT
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 2010-10-14 13:48:20 PDT
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 2010-10-14 15:54:36 PDT
I think it's only "cache-control: no-store" that's not cached at all, fwiw.
Comment 23 Rick Alther 2010-10-19 10:49:31 PDT
You're correct - only no-store content should not be stored at the user agent.  Thanks for the clarification.
Comment 24 Mikko Rantalainen 2011-06-28 04:25:55 PDT
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 [:bz] (still a bit busy) 2011-06-28 07:08:09 PDT
I _think_ that all of those statements are correct.

Note You need to log in before you can comment on or make changes to this bug.