593077 - Remove SSL 2.0 support

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Wan-Teh Chang]

Today SSL 2.0 is disabled by default, but there is a
hidden preference security.enable_ssl2 to enable it.

Searching for "ssl2" in about:config, I found seven
SSL 2.0 preferences, all disabled by default:

  security.enable_ssl2;false
  security.ssl2.des_64;false
  security.ssl2.des_ede3_192;false
  security.ssl2.rc2_128;false
  security.ssl2.rc2_40;false
  security.ssl2.rc4_128;false
  security.ssl2.rc4_40;false

I propose that we remove SSL 2.0 support completely.

If it's too late to make this change in mozilla 2.0
(Firefox 4.0), I request to make this change
post-mozilla 2.0.

Comment 1

[Kai Engert [:KaiE:]]

If I remember correctly, Bob prefers to keep the SSL 2 code, but I can't find the email now...

Comment 2

[Wan-Teh Chang]

Kai: Bob objected to the removal of SSL 2.0 from NSS's SSL library.
This bug is about removing SSL 2.0 from Mozilla client applications.

Comment 3

[Kai Engert [:KaiE:]]

Wan-Teh, thanks for clarifying.

Comment 4

[Robert Relyea]

Ah! I'm OK with removal from the Mozilla client apps (or any new products our there).

bob

Comment 5

[Asa Dotzler [:asa]]

WTC, can you share Chrome data on how often it was used when you all collected metrics. Sounds like rarely enough for Chrome to go ahead with the removal but I'm curious if it was a close call or nor.

Comment 6

[Wan-Teh Chang]

I don't have good data yet because my SSL version statistics
collection code has only made it to Chrome's Dev channel.
It won't be in Chrome's Stable channel until Feb. 2011,
when Chrome 9.0 is released to Stable.  I will share the
data when I have them.

My proposal is to remove SSL 2.0 support unless someone
can provide data otherwise.  The people who still need to
use SSL 2.0 today should be burdened with gathering the
data to support that.

Comment 7

[Wan-Teh Chang]

Preliminary numbers from Chrome 9.0 Dev and Beta channel users:

Number of connections of each version of SSL/TLS.
Note: normalized to 1 billion TLS 1.0 connections.

SSL 2.0: 352
SSL 3.0: 161,317,694
TLS 1.0: 1,000,000,000

Comment 8

[Robert Relyea]

Thinking about this, I think the best way forward is to #ifdef out SSL2 by default in NSS 3.13. That way I can still turn it on in products that need backward compatibility, but new products won't have it. Mozilla could ship without it. At some point the backward compatibility support will no longer be an issue and it can be removed altogether.

bob

Comment 9

[Wan-Teh Chang]

Bob, this bug is concerned with Mozilla only.  I want to disable
SSL 2.0 in Mozilla first, before removing the SSL 2.0 code in NSS.

Note: #ifdef out SSL2 in NSS will make it even harder to modify
the SSL library in NSS, so I would rather leave SSL2 alone until
you don't need backward compatibility support.  (My motivation
is not to reduce code size.)

Comment 10

[Robert Relyea]

> Bob, this bug is concerned with Mozilla only.

I understand. I'm talking about all the cases.

> #ifdef out SSL2 in NSS will make it even harder to modify
> the SSL library in NSS.

Here's my issue. I either need to abandon any modifications NSS provides in operating systems like RHEL 4, 5, and 6, or I need to keep backporting SSL2 support to these OS's. By providing #ifdefs, we can turn SSL2 off on *future* versions of NSS, and still allow us to keep moving forward in RHEL 4, 5, and 6. At some point we can drop the #ifdefs altogether.

I'm trying to come up with a solution which allows us to remove SSL2 without hosing myself and requiring nasty forks.

I guess we could simply disable the ability to turn on SSL2 in 3.13, and accomplish the same thing I need. I wish we had done that in 3.12 before RHEL 6 went out;(.

bob

Comment 11

[Robert Relyea]

Bleh, I can read English.....

> this bug is concerned with Mozilla only.

Obviously I didn't understand..... Sorry. Removing client SSL2 support sounds fine to me...

We should also get this started for NSS in general since we have a longer to go there.

Bob "I need to read the whole comment history" relyea

Comment 12

[Wan-Teh Chang]

Bob: in NSS, we can start by changing the default of the
SSL_ENABLE_SSL2 option to false.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Let's start by simply renaming the pref used in the code so that every user that currently has it set to "on" will get reverted to the default "off", then when nobody complains, remove the code that was conditional on the pref. This will fit in with Mozilla's new process for (permanently) enabling/disabling functionality it the product

Comment 14

[Kai Engert [:KaiE:]]

Please explain in more detail how you envision this pref reversion to work.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Replace "security.enable_ssl2" with "security.insecure_enable_ssl2" throughout the source code. The default value will be false. This would have the effect that everybody that enabled SSL 2.0 would now have it disabled again. In order to re-enable it, they would have to toggle the security.insecure_enable_ssl2 pref. After we're certain we can kill SSL 2.0 support (the day after Firefox 6 ships), we would remove all the code that is conditional on the pref. In the unlikely event that we would need to undo this change for the Firefox 6 release, we would revert the patch, replacing "security.insecure_enable_ssl2" with ""security.enable_ssl2".

Comment 16

[Wan-Teh Chang]

I missed the best time to get the statistics from Chrome 9.0 users.  (SSL 2.0
was removed in Chrome 10.0.)  The  best Chrome 9.0 numbers I can get
easily today is the 28-day period ending on 2011-03-04.  There is a 0.5%
contribution to the numbers below from Chrome 10.0.648.82 (Beta) Windows
users.

Number of connections of each version of SSL/TLS.
Note: normalized to 1 billion TLS 1.0 connections.

SSL 2.0:                  1,354
SSL 3.0:     225,362,493
TLS 1.0: 1,000,000,000

Another piece of info is that the removal of SSL 2.0 from Chrome has been
uneventful in spite of the nonzero SSL 2.0 connections made by Chrome
9.0 users.

I would just remove the SSL 2.0 support from Mozilla in one shot.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #16)
> I would just remove the SSL 2.0 support from Mozilla in one shot.

Thanks for the information, Wan-Teh. It's really helpful. I agree that Wan-Teh's approach is also acceptable. If we need to revert the change, we will have to backout the patch instead of flipping the pref, but that seems highly unlikely to ever be necessary.

Wan-Teh, do you ever use the SSL 2.0 compatible hello for SSL 3.0 in Chrome?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

I think the differences between Firefox and Chrome's userbases (both in terms of size and age-of-population) makes the risk of removing support causing breakage significantly greater for us. I am not opposed to removing support in one step, but I just want to caution against giving anecdotal evidence of Chrome's experience too much value when evaluating the decision.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Gavin, I generally agree with you. AFAICT, backout of the patch is also an acceptable reversion strategy and not much different than what I proposed in comment 15. If we make this change now we will maximize nightly/Aurora/beta coverage before Firefox 6 is released. SSL 2.0 itself is a pretty insecure protocol and SSL-2.0-only server likely has many other unpatched vulnerabilities. We have been shipping with SSL 2.0 off by default for a long time and the user can't tell if they've actually enabled it (unlike in IE) unless they use about:config, because we already removed the UI for it in Firefox 3.x.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)

I concur - I have no objection to this plan.

Comment 21

[Kai Engert [:KaiE:]]

First, let me say, I'm OK to permanently remove SSL 2 support at the PSM level.

(If users really need this, they can stick with an older version of Firefox, until they fixed their environment; something they really should.)

I'd prefer to avoid a new pref for the same purpose, and avoid user confusion potentially caused by this.


However, I'm afraid, removing SSL 2 is only part of the story.

If a user has SSL 2 enabled in the prefs, this also has the effect of permanently enabling SSL 2 hellos for all SSL/TLS connection.

I think we must come up with a plan that covers both
  "SSL 2 protocol / ciphers"
and
  "SSL 2 compatible hellos".

Why?

Because SSL 2 compatible hellos are part of "smartness" that has been implemented in PSM to make SSL work more often.

Some servers are "TLS intolerant".
If we detect such a site, we attempt to fallback to use an SSL 2 compatible hello.

This fallback is unconditional.
Even if SSL 2 is completely disabled, we will still enable the old hellos in the "make it work" fallback scenario.

Are we certain it is safe to stop doing so?
I suspect we're not.
I'm afraid there may be still a significant number of sites that are TLS intolerant.


A major unknown might be:
Are there users who have enabled SSL v2, because our automatic fallback didn't work with their SSL v3 servers, but enabling SSL v2 + hellos magically made it work?

If we think that's possible, then the question is:
Even if we remove the SSL v2 pref, do we need a new pref, that allows users to unconditionally enable the use of SSL v2 hellos?

I don't know, but maybe we should add it, as a backwards compatibility safety net?


Having said that, here is a proposal:

- remove pref "security.enable_ssl2" from the defaults
- remove the code in PSM that checks for that SSL 2 pref
- add a call to PSM that will unconditionally disable SSL 2
- add a new (hidden) pref 
  "security.enable_ssl2_compatible_hello_always" (default off)
- make that pref discoverable in about:config,
  by including a default value


(I'd also like to propose, let's keep this separate from any discussions whether to keep or not to keep the current fallback on TLS intolerance. If you would like to propose to tamper with that, let's do that in a separate bug.)

Comment 22

[Kai Engert [:KaiE:]]

(Note, the reason why I propose to include "always" in pref name "security.enable_ssl2_compatible_hello_alway": At a future time, we might want to experiment with another pref, that disables the use of v2 hellos in our TLS intolerance fallback code? But as I propose at the end of the previous comment, if there is need to discuss this, we should have a separate bug.)

Comment 23

[Kai Engert [:KaiE:]]

Attachment: patch v1

I propose at least one review and at least one additional feedback+ that the plan sounds good.

Comment 24

[Wan-Teh Chang]

Kai: SSL_V2_COMPATIBLE_HELLO should also be permanently disabled.
It is useful to avoid a long timeout with certain SSL 2.0 servers.

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [patch v2] Remove SSL 2.0 and Fortezza support

Kai, I agree with Wan-Teh that we should not support the SSL 2.0 compatible hello either. Here is a patch that removes the SSL 2.0 support and also removes the old fortezza support at the same time.

Comment 26

[Wan-Teh Chang]

Comment on attachment 543520 [details] [diff] [review]
[patch v2] Remove SSL 2.0 and Fortezza support

>diff --git a/security/manager/ssl/src/nsNSSCertHelper.cpp b/security/manager/ssl/src/nsNSSCertHelper.cpp
>--- a/security/manager/ssl/src/nsNSSCertHelper.cpp
>+++ b/security/manager/ssl/src/nsNSSCertHelper.cpp
>@@ -1919,13 +1919,9 @@ ProcessSubjectPublicKeyInfo(CERTSubjectP
>                                                     params, 3, text);
>         break;
>       }
>-      case dhKey:
>       case dsaKey:
>-      case fortezzaKey:
>-      case keaKey:
>          /* Too many parameters, to rarely used to bother displaying it */
>          break;
>-      case nullKey:
>       default:
>          /* Algorithm unknown */
>          break;

This also removes dhKey.  If that's intentional, please document that in
the changeset's description.

Please fix the typo "to rarely used" => "too rarely used".

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [patch v3] Remove SSL 2.0 and Fortezza support

This is the same as the previous patch, but clarifies the change in ProcessSubjectPublicKeyInfo and fixes the typo.

Note that there is no functional change in ProcessSubjectPublicKeyInfo in either version of my path; I was just cleaning up useless references to Fortezza and KEA, and I just combined explicit cases for dhKey and dsaKey with the default case; the body of all the cases was/is just a "break;".

Comment 28

[Kai Engert [:KaiE:]]

Comment on attachment 543520 [details] [diff] [review]
[patch v2] Remove SSL 2.0 and Fortezza support

If I understand correctly, this patch is obsoleted by your newer patch. Let's mark it as such.

Comment 29

[Kai Engert [:KaiE:]]

Comment on attachment 543540 [details] [diff] [review]
[patch v3] Remove SSL 2.0 and Fortezza support

r-


>diff --git a/security/manager/ssl/src/nsCipherInfo.cpp b/security/manager/ssl/src/nsCipherInfo.cpp
>--- a/security/manager/ssl/src/nsCipherInfo.cpp
>+++ b/security/manager/ssl/src/nsCipherInfo.cpp
>@@ -106,20 +106,17 @@ NS_IMETHODIMP nsCipherInfo::GetLongName(
>   aLongName = ToNewCString(nsDependentCString(mInfo.cipherSuiteName));
>   return NS_OK;
> }
> 
> NS_IMETHODIMP nsCipherInfo::GetIsSSL2(PRBool *aIsSSL2)
> {
>   NS_ENSURE_ARG_POINTER(aIsSSL2);
> 
>-  if (!mHaveInfo)
>-    return NS_ERROR_NOT_AVAILABLE;
>-
>-  *aIsSSL2 = SSL_IS_SSL2_CIPHER(mInfo.cipherSuite);
>+  *aIsSSL2 = PR_FALSE;
>   return NS_OK;
> }


Please keep this, It's just runtime information.

If someone hacks the Mozilla code to keep SSL 2 support in their application, we should still correctly report whether a cipher is SSL 2.



>diff --git a/security/manager/ssl/src/nsNSSComponent.cpp b/security/manager/ssl/src/nsNSSComponent.cpp
>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>@@ -1809,19 +1798,16 @@ nsNSSComponent::InitializeNSS(PRBool sho
> 
>       PK11_SetPasswordFunc(PK11PasswordPrompt);
> 
>       // Register an observer so we can inform NSS when these prefs change
>       nsCOMPtr<nsIPrefBranch2> pbi = do_QueryInterface(mPrefBranch);
>       pbi->AddObserver("security.", this, PR_FALSE);
> 
>       PRBool enabled;
>-      mPrefBranch->GetBoolPref("security.enable_ssl2", &enabled);
>-      SSL_OptionSetDefault(SSL_ENABLE_SSL2, enabled);
>-      SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, enabled);


Our code must make it clear what default settings it wants.

Please add 
        SSL_OptionSetDefault(SSL_ENABLE_SSL2, PR_FALSE);
        SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, PR_FALSE);


>diff --git a/security/manager/ssl/src/nsNSSErrors.cpp b/security/manager/ssl/src/nsNSSErrors.cpp
>@@ -54,17 +54,16 @@ nsNSSErrors::getDefaultErrorStringName(P
>     case SSL_ERROR_BAD_CERT_DOMAIN: id_str = "SSL_ERROR_BAD_CERT_DOMAIN"; break;
>-    case SSL_ERROR_SSL2_DISABLED: id_str = "SSL_ERROR_SSL2_DISABLED"; break;
>     case SSL_ERROR_BAD_MAC_READ: id_str = "SSL_ERROR_BAD_MAC_READ"; break;

No, please keep this.
This is a mapping for error codes returned by NSS.
This table is actually automatically generated.

As long as this error code is contained in NSS, let's keep the table complete and keep this mapping line.



>@@ -343,20 +342,16 @@ nsNSSErrors::getOverrideErrorStringName(
> {
>   const char *id_str = nsnull;
> 
>   switch (aErrorCode) {
>     case SSL_ERROR_SSL_DISABLED:
>       id_str = "PSMERR_SSL_Disabled";
>       break;
>   
>-    case SSL_ERROR_SSL2_DISABLED:
>-      id_str = "PSMERR_SSL2_Disabled";
>-      break;


I don't see why you want this removed.
I personally would prefer to keep it.

This code overrides the default error strings provided by NSS with PSM's own, more human readable error code.
Given that NSS still contains this error code, I'd just keep that.

(If you had a good argument to remove it anyway, you would have to remove the string from pipnss.properties, too.)



>diff --git a/security/manager/ssl/src/nsNSSIOLayer.cpp b/security/manager/ssl/src/nsNSSIOLayer.cpp
>--- a/security/manager/ssl/src/nsNSSIOLayer.cpp
>+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp
>@@ -3688,23 +3686,19 @@ nsSSLIOLayerSetOptions(PRFileDesc *fd, P
>   nsNSSShutDownPreventionLock locker;
>   if (forSTARTTLS || proxyHost) {
>     if (SECSuccess != SSL_OptionSet(fd, SSL_SECURITY, PR_FALSE)) {
>       return NS_ERROR_FAILURE;
>     }
>     infoObject->SetHasCleartextPhase(PR_TRUE);
>   }
> 
>-  if (forSTARTTLS) {
>-    if (SECSuccess != SSL_OptionSet(fd, SSL_ENABLE_SSL2, PR_FALSE)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-    if (SECSuccess != SSL_OptionSet(fd, SSL_V2_COMPATIBLE_HELLO, PR_FALSE)) {
>-      return NS_ERROR_FAILURE;
>-    }

Ok to remove


>+  if (SECSuccess != SSL_OptionSet(fd, SSL_ENABLE_SSL2, PR_FALSE) || 
>+      SECSuccess != SSL_OptionSet(fd, SSL_V2_COMPATIBLE_HELLO, PR_FALSE)) {
>+    return NS_ERROR_FAILURE;

Not necessary, if we set these as the default globally, as I have proposed (see above).



>@@ -3714,20 +3708,16 @@ nsSSLIOLayerSetOptions(PRFileDesc *fd, P
>-    
>-    if (!forSTARTTLS &&
>-        SECSuccess != SSL_OptionSet(fd, SSL_V2_COMPATIBLE_HELLO, PR_TRUE))
>-      return NS_ERROR_FAILURE;


Ok to remove, under your assumption that v2-compatible-hellos are no longer necessary for backwards compatibility.


Earlier in this bug, I had argued to keep the ability to enable v2-hellos by using a new pref.
When I recommneded to do so, I was under the impression that only two versions of hello messages exist - v2-hellos and tls-hellos.

If SSL v3 has its own version of hello messages, then I agree that v2-hellos should be considered obsolete.

If SSL v3 has its own version of hello messages,
and if any calls to libssl are required to enable the use of such older v3-hellos,
then such a call must be added here.

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #29)
> >       PRBool enabled;
> >-      mPrefBranch->GetBoolPref("security.enable_ssl2", &enabled);
> >-      SSL_OptionSetDefault(SSL_ENABLE_SSL2, enabled);
> >-      SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, enabled);
> 
> 
> Our code must make it clear what default settings it wants.
> 
> Please add 
>         SSL_OptionSetDefault(SSL_ENABLE_SSL2, PR_FALSE);
>         SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, PR_FALSE);

Kai, this is already done with SSL_OptionSet. I think we should avoid SSL_OptionSetDefault so that, when libssl gets linked into libxul, we don't have to link SSL_OptionSetDefault in. And, also, I think it would be better to have the options set in one place, instead of two places, as much as possible. In particular, I would like the security-sensitive options to be insensitive to the setting of SSL_OptionSetDefault as much as possible, so that we don't have to worry about extensions calling SSL_OptionSetDefault to configure *their* internal SSL connections, unintentionally messing up the security of our internal connections.

> I don't see why you want this removed.
> I personally would prefer to keep it.

If we don't remove this cruft now, then it will never get removed. And, I think it's better if we can help (future) localizers out by removing unused messages when possible.

I removed all the error SSL2-related error message logic/data because it is all dead code. In the extremely unlikely event that somebody would hack SSL2 support back into Gecko, that is not something that we support, and they get what they deserve--actually everything would continue to work fine, except the error messages would be a little bit less descriptive and/or not localized.

> (If you had a good argument to remove it anyway, you would have to remove
> the string from pipnss.properties, too.)

I would rather remove all the SSL2-related logic and data, including from pipnss.properties, for the reasons I described above. Note that this error cannot occur when SSL 2.0 is disabled.

> If SSL v3 has its own version of hello messages, then I agree that v2-hellos
> should be considered obsolete.

SSLv3 has its own version of the hello messages, and they are used automatically whenever TLS is disabled.

Comment 31

[Kai Engert [:KaiE:]]

(In reply to comment #30)
> (In reply to comment #29)
> > >       PRBool enabled;
> > >-      mPrefBranch->GetBoolPref("security.enable_ssl2", &enabled);
> > >-      SSL_OptionSetDefault(SSL_ENABLE_SSL2, enabled);
> > >-      SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, enabled);
> > 
> > 
> > Our code must make it clear what default settings it wants.
> > 
> > Please add 
> >         SSL_OptionSetDefault(SSL_ENABLE_SSL2, PR_FALSE);
> >         SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, PR_FALSE);
> 
> Kai, this is already done with SSL_OptionSet.


I can't find that in PSM, where exactly does it happen?


> I think we should avoid
> SSL_OptionSetDefault so that, when libssl gets linked into libxul, we don't
> have to link SSL_OptionSetDefault in.


This doesn't make sense to me.

Immediately below the lines you are removing, there are 6 additional calls to SSL_OptionSetDefault.

(We are talking about nsNSSComponent::InitializeNSS).



> And, also, I think it would be better
> to have the options set in one place, instead of two places, as much as
> possible.


I have not asked for two places. Maybe you are misunderstanding.

You are removing this code in nsNSSComponent::InitializeNSS,
and my proposal is for the same location.

This is where we set all the other defaults, like SSL_ENABLE_SSL3, SSL_ENABLE_TLS, SSL_REQUIRE_SAFE_NEGOTIATION, etc., and seems to be the natural place to disable SSL v2 by default.


> In particular, I would like the security-sensitive options to be
> insensitive to the setting of SSL_OptionSetDefault as much as possible,

Sorry, that's how we do it. I assume you agree that SSL_REQUIRE_SAFE_NEGOTIATION is a security-sensitive option, and we only use SSL_OptionSetDefault to configure it.

Let's configure all global SSL options defaults globally, during PSM init time, at the very place where you are removing this code.


> so
> that we don't have to worry about extensions calling SSL_OptionSetDefault 


An extension would be very much badly behaving if it changed an application default. I'm borrowing your argument "we don't support that", and you get unexpected behaviour if a bad extension does that.

Extensions are supposed to configure their connections individually, and not mess with application-wide defaults.

If you want to eventually (later) improve that, and force our default settings for every socket that the core code opens, by adding invidiual SSL_OptionSet calls, let's do that consistently in a separate bug.


> to
> configure *their* internal SSL connections, unintentionally messing up the
> security of our internal connections.

I would expect, if an addon decides to do C level code and access NSS directly by C, thereby circumventing the recommended XPCOM/IDL layer, they should be very aware of what they're doing. I wouldn't allow them to defense themselves with "oops, I unintenioally understood what I was doing".

If they mess with NSS directly, they are supposed to be careful and not mess our internals. We don't support addons that mess with out internals.


> > If SSL v3 has its own version of hello messages, then I agree that v2-hellos
> > should be considered obsolete.
> 
> SSLv3 has its own version of the hello messages, and they are used
> automatically whenever TLS is disabled.

Ok, I found three hosts that don't support TLS, but only support SSL 3. I used a patch that doesn't fallback to v2-hellos on TLS intolerant sites. I was able to confirm that those sites do correctly fall back from TLS to SSL 3, even with v2-hellos disabled. This gives me confidence to agree to unconditionally disable v2-hellos.

BTW, the sites are 
https://www.banner.iup.edu/pls/production/twbkwbis.P_GenMenu?name=homepage
https://www2.delphion.com/
https://www.afer.asso.fr

Comment 32

[Kai Engert [:KaiE:]]

(In reply to comment #30)
> > I don't see why you want this removed.
> > I personally would prefer to keep it.
> 
> If we don't remove this cruft now, then it will never get removed. 

You can make a dependency on something that removes SSL 2 from NSS.

As long as NSS can produce error code SSL_ERROR_SSL2_DISABLED,
we must have code that handles it and produces an error string.


> And, I
> think it's better if we can help (future) localizers out by removing unused
> messages when possible.

I don't buy your argument, because the error string is general purpose.
It currently says:
  "Can't connect securely because the site uses an older, 
   insecure version of the SSL protocol."

This sounds like a great string to reuse in the future, when we e.g. disable support for SSL 3.


> I removed all the error SSL2-related error message logic/data because it is
> all dead code.

I'm not asking for any logic. We keep the function GetIsSSL2, and I ask that it remains correct. Doing so doesn't require any burden.

I won't agree to change correct code into incorrect code.

Comment 33

[Kai Engert [:KaiE:]]

I don't understand why we are argueing so hard.

I have agreed to most of your changes, I agree with removing support for SSL 2 and Fortezza.

My requests are for conservative purposes and for code correctness, and don't change your intentions.

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: v4: Remove SSL 2.0 and Fortezza

(In reply to comment #29)
> > NS_IMETHODIMP nsCipherInfo::GetIsSSL2(PRBool *aIsSSL2)
> > {
> >   NS_ENSURE_ARG_POINTER(aIsSSL2);
> > 
> >-  if (!mHaveInfo)
> >-    return NS_ERROR_NOT_AVAILABLE;
> >-
> >-  *aIsSSL2 = SSL_IS_SSL2_CIPHER(mInfo.cipherSuite);
> >+  *aIsSSL2 = PR_FALSE;
> >   return NS_OK;
> > }
> 
> Please keep this, It's just runtime information.

I kept the change, adding a comment explaining that it is impossible to get an instance to an nsCipherInfo for an SSL 2 cipher suite because I removed the SSL 2 cipher suites from the table which is used to look up the info.

Other comments were addressed as you wished.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 543540 [details] [diff] [review]
[patch v3] Remove SSL 2.0 and Fortezza support

Review of attachment 543540 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 36

[Kai Engert [:KaiE:]]

Comment on attachment 547176 [details] [diff] [review]
v4: Remove SSL 2.0 and Fortezza

(In reply to comment #34)
> Created attachment 547176 [details] [diff] [review] [review]
> v4: Remove SSL 2.0 and Fortezza
> 
> (In reply to comment #29)
> > > NS_IMETHODIMP nsCipherInfo::GetIsSSL2(PRBool *aIsSSL2)
> > > {
> > >   NS_ENSURE_ARG_POINTER(aIsSSL2);
> > > 
> > >-  if (!mHaveInfo)
> > >-    return NS_ERROR_NOT_AVAILABLE;
> > >-
> > >-  *aIsSSL2 = SSL_IS_SSL2_CIPHER(mInfo.cipherSuite);
> > >+  *aIsSSL2 = PR_FALSE;
> > >   return NS_OK;
> > > }
> > 
> > Please keep this, It's just runtime information.
> 
> I kept the change, adding a comment explaining that it is impossible to get
> an instance to an nsCipherInfo for an SSL 2 cipher suite because I removed
> the SSL 2 cipher suites from the table which is used to look up the info.

The comment doesn't matter. You are changing a small independent class from being correct to incorrect. That's bad style. From the perspecive of nsCipherInfo it doesn't matter if the outside world will no longer construct SSL 2 object. As long as the class declares an interface, it must work correctly.

As I said already, I won't agree to change code from correct to incorrect.

Either you keep the function correct as is, or you remove it completely.

You might be able to change interface nsICipherInfo and remove the IsSSL2 attribute. Remember to change the UUID if you do.

Comment 37

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: v5: Remove SSL 2.0 and Fortezza support, without changes to nsICipherInfo

I do not need to change nsICipherInfo. We can actually remove the entire interface and I will file another bug to do that.

Comment 38

[Kai Engert [:KaiE:]]

Comment on attachment 551602 [details] [diff] [review]
v5: Remove SSL 2.0 and Fortezza support, without changes to nsICipherInfo

r=kaie but please address one nit prior to checkin:

>       PRBool enabled;
>-      mPrefBranch->GetBoolPref("security.enable_ssl2", &enabled);
>-      SSL_OptionSetDefault(SSL_ENABLE_SSL2, enabled);
>-      SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, enabled);
>       mPrefBranch->GetBoolPref("security.enable_ssl3", &enabled);
>+      SSL_OptionSetDefault(SSL_ENABLE_SSL2, PR_FALSE);
>+      SSL_OptionSetDefault(SSL_V2_COMPATIBLE_HELLO, PR_FALSE);
>       SSL_OptionSetDefault(SSL_ENABLE_SSL3, enabled);
>       mPrefBranch->GetBoolPref("security.enable_tls", &enabled);
>       SSL_OptionSetDefault(SSL_ENABLE_TLS, enabled);

The lines that deal with the SSL3 bool should be kept next to each other.

Please move the existing line that sets SSL_ENABLE_SSL3 above the new lines.
Thanks.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/21fe1276adce

Comment 40

[Ed Morley [:emorley]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #39)
> http://hg.mozilla.org/mozilla-central/rev/21fe1276adce

Nit from comment 38 present in landed version.

Comment 41

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #38)
> The lines that deal with the SSL3 bool should be kept next to each other.
> 
> Please move the existing line that sets SSL_ENABLE_SSL3 above the new lines.
> Thanks.

Instead of moving the lines that set SSL_ENABLE_SSL3 above the new lines, I moved the new lines so they were above the other things. That way, things follow a logical order: SSL2 -> SSL3 -> TLS.

Comment 42

[Robert Relyea]

Comment on attachment 531584 [details] [diff] [review]
patch v1

kai has checked in V5, clearing feedback request.

For the records I support this change (even if comments to the contrary can be found in the body of this big:).

bob

Comment 43

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/Introduction_to_SSL

This change is also mentioned on Firefox 8 for developers and in:

https://developer.mozilla.org/en/Firefox/Updating_add-ons_for_Firefox_8