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)
@@ -786,7 +786,7 @@
     }
 
     ss->version = PR_MIN(peerVersion, ss->vrange.max);
-    PORT_Assert(ssl3_VersionIsSupported(ssl_variant_stream, ss->version));
+    PORT_Assert(ssl3_VersionIsSupported(ss->protocolVariant, ss->version));

[Ryan Sleevi, 2012/08/15 03:39:07]

This is related to your change to ekr's patch, right?

[wtc, 2012/08/15 16:16:59]

Yes, this fixes an unrelated bug that I discovered while
studying the code that modifies ss->version.  I can remove
this bug fix from the CL.

 
     return SECSuccess;
 }
@@ -2286,8 +2286,9 @@
 
     if (capRecordVersion) {
 	/* ssl_SEND_FLAG_CAP_RECORD_VERSION can only be used with
-	 * TLS ClientHello. */
+	 * TLS initial ClientHello. */

[agl, 2012/08/15 04:02:23]

(nit: I think there should be a 'the' in this comment.)

 	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,33 @@
 	    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) {

[Ryan Sleevi, 2012/08/15 03:39:07]

if (sidOK), isn't a guarantee that firstHsDone is true, since it means we have
previously negotiated a session?

[agl, 2012/08/15 04:02:23]

I'm not quite sure that I understand your question, but I read sidOK to be that
we have a session to attempt a resumption with. But that's doesn't mean that
we've already done a handshake.

[Ryan Sleevi, 2012/08/15 04:09:44]

If we have a session to attempt a resumption with, doesn't that mean we've
exchanged Finished messages at least once, hence we've done a handshake?

Where I'm trying to understand MSFTs bug and this implementation is what the
expected result is when
1) Resuming a previous session on a new connection
2) Resuming a previous session on an existing connection (since the session ID
is not invalidated until the server says it is, and it could request
renegotiation but not invalidate the session)

The intermingling of session variables (sidOK, sid->version) with connection
variables (ss->firstHsDone, ss->clientHelloVersion) is what is giving me trouble
in reasoning.

[wtc, 2012/08/15 16:16:59]

If ss->firstHsDone is true, it means the first handshake
on this SSL socket ('ss') is done.  The session we're
requesting to resume ('sid') was negotiated on some other
SSL socket.
This is the sidOK=true ss->firstHsDone=false case.  The
expected result is to set client_version of ClientHello
to the version in 'sid'.  NSS has always been doing this.
I just added a comment (lines 4091-4096) to explain the
rationale.
This is the sidOK=true ss->firstHsDone=true case.  This
CL changes the behavior of NSS in this case.  NSS used to
set client_version of the renegotiation ClientHello to
sid->version.  Because of the SChannel bug, NSS will need
to set client_version of the renegotiation ClientHello
to the client_version of the initial ClientHello, which
is still available in ss->clientHelloVersion at this point.
(That value will be overwritten on line 4263 below, where
I added an assertion.)

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

I found that my reply has a typo. It should read (with the
change highlighted in CAPS):

If ss->firstHsDone is true, it means the first handshake on this SSL socket
('ss') is done. The session we're requesting to resume ('sid') MAY HAVE BEEN
negotiated on
some other SSL socket.

+		if (sid->version <= ss->clientHelloVersion) {
+		    ss->version = ss->clientHelloVersion;

[Ryan Sleevi, 2012/08/15 03:39:07]

It seems implicit in this is the assumption that sid->version cannot be greater
than ss->clientHelloVersion, right? Is that a safe assumption [it would seem so,
I just want to confirm]

ssl3_NegotiateVersion ensures that sid->version is bounded within (min, max), so
I wanted to make sure we couldn't accidentally blow over max here.

[wtc, 2012/08/15 16:16:59]

In this case, the SChannel bug requires us to set
ss->version to ss->clientHelloVersion.  So if we want to
resume the session 'sid', we need to make sure sid->version
is enabled on this SSL socket.

Since the session 'sid' may have been negotiated on a
different SSL socket with a different version range,
sid->version may be greater than ss->clientHelloVersion.
While answering this question, I noticed that we should
also ensure sid->version >= ss->vrange.min.

+		} else {
+		    sidOK = PR_FALSE;

[agl, 2012/08/15 04:02:23]

Is this case ok? If we reject sid because it's version is greater than
clientHelloVersion, might we still end up sending a version other than
clientHelloVersion in the renegotiation?

I'm not sure why this section isn't just:

if (sidOK && ssl3_NegotiateVersion(ss, sid->version, PR_FALSE) != SECSuccess) {
    sidOK = PR_FALSE;
}

if (ss->firstHsDone)
  ss->version = ss->clientHelloVersion;

[wtc, 2012/08/15 16:16:59]

This case will be handled correctly on lines 4152-4153
below.
I considered writing the code that way before, but
ssl3_NegotiateVersion is quite right for the renegotiation
case, where the version upper bound is ss->clientHelloVersion
instead of ss->vrange.max.

I also considered moving the SChannel bug workaround logic
into the ssl3_NegotiateVersion function.  Since that
function is also used by server-side code, I didn't want
to make it more complicated.  I will look into that again.

+		}
+	    } else {
+		if (ssl3_NegotiateVersion(ss, sid->version,
+					  PR_FALSE) != SECSuccess) {

[Ryan Sleevi, 2012/08/15 03:39:07]

nit: Shouldn't it be two tabs, then 4 spaces (matching indent of the if), then
spaces to align the PR_FALSE with the opening paren on 4113?

Same with line 4156

[wtc, 2012/08/15 16:16:59]

The original code also uses the maximum number of tabs,
followed by just enough spaces to align.  I think as long
as the line starts with a tab, it'll look aligned in a
diff.

+		    sidOK = PR_FALSE;
+		}
+	    }
 	}
 
 	if (!sidOK) {
@@ -4101,7 +4126,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 +4141,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;
+	} 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);
 	if (!sid) {
@@ -4219,6 +4256,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;
@@ -4337,8 +4378,9 @@
 	    ssl_renegotiation_info_xtn;
     }
 
+    ss->ssl3.hs.ws = wait_server_hello;

[Ryan Sleevi, 2012/08/15 03:39:07]

Did you mean to move this? It doesn't seem related.

Previously, the wait state would only be updated if rv == SECSuccess, where rv
was set on 4386

[wtc, 2012/08/15 16:16:59]

Yes, this line needs to be moved here to support the
assertion on line 2293.

The NSS code that sets ss->ssl3.hs.ws when sending
ClientHello is not very clean.

For the initial ClientHello, ss->ssl3.hs.w3 is initialized
to wait_server_hello early in this function, inside the
ssl3_InitState call on line 4025:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

It then sets ss->ssl3.hs.ws to wait_server_hello again
here.

For a renegotiation ClientHello, the ssl3_InitState call
on line 4025 has no effect (because ss->ssl3.initialized
is true), so ss->ssl3.hs.ws is idle_handshake at this
point.  Therefore, if I don't move this assignment before
the ssl3_FlushHandshake call, the assertion on line 2293
will fail in a renegotiation.

Another solution would be to remove all the assertions
on line 2287-2294.  It does seem better to transition to
a new ss->ssl3.hs.ws only when everything succeeds.

     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);
@@ -4346,7 +4388,6 @@
 	return rv;	/* error code set by ssl3_FlushHandshake */
     }
 
-    ss->ssl3.hs.ws = wait_server_hello;
     return rv;
 }