Closed
Bug 936828
Opened 8 years ago
Closed 8 years ago
Change order of cipher suites offered in client hello to match modern best practices
Categories
(NSS :: Libraries, enhancement)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.15.4
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(2 files, 1 obsolete file)
17.06 KB,
patch

briansmith
:
review+
briansmith
:
checkedin+

Details  Diff  Splinter Review 
1.72 KB,
patch

briansmith
:
checkedin+

Details  Diff  Splinter Review 
See the description in the comment of the patch for the logic used to do the ordering. This patch adds a debugbuildonly 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 WanTeh'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 nonsensical: 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 AES128/Camellia128 isn't good enough can disable 128bit cipher suites to force the 256bit variants to be chosen, but otherwise AES128 is secure enough and better overall because it is faster.
Attachment #829784 
Flags: review?(wtc)
Updated•8 years ago

Attachment #829784 
Attachment is patch: true
Attachment #829784 
Attachment mime type: text/xpatch → text/plain
Comment 1•8 years ago


Comment on attachment 829784 [details] [diff] [review] changecipherorder.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 128bit 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 1024bit public key exchange keys */ You may want to note these RSA_EXPORT1024 cipher suites are specified in draftietftls56bitciphersuites01. (They are not in the TLS 1.0 RFC.) Another difference is that these two cipher suites use 56bit 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 noencryption cipher suites > + * * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA. > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, then 3DES, then SEED, then RC4. It seems that SEED should be listed before 3DES. SEED should be comparable to Camellia128. @@ +25,5 @@ > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. Nit: GCMSHA256 => GCM (the "SHA256" refers to the TLS 1.2 PRF) @@ +26,5 @@ > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. > + * * Within message authentication algorithm sections, order by assymetric assymetric => asymmetric (one s, two m's) @@ +29,5 @@ > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. > + * * 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 3233). @@ +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.
Attachment #829784 
Flags: review?(wtc) → review+
Assignee  
Comment 2•8 years ago


http://hg.mozilla.org/projects/nss/rev/968dc8105c00
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution:  → FIXED
Assignee  
Comment 3•8 years ago


Comment on attachment 829784 [details] [diff] [review] changecipherorder.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 1024bit 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 noencryption cipher suites > + * * Order by key exchange algorithm: ECDHE, then DHE, then ECDH, RSA. > + * * Within key agreement sections, order by symmetric encryption algorithm: > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, 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: > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. done. @@ +26,5 @@ > + * AES128, then Camellia128, then AES256, then Camellia256, then > + * FIPS3DES, then 3DES, then SEED, then RC4. > + * * Within symmetric algorithm sections, order by message authentication > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. > + * * Within message authentication algorithm sections, order by assymetric done. @@ +29,5 @@ > + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. > + * * 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.
Comment 4•8 years ago


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 + * AES128, then Camellia128, then AES256, then Camellia256, then SEED, 2.36 + * then FIPS3DES, then 3DES, then RC4. AES is commonly accepted as a 2.37 + * strong cipher internationally, and is often hardwareaccelerated. 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 128bit and 256bit symmetric encryption algorithms except RC4. 2.42 + * * Within symmetric algorithm sections, order by message authentication 2.43 + * algorithm: GCMSHA256, then HMACSHA1, then HMACSHA256, then HMACMD5. "GCMSHA256" should be changed to "GCM". GCM does not use SHA256. The "GCM_SHA256" in AESGCM cipher suites refers to the TLS PRF based on SHA256.
Assignee  
Comment 5•8 years ago


This is the version of the patch that I checked in.
Attachment #829784 
Attachment is obsolete: true
Attachment #833409 
Flags: review+
Attachment #833409 
Flags: checkedin+
Assignee  
Comment 6•8 years ago


I fixed the comments with this patch. FWIW, CRYPTREC and ENISA both consider 3DES legacyuseonly. But, I took your suggestion to avoid as much of the political component and handwaviness as possible. I also fixed s/GCMSHA256/GCM/.
Attachment #833410 
Flags: checkedin+
You need to log in
before you can comment on or make changes to this bug.
Description
•