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

net/third_party/nss/ssl/ssl3con.c

 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
  * ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
@@ skipping 2267 matching lines @@
     SSL_TRC(3, ("%d: SSL3[%d] SendRecord type: %s nIn=%d",
                 SSL_GETPID(), ss->fd, ssl3_DecodeContentType(type),
                 nIn));
     PRINT_BUF(3, (ss, "Send record (plain text)", pIn, nIn));
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss) );
 
     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);
     }
 
     if (ss->ssl3.initialized == PR_FALSE) {
         /* This can happen on a server if the very first incoming record
         ** looks like a defective ssl3 record (e.g. too long), and we're
         ** trying to send an alert.
         */
         PR_ASSERT(type == content_alert);
@@ skipping 1702 matching lines @@
 ssl3_SendClientHello(sslSocket *ss, PRBool resending)
 {
     sslSessionID *   sid;
     ssl3CipherSpec * cwSpec;
     SECStatus        rv;
     int              i;
     int              length;
     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;
 
     SSL_TRC(3, ("%d: SSL3[%d]: send client_hello handshake", SSL_GETPID(),
                 ss->fd));
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss) );
 
@@ skipping 56 matching lines @@
             }
         }
         /* If we previously did client-auth, make sure that the token that
         ** holds the private key still exists, is logged in, hasn't been
         ** removed, etc.
         */
         if (sidOK && !ssl3_ClientAuthTokenPresent(sid)) {
             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) {
             SSL_AtomicIncrementLong(& ssl3stats.sch_sid_cache_not_ok );
             (*ss->sec.uncache)(sid);
             ssl_FreeSID(sid);
             sid = NULL;
         }
     }
 
     if (sid) {
-        serverVersionKnown = PR_TRUE;
+        requestingResume = PR_TRUE;
         SSL_AtomicIncrementLong(& ssl3stats.sch_sid_cache_hits );
 
         /* Are we attempting a stateless session resume? */
         if (sid->version > SSL_LIBRARY_VERSION_3_0 &&
             sid->u.ssl3.sessionTicket.ticket.data)
             SSL_AtomicIncrementLong(& ssl3stats.sch_sid_stateless_resumes );
 
         PRINT_BUF(4, (ss, "client, found session-id:", sid->u.ssl3.sessionID,
                       sid->u.ssl3.sessionIDLength));
 
         ss->ssl3.policy = sid->u.ssl3.policy;
     } 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) {
             return SECFailure;  /* memory error is set */
         }
     }
 
     isTLS = (ss->version > SSL_LIBRARY_VERSION_3_0);
     ssl_GetSpecWriteLock(ss);
     cwSpec = ss->ssl3.cwSpec;
     if (cwSpec->mac_def->mac == mac_null) {
         /* SSL records are not being MACed. */
@@ skipping 77 matching lines @@
         1 + numCompressionMethods + total_exten_len;
     if (IS_DTLS(ss)) {
         length += 1 + ss->ssl3.hs.cookieLen;
     }
 
     rv = ssl3_AppendHandshakeHeader(ss, client_hello, length);
     if (rv != SECSuccess) {
         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;
 
         version = dtls_TLSVersionToDTLSVersion(ss->clientHelloVersion);
         rv = ssl3_AppendHandshakeNumber(ss, version, 2);
     } else {
         rv = ssl3_AppendHandshakeNumber(ss, ss->clientHelloVersion, 2);
     }
     if (rv != SECSuccess) {
@@ skipping 99 matching lines @@
         PORT_Assert(!maxBytes);
     } 
     if (ss->ssl3.hs.sendingSCSV) {
         /* Since we sent the SCSV, pretend we sent empty RI extension. */
         TLSExtensionData *xtnData = &ss->xtnData;
         xtnData->advertised[xtnData->numAdvertised++] = 
             ssl_renegotiation_info_xtn;
     }
 
     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);
     if (rv != SECSuccess) {
         return rv;      /* error code set by ssl3_FlushHandshake */
     }
 
     ss->ssl3.hs.ws = wait_server_hello;
     return rv;
 }
@@ skipping 6440 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 */