Skip to content

Commit

Permalink
When renegotiating, continue to use the client_version used in the
Browse files Browse the repository at this point in the history
initial ClientHello to work around a Windows SChannel bug.

Cap the record layer version number to TLS 1.0 only for the initial
ClientHello. The record layer version number of the ClientHello in
a renegotiation should use the currently negotiated version number.

R=agl@chromium.org,rsleevi@chromium.org
BUG=141629
TEST=Visit https://solutionscenter.naradana.net/, an IIS server that
requests (but doesn't require) client certificates over renegotiation.
The page should be laid out correctly.

Review URL: https://chromiumcodereview.appspot.com/10828269

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@152116 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
wtc@chromium.org committed Aug 17, 2012
1 parent c0495e8 commit 9808588
Show file tree
Hide file tree
Showing 5 changed files with 242 additions and 45 deletions.
9 changes: 7 additions & 2 deletions net/third_party/nss/README.chromium
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,20 @@ Patches:
* Don't crash when the SSL keylog file cannot be opened.
patches/sslkeylogerror.patch

* Set the record layer version number of ClientHello to at most TLS 1.0
if we don't know what protocol version the server supports.
* Set the record layer version number of the initial ClientHello to at
most TLS 1.0 if we don't know what protocol version the server supports.
https://bugzilla.mozilla.org/show_bug.cgi?id=774547
patches/recordlayerversion.patch

* Replace hardcoded ssl_variant_stream by ss->protocolVariant.
https://bugzilla.mozilla.org/show_bug.cgi?id=681065
patches/sslprotocolvariant.patch

* When renegotiating, continue to use the client_version used in the
initial ClientHello to work around a Windows SChannel bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=783448
patches/renegoclientversion.patch

Apply the patches to NSS by running the patches/applypatches.sh script. Read
the comments at the top of patches/applypatches.sh for instructions.

Expand Down
4 changes: 3 additions & 1 deletion net/third_party/nss/patches/applypatches.sh
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ patch -p5 < $patches_dir/tlsunique.patch

patch -p5 < $patches_dir/sslkeylogerror.patch

patch -p4 < $patches_dir/recordlayerversion.patch
patch -p5 < $patches_dir/recordlayerversion.patch

patch -p5 < $patches_dir/sslprotocolvariant.patch

patch -p5 < $patches_dir/renegoclientversion.patch
70 changes: 40 additions & 30 deletions net/third_party/nss/patches/recordlayerversion.patch
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
Index: net/third_party/nss/ssl/sslimpl.h
Index: mozilla/security/nss/lib/ssl/sslimpl.h
===================================================================
--- net/third_party/nss/ssl/sslimpl.h (revision 146623)
+++ net/third_party/nss/ssl/sslimpl.h (working copy)
@@ -294,6 +294,8 @@
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v
retrieving revision 1.106
diff -u -p -r1.106 sslimpl.h
--- mozilla/security/nss/lib/ssl/sslimpl.h 14 Jun 2012 19:03:29 -0000 1.106
+++ mozilla/security/nss/lib/ssl/sslimpl.h 17 Aug 2012 02:10:02 -0000
@@ -251,6 +251,8 @@ struct sslSocketOpsStr {
#define ssl_SEND_FLAG_NO_BUFFER 0x20000000
#define ssl_SEND_FLAG_USE_EPOCH 0x10000000 /* DTLS only */
#define ssl_SEND_FLAG_NO_RETRANSMIT 0x08000000 /* DTLS only */
Expand All @@ -11,27 +14,30 @@ Index: net/third_party/nss/ssl/sslimpl.h
#define ssl_SEND_FLAG_MASK 0x7f000000

/*
@@ -1414,6 +1416,7 @@
@@ -1327,6 +1329,7 @@ extern SECStatus
ssl3_CompressMACEncryptRecord(ssl3CipherSpec * cwSpec,
PRBool isServer,
PRBool isDTLS,
+ PRBool capRecordVersion,
SSL3ContentType type,
const SSL3Opaque * pIn,
PRUint32 contentLen,
Index: net/third_party/nss/ssl/ssl3con.c
Index: mozilla/security/nss/lib/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c (revision 146623)
+++ net/third_party/nss/ssl/ssl3con.c (working copy)
@@ -2057,6 +2057,7 @@
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v
retrieving revision 1.186
diff -u -p -r1.186 ssl3con.c
--- mozilla/security/nss/lib/ssl/ssl3con.c 30 Jul 2012 00:47:36 -0000 1.186
+++ mozilla/security/nss/lib/ssl/ssl3con.c 17 Aug 2012 02:10:02 -0000
@@ -2060,6 +2060,7 @@ SECStatus
ssl3_CompressMACEncryptRecord(ssl3CipherSpec * cwSpec,
PRBool isServer,
PRBool isDTLS,
+ PRBool capRecordVersion,
SSL3ContentType type,
const SSL3Opaque * pIn,
PRUint32 contentLen,
@@ -2216,8 +2217,13 @@
@@ -2219,8 +2220,13 @@ ssl3_CompressMACEncryptRecord(ssl3Cipher
wrBuf->buf[11] = MSB(cipherBytes);
wrBuf->buf[12] = LSB(cipherBytes);
} else {
Expand All @@ -47,7 +53,7 @@ Index: net/third_party/nss/ssl/ssl3con.c
wrBuf->buf[3] = MSB(cipherBytes);
wrBuf->buf[4] = LSB(cipherBytes);
}
@@ -2247,7 +2253,14 @@
@@ -2250,7 +2256,14 @@ ssl3_CompressMACEncryptRecord(ssl3Cipher
* all ciphertext into the pending ciphertext buffer.
* ssl_SEND_FLAG_USE_EPOCH (for DTLS)
* Forces the use of the provided epoch
Expand All @@ -63,32 +69,33 @@ Index: net/third_party/nss/ssl/ssl3con.c
*/
PRInt32
ssl3_SendRecord( sslSocket * ss,
@@ -2260,6 +2273,7 @@
@@ -2263,6 +2276,7 @@ ssl3_SendRecord( sslSocket * ss
sslBuffer * wrBuf = &ss->sec.writeBuf;
SECStatus rv;
PRInt32 totalSent = 0;
+ PRBool capRecordVersion;

SSL_TRC(3, ("%d: SSL3[%d] SendRecord type: %s nIn=%d",
SSL_GETPID(), ss->fd, ssl3_DecodeContentType(type),
@@ -2268,6 +2282,16 @@
@@ -2271,6 +2285,17 @@ ssl3_SendRecord( sslSocket * ss

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
@@ -2324,7 +2348,8 @@
@@ -2327,7 +2352,8 @@ ssl3_SendRecord( sslSocket * ss

rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
ss->sec.isServer, IS_DTLS(ss),
Expand All @@ -98,7 +105,7 @@ Index: net/third_party/nss/ssl/ssl3con.c
if (rv != SECSuccess)
goto spec_locked_loser;

@@ -2337,7 +2362,8 @@
@@ -2340,7 +2366,8 @@ ssl3_SendRecord( sslSocket * ss

rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
ss->sec.isServer, IS_DTLS(ss),
Expand All @@ -108,15 +115,15 @@ Index: net/third_party/nss/ssl/ssl3con.c
&secondRecord);
if (rv == SECSuccess) {
PRINT_BUF(50, (ss, "send (encrypted) record data [2/2]:",
@@ -2349,6 +2375,7 @@
@@ -2352,6 +2379,7 @@ ssl3_SendRecord( sslSocket * ss
rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
ss->sec.isServer,
IS_DTLS(ss),
+ capRecordVersion,
type, pIn,
contentLen, wrBuf);
} else {
@@ -2560,6 +2587,8 @@
@@ -2563,6 +2591,8 @@ ssl3_FlushHandshake(sslSocket *ss, PRInt
static SECStatus
ssl3_FlushHandshakeMessages(sslSocket *ss, PRInt32 flags)
{
Expand All @@ -125,7 +132,7 @@ Index: net/third_party/nss/ssl/ssl3con.c
PRInt32 rv = SECSuccess;

PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
@@ -2568,9 +2597,9 @@
@@ -2571,9 +2601,9 @@ ssl3_FlushHandshakeMessages(sslSocket *s
if (!ss->sec.ci.sendBuf.buf || !ss->sec.ci.sendBuf.len)
return rv;

Expand All @@ -138,43 +145,46 @@ Index: net/third_party/nss/ssl/ssl3con.c
PORT_SetError(SEC_ERROR_INVALID_ARGS);
rv = SECFailure;
} else {
@@ -3981,8 +4010,10 @@
@@ -4000,8 +4030,10 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
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));
@@ -4070,6 +4101,7 @@
@@ -4090,6 +4122,7 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
}

if (sid) {
+ serverVersionKnown = PR_TRUE;
+ requestingResume = PR_TRUE;
SSL_AtomicIncrementLong(& ssl3stats.sch_sid_cache_hits );

/* Are we attempting a stateless session resume? */
@@ -4305,7 +4337,11 @@
@@ -4325,7 +4358,11 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
ssl_renegotiation_info_xtn;
}

- rv = ssl3_FlushHandshake(ss, 0);
+ 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 */
}
Index: net/third_party/nss/ssl/dtls1con.c
Index: mozilla/security/nss/lib/ssl/dtlscon.c
===================================================================
--- net/third_party/nss/ssl/dtls1con.c (revision 146623)
+++ net/third_party/nss/ssl/dtls1con.c (working copy)
@@ -834,7 +834,8 @@
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v
retrieving revision 1.3
diff -u -p -r1.3 dtlscon.c
--- mozilla/security/nss/lib/ssl/dtlscon.c 4 Jul 2012 15:21:47 -0000 1.3
+++ mozilla/security/nss/lib/ssl/dtlscon.c 17 Aug 2012 02:10:02 -0000
@@ -802,7 +802,8 @@ dtls_CompressMACEncryptRecord(sslSocket

if (cwSpec) {
rv = ssl3_CompressMACEncryptRecord(cwSpec, ss->sec.isServer, PR_TRUE,
Expand Down
114 changes: 114 additions & 0 deletions net/third_party/nss/patches/renegoclientversion.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
Index: mozilla/security/nss/lib/ssl/ssl3con.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v
retrieving revision 1.186
diff -u -p -r1.186 ssl3con.c
--- mozilla/security/nss/lib/ssl/ssl3con.c 30 Jul 2012 00:47:36 -0000 1.186
+++ mozilla/security/nss/lib/ssl/ssl3con.c 17 Aug 2012 02:23:29 -0000
@@ -4028,6 +4028,23 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
return rv;
}

+ /*
+ * During a renegotiation, ss->clientHelloVersion will be used again to
+ * work around a Windows SChannel bug. Ensure that it is still enabled.
+ */
+ if (ss->firstHsDone) {
+ if (SSL3_ALL_VERSIONS_DISABLED(&ss->vrange)) {
+ PORT_SetError(SSL_ERROR_SSL_DISABLED);
+ return SECFailure;
+ }
+
+ if (ss->clientHelloVersion < ss->vrange.min ||
+ ss->clientHelloVersion > ss->vrange.max) {
+ PORT_SetError(SSL_ERROR_NO_CYPHER_OVERLAP);
+ return SECFailure;
+ }
+ }
+
/* We ignore ss->sec.ci.sid here, and use ssl_Lookup because Lookup
* handles expired entries and other details.
* XXX If we've been called from ssl2_BeginClientHandshake, then
@@ -4075,9 +4092,41 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
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) {
+ ss->version = ss->clientHelloVersion;
+ } else {
+ sidOK = PR_FALSE;
+ }
+ } else {
+ if (ssl3_NegotiateVersion(ss, sid->version,
+ PR_FALSE) != SECSuccess) {
+ sidOK = PR_FALSE;
+ }
+ }
}

if (!sidOK) {
@@ -4104,10 +4153,22 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
} 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) {
@@ -4207,6 +4268,10 @@ ssl3_SendClientHello(sslSocket *ss, PRBo
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;
Loading

0 comments on commit 9808588

Please sign in to comment.