When renegotiating, continue to use the client_version used in the initial ClientHello

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c	(revision 151171)
+++ net/third_party/nss/ssl/ssl3con.c	(working copy)
@@ -2285,9 +2285,10 @@
     capRecordVersion = ((flags & ssl_SEND_FLAG_CAP_RECORD_VERSION) != 0);
 
     if (capRecordVersion) {
-	/* ssl_SEND_FLAG_CAP_RECORD_VERSION can only be used with
-	 * TLS ClientHello. */
+	/* ssl_SEND_FLAG_CAP_RECORD_VERSION can only be used with the
+	 * TLS initial ClientHello. */
 	PORT_Assert(!IS_DTLS(ss));
+	PORT_Assert(!ss->firstHsDone);
 	PORT_Assert(type == content_handshake);
 	PORT_Assert(ss->ssl3.hs.ws == wait_server_hello);
     }
@@ -4010,7 +4011,7 @@
     int              num_suites;
     int              actual_count = 0;
     PRBool           isTLS = PR_FALSE;
-    PRBool           serverVersionKnown = PR_FALSE;
+    PRBool           requestingResume = PR_FALSE;
     PRInt32          total_exten_len = 0;
     unsigned         numCompressionMethods;
     PRInt32          flags;
@@ -4087,9 +4088,41 @@
 	    sidOK = PR_FALSE;
 	}
 
-	if (sidOK && ssl3_NegotiateVersion(ss, sid->version,
-					   PR_FALSE) != SECSuccess) {
-	    sidOK = PR_FALSE;
+	/* TLS 1.0 (RFC 2246) Appendix E says:
+	 *   Whenever a client already knows the highest protocol known to
+	 *   a server (for example, when resuming a session), it should
+	 *   initiate the connection in that native protocol.
+	 * So we pass sid->version to ssl3_NegotiateVersion() here, except
+	 * when renegotiating.
+	 *
+	 * Windows SChannel compares the client_version inside the RSA
+	 * EncryptedPreMasterSecret of a renegotiation with the
+	 * client_version of the initial ClientHello rather than the
+	 * ClientHello in the renegotiation. To work around this bug, we
+	 * continue to use the client_version used in the initial
+	 * ClientHello when renegotiating.
+	 */
+	if (sidOK) {
+	    if (ss->firstHsDone) {
+		/*
+		 * The client_version of the initial ClientHello is still
+		 * available in ss->clientHelloVersion. Ensure that
+		 * sid->version is bounded within
+		 * [ss->vrange.min, ss->clientHelloVersion], otherwise we
+		 * can't use sid.
+		 */
+		if (sid->version >= ss->vrange.min &&
+		    sid->version <= ss->clientHelloVersion) {

[Ryan Sleevi, 2012/08/16 00:33:25]

I'm still having trouble parsing this, in part because it's ignoring
ss->vrange.max (unlike line 4121)

Should there also be a check that sid->version <= ss->vrange.max ? I'm not fully
sure when such a situation could arise, which is why I'm having trouble with
this block, trying to contrive one :)

[wtc, 2012/08/16 01:40:06]

This is an important question.  Here is the reasoning.

1. Line 788 in ssl3_NegotiateVersion ensures that
ss->version <= ss->vrange.max:

  788 ss->version = PR_MIN(peerVersion, ss->vrange.max);

2. If ss->firstHsDone is false, ss->version is set by
a ssl3_NegotiateVersion call (on either line 4121 or
line 4163).

3. ss->version is copied to ss->clientHelloVersion on
line 4271:

    4271 ss->clientHelloVersion = ss->version;

4. Therefore, as long as ss->vrange.max is not changed
in between, we know ss->clientHelloVersion <= ss->vrange.max
here.  So sid->version <= ss->clientHelloVersion implies
sid->version <= ss->vrange.max.

+		    ss->version = ss->clientHelloVersion;
+		} else {
+		    sidOK = PR_FALSE;
+		}
+	    } else {
+		if (ssl3_NegotiateVersion(ss, sid->version,
+					  PR_FALSE) != SECSuccess) {
+		    sidOK = PR_FALSE;
+		}
+	    }
 	}
 
 	if (!sidOK) {
@@ -4101,7 +4134,7 @@
     }
 
     if (sid) {
-	serverVersionKnown = PR_TRUE;
+	requestingResume = PR_TRUE;
 	SSL_AtomicIncrementLong(& ssl3stats.sch_sid_cache_hits );
 
 	/* Are we attempting a stateless session resume? */
@@ -4116,10 +4149,22 @@
     } else {
 	SSL_AtomicIncrementLong(& ssl3stats.sch_sid_cache_misses );
 
-	rv = ssl3_NegotiateVersion(ss, SSL_LIBRARY_VERSION_MAX_SUPPORTED,
-				   PR_TRUE);
-	if (rv != SECSuccess)
-	    return rv;	/* error code was set */
+	/*
+	 * Windows SChannel compares the client_version inside the RSA
+	 * EncryptedPreMasterSecret of a renegotiation with the
+	 * client_version of the initial ClientHello rather than the
+	 * ClientHello in the renegotiation. To work around this bug, we
+	 * continue to use the client_version used in the initial
+	 * ClientHello when renegotiating.
+	 */
+	if (ss->firstHsDone) {
+	    ss->version = ss->clientHelloVersion;

[Ryan Sleevi, 2012/08/16 00:33:25]

Is it possible for the socket to be re-configured to negotiate a different set
of versions? This is caught by line 4163, but is not caught here, AFAICT.

[wtc, 2012/08/16 01:40:06]

An application can do that, but a lot of NSS code will break.
Ideally we should change SSL_VersionRangeSet to fail if
the SSL socket has already used the current ss->vrange.

+	} else {
+	    rv = ssl3_NegotiateVersion(ss, SSL_LIBRARY_VERSION_MAX_SUPPORTED,
+				       PR_TRUE);
+	    if (rv != SECSuccess)
+		return rv;	/* error code was set */
+	}
 
 	sid = ssl3_NewSessionID(ss, PR_FALSE);

[wtc, 2012/08/16 01:40:06]

I considered handling the SChannel bug only once, at line
4174, for both the sid and no-sid cases.  But it is this
ssl3_NewSessionID call that prevented that, because
ss->version must be correct when we pass 'ss' to the
ssl3_NewSessionID call.

 	if (!sid) {
@@ -4219,6 +4264,10 @@
 	return rv;	/* err set by ssl3_AppendHandshake* */
     }
 
+    if (ss->firstHsDone) {
+	/* Work around the Windows SChannel bug described above. */ 
+	PORT_Assert(ss->version == ss->clientHelloVersion);
+    }
     ss->clientHelloVersion = ss->version;
     if (IS_DTLS(ss)) {
 	PRUint16 version;
@@ -4338,7 +4387,7 @@
     }
 
     flags = 0;
-    if (!serverVersionKnown && !IS_DTLS(ss)) {
+    if (!ss->firstHsDone && !requestingResume && !IS_DTLS(ss)) {
 	flags |= ssl_SEND_FLAG_CAP_RECORD_VERSION;
     }
     rv = ssl3_FlushHandshake(ss, flags);