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 786 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. */

[wtc, 2013/09/24 17:17:46]

I think this is correct for the server side but wrong for the client side.

On the client side, we also need to allow a cipher suite that is applicable
to a version lower than the given version, because a ClientHello that contains
|version| actually advertises support for versions up to and including
|version|.

This affects the EXPORT cipher suites that are allowed in TLS 1.0 but disallowed
in TLS 1.1+.

So, one way to fix this is to add a variant of ssl3_CipherSuiteAllowedForVersion
that either takes a version range as input or (less preferrable) will consider
all versions lower than the given version. And then, we call this new function
manually in ssl3_SendClientHello and its SSL2 counterpart.

Perhaps just one function that takes a version range would be better: on the
server side we'll just set the min and max versions to the same value.

[agl, 2013/09/24 18:58:22]

That's a very good point, thanks for that.

Have changed ssl3_CipherSuiteAllowedForVersion to
ssl3_CipherSuiteAllowedForVersionRange.

 /* 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,
+             PRUint16 version)
 {
     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_CipherSuiteAllowedForVersion(suite->cipher_suite,
+                                                      version));
 }
 
-/* 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. */
 /* called from ssl3_SendClientHello and ssl3_ConstructV2CipherSpecsHack */
 static int
-count_cipher_suites(sslSocket *ss, int policy, PRBool enabled)
+count_cipher_suites(sslSocket *ss, int policy, PRBool enabled,
+                    PRUint16 version)
 {
     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, version))
             count++;
     }
     if (count <= 0) {
         PORT_SetError(SSL_ERROR_SSL_DISABLED);
     }
     return count;
 }
 
 /*
  * Null compression, mac and encryption functions
@@ skipping 4349 matching lines @@
 
     if (IS_DTLS(ss)) {
         ssl3_DisableNonDTLSSuites(ss);
     }
 
     if (!ssl3_HasGCMSupport()) {
         ssl3_DisableGCMSuites(ss);
     }
 
     /* how many suites are permitted by policy and user preference? */
-    num_suites = count_cipher_suites(ss, ss->ssl3.policy, PR_TRUE);
+    num_suites = count_cipher_suites(ss, ss->ssl3.policy, PR_TRUE,
+                                     ss->version);
     if (!num_suites)
         return SECFailure;      /* count_cipher_suites has set error code. */
     if (ss->ssl3.hs.sendingSCSV) {
         ++num_suites;   /* make room for SCSV */
     }
 
     /* count compression methods */
     numCompressionMethods = 0;
     for (i = 0; i < compressionMethodsCount; i++) {
         if (compressionEnabled(ss, compressions[i]))
@@ skipping 69 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->version)) {
             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)) {
+            if (!config_match(suite, ss->ssl3.policy, PR_TRUE, ss->version)) {
                 break;  /* failure */
             }
-            if (!ssl3_CipherSuiteAllowedForVersion(suite->cipher_suite,
-                                                   ss->version)) {
-                desc    = handshake_failure;
-                errCode = SSL_ERROR_CIPHER_DISALLOWED_FOR_VERSION;
-                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 1648 matching lines @@
         }
         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, ss->version))
             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)) {
+        if (!config_match(suite, ss->ssl3.policy, PR_TRUE, ss->version)) {
             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)) {
+        if (!config_match(suite, ss->ssl3.policy, PR_TRUE, ss->version)) {
             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 3675 matching lines @@
     PORT_Assert(ss != 0);
     if (!ss) {
         PORT_SetError(PR_INVALID_ARGUMENT_ERROR);
         return SECFailure;
     }
     if (SSL3_ALL_VERSIONS_DISABLED(&ss->vrange)) {
         *size = 0;
         return SECSuccess;
     }
     if (cs == NULL) {
-        *size = count_cipher_suites(ss, SSL_ALLOWED, PR_TRUE);
+        *size = count_cipher_suites(ss, SSL_ALLOWED, PR_TRUE, ss->vrange.max);

[agl, 2013/09/23 18:39:10]

I am somewhat unsure about this (and on line 12324, below). I'm using
ss->vrange.max because in sslcon.c:3139, we find:

    ss->clientHelloVersion = SSL3_ALL_VERSIONS_DISABLED(&ss->vrange) ?
	SSL_LIBRARY_VERSION_2 : ss->vrange.max;

We know that SSL3_ALL_VERSIONS_DISABLED() is false here because of line 12312.
Thus the ClientHello version should be ss->vrange.max.

[wtc, 2013/09/24 17:24:13]

Your analysis is correct. At this point, some SSL 3.x version is enabled, so
it's
correct to pass ss->vrange.max. (As I noted before, we will need to consider the
entire version range here.)

         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.max)) {
             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 */