NSS: don't advertise TLS 1.2-only ciphersuites in a TLS 1.1 ClientHello.

net/third_party/nss/ssl/ssl3con.c

 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */
 
@@ skipping 613 matching lines @@
     if ((sizeof *x) == sizeof(PRInt32)) {
         PR_ATOMIC_INCREMENT((PRInt32 *)x);
     } else {
         tooLong * tl = (tooLong *)x;
         if (PR_ATOMIC_INCREMENT(&tl->low) == 0)
             PR_ATOMIC_INCREMENT(&tl->high);
     }
 }
 
 static PRBool
-ssl3_CipherSuiteAllowedForVersion(ssl3CipherSuite cipherSuite,
-                                  SSL3ProtocolVersion version)
+ssl3_CipherSuiteAllowedForVersionRange(
+    ssl3CipherSuite cipherSuite,
+    SSLVersionRange vrange)

[wtc, 2013/09/25 00:42:14]

Nit: use const SSLVersionRange *vrange ?

I know SSLVersionRange is a simple struct and can be easily passed by value.
This is just to be consistent with the SSL_VersionRangeSet and
SSL_VersionRangeSetDefault functions.

This comment also applies to the config_match() function on line 820.

[agl, 2013/09/25 16:21:53]

Done.

 {
     switch (cipherSuite) {
     /* See RFC 4346 A.5. Export cipher suites must not be used in TLS 1.1 or
      * later. This set of cipher suites is similar to, but different from, the
      * set of cipher suites considered exportable by SSL_IsExportCipherSuite.
      */
     case SSL_RSA_EXPORT_WITH_RC4_40_MD5:
     case SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5:
     /*   SSL_RSA_EXPORT_WITH_DES40_CBC_SHA:      never implemented
      *   SSL_DH_DSS_EXPORT_WITH_DES40_CBC_SHA:   never implemented
      *   SSL_DH_RSA_EXPORT_WITH_DES40_CBC_SHA:   never implemented
      *   SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA:  never implemented
      *   SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA:  never implemented
      *   SSL_DH_ANON_EXPORT_WITH_RC4_40_MD5:     never implemented
      *   SSL_DH_ANON_EXPORT_WITH_DES40_CBC_SHA:  never implemented
      */
-        return version <= SSL_LIBRARY_VERSION_TLS_1_0;
+        return vrange.min <= SSL_LIBRARY_VERSION_TLS_1_0;
+    case TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305:
+    case TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305:
     case TLS_DHE_RSA_WITH_AES_256_CBC_SHA256:
     case TLS_RSA_WITH_AES_256_CBC_SHA256:
     case TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256:
     case TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256:
     case TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256:
     case TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256:
     case TLS_DHE_RSA_WITH_AES_128_CBC_SHA256:
     case TLS_DHE_RSA_WITH_AES_128_GCM_SHA256:
     case TLS_RSA_WITH_AES_128_CBC_SHA256:
     case TLS_RSA_WITH_AES_128_GCM_SHA256:
     case TLS_RSA_WITH_NULL_SHA256:
-        return version >= SSL_LIBRARY_VERSION_TLS_1_2;
+        return vrange.max >= SSL_LIBRARY_VERSION_TLS_1_2;
     default:
         return PR_TRUE;
     }
 }
 
 /* return pointer to ssl3CipherSuiteDef for suite, or NULL */
 /* XXX This does a linear search.  A binary search would be better. */
 static const ssl3CipherSuiteDef *
 ssl_LookupCipherSuiteDef(ssl3CipherSuite suite)
 {
@@ skipping 122 matching lines @@
         }
     }
     PORT_Assert(numPresent > 0 || numEnabled == 0);
     if (numPresent <= 0) {
         PORT_SetError(SSL_ERROR_NO_CIPHERS_SUPPORTED);
     }
     return numPresent;
 }
 
 
-/* return PR_TRUE if suite matches policy and enabled state */
+/* return PR_TRUE if suite matches policy, enabled state and is applicable to
+ * the given version range. */
 /* It would be a REALLY BAD THING (tm) if we ever permitted the use
 ** of a cipher that was NOT_ALLOWED.  So, if this is ever called with
 ** policy == SSL_NOT_ALLOWED, report no match.
 */
 /* adjust suite enabled to the availability of a token that can do the
  * cipher suite. */
 static PRBool
-config_match(ssl3CipherSuiteCfg *suite, int policy, PRBool enabled)
+config_match(ssl3CipherSuiteCfg *suite, int policy, PRBool enabled,
+             SSLVersionRange vrange)
 {
     PORT_Assert(policy != SSL_NOT_ALLOWED && enabled != PR_FALSE);
     if (policy == SSL_NOT_ALLOWED || !enabled)
         return PR_FALSE;
     return (PRBool)(suite->enabled &&
                     suite->isPresent &&
                     suite->policy != SSL_NOT_ALLOWED &&
-                    suite->policy <= policy);
+                    suite->policy <= policy &&
+                    ssl3_CipherSuiteAllowedForVersionRange(
+                        suite->cipher_suite, vrange));
 }
 
-/* return number of cipher suites that match policy and enabled state */
+/* return number of cipher suites that match policy, enabled state and are
+ * applicable for the given protocol version. */

[wtc, 2013/09/25 00:42:14]

"the given protocol version" is wrong because this function doesn't take a
|version| nor a |vrange| argument.

I think this should say "the configured protocol version range".

[agl, 2013/09/25 16:21:53]

Done.

 /* called from ssl3_SendClientHello and ssl3_ConstructV2CipherSpecsHack */
 static int
 count_cipher_suites(sslSocket *ss, int policy, PRBool enabled)
 {
     int i, count = 0;
 
     if (SSL3_ALL_VERSIONS_DISABLED(&ss->vrange)) {
         return 0;
     }
     for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) {
-        if (config_match(&ss->cipherSuites[i], policy, enabled))
+        if (config_match(&ss->cipherSuites[i], policy, enabled, ss->vrange))
             count++;
     }
     if (count <= 0) {
         PORT_SetError(SSL_ERROR_SSL_DISABLED);
     }
     return count;
 }
 
 /*
  * Null compression, mac and encryption functions
@@ skipping 4439 matching lines @@
         /* Add the actual SCSV */
         rv = ssl3_AppendHandshakeNumber(ss, TLS_EMPTY_RENEGOTIATION_INFO_SCSV,
                                         sizeof(ssl3CipherSuite));
         if (rv != SECSuccess) {
             return rv;  /* err set by ssl3_AppendHandshake* */
         }
         actual_count++;
     }
     for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) {
         ssl3CipherSuiteCfg *suite = &ss->cipherSuites[i];
-        if (config_match(suite, ss->ssl3.policy, PR_TRUE)) {
+        if (config_match(suite, ss->ssl3.policy, PR_TRUE, ss->vrange)) {
             actual_count++;
             if (actual_count > num_suites) {
                 /* set error card removal/insertion error */
                 PORT_SetError(SSL_ERROR_TOKEN_INSERTION_REMOVAL);
                 return SECFailure;
             }
             rv = ssl3_AppendHandshakeNumber(ss, suite->cipher_suite,
                                             sizeof(ssl3CipherSuite));
             if (rv != SECSuccess) {
                 return rv;      /* err set by ssl3_AppendHandshake* */
@@ skipping 1044 matching lines @@
 
     /* find selected cipher suite in our list. */
     temp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length);
     if (temp < 0) {
         goto loser;     /* alert has been sent */
     }
     ssl3_config_match_init(ss);
     for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) {
         ssl3CipherSuiteCfg *suite = &ss->cipherSuites[i];
         if (temp == suite->cipher_suite) {
-            if (!config_match(suite, ss->ssl3.policy, PR_TRUE)) {
+            SSLVersionRange vrange = {ss->version, ss->version};
+            if (!config_match(suite, ss->ssl3.policy, PR_TRUE, vrange)) {
                 break;  /* failure */
             }
-            if (!ssl3_CipherSuiteAllowedForVersion(suite->cipher_suite,
-                                                   ss->version)) {
-                desc    = handshake_failure;
-                errCode = SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION;

[wtc, 2013/09/25 00:42:14]

Hmm... this change causes us to lose the more informative error code
SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION (as opposed to
SSL_ERROR_NO_CYPHER_OVERLAP). I wonder if we can avoid this easily.

Note: this is the only place we use SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION.

One simple fix is to call ssl3_CipherSuiteAllowedForVersionRange() redundantly
if
config_match() fails, to refine the error code. A more complicated fix would be
to make ssl3_CipherSuiteAllowedForVersionRange() return or set an error code.

[agl, 2013/09/25 16:21:53]

Done.

-                goto alert_loser;
-            }
         
             suite_found = PR_TRUE;
             break;      /* success */
         }
     }
     if (!suite_found) {
         desc    = handshake_failure;
         errCode = SSL_ERROR_NO_CYPHER_OVERLAP;
         goto alert_loser;
     }
@@ skipping 1620 matching lines @@
         goto alert_loser;
     }
 #endif
 
     /* If we already have a session for this client, be sure to pick the
     ** same cipher suite and compression method we picked before.
     ** This is not a loop, despite appearances.
     */
     if (sid) do {
         ssl3CipherSuiteCfg *suite;
+#ifdef PARANOID
+        SSLVersionRange vrange = {ss->version, ss->version};
+#endif
 
         /* Check that the cached compression method is still enabled. */
         if (!compressionEnabled(ss, sid->u.ssl3.compression))
             break;
 
         /* Check that the cached compression method is in the client's list */
         for (i = 0; i < comps.len; i++) {
             if (comps.data[i] == sid->u.ssl3.compression)
                 break;
         }
         if (i == comps.len)
             break;
 
         suite = ss->cipherSuites;
         /* Find the entry for the cipher suite used in the cached session. */
         for (j = ssl_V3_SUITES_IMPLEMENTED; j > 0; --j, ++suite) {
             if (suite->cipher_suite == sid->u.ssl3.cipherSuite)
                 break;
         }
         PORT_Assert(j > 0);
         if (j <= 0)
             break;
 #ifdef PARANOID
         /* Double check that the cached cipher suite is still enabled,
          * implemented, and allowed by policy.  Might have been disabled.
          * The product policy won't change during the process lifetime.  
          * Implemented ("isPresent") shouldn't change for servers.
          */
-        if (!config_match(suite, ss->ssl3.policy, PR_TRUE))
+        if (!config_match(suite, ss->ssl3.policy, PR_TRUE, vrange))
             break;
 #else
         if (!suite->enabled)
             break;
 #endif
         /* Double check that the cached cipher suite is in the client's list */
         for (i = 0; i + 1 < suites.len; i += 2) {
             PRUint16 suite_i = (suites.data[i] << 8) | suites.data[i + 1];
             if (suite_i == suite->cipher_suite) {
                 ss->ssl3.hs.cipher_suite = suite->cipher_suite;
@@ skipping 27 matching lines @@
     ** offered TLS 1.1 but offered only export cipher suites by choosing TLS
     ** 1.0 and selecting one of those export cipher suites. However, a secure
     ** TLS 1.1 client should not have export cipher suites enabled at all,
     ** and a TLS 1.1 client should definitely not be offering *only* export
     ** cipher suites. Therefore, we refuse to negotiate export cipher suites
     ** with any client that indicates support for TLS 1.1 or higher when we
     ** (the server) have TLS 1.1 support enabled.
     */
     for (j = 0; j < ssl_V3_SUITES_IMPLEMENTED; j++) {
         ssl3CipherSuiteCfg *suite = &ss->cipherSuites[j];
-        if (!config_match(suite, ss->ssl3.policy, PR_TRUE) ||
-            !ssl3_CipherSuiteAllowedForVersion(suite->cipher_suite,
-                                               ss->version)) {
+        SSLVersionRange vrange = {ss->version, ss->version};
+        if (!config_match(suite, ss->ssl3.policy, PR_TRUE, vrange)) {
             continue;
         }
         for (i = 0; i + 1 < suites.len; i += 2) {
             PRUint16 suite_i = (suites.data[i] << 8) | suites.data[i + 1];
             if (suite_i == suite->cipher_suite) {
                 ss->ssl3.hs.cipher_suite = suite->cipher_suite;
                 ss->ssl3.hs.suite_def =
                     ssl_LookupCipherSuiteDef(ss->ssl3.hs.cipher_suite);
                 goto suite_found;
             }
@@ skipping 512 matching lines @@
 
     /* Select a cipher suite.
     **
     ** NOTE: This suite selection algorithm should be the same as the one in
     ** ssl3_HandleClientHello().
     **
     ** See the comments about export cipher suites in ssl3_HandleClientHello().
     */
     for (j = 0; j < ssl_V3_SUITES_IMPLEMENTED; j++) {
         ssl3CipherSuiteCfg *suite = &ss->cipherSuites[j];
-        if (!config_match(suite, ss->ssl3.policy, PR_TRUE) ||
-            !ssl3_CipherSuiteAllowedForVersion(suite->cipher_suite,
-                                               ss->version)) {
+        SSLVersionRange vrange = {ss->version, ss->version};
+        if (!config_match(suite, ss->ssl3.policy, PR_TRUE, vrange)) {
             continue;
         }
         for (i = 0; i+2 < suite_length; i += 3) {
             PRUint32 suite_i = (suites[i] << 16)|(suites[i+1] << 8)|suites[i+2];
             if (suite_i == suite->cipher_suite) {
                 ss->ssl3.hs.cipher_suite = suite->cipher_suite;
                 ss->ssl3.hs.suite_def =
                     ssl_LookupCipherSuiteDef(ss->ssl3.hs.cipher_suite);
                 goto suite_found;
             }
@@ skipping 3682 matching lines @@
         return SECSuccess;
     }
     if (cs == NULL) {
         *size = count_cipher_suites(ss, SSL_ALLOWED, PR_TRUE);
         return SECSuccess;
     }
 
     /* ssl3_config_match_init was called by the caller of this function. */
     for (i = 0; i < ssl_V3_SUITES_IMPLEMENTED; i++) {
         ssl3CipherSuiteCfg *suite = &ss->cipherSuites[i];
-        if (config_match(suite, SSL_ALLOWED, PR_TRUE)) {
+        if (config_match(suite, SSL_ALLOWED, PR_TRUE, ss->vrange)) {
             if (cs != NULL) {
                 *cs++ = 0x00;
                 *cs++ = (suite->cipher_suite >> 8) & 0xFF;
                 *cs++ =  suite->cipher_suite       & 0xFF;
             }
             count++;
         }
     }
     *size = count;
     return SECSuccess;
@@ skipping 117 matching lines @@
             PORT_Free(ss->ssl3.hs.recvdFragments.buf);
         }
     }
 
     ss->ssl3.initialized = PR_FALSE;
 
     SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
 }
 
 /* End of ssl3con.c */