The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

(Depends on: 1 bug, {perf})

Trunk
mozilla1.9.3a3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments)

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.
Created attachment 424998 [details] [diff] [review]
part 1: set the pref to cache all SSL content
[Checkin: Comment 8]
Attachment #424998 - Flags: review?(bzbarsky)
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.
Attachment #425001 - Flags: review?(bzbarsky)
I'd really like Jesse and Daniel to give this a once-over...

Comment 4

7 years ago
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.
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).
Attachment #424998 - Flags: review?(bzbarsky) → review+
Comment on attachment 424998 [details] [diff] [review]
part 1: set the pref to cache all SSL content
[Checkin: Comment 8]

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

r=bzbarsky
Attachment #425001 - Flags: review?(bzbarsky) → review+
thanks, checked in

http://hg.mozilla.org/mozilla-central/rev/51f9d293c939
http://hg.mozilla.org/mozilla-central/rev/c4a6cd1c88ec
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2

Comment 9

7 years ago
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?
Created attachment 426899 [details] [diff] [review]
add missing UsingSSL() check

Oops. Excellent catch.
Attachment #426899 - Flags: review?(bzbarsky)

Updated

7 years ago
Duplicate of this bug: 309368
Attachment #426899 - Flags: review?(bzbarsky) → review+

Comment 12

7 years ago
This bug should not be RESOLVED FIXED.
I'm actually going to reopen this until the patch from comment 10 lands.  That landing needs to block 1.9.3.
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Resolution: FIXED → ---

Updated

7 years ago
Blocks: 547537

Comment 14

7 years ago
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

7 years ago
Contrary to SeaMonkey trunk Firefox trunk can be run with browser.cache.disk_cache_ssl set to false without problems.
Attachment #424998 - Attachment description: part 1: set the pref to cache all SSL content → part 1: set the pref to cache all SSL content [Checkin: Comment 8]
Attachment #425001 - Attachment description: part 2: remove the code for cache-control public → part 2: remove the code for cache-control public [Checkin: Comment 8]
> This patch seems to have caused bug 547537

Yes, because the patch from comment 10 did not land...
No longer blocks: 547537
Depends on: 547537
Since the tree's closed right now, marking checkin-needed so that someone else can get to this before I can.
Keywords: checkin-needed
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/a6ff3b5d364b

Possible to write tests for it?
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a2 → mozilla1.9.3a3
blocking2.0: ? → betaN+

Comment 19

7 years ago
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

7 years ago
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

7 years ago
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

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

Comment 23

7 years ago
You're correct - only no-store content should not be stored at the user agent.  Thanks for the clarification.
Depends on: 660749

Comment 24

6 years ago
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
I _think_ that all of those statements are correct.

Updated

4 years ago
Blocks: 810085
You need to log in before you can comment on or make changes to this bug.