We should implement prefs that allow the user to directly set the minimum and maximum version of SSL/TLS supported, in a way that doesn't require us to add new prefs every time libssl adds a new TLS version.

This patch drops support for the security.enable_tls and security.enable_ssl3 prefs with new prefs security.tls.version.min and security.tls.version.max. The values/meanings for these prefs are as follows: 0: SSL 3.0 1: TLS 1.0 2: TLS 1.1 3: TLS 1.2 ... n: TLS 1.(n-1) The patch has not been tested yet. I am just looking for feedback on the approach.

In my opinion a policy decision is necessary whether or not migration from old preferences to new preferences is necessary. I've started an email discussion thread to reach this discussion.

Limi agrees with resetting the set of enabled cipher suites to the defaults and Wan-Teh also thinks it is a good idea.

I'm ambivalent about the min/max approach. True, it's simpler and exensible, but I can easily imagine some future point where TLS 1.5 introduces a feature with an obscure way to abuse it that then requires a TLS 1.7 to fix, or was accidentally fixed by other changes in 1.6. You don't want to set the "min" to 1.6 and kill connectivity with all the legacy 1.2 servers, but you'd also not like to set "max" to 1.4 and lose out on whatever protocol goodness was added in 1.6 It seems like a fairly hypothetical case, but the BEAST attack only worked against certain cipher suites so who's to say there won't be an attack against a protocol level? What's the problem with adding a new pref in the rare cases we add a new TLS protocol level?

(In reply to Daniel Veditz [:dveditz] from comment #4) > or was accidentally fixed by other changes in 1.6. You don't want to set > the "min" to 1.6 and kill connectivity with all the legacy 1.2 servers, but > you'd also not like to set "max" to 1.4 and lose out on whatever protocol > goodness was added in 1.6 > > It seems like a fairly hypothetical case, but the BEAST attack only worked > against certain cipher suites so who's to say there won't be an attack > against a protocol level? Wan-Teh and I discussed this at length when we designed the libssl API. In the extremely, extremely unlikely event that we have to disable some protocol version in the middle of the range, we will have a new way of doing that in libssl with an API that hasn't even been designed yet, because we don't think it will become necessary. That is, there is NO way to disable a TLS version in the middle using libssl as it exists now, and this is an intentional design decision. Further, the SSL/TLS protocols themselves do not really support disabling a version in the middle of a range. It is likely that, in the unbelievably unlikely case we would have to do so, we would have to do a lot of work at the libssl and/or PSM and/or Necko levels, like we did to implement the insecure fallback to SSL 3.0 that we do now. Further, it is very unlikely that there are going to be too many more TLS versions, because TLS 1.2 is basically completely extensible. I suggest that we take the same approach here in Gecko that we took in libssl: YANGNI. > What's the problem with adding a new pref in the rare cases we add > a new TLS protocol level? See bug 733632 comment 0. That talks about difficulties at the UI level, but the same difficulties apply with interpreting the prefs. In particular, how do you handle the case where the user has enabled TLS 1.0, disabled TLS 1.1, and enabled TLS 1.2, given that libssl won't let you actually do that? Again, let's keep things simple, and let's avoid building and maintaining footguns.

Comment on attachment 603559 [details] [diff] [review] WIP: Allow the user to enable any version of TLS that libssl supports, maintaining our current defaults (In reply to Brian Smith (:bsmith) from comment #5) > See bug 733632 comment 0. That talks about difficulties at the UI level, but > the same difficulties apply with interpreting the prefs. In particular, how > do you handle the case where the user has enabled TLS 1.0, disabled TLS 1.1, > and enabled TLS 1.2, given that libssl won't let you actually do that? Not sure why you asked for my feedback then. The initial comment in this bug made it sound like that was an option and you were proposing an alternative. Sounds like this bug is really "make prefs match the capabilities of NSS". Also note, in case it wasn't clear: I was only talking about back-end hidden prefs that could be used by, say, the "hotfix addon" if necessary, not anything with UI that's exposed to users. I fully support removing the UI for these prefs and resetting the defaults.

> I fully support removing the UI for these prefs and resetting the defaults. Thanks Dan. Sorry I was unclear in what I was requesting. Resetting the defaults is the main thing I was requesting feedback about. I know that is an unusual thing to do and I want to make sure we're comfortable doing it. Kai, what is your current thinking?

In email, Kai suggested that we either keep the current style of boolean defaults or we implement the migration from the old style to the new style, with the possibility of removing that migration step at one point or another (e.g. the next ESR release). I am planning to implement the migration step from the old style to the new style of prefs, because I think the old style will be confusing for people that dig around in about:config in the future and see "I enabled SSL 3.0 and TLS 1.1 but disabled TLS 1.0; yet Firefox is still using TLS 1.0 for some sites." Kai, I know that most of your objection to the resetting to defaults was the re-enabling of SSL 3.0 for people who had disabled it. Do you object to re-enabling TLS 1.0 if it has been disabled? IMO, disabling TLS 1.0 is really a mistake and indicates that the user things that 3.0 > 1.0 so SSL 3.0 must be better, or that they heard that TLS 1.0 is vulnerable to the BEAST attack but don't realize that SSL 3.0 is too. (See bug 687816 for a serious real-life example of this misunderstanding at Mozilla.) So, my proposed migration rule is: * If security.enable_ssl3 is set to false then set security.tls.version.min=1; * delete the security.enable_ssl3 pref * delete the security.enable_tls pref. Does this seem reasonable to you? When I was researching how to do the migration, I stumbled across bug 690370, linked from https://developer.mozilla.org/En/A_Brief_Guide_to_Mozilla_Preferences, which you might find interesting.

rstrong: After reading the wiki page https://developer.mozilla.org/En/A_Brief_Guide_to_Mozilla_Preferences, I have become unsure about how to about how to rewrite default prefs that live in the defaults/pref/ folder. Basically, I want to replace a pref "enable_ssl3" with a different pref "tls.version.min" in any default/prefs/*.js file in which enable_ssl3 appears. Presumably that has to be done at install/update time because updating files in the install dir requires admin access. Is there any example code that illustrates how to do that?

I'm leaving this discussion, because I don't have time to focus on it, sorry.

(In reply to Brian Smith (:bsmith) from comment #9) > rstrong: After reading the wiki page > https://developer.mozilla.org/En/A_Brief_Guide_to_Mozilla_Preferences, I > have become unsure about how to about how to rewrite default prefs that live > in the defaults/pref/ folder. > > Basically, I want to replace a pref "enable_ssl3" with a different pref > "tls.version.min" in any default/prefs/*.js file in which enable_ssl3 > appears. Presumably that has to be done at install/update time because > updating files in the install dir requires admin access. Is there any > example code that illustrates how to do that? Missed this. Not sure what you are asking for... only default prefs are stored in the install dir (packaged in the omni for the most part) and changing them so the app is built / distributed with the new values changes them when the app is updated.

(In reply to Robert Strong [:rstrong] (do not email) from comment #12) > (In reply to Brian Smith (:bsmith) from comment #9) > > rstrong: After reading the wiki page > > https://developer.mozilla.org/En/A_Brief_Guide_to_Mozilla_Preferences, I > > have become unsure about how to about how to rewrite default prefs that live > > in the defaults/pref/ folder. > > > > Basically, I want to replace a pref "enable_ssl3" with a different pref > > "tls.version.min" in any default/prefs/*.js file in which enable_ssl3 > > appears. Presumably that has to be done at install/update time because > > updating files in the install dir requires admin access. Is there any > > example code that illustrates how to do that? > Missed this. > > Not sure what you are asking for... only default prefs are stored in the > install dir (packaged in the omni for the most part) and changing them so > the app is built / distributed with the new values changes them when the app > is updated. The only scenario where we can't do this at launch seems to be if the sysadmin has added an all.js or defaults/profile/user.js file. Do we need to update those automatically?

App update cannot touch every user profile and customizations are not known to app update (no way for it to know specific component requirements since it is a separate app) so it is up to the application code to handle those cases.

(In reply to Robert Strong [:rstrong] (do not email) from comment #14) > App update cannot touch every user profile and customizations are not known > to app update (no way for it to know specific component requirements since > it is a separate app) so it is up to the application code to handle those > cases. Not even inside the application directory (e.g. C:\Program Files (x86)\Mozilla Firefox\defaults\profile\user.js)?

No, app update is a separate standalone app that doesn't have the preferences code or any other code similar to that. It can remove specific files, an entire dir, etc. but then it would hurt deployments that use those files for other settings in those same files.

Not *quite* sure what the empty namespace is for; it didn't apply properly so I left it out of the patch.

Lacks error handling, not sure what errors should be fatal.

The reason we were looking at doing this in the installer is because the prefs we are primarily concerned about are ones that system administrators set and lock down in enterprise deployments. It seems like it is too difficult to migrate those prefs because the installer hasn't have any support for doing that. (Thanks rstrong for the help in explaining that.) It seems like there is a very wide consensus for removing the UI. Also, people don't like the idea of resetting users' preferences for them. I am actually one of the people that hate that too. But, also, in the last several months I've had to help several people who thought Firefox was broken because they had these prefs set wrongly. Most commonly, it was people who turned off TLS 1.0 because 3.0 > 1.0 and they heard that TLS 1.0 was bad because of the BEAST news. We know that there is basically no difference in security between SSL 3.0 and TLS 1.0 in practice. The people who are required to conform to particular (government) regulations and whatnot need to read our release notes and deal with changes we make anyway, so notifying them in the release notes about the change should be more than sufficient. I am sure our relnotes will have notifications of much more controversial decisions in the near future anyway. I will write the relnote myself. Alex, thanks for pushing this forward. I will look over the patches soon. Since I was the original author I will need Honza or another PSM peer to review them. I will set the review flags after I've double-checked the patches.

- Default to enable SSL 3.0 if security.enable_ssl3 is unset. - Use ternary operator to convert enabled to int (not sure if cast is appropriate here)

Grr, I a typo (check max twice instead of min/max).

Apologies if this is erroneous or unnecessary, but I get the following errors when attempting to build mozilla-central + the 712765 patch: /home/tester/Desktop/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1139:40: error: cannot convert ‘SSLVersionRange* {aka SSLVersionRangeStr*}’ to ‘SSLProtocolVariant’ for argument ‘1’ to ‘SECStatus SSL_VersionRangeSetDefault(SSLProtocolVariant, const SSLVersionRange*)’ /home/tester/Desktop/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1142:44: error: cannot convert ‘SSLVersionRange* {aka SSLVersionRangeStr*}’ to ‘SSLProtocolVariant’ for argument ‘1’ to ‘SECStatus SSL_VersionRangeSetDefault(SSLProtocolVariant, const SSLVersionRange*)’ The dirty hack of changing SSL_VersionRangeSetDefault(&range) to SSL_VersionRangeSetDefault(ssl_variant_stream, &range) seems to work...

Huh. That should actually specify the ssl_variant_stream. Perhaps the API changed between patches. I don't *believe* that any DTLS code uses this; I'll take a look at it. If anything does, it's probably safe just to call SSL_VersionRangeSetDefault twice with _stream and _datagram.

Doesn't look to me like there's any code that actually uses DTLS, other than DTLS-SRTP, which has nothing to do with this, so I've gone ahead and added the ssl_variant_stream parameter.

Alright, so apparently that API doesn't work the way I thought it did. Again! And this one actually builds!

#714197, #715013 build and work for me. (including migration)

Comment on attachment 717548 [details] [diff] [review] WIP: Migrate old preferences [v4] You might want to do a ClearUserPref(), so that the profile doesn't have a now-meaningless pref floating around forever. A couple other things I don't feel strongly about: * The pref value are going to be confusing because you're mapping integers to a version number. It's really confusing to think about things like 3 == "2.1" and 4 == "3". You might want to consider using strings instead, or even nsIVersionComparator. But in the end there's only a tiny number of people who will ever be expected to use these, so whatever is simple is fine. * TBH, I'm not terribly excited about adding min/max prefs at all, but I guess it is incrementally better for the issue noted at the bottom of comment 5. Ideally we would just (1) ship with a set of unchangeable (internal) global defaults and (2) use nsIPermissionManager to allow changing the defaults on a per-broken-site basis.

(In reply to Justin Dolske [:Dolske] from comment #28) > Comment on attachment 717548 [details] [diff] [review] > WIP: Migrate old preferences [v4] > > You might want to do a ClearUserPref(), so that the profile doesn't have a > now-meaningless pref floating around forever. Originally, I had two DeleteBranch() calls in my patch. What happens then if the user, for example, has two browsers that are syncing prefs (e.g. with Weave/Sync) that are different versions? Will the old one then be defaulting to the old values? > * The pref value are going to be confusing because you're mapping integers > to a version number. It's really confusing to think about things like 3 == > "2.1" and 4 == "3". You might want to consider using strings instead, or > even nsIVersionComparator. But in the end there's only a tiny number of > people who will ever be expected to use these, so whatever is simple is fine. This was the original implementation by bsmith; I had 3 == SSL 3.0 and 4 == TLS 1.0 and 5 == TLS 1.1, etc. This (bsmith's) implementation makes it slightly easier to extend for TLS 1.2 as well. > * TBH, I'm not terribly excited about adding min/max prefs at all, but I > guess it is incrementally better for the issue noted at the bottom of > comment 5. Ideally we would just (1) ship with a set of unchangeable > (internal) global defaults and (2) use nsIPermissionManager to allow > changing the defaults on a per-broken-site basis. Basically, it's just better than the old prefs. Perhaps it would be a good idea to use nsIPermissionManager or similar to handle TLS version intolerance in a more reliable manner, though. (see bug 839310)

(In reply to Alex Xu from comment #29) > > You might want to do a ClearUserPref(), so that the profile doesn't have a > > now-meaningless pref floating around forever. > > Originally, I had two DeleteBranch() calls in my patch. What happens then if > the user, for example, has two browsers that are syncing prefs (e.g. with > Weave/Sync) that are different versions? Will the old one then be defaulting > to the old values? services.sync.prefs.sync.security.enable_ssl3 should be removed from firefox.js, too. No need to sync dead prefs, and this problem should go away. Also, we are deeeep into edgecase territory. :)

Alex, you do not need to sync or migrate the old prefs. We can just ignore them and start using the new ones. Justin: We need the ".min" pref for some enterprise deployments that must disable SSLv3 due to government regulations. We need the ".max" pref so that people can test TLS 1.1 and TLS 1.2 support before we enable either of them by default, and to make it easier to rollback the enabling of TLS 1.1 or TLS 1.2 in case a last-minute regression is found.

Fair enough!

Justin, the version range API is documented at https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl.h#251 Alex: Thank you for rebasing this patch onto mozilla-central!

Comment on attachment 717548 [details] [diff] [review] WIP: Migrate old preferences [v4] Alex, thank you for doing this, but based on the research that Monica did, something like 2% of users (i.e. quite a few million) have these preferences mis-set. This is a good opportunity to just automatically fix this for those users. We will need to add a note to the release notes about this change so that enterprise users that actually have a reason to care about the minimum version will be aware that they need to update their configurations.

I, as a user, quite a long time ago, deliberately disabled SSL 3.0 so that the only TLS protocol my browser will use is 1.0, and nearly every site still works. If a website I am about to enter my credit card number into is so outdated that it doesn't support TLS 1.0, I want to know about it; I don't want my browser to just automatically downgrade without my permission. Firefox updates itself automatically without displaying the release notes of the new version. I wouldn't be happy if my TLS settings were changed without my permission without notifying me, and I don't think putting a line into the release notes it enough notice.

(In reply to stephen.mayba from comment #36) >... > Firefox updates itself automatically without displaying the release notes of > the new version. I wouldn't be happy if my TLS settings were changed > without my permission without notifying me, and I don't think putting a line > into the release notes it enough notice. Note: this is configurable so if it were decided to display release notes it could easily be done.

I, as a user, would like also that you change the GUI so that is better understand by the average "Joe" in selection boxes "SSL 3.0", "TLS 1.0" rename them to: "SSL 3.0", "TLS 1.0 (SSL 3.1)", "TLS 1.1 (SSL 3.2)" and "TLS 1.2 (SSL 3.3)" so no user will be confused in the future with SSL 3.0 vs TLS 1.2 for example, and will be immediately visible that TLS 1.2 is the SSL 3.3... better then SSL 3.0 of course. If the user can not disable TLS 1.1 and have SSL 3.0 and TLS 1.0 working, when removing the selection of the TLS 1.1 should also remove the selection of the SSL 3.0 and TLS 1.0... and if selects TLS 1.0, and has TLS 1.2 select, should also select TLS 1.1, since by the current design they must be enable. (Advanced) Users should also be able to reorder the cipher order, so that if vulnerabilities are found (like they have already been in RC4 and CBC algorithms... but not in GCM), the user (or administrators) can try to force the better algorithms first. Design should be changed in order to allow have any protocol enable/ disable without affecting the others... but that's another discussion.

For the last, LAST time: 1. Prefs are being reset because many users have them screwed up and blame Firefox. 2. If you *really* want "more security", edit about:config. There's about a million other things to fiddle with without introducing unnecessary complexity to the UI. Now can we *please* get bug 733632 landed in mozilla-central?

Making clear the TLS vs SSL in the GUI is just to make it more helpful for the normal user... so that they don't screw things just because they think ssl 3.0 is much better then TLS 1.2 (that is in reality SSL 3.3). In the about:config I can't order the ciphers, just enable/ disable... not good enough I think, and I already explain why. And I agree with the reset... although I myself will need to reconfigure everything again... as long their is a clear advice for the users... it's ok.

Comment on attachment 728558 [details] [diff] [review] Allow the user to enable any version of TLS that libssl supports, maintaining our current defaults Review of attachment 728558 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, one small fix. ::: browser/app/profile/firefox.js @@ +1017,5 @@ > pref("services.sync.prefs.sync.security.OCSP.enabled", true); > pref("services.sync.prefs.sync.security.OCSP.require", true); > pref("services.sync.prefs.sync.security.default_personal_cert", true); > +pref("services.sync.prefs.sync.security.security.tls.version.min", 0); > +pref("services.sync.prefs.sync.security.security.tls.version.max", 1); The values here are just true/false (sync the pref or not). s/[01]/true/

https://hg.mozilla.org/integration/mozilla-inbound/rev/882345a7a0a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/04dbe811e4a0

(In reply to Justin Dolske [:Dolske] from comment #41) > The values here are just true/false (sync the pref or not). > > s/[01]/true/ Thanks for the review, dolske! I made this change before checking in the patches.