See the description in the comment of the patch for the logic used to do the ordering. This patch adds a debug-build-only runtime check to verify that SSL_ImplementedCiphers (in sslenum.c) and cipherSuites (in ssl3con.c) are in consistent order. This should make the patch review much easier. I recommend reviewing the patch by applying it and reviewing the resultant order, instead of trying to read the diff. At Wan-Teh's request, I put the (EC)DHE_ECDSA cipher suites in front of the (EC)DHE_RSA cipher suites. Without this patch, Firefox uses this ordering, which borders on being completely non-sensical: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA TLS_DHE_RSA_WITH_AES_256_CBC_SHA TLS_DHE_DSS_WITH_AES_256_CBC_SHA TLS_RSA_WITH_CAMELLIA_256_CBC_SHA TLS_RSA_WITH_AES_256_CBC_SHA TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA TLS_ECDHE_ECDSA_WITH_RC4_128_SHA TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA TLS_ECDHE_RSA_WITH_RC4_128_SHA TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA TLS_DHE_RSA_WITH_AES_128_CBC_SHA TLS_DHE_DSS_WITH_AES_128_CBC_SHA TLS_RSA_WITH_CAMELLIA_128_CBC_SHA TLS_RSA_WITH_AES_128_CBC_SHA TLS_RSA_WITH_RC4_128_SHA TLS_RSA_WITH_RC4_128_MD5 TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA TLS_RSA_WITH_3DES_EDE_CBC_SHA With this patch, Firefox uses this order: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA TLS_ECDHE_ECDSA_WITH_RC4_128_SHA TLS_ECDHE_RSA_WITH_RC4_128_SHA TLS_DHE_RSA_WITH_AES_128_CBC_SHA TLS_DHE_DSS_WITH_AES_128_CBC_SHA TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA TLS_DHE_RSA_WITH_AES_256_CBC_SHA TLS_DHE_DSS_WITH_AES_256_CBC_SHA TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA TLS_RSA_WITH_AES_128_CBC_SHA TLS_RSA_WITH_CAMELLIA_128_CBC_SHA TLS_RSA_WITH_AES_256_CBC_SHA TLS_RSA_WITH_CAMELLIA_256_CBC_SHA TLS_RSA_WITH_3DES_EDE_CBC_SHA TLS_RSA_WITH_RC4_128_SHA TLS_RSA_WITH_RC4_128_MD5 This patch makes the assumptions that (a) AES is going to be faster than Camellia consistently and in general AES is preferable to Camellia for any application that would enable both, (b) RSA key exchange is worse, overall, than RC4 or 3DES are, (c) anybody for whom AES-128/Camellia-128 isn't good enough can disable 128-bit cipher suites to force the 256-bit variants to be chosen, but otherwise AES-128 is secure enough and better overall because it is faster.

Comment on attachment 829784 [details] [diff] [review] change-cipher-order.patch Review of attachment 829784 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Your proposed cipher suite ordering looks reasonable. The only change I would suggest is to list SEED with the other 128-bit symmetric encryption algorithms, rather than after 3DES. I am also tempted to remove the two nonstandard FIPS cipher suites, but we probably should do that in a separate patch. ::: lib/ssl/ssl3con.c @@ +146,5 @@ > { SSL_DHE_DSS_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > { SSL_RSA_FIPS_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > { SSL_RSA_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > + > + /* export ciphersuites with 1024-bit public key exchange keys */ You may want to note these RSA_EXPORT1024 cipher suites are specified in draft-ietf-tls-56-bit-ciphersuites-01. (They are not in the TLS 1.0 RFC.) Another difference is that these two cipher suites use 56-bit symmetric encryption keys. In lines 150 and 154, let's call these temporary RSA keys" or "ephemeral RSA keys". The TLS 1.0 RFC doesn't use the term "key exchange keys". @@ +174,5 @@ > +{ > + unsigned int i; > + > + /* Note that SSL_ImplementedCiphers has more elements than cipherSuites > + * because it SSL_ImplementedCiphers includes SSL 2.0 cipher suites. We need to first assert that SSL_NumImplementedCiphers >= PR_ARRAY_SIZE(cipherSuites). ::: lib/ssl/sslenum.c @@ +23,5 @@ > + * * Export/weak/obsolete cipher suites before no-encryption cipher suites > + * * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA. > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. It seems that SEED should be listed before 3DES. SEED should be comparable to Camellia-128. @@ +25,5 @@ > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. Nit: GCM-SHA256 => GCM (the "SHA256" refers to the TLS 1.2 PRF) @@ +26,5 @@ > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. > + * * Within message authentication algorithm sections, order by assymetric assymetric => asymmetric (one s, two m's) @@ +29,5 @@ > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. > + * * Within message authentication algorithm sections, order by assymetric > + * signature algorithm: ECDSA, then RSA, then DSS. > + * > + * ECDHE before DHE before ECDH before RSA Delete these two lines (lines 32-33). @@ +79,5 @@ > TLS_RSA_WITH_AES_128_CBC_SHA256, > + TLS_RSA_WITH_CAMELLIA_128_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA, > + TLS_RSA_WITH_CAMELLIA_256_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA256, Swap TLS_RSA_WITH_CAMELLIA_256_CBC_SHA and TLS_RSA_WITH_AES_256_CBC_SHA256. @@ +82,5 @@ > + TLS_RSA_WITH_CAMELLIA_256_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA256, > + SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA, > + SSL_RSA_WITH_3DES_EDE_CBC_SHA, > + TLS_RSA_WITH_SEED_CBC_SHA, I would list TLS_RSA_WITH_SEED_CBC_SHA immediately after TLS_RSA_WITH_CAMELLIA_128_CBC_SHA.

Comment on attachment 829784 [details] [diff] [review] change-cipher-order.patch Review of attachment 829784 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +146,5 @@ > { SSL_DHE_DSS_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > { SSL_RSA_FIPS_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > { SSL_RSA_WITH_DES_CBC_SHA, SSL_ALLOWED, PR_FALSE, PR_FALSE}, > + > + /* export ciphersuites with 1024-bit public key exchange keys */ I didn't make this change. This comment was just copy/pasted from SSL_ImplementedCiphers to make it easier to see how the two arrays line up. If we want to improve the comment, let's do that separately, and to both arrays. @@ +174,5 @@ > +{ > + unsigned int i; > + > + /* Note that SSL_ImplementedCiphers has more elements than cipherSuites > + * because it SSL_ImplementedCiphers includes SSL 2.0 cipher suites. done. ::: lib/ssl/sslenum.c @@ +23,5 @@ > + * * Export/weak/obsolete cipher suites before no-encryption cipher suites > + * * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA. > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. I moved SEED to be ahead of 3DES. I also added a comment motivating the ordering of the symmetric encryption algorithms. @@ +25,5 @@ > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. done. @@ +26,5 @@ > + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then > + * FIPS-3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. > + * * Within message authentication algorithm sections, order by assymetric done. @@ +29,5 @@ > + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. > + * * Within message authentication algorithm sections, order by assymetric > + * signature algorithm: ECDSA, then RSA, then DSS. > + * > + * ECDHE before DHE before ECDH before RSA done. @@ +79,5 @@ > TLS_RSA_WITH_AES_128_CBC_SHA256, > + TLS_RSA_WITH_CAMELLIA_128_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA, > + TLS_RSA_WITH_CAMELLIA_256_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA256, done. @@ +82,5 @@ > + TLS_RSA_WITH_CAMELLIA_256_CBC_SHA, > + TLS_RSA_WITH_AES_256_CBC_SHA256, > + SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA, > + SSL_RSA_WITH_3DES_EDE_CBC_SHA, > + TLS_RSA_WITH_SEED_CBC_SHA, Moved just ahead of the 3DES cipher suites.

Brian: it is best to attach the final patch that you check in so we can comment on it more easily. Re: your description of the criteria for cipher suite ordering: 2.34 + * * Within key agreement sections, order by symmetric encryption algorithm: 2.35 + * AES-128, then Camellia-128, then AES-256, then Camellia-256, then SEED, 2.36 + * then FIPS-3DES, then 3DES, then RC4. AES is commonly accepted as a 2.37 + * strong cipher internationally, and is often hardware-accelerated. 2.38 + * Camellia also has wide international support across standards 2.39 + * organizations. SEED is only recommended by the Korean government. 3DES 2.40 + * and RC4 are now considered deprecated or forbidden by many standards 2.41 + * organizations. I don't think Triple DES is forbidden by many standards organizations. Triple DES's security strength of 112 bits alone is enough to justify being after all the 128-bit and 256-bit symmetric encryption algorithms except RC4. 2.42 + * * Within symmetric algorithm sections, order by message authentication 2.43 + * algorithm: GCM-SHA256, then HMAC-SHA1, then HMAC-SHA256, then HMAC-MD5. "GCM-SHA256" should be changed to "GCM". GCM does not use SHA-256. The "GCM_SHA256" in AES-GCM cipher suites refers to the TLS PRF based on SHA-256.

This is the version of the patch that I checked in.

I fixed the comments with this patch. FWIW, CRYPTREC and ENISA both consider 3DES legacy-use-only. But, I took your suggestion to avoid as much of the political component and hand-waviness as possible. I also fixed s/GCM-SHA256/GCM/.