938369 - Match cipher names between NSS and IANA

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Vehent]

The following ciphers, taken from http://mxr.mozilla.org/nss/source/lib/ssl/sslproto.h , have a name that is different from the official IANA name from http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml

It would be useful to provide a way to use IANA names in NSS. 

      NSS NAME                             IANA NAME
--------------------------------------+-------------------------------------------
SSL_NULL_WITH_NULL_NULL                TLS_NULL_WITH_NULL_NULL
SSL_RSA_WITH_NULL_MD5                  TLS_RSA_WITH_NULL_MD5
SSL_RSA_WITH_NULL_SHA                  TLS_RSA_WITH_NULL_SHA
SSL_RSA_EXPORT_WITH_RC4_40_MD5         TLS_RSA_EXPORT_WITH_RC4_40_MD5
SSL_RSA_WITH_RC4_128_MD5               TLS_RSA_WITH_RC4_128_MD5
SSL_RSA_WITH_RC4_128_SHA               TLS_RSA_WITH_RC4_128_SHA
SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5     TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5
SSL_RSA_WITH_IDEA_CBC_SHA              TLS_RSA_WITH_IDEA_CBC_SHA
SSL_RSA_EXPORT_WITH_DES40_CBC_SHA      TLS_RSA_EXPORT_WITH_DES40_CBC_SHA
SSL_RSA_WITH_DES_CBC_SHA               TLS_RSA_WITH_DES_CBC_SHA
SSL_DH_RSA_WITH_3DES_EDE_CBC_SHA       TLS_DH_RSA_WITH_3DES_EDE_CBC_SHA
SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA  TLS_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA
SSL_DHE_DSS_WITH_DES_CBC_SHA           TLS_DHE_DSS_WITH_DES_CBC_SHA
SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA      TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA
SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA  TLS_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA
SSL_DHE_RSA_WITH_DES_CBC_SHA           TLS_DHE_RSA_WITH_DES_CBC_SHA
SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA      TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
SSL_DH_ANON_EXPORT_WITH_RC4_40_MD5     TLS_DH_anon_EXPORT_WITH_RC4_40_MD5
SSL_DH_ANON_WITH_RC4_128_MD5           TLS_DH_anon_WITH_RC4_128_MD5
SSL_DH_ANON_EXPORT_WITH_DES40_CBC_SHA  TLS_DH_anon_EXPORT_WITH_DES40_CBC_SHA
TLS_DH_ANON_WITH_AES_128_CBC_SHA       TLS_DH_anon_WITH_AES_128_CBC_SHA
TLS_DH_ANON_WITH_AES_256_CBC_SHA       TLS_DH_anon_WITH_AES_256_CBC_SHA
TLS_DH_ANON_WITH_CAMELLIA_128_CBC_SHA  TLS_DH_anon_WITH_CAMELLIA_128_CBC_SHA
TLS_DH_ANON_WITH_CAMELLIA_256_CBC_SHA  TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA

Comment 1

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

Attachment: rename-cipher-suites.patch

This patch is important for Firefox because Firefox displays the cipher suite name string given by NSS in its UI (Lock Icon -> More Information -> Security tab -> bottom). NSS uses the stringified version of the macro name (#x) for the cipher suite for that string, e.g. "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA"; this should instead be "TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA".

Besides that change, the secondary purpose of this patch is to make it easier to map the IANA names to the names used in NSS (by making them exactly the same), for documentation purposes.

I split this into two patches to make review easier.

This patch:

1. Renames all cipher suites to their IANA names
2. #defines <oldname> <newname>
3. Adds a way to disable the old, deprecated names during compilation, for both users of NSS and for NSS internally.
4. Changes the build system of NSS to disable the old names.

I will attach a second patch that changes all the occurrences of the old names under cmd/*.

Comment 2

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

Attachment: rename-cipher-suites-cmd.patch

This patch changes all the occurrences of the old names to occurrences of the new names, except in tests/pkcs11/netscape/suites/security/ssl/sslt.rep, because I don't understand what that file is about or how it is used.

This recursive grep may make it esaier for you to review this patch, because it makes it clear that I've not overlooked any occurrences of the old names.

egrep -r "SSL_NULL_WITH_NULL_NULL|SSL_RSA_WITH_NULL_MD5|SSL_RSA_WITH_NULL_SHA|SSL_RSA_EXPORT_WITH_RC4_40_MD5|SSL_RSA_WITH_RC4_128_MD5|SSL_RSA_WITH_RC4_128_SHA|SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5|SSL_RSA_WITH_IDEA_CBC_SHA|SSL_RSA_EXPORT_WITH_DES40_CBC_SHA|SSL_RSA_WITH_DES_CBC_SHA|SSL_DH_RSA_WITH_3DES_EDE_CBC_SHA|SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA|SSL_DHE_DSS_WITH_DES_CBC_SHA|SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA|SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA|SSL_DHE_RSA_WITH_DES_CBC_SHA|SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA|SSL_DH_ANON_WITH_RC4_128_MD5|SSL_DH_ANON_EXPORT_WITH_DES40_CBC_SHA|SSL_DH_ANON_EXPORT_WITH_RC4_40_MD5|TLS_DH_ANON_WITH_AES_128_CBC_SHA|TLS_DH_ANON_WITH_AES_256_CBC_SHA|TLS_DH_ANON_WITH_CAMELLIA_128_CBC_SHA|TLS_DH_ANON_WITH_CAMELLIA_256_CBC_SHA" .

Comment 3

[:Cykesiopka]

Comment on attachment 8344147 [details] [diff] [review]
rename-cipher-suites.patch

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

::: lib/ssl/sslproto.h
@@ +80,5 @@
>  #define SSL_EN_IDEA_128_CBC_WITH_MD5		0xFF05
>  #define SSL_EN_DES_64_CBC_WITH_MD5		0xFF06
>  #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5	0xFF07
>  
> +/* Deprecated libssl names replaced by original TLS IANA-registerd names */

s/registerd/registered/ ?

Comment 4

[Wan-Teh Chang]

Comment on attachment 8344147 [details] [diff] [review]
rename-cipher-suites.patch

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

::: lib/ssl/sslproto.h
@@ +80,5 @@
>  #define SSL_EN_IDEA_128_CBC_WITH_MD5		0xFF05
>  #define SSL_EN_DES_64_CBC_WITH_MD5		0xFF06
>  #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5	0xFF07
>  
> +/* Deprecated libssl names replaced by original TLS IANA-registerd names */

The SSL_xxx names are not "libssl names". These cipher suites were first
defined in SSL 3.0 (RFC 6101), so the SSL_xxx names are actually the SSL
3.0 names of these cipher suites.

The xxx_ANON_xxx names are libssl names.

@@ +112,1 @@
>  /* SSL v3 Cipher Suites */

This comment, and the "New TLS cipher suites" comment on line 150,
should be updated.

Comment 5

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

Comment on attachment 8344147 [details] [diff] [review]
rename-cipher-suites.patch

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

::: lib/ssl/sslproto.h
@@ +80,5 @@
>  #define SSL_EN_IDEA_128_CBC_WITH_MD5		0xFF05
>  #define SSL_EN_DES_64_CBC_WITH_MD5		0xFF06
>  #define SSL_EN_DES_192_EDE3_CBC_WITH_MD5	0xFF07
>  
> +/* Deprecated libssl names replaced by original TLS IANA-registerd names */

Thanks. I updated the comment to say this:

/* Deprecated SSL 3.0 & libssl names replaced by IANA-registered TLS names. */

@@ +112,1 @@
>  /* SSL v3 Cipher Suites */

Thanks. I removed this comment and the "New TLS cipher suites" comment.

@@ +125,1 @@
>  						       

I removed this unnecessary whitespace (3x).

Comment 6

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

Attachment: rename-cipher-suites.patch [v2, addresses previously-received feedback]

Comment 7

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

Attachment: rename-cipher-suites-cmd.patch [v2, fixes whitespace issue]

Comment 8

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

Comment on attachment 8383439 [details] [diff] [review]
rename-cipher-suites.patch [v2, addresses previously-received feedback]

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

Elio, Bob suggested it would be good to ask you to review these changes. Let me know if you have any questions.

Comment 9

[Elio Maldonado]

Comment on attachment 8383439 [details] [diff] [review]
rename-cipher-suites.patch [v2, addresses previously-received feedback]

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

r+, see small nit about whitespace

::: lib/ssl/derive.c
@@ +629,4 @@
>      srvPubkey = CERT_ExtractPublicKey(cert);
>      if (!srvPubkey)
>          return SECFailure;
>  	

NIT: remove whitespace here anad two lines down

Comment 10

[Elio Maldonado]

Comment on attachment 8383440 [details] [diff] [review]
rename-cipher-suites-cmd.patch [v2, fixes whitespace issue]

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

::: cmd/ssltap/ssltap.c
@@ +504,5 @@
>           (cs_int == TLS_ECDH_ECDSA_WITH_NULL_SHA) ||
>           (cs_int == TLS_ECDHE_ECDSA_WITH_NULL_SHA) ||
>           (cs_int == TLS_ECDH_RSA_WITH_NULL_SHA) ||
>           (cs_int == TLS_ECDHE_RSA_WITH_NULL_SHA));
>  } 

whitespace

::: cmd/strsclnt/strsclnt.c
@@ +89,5 @@
>  #define NO_FULLHS_PERCENTAGE -1
>  
>  /* This global string is so that client main can see 
>   * which ciphers to use. 
>   */

whitespace

Comment 11

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

Attachment: add-missing-cipher-suite-aliases.patch

Elio, thanks for the reviews. It turns out that I was missing some aliases in my original patch, which I caught before I checked it in. Please review this patch retroactively.

I did not make the whitespace changes you suggested since my patches did not touch those lines. We should consider fixing the trailing whitespace issue throughout NSS, in a separate bug.

Comment 12

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

I folded all three patches together and checked them in:
http://hg.mozilla.org/projects/nss/rev/ab3a00d03c39

Comment 13

[Elio Maldonado]

Comment on attachment 8388289 [details] [diff] [review]
add-missing-cipher-suite-aliases.patch

r+, I didn't catch those in previous review.