Last Comment Bug 424872 - Support form restore for securely transmitted site (HTTPS)
: Support form restore for securely transmitted site (HTTPS)
Status: RESOLVED FIXED
: dataloss, ux-error-prevention, ux-undo
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: Firefox 4.0b7
Assigned To: Mike Beltzner [:beltzner, not reading bugmail]
:
Mentors:
: 418505 (view as bug list)
Depends on: 627472 636399
Blocks: 566486
  Show dependency treegraph
 
Reported: 2008-03-24 15:25 PDT by Mike Beltzner [:beltzner, not reading bugmail]
Modified: 2011-04-23 10:35 PDT (History)
30 users (show)
mbeltzner: blocking‑firefox3-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta8+


Attachments
set privacy_level and privacy_level_deferred to 0 (1.43 KB, patch)
2010-10-01 09:45 PDT, Mike Beltzner [:beltzner, not reading bugmail]
paul: review+
Details | Diff | Splinter Review

Description Mike Beltzner [:beltzner, not reading bugmail] 2008-03-24 15:25:28 PDT
The ssl caching improvements should now allow us to set these defaults to improve behaviour on session restore.
Comment 1 Simon Bünzli 2008-03-24 16:39:59 PDT
Mike: Could you please clarify what these "ssl caching improvements" are and why we would suddenly no longer care about storing potentially delicate information on the disk without at least giving the user some UI to get back to Firefox 2.0's privacy level? Considering bug 345345, I'm quite worried about what this might mean for users of public computers...
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-24 22:06:56 PDT
Simon: Shaver and Connor have more details, but as I understand it, we know have better SSL caching support in terms of performance; previously this had been keeping us from turning this preference on by default. See also bug 424874.

In terms of user experience, we're encouraging users to restart their browser and telling them that we'll restore state. With the existing defaults, any state on SSL sites (half written emails, in process transactions) will be lost.

I don't think that bug 345345 changes anything; users on public machines should be sure to log out of their sessions before walking away from a computer. Administrators of public machines should be setting their machines so that the Clear Private Data settings wipe and clear all SSL sessions.

Hm, though, we should probably make sure that browser.sessionstore.privacy_level=0 respects the Clear Private Data setting. I just tested a crash and restore with CPD set to always clear and while history, cookies and passwords were cleared after a crash (as I would expect) my SSL session was restored.
Comment 3 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-24 22:11:47 PDT
We can and should save session state for SSL pages if they're served with "Cache-Control: public" -- that's what controls the disk cache.  If that's what this preference means, then we should change it.  If instead the preference means "save session state for SSL pages regardless of Cache-Control instructions", then we should not change it.  At least, not on the basis of our other SSL caching change alone.

Who can point me at a test that verifies either behaviour? :)
Comment 4 Jo Hermans 2008-03-25 02:29:24 PDT
(In reply to comment #3)
> We can and should save session state for SSL pages if they're served with
> "Cache-Control: public" -- that's what controls the disk cache.  If that's what
> this preference means, then we should change it. 

That was recently added in bug 345181.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-25 12:19:52 PDT
(In reply to comment #3)
> We can and should save session state for SSL pages if they're served with
> "Cache-Control: public" -- that's what controls the disk cache.  If that's what
> this preference means, then we should change it.  If instead the preference
> means "save session state for SSL pages regardless of Cache-Control
> instructions", then we should not change it.  At least, not on the basis of our
> other SSL caching change alone.

Yeah, the preference controls the latter (session store code doesn't check about Cache-Control headers). Bug 424874 is where the "ssl caching improvements" come into play.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-28 08:07:13 PDT
(In reply to comment #3)
> this preference means, then we should change it.  If instead the preference
> means "save session state for SSL pages regardless of Cache-Control
> instructions", then we should not change it.  At least, not on the basis of our
> other SSL caching change alone.

That's a fair comment, I guess. I saw the coupling of this with SSL caching as a way to ensure that changing the default privacy level didn't ravage poor, poor Ts with session-restore.

Beyond that, however, I think that restoring SSL sessions is something we *need* session restore to do from a user experience perspective. I would submit that most users would not expect that some of their pages with half-written content or half-completed tasks would not be restored when they are restarting their browser to complete a security update or an add-on installation.
Comment 7 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-28 09:09:06 PDT
I think the user experience there should perhaps involve warning them about pages with state we won't save, possibly with a Help link to a page that tells them about the privacy tradeoff.  (Namely, that if their browser doesn't come back up, whatever they were entering on that site will be sitting on their disk.  Though since this pref doesn't just apply to the browser-restart case, AFAICT, the exposure is really for _all_ SSL sites that are active when they close their browser, even if by hand, if session store is active.)  

Caching form contents for pages that we will reload (rather than restore from cache) may be a security hazard, as well -- we saw attacks years ago related to changing the type of a form element from "hidden" to "file" on reload, which let the attacker put arbitrary values in the file element, f.e.  Given that I don't think our when-to-reload logic is sound right now (bug 423800), doing this would make me nervous; ssl pages are more likely to be sensitive to abuse, if anything.

There are other things we can do, like encrypting the session data with a one-time key and then passing it to the new process via a file descriptor or other private channel, or using a shared mapping that is private to new and old processes, which would give us the ability to preserve this state within the "session" without leaving it in plain text on the disk.  They are not in scope for 1.9, and may indeed not be worth the effort at all.
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-28 11:41:55 PDT
No arguments with comment 7, and we looked into a better way of alerting users about the dataloss potential only in cases when they have pages that can't be stored, but that's a future.

As for the potential attack: is that type of attack exclusive to the SSL case? It doesn't seem to be, but maybe I'm misunderstanding. It sounds like such an attack requires malicious web content, and I'm not sure why that means SSL pages would be more sensitive to the abuse.

What if we created a new state which was a "one-time" caching of the content for the restart-the-browser case?
Comment 9 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-28 12:02:54 PDT
It's not exclusive to the SSL case, but the expectation (I think codified in various standards) is that SSL content isn't stored on disk unless the site explicitly permits it.

A one-time caching I think would be fine, except that if the browser doesn't come back up, the data is still there.  Maybe that's OK -- it's sort of the same as if we dump core, or have SSL-sent data fragments in our minidumps.
Comment 10 Mike Connor [:mconnor] 2008-05-13 12:04:41 PDT
What we should do instead is figure out whether the page content is being cached (i.e. cache-control: public or the disk cache ssl pref is flipped) and follow those behaviours.
Comment 11 Nicolas Mandil (:NicolasWeb) 2010-09-19 09:08:59 PDT
I think that bug 418505 is a DUP, right ?

Isn't it a nice idea to change the title to "Support form restore for securely transmitted site (HTTPS)" ?
That's the tweaked title Limi gave it in the 6th dot of http://limi.net/articles/24-bugs-firefox-4?utm_source=feedburner&utm_medium=feed&utm_campaign=Feed%3A+on-firefox+%28Alexander+Limi+on+Firefox%29
This proposed change is for searching in bugs DB purpose
Comment 12 Nicolas Mandil (:NicolasWeb) 2010-09-19 09:09:12 PDT
*** Bug 418505 has been marked as a duplicate of this bug. ***
Comment 13 Alex Faaborg [:faaborg] (Firefox UX) 2010-09-21 12:36:34 PDT
Requesting blocking for the simple pref change.  I believe we should address the dataloss issue now, and then follow up later with potentially clearing this information based on a timeout, or an intentional browser close.

Note that the main reason we don't find this to be an incredibly frustrating aspect of our product is because we have all individually flipped the pref already.  I've been refusing to do that, and the result recently has been that I've been losing a significant amount of data.
Comment 14 Alex Limi (:limi) — Firefox UX Team 2010-09-21 15:34:36 PDT
It's a pretty common scenario even here at Mozilla, people lose Bugzilla comments and forum posts all the time since these are posted using https.

If you want to keep things secret and outside your session — that's why we have Private Mode. I believe that this preference was originally decided back when Private Mode didn't exist?
Comment 15 Alex Faaborg [:faaborg] (Firefox UX) 2010-09-21 16:54:34 PDT
>It's a pretty common scenario even here at Mozilla, people lose Bugzilla
>comments and forum posts all the time since these are posted using https.

yeah, but usually after the answer is "I'm sorry you lost that wiki page you spent 45 minutes writing, you should flip your pref."  In general most of us have it flipped, and have stopped losing large quantities of work.
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-21 21:43:52 PDT
This bug is predicated on the idea that the data will be cleared on regular browser shutdown unless the user has set their preferences to always store the data. This should cover the case of:

 - crashes,
 - restarts for updates,
 - when a user has said they always want their state changed

I don't think it's a simple pref flip; all of that needs to be taken into account.
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-22 09:47:34 PDT
(Well, it could be a simple pref flip, but that would ignore the fact that by default we'd store SSL content on disk on shutdown since we're moving the session restore choice to startup - that makes me a little nervous, privacy wise, but I'm not sure if that concern outweighs the bad dataloss experiences around not having your half-written SSL form content saved)
Comment 18 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-09-22 10:43:39 PDT
(In reply to comment #17)
> (Well, it could be a simple pref flip, but that would ignore the fact that by
> default we'd store SSL content on disk on shutdown since we're moving the
> session restore choice to startup - that makes me a little nervous, privacy
> wise, but I'm not sure if that concern outweighs the bad dataloss experiences
> around not having your half-written SSL form content saved)

Just so we all know what's actually

As it is, there are currently 2 prefs that control this. One is for quitting with the knowledge you'll be restoring your session (browser.sessionstore.privacy_level) and the 2nd for when you quit and haven't explicitly said you'll be restoring at startup (browser.sessionstore.privacy_level_deferred).

privacy_level defaults to 1 (save for non-ssl only)
privacy_level_deferred defaults to 2 (save nowhere)

The pref isn't just about form data. It also controls whether or not we save session cookies, session storage, if we serialize post data.
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-30 19:57:50 PDT
Hm, well, in that case, I'd suggest reasonable defaults would be:

browser.sessionstore.privacy_level = 0
browser.sessionstore.privacy_level_deferred = 1

Rationale:

 - if the user has specifically indicated that they want their state saved, then we should save that state as best we can; this will also save that state in cases of system-required restarts, like application/add-on update

 - if the user hasn't indicated that, but then at some later point attempts to restore their session, then we should restore as much as is safe to restore, and at that point we assume SSL content was priv'd. note that this does mean that non-SSL session cookies used to mediate logins will also be restored

Paul: what happens in the case of a browser crash? Would we get the data from privacy_level_deferred unless the user had a priori said they always want their session saved?
Comment 20 Jesse Ruderman 2010-09-30 20:25:37 PDT
I think both should be 0.  We're moving toward all session restore being a "deferred" decision by getting rid of the at-quit prompt.  Tying this local-security-vs-usability tradeoff to SSL means (1) Bugzilla users will still have to tweak hidden prefs and (2) sites like Bugzilla will be less inclined to use SSL.  Tying the tradeoff to cache-control:no-store (rather than SSL) *might* make sense.
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-30 21:00:55 PDT
AIUI, though, that would mean that we write the SSL-based content to disk at all times, in an unencrypted format. Previously we'd had privacy_level=1 because that was considered a no-no and spec violation.

I mean, I'm fine with comment 20 - but surprised to hear it from Jesse! :)
Comment 22 Boris Zbarsky [:bz] 2010-09-30 21:25:06 PDT
> that would mean that we write the SSL-based content to disk at
> all times, in an unencrypted format.

We do that anyway, modulo Cache-Control: private and no-store; it's called the HTTP cache.  We should key off the same criteria here, as Jesse says.

Ideally, in fact, the document would just surface its caching status (since we would want it here as well as for both bfcache form state restoration and regular history form state restoration), right?
Comment 23 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-10-01 00:23:15 PDT
(In reply to comment #19)
> Paul: what happens in the case of a browser crash? Would we get the data from
> privacy_level_deferred unless the user had a priori said they always want their
> session saved?

No, it would always be privacy_level. privacy_level_deferred is only used when quitting.
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-01 07:51:18 PDT
> No, it would always be privacy_level. privacy_level_deferred is only used when
> quitting.

Excellent. That's what I was hoping.

> We do that anyway, modulo Cache-Control: private and no-store; it's called the
> HTTP cache.  We should key off the same criteria here, as Jesse says.

Huh, I had thought earlier assertions were that we didn't store that information beyond a session as per spec. OK, 0 and 0 it is for the defaults, then, I guess.
Comment 25 Boris Zbarsky [:bz] 2010-10-01 08:33:37 PDT
> Huh, I had thought earlier assertions were that we didn't store that
> information beyond a session as per spec.

We used to not cache SSL content on disk at all.  Then we changed to caching it on disk if it had Cache-Control: public.  Then we changed to caching it on disk unless either no-store or private.

See bug 345181 and bug 531801.  Then again, the first of these being fixed is what prompted this bug to be filed...
Comment 26 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-01 09:45:12 PDT
Created attachment 480140 [details] [diff] [review]
set privacy_level and privacy_level_deferred to 0

Sets both prefs to 0
Comment 27 Boris Zbarsky [:bz] 2010-10-01 10:54:29 PDT
We do need to not store data for private/no-store stuff before flipping the prefs, right?
Comment 28 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-01 11:07:50 PDT
I don't think that's a requirement, no. These prefs govern whether or not the content will be restored. If we think we're caching it inappropriately (regardless of the user's decision whether or not to restore it) that's a separate issue, IMO.
Comment 29 Boris Zbarsky [:bz] 2010-10-01 11:26:50 PDT
I thought the privacy_level pref affected what's saved, not just what's restored.  Is that not the case?
Comment 30 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-10-01 13:57:18 PDT
(In reply to comment #29)
> I thought the privacy_level pref affected what's saved, not just what's
> restored.  Is that not the case?

It affects what's saved. When we restore, we'll restore whatever we read off disk.
Comment 31 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-01 14:30:12 PDT
GAH! Fine. That's why I proposed 0/1 as the defaults. Go back and re-read comment 19. We should save EVERYTHING when we have a priori knowledge that the user is going to want to restore their session. Otherwise, we should only save the non-SSL things, if we care about not inadvertantly saving SSL-issued content to disk.

I don't know the status of "not storing data for private/no-store stuff" - that's still another bug, IMO.
Comment 32 Jesse Ruderman 2010-10-01 16:12:20 PDT
I really don't see how it makes sense to turn off session restore for otherwise cacheable SSL pages.  Session restore information is generally less privileged than the page itself, and the advantage of keeping it is higher (avoiding dataloss vs improving performance).
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-04 06:51:22 PDT
Jesse: that certainly makes sense to me, yes. I'm really really having trouble understanding the intricacies of how we decide what to store or not, and how that interacts with the cache control settings.

I *believe* that we should save all user generated content by default. It's the most friendly and helpful thing to do (though can ultimately be abused by local snoopers - but that's what Private Browsing Mode is for, IMO).

I am uncertain why this relates at all to the cache-control information. If the website says that we shouldn't cache its content, then we should observe that instruction, but not have that affect how we save user generated content (such as the page location or any information they have typed into a textarea).

The cache control bug seems separate (is there one on that issue?) and I go back to believing that 0/0 are the appropriate preferences. Paul: the patch as it stands is ready for your review.
Comment 34 Justin Dolske [:Dolske] 2010-10-04 21:30:42 PDT
(In reply to comment #33)

> I am uncertain why this relates at all to the cache-control information.

My understanding is that this was a hint to avoiding saving sensitive information. EG, if https://nuclear-launch-codes.com or https://rate-your-proctologist.org says Cache-Control: OMG NEVER STORE THIS DATA, then maybe the browser ideally ought not to be storing stuff the user typed either.

But it's fair to ask if that's really a good indication for the sensitivity of the _user's_ data, and if it outweighs the cost of dataloss.
Comment 35 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-10-08 11:20:12 PDT
Comment on attachment 480140 [details] [diff] [review]
set privacy_level and privacy_level_deferred to 0

It sounds like we're all now aware of what changing these values actually means in relation to the data stored on disk, so r=me
Comment 36 :Felipe Gomes (needinfo me!) 2010-10-08 11:55:30 PDT
Pushed: http://hg.mozilla.org/mozilla-central/rev/4874be6eedbe
Comment 37 Ed Morley [:emorley] 2011-01-04 16:25:45 PST
If I'm just misunderstanding the situation, then apologies for the spam, but after reading:
http://limi.net/articles/firefox-preferences

...the implication is that prefs like this are only reset on new profiles - meaning the vast majority of Firefox users wouldn't see the benefit of this patch, given that they have no idea how to make a new profile.

Is this indeed the case, or will upgraders from 3.6.x -> 4.0 final have the pref flipped automatically, even on current profiles? (eg: like image.discardable =false was flipped automatically on nightlies a while back). 

Thanks!
Comment 38 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-01-04 16:33:25 PST
(In reply to comment #37)
> ...the implication is that prefs like this are only reset on new profiles -
> meaning the vast majority of Firefox users wouldn't see the benefit of this
> patch, given that they have no idea how to make a new profile.

Perhaps what Alex didn't make clear there is that if that prefs only retain *custom* values. If you never changed the preference value from the default (either manually or add-on or whatever), then you will get the new value that Firefox sets as the default in the new version.

If you changed the value, then we keep & use that value. Most people haven't, especially for this pref so it would effect very few people. And those that have changed the pref have probably done it intentionally.

tl;dr the pref will be flipped automatically for most people.
Comment 39 Ed Morley [:emorley] 2011-01-04 16:40:28 PST
Thank you - makes sense. (Just showing my ignorance with the prefs system, but now I know at least!)

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