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

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
diff --git a/net/third_party/nss/ssl/ssl3con.c b/net/third_party/nss/ssl/ssl3con.c
index bc54c995b60c80d539c3fe86f77450e3f31a203f..6f12efb905830c97effe3cc0510723d3add0210a 100644
--- a/net/third_party/nss/ssl/ssl3con.c
+++ b/net/third_party/nss/ssl/ssl3con.c
@@ -804,7 +804,8 @@ ssl3_config_match_init(sslSocket *ss)
 }
 
 
-/* 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.
@@ -812,7 +813,8 @@ ssl3_config_match_init(sslSocket *ss)
 /* 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)
@@ -820,13 +822,17 @@ config_match(ssl3CipherSuiteCfg *suite, int policy, PRBool enabled)
     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;
 
@@ -834,7 +840,7 @@ count_cipher_suites(sslSocket *ss, int policy, PRBool enabled)
     	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) {
@@ -5204,7 +5210,8 @@ ssl3_SendClientHello(sslSocket *ss, PRBool resending)
     }
 
     /* 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) {
@@ -5294,7 +5301,7 @@ ssl3_SendClientHello(sslSocket *ss, PRBool resending)
     }
     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 */
@@ -6359,15 +6366,9 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     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 */
@@ -8036,7 +8037,7 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
 	 * 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)
@@ -8084,9 +8085,7 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     */
     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) {
@@ -8619,9 +8618,7 @@ ssl3_HandleV2ClientHello(sslSocket *ss, unsigned char *buffer, int length)
     */
     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) {
@@ -12317,14 +12314,14 @@ ssl3_ConstructV2CipherSpecsHack(sslSocket *ss, unsigned char *cs, int *size)
 	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;