Bug 1240529 - Remove RC2 cipher suites

Bug 1240529 - Remove RC2 cipher suites

[mt, 2016-01-18 20:49:30]

LGTM

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#oldcode461
lib/ssl/ssl3con.c:461: };
It might pay to add PR_STATIC_ASSERT(PR_ARRAY_SIZE(cipher_suite_defs) ==
ssl_V3_SUITES_IMPLEMENTED) here.

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#newcode476
lib/ssl/ssl3con.c:476: { calg_rc2      , CKM_RC2_CBC			}, /* deprecated */
You can probably safely delete this line, and the calg_rc2 enum

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h#newcode73
lib/ssl/sslproto.h:73: #define SSL_CK_RC2_128_CBC_EXPORT40_WITH_MD5    0x04 /*
deprecated */
The guidance I've gotten is that these very old #defines and enumeration values
can be removed, even though they are in a public header.

[ekr-rietveld, 2016-01-19 01:35:26]

I see a reference to RC2 in
ssl3_CipherSuiteAllowedForVersionRange that I think you want to remove.

https://codereview.appspot.com/289790043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:115: -1, /* SSL_FORTEZZA_DMS_WITH_NULL_SHA,	 * h */
Suggest cleaning up these tabs while you're in these lines.

https://codereview.appspot.com/289790043/diff/1/cmd/ssltap/ssltap.c
File cmd/ssltap/ssltap.c (left):

https://codereview.appspot.com/289790043/diff/1/cmd/ssltap/ssltap.c#oldcode350
cmd/ssltap/ssltap.c:350: case 0x000061:    cs_str =
"TLS/RSA-EXPORT1024/RC2CBC56/MD5";	break;
You shouldn't remove these, because people may still want to inspect connections
which do RC2.

https://codereview.appspot.com/289790043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:53: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,       *
D */
Fix alignment here and below

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c#newcode63
cmd/tstclnt/tstclnt.c:63: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,	* D */
Fix alignment and remove tabs

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c#newcode76
cmd/tstclnt/tstclnt.c:76: -1, /* TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5,		* g */
Alignment/tabs

https://codereview.appspot.com/289790043/diff/1/cmd/vfyserv/vfyutil.c
File cmd/vfyserv/vfyutil.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/vfyserv/vfyutil.c#newcode21
cmd/vfyserv/vfyutil.c:21: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5, * D */
Alignment/tabs here and below.

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#newcode476
lib/ssl/ssl3con.c:476: { calg_rc2      , CKM_RC2_CBC			}, /* deprecated */
On 2016/01/18 20:49:30, mt wrote:
> You can probably safely delete this line, and the calg_rc2 enum

If you opt to do that, also perhaps remove skipjack on line 480

[ttaubert, 2016-01-19 11:34:49]

https://codereview.appspot.com/289790043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:115: -1, /* SSL_FORTEZZA_DMS_WITH_NULL_SHA,	 * h */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> Suggest cleaning up these tabs while you're in these lines.

Done.

https://codereview.appspot.com/289790043/diff/1/cmd/ssltap/ssltap.c
File cmd/ssltap/ssltap.c (left):

https://codereview.appspot.com/289790043/diff/1/cmd/ssltap/ssltap.c#oldcode350
cmd/ssltap/ssltap.c:350: case 0x000061:    cs_str =
"TLS/RSA-EXPORT1024/RC2CBC56/MD5";	break;
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> You shouldn't remove these, because people may still want to inspect
connections
> which do RC2.

Reverted changes to this file.

https://codereview.appspot.com/289790043/diff/1/cmd/strsclnt/strsclnt.c
File cmd/strsclnt/strsclnt.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/strsclnt/strsclnt.c#newco...
cmd/strsclnt/strsclnt.c:53: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,       *
D */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> Fix alignment here and below

Done.

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c#newcode63
cmd/tstclnt/tstclnt.c:63: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,	* D */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> Fix alignment and remove tabs

Done.

https://codereview.appspot.com/289790043/diff/1/cmd/tstclnt/tstclnt.c#newcode76
cmd/tstclnt/tstclnt.c:76: -1, /* TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5,		* g */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> Alignment/tabs

Done.

https://codereview.appspot.com/289790043/diff/1/cmd/vfyserv/vfyutil.c
File cmd/vfyserv/vfyutil.c (right):

https://codereview.appspot.com/289790043/diff/1/cmd/vfyserv/vfyutil.c#newcode21
cmd/vfyserv/vfyutil.c:21: -1, /* SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5, * D */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> Alignment/tabs here and below.

Done.

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#oldcode461
lib/ssl/ssl3con.c:461: };
On 2016/01/18 20:49:30, mt wrote:
> It might pay to add PR_STATIC_ASSERT(PR_ARRAY_SIZE(cipher_suite_defs) ==
> ssl_V3_SUITES_IMPLEMENTED) here.

cipher_suite_defs[] has 72 entries right now, whereas
ssl_V3_SUITES_IMPLEMENTED=63. Should we do that in a follow-up and remove
surplus entries from cipher_suite_defs[]?

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#newcode476
lib/ssl/ssl3con.c:476: { calg_rc2      , CKM_RC2_CBC			}, /* deprecated */
On 2016/01/19 01:35:26, ekr-webrtc wrote:
> On 2016/01/18 20:49:30, mt wrote:
> > You can probably safely delete this line, and the calg_rc2 enum
> 
> If you opt to do that, also perhaps remove skipjack on line 480

Removed both lines, had to touch SSLCipherAlgorithm in sslt.h because alg2Mech[]
is indexed by those values. Is it okay to change all those values or should we
rather insert null entries into alg2Mech[]?

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h#newcode73
lib/ssl/sslproto.h:73: #define SSL_CK_RC2_128_CBC_EXPORT40_WITH_MD5    0x04 /*
deprecated */
On 2016/01/18 20:49:30, mt wrote:
> The guidance I've gotten is that these very old #defines and enumeration
values
> can be removed, even though they are in a public header.

Removed them and the IDEA_128_CBC below. Cleaned up ssl_Specs[],
ssl_cipherName[], and the SSL_CK_* and SSL_EN_* values in sslproto.h.

Wasn't sure if it's okay to change all those values but I just went for it. Let
me know if I have to revert that and add placeholders.

[wtc1, 2016-01-19 17:50:07]

Review comments on patch set 2:

I only reviewed the changes in lib/ssl.

1. The changes to the public headers sslproto.h and sslt.h
are incorrect. Please take the opportunity to review the
relevant item in the NSS Developer Tutorial:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Developer_T...

and help us improve it if it is not clear enough. Thanks.

2. I suggest we not remove the SSLv2 RC2 cipher suites and
just let them be removed along with the rest of the SSLv2
code.

3. Since the only SSLv3 RC2 cipher suite is an export
cipher suite, it also seems fine to just wait until we
remove all export cipher suites. Tim, I guess you see an
advantage in removing the SSLv3 RC2 cipher suite sooner?
For example, I see that it allows us to remove the
ssl3_ParamFromIV function and the effKeyBits local variable
in ssl3con.c.

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslcon.c
File lib/ssl/sslcon.c (right):

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslcon.c#newcode25
lib/ssl/sslcon.c:25: #define ssl2_NUM_SUITES_IMPLEMENTED 4

IMPORTANT: I suggest we not touch the SSLv2 code because it will be completely
removed soon.

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (left):

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslimpl.h#oldcode464
lib/ssl/sslimpl.h:464: cipher_rc2_40,

sslimpl.h is a private header, so it is fine to remove these enumerators.

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslproto.h#newcode73
lib/ssl/sslproto.h:73: #define SSL_CK_DES_192_EDE3_CBC_WITH_MD5        0x04

BUG: sslproto.h is a public header, so in general we should not remove macros
defined in this header, unless we are sure the macros aren't being used.

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/289790043/diff/20001/lib/ssl/sslt.h#newcode105
lib/ssl/sslt.h:105: ssl_calg_aes_gcm  = 8

BUG: for backward compatibility, we cannot change the values of the enumerators
in a public header file. (This header is a public header.)

In fact, this is why we explicitly assign values to enumerators in public
headers, even if the values are the same as what the compiler would assign.
These explicit values are intended to suggest the values matter.

In this case, we can leave ssl_calg_rc2 there and just add a "deprecated, now
unused" comment, like what we did with ssl_calg_fortezza.

[wtc1, 2016-01-19 17:53:48]

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h
File lib/ssl/sslproto.h (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h#newcode73
lib/ssl/sslproto.h:73: #define SSL_CK_RC2_128_CBC_EXPORT40_WITH_MD5    0x04 /*
deprecated */
On 2016/01/18 20:49:30, mt wrote:
> The guidance I've gotten is that these very old #defines and enumeration
values
> can be removed, even though they are in a public header.

Martin, since you approved these changes, especially the changes to
sslt.h, please also review and improve the NSS Developer Tutorial's
description of this issue:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Developer_T...

Thanks!

[mt, 2016-01-19 23:03:06]

On 2016/01/19 17:53:48, wtc1 wrote:
> https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h
> File lib/ssl/sslproto.h (right):
> 
> https://codereview.appspot.com/289790043/diff/1/lib/ssl/sslproto.h#newcode73
> lib/ssl/sslproto.h:73: #define SSL_CK_RC2_128_CBC_EXPORT40_WITH_MD5    0x04 /*
> deprecated */
> On 2016/01/18 20:49:30, mt wrote:
> > The guidance I've gotten is that these very old #defines and enumeration
> values
> > can be removed, even though they are in a public header.
> 
> Martin, since you approved these changes, especially the changes to
> sslt.h, please also review and improve the NSS Developer Tutorial's
> description of this issue:
> 
>
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Developer_T...
> 
> Thanks!

Thanks for the reminder Wan-Teh.

[mt, 2016-01-19 23:03:41]

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#oldcode461
lib/ssl/ssl3con.c:461: };
On 2016/01/19 11:34:49, ttaubert wrote:
> On 2016/01/18 20:49:30, mt wrote:
> > It might pay to add PR_STATIC_ASSERT(PR_ARRAY_SIZE(cipher_suite_defs) ==
> > ssl_V3_SUITES_IMPLEMENTED) here.
> 
> cipher_suite_defs[] has 72 entries right now, whereas
> ssl_V3_SUITES_IMPLEMENTED=63. Should we do that in a follow-up and remove
> surplus entries from cipher_suite_defs[]?

Yeah, a follow-up would be ideal.  I don't see any reason to include definitions
for stuff we don't implement.

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#newcode476
lib/ssl/ssl3con.c:476: { calg_rc2      , CKM_RC2_CBC			}, /* deprecated */
On 2016/01/19 11:34:49, ttaubert wrote:
> On 2016/01/19 01:35:26, ekr-webrtc wrote:
> > On 2016/01/18 20:49:30, mt wrote:
> > > You can probably safely delete this line, and the calg_rc2 enum
> > 
> > If you opt to do that, also perhaps remove skipjack on line 480
> 
> Removed both lines, had to touch SSLCipherAlgorithm in sslt.h because
alg2Mech[]
> is indexed by those values. Is it okay to change all those values or should we
> rather insert null entries into alg2Mech[]?

Hmm, I think that we might need to keep the dead values here in that case. 
There might be code that has calg_aes_gcm value and we don't want to have that
get out of sync.  I think that you can delete the calg_rc2 value from sslt.h,
but you need to keep the values the same.

https://codereview.appspot.com/289790043/diff/1/lib/ssl/ssl3con.c#newcode5578
lib/ssl/ssl3con.c:5578: CKM_SKIPJACK_CBC64,
I don't know how far you want to go here, but this entire list is bad.  We can
probably reduce this to AES.  Can you file a follow-up?