Move the core state machine of SSLClientSocketNSS into a thread-safe Core

net/third_party/nss/ssl/sslinfo.c

Index: net/third_party/nss/ssl/sslinfo.c
diff --git a/net/third_party/nss/ssl/sslinfo.c b/net/third_party/nss/ssl/sslinfo.c
index 0cb46d6e2f52a2ab06df61bd5de1b1a3b342dc89..6dc3cab16cd948f28513b100a35d7d9ef63de771 100644
--- a/net/third_party/nss/ssl/sslinfo.c
+++ b/net/third_party/nss/ssl/sslinfo.c
@@ -376,8 +376,13 @@ SSL_ExportKeyingMaterial(PRFileDesc *fd,
 	return SECFailure;
     }
 
+    ssl_GetRecvBufLock(ss);

[wtc, 2012/05/30 22:54:29]

The changes to this file should be moved to a separate CL.

Are you sure this function needs to get RecvBufLock?
It doesn't read data from the transport.

[Ryan Sleevi, 2012/05/30 23:20:10]

No, this is important to avoiding the need to modify
SSLClientSocketNSS::ExportKeyingMaterial.
RecvBufLock is the only guard for ss->version, which is meant to be one of the
'atomic longs' but that is dependent on platform and thus not a safe assumption.

SSL3HandshakeLock guards ss->ssl3

[wtc, 2012/05/31 01:23:42]

I studied the functions in sslsecur.c as examples.  They
showed that ss->version seems to be protected by
ssl_Get1stHandshakeLock rather than ssl_GetRecvBufLock.
So I think we should use ssl_Get1stHandshakeLock instead.

We need to add an NSS patch file for these changes.

[Ryan Sleevi, 2012/05/31 01:31:14]

1stHandshakeLock is itself guarded by RecvBufLock. I'll dig through the notes
again, but there were writes to ss->version /before/ 1stHandshakeLock (in
particular, renegotiation, IIRC), hence the broad lock.
Agreed. Wanted to make the changes stick first.

[wtc, 2012/06/01 01:02:38]

1stHandshakeLock is broader than RecvBufLock.

I found places where NSS reads or writes ss->version
while holding only 1stHandshakeLock.  The place where
NSS writes ss->version while holding only 1stHandshakeLock
is in ssl3_SendClientHello, when it copies the version from the
SID structure.

Unless we are modifying ss->version based on info just
received from the peer, we don't need to hold RecvBufLock.

[Ryan Sleevi, 2012/06/04 21:51:50]

ssl3_SendClientHello expects HaveSSL3HandshakeLock.

http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl3con.c?mark=39...

ssl3_HandleV2ClientHello (really only for servers) is the one I was thinking of
that didn't have it locked appropriately, because it originates in the SSL2
layer, and thus only has RecvBuf.

Thus, I think this is the correct locking sequence, even though
SSLServerSocketNSS has (not yet?) been ported to use a ref-counted core.

[wtc, 2012/06/04 23:44:58]

I am not suggesting that we remove the ssl_GetSSL3HandshakeLock
call. I am suggesting that we either remove the
ssl_GetRecvBufLock call or replace the ssl_GetRecvBufLock
call with a ssl_Get1stHandshakeLock call.

The RecvBufLock has to be held during ssl3_HandleV2ClientHello
because ssl3_HandleV2ClientHello is using data in
ss->gs.buf to set ss->version.  The RecvBufLock is
used to protect ss->gs.buf, as documented in this
comment:

   PZMonitor *   recvBufLock;	/* locks low level recv buffers. */

All the ssl3_Handle<HandshakeMessate> functions need to
hold the RecvBufLock for this reason.

I think it is enough for SSL_ExportKeyingMaterial to
hold just the SSL3HandshakeLock.

[Ryan Sleevi, 2012/06/05 00:01:59]

Apologies, but I still don't understand why you're suggesting this.

SSL3HandshakeLock is too narrow by itself (doesn't cover the server case)
1stHandshakeLook is too narrow by itself (doesn't cover renegotiation
handshakes)

The only one that covers the entire state machine is RecvBufLock. As you note,
/all/ ssl[23]_Handle<HandshakeMessage> functions need to hold this lock.

The remaining locking is to ensure that the lock ordering (eg: must hold HS
lock, must hold SpecReadLock) is respected, even though RecvBufLock should cover
all of this.

+    ssl_GetSSL3HandshakeLock(ss);
+
     if (ss->version < SSL_LIBRARY_VERSION_3_1_TLS) {
 	PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
+	ssl_ReleaseSSL3HandshakeLock(ss);
+	ssl_ReleaseRecvBufLock(ss);
 	return SECFailure;
     }
 
@@ -388,13 +393,18 @@ SSL_ExportKeyingMaterial(PRFileDesc *fd,
     }
     val = PORT_Alloc(valLen);
     if (!val) {
+	ssl_ReleaseSSL3HandshakeLock(ss);
+	ssl_ReleaseRecvBufLock(ss);
 	return SECFailure;
     }
     i = 0;
+
     PORT_Memcpy(val + i, &ss->ssl3.hs.client_random.rand, SSL3_RANDOM_LENGTH);
     i += SSL3_RANDOM_LENGTH;
     PORT_Memcpy(val + i, &ss->ssl3.hs.server_random.rand, SSL3_RANDOM_LENGTH);
     i += SSL3_RANDOM_LENGTH;
+    ssl_ReleaseSSL3HandshakeLock(ss);

[wtc, 2012/05/30 22:54:29]

BUG: this function releases SSL3HandshakeLock twice, here
and on line 428.

[Ryan Sleevi, 2012/05/30 23:20:10]

Well spotted.

+
     if (hasContext) {
 	val[i++] = contextLen >> 8;
 	val[i++] = contextLen;
@@ -415,6 +425,8 @@ SSL_ExportKeyingMaterial(PRFileDesc *fd,
 					 valLen, out, outLen);
     }
     ssl_ReleaseSpecReadLock(ss);
+    ssl_ReleaseSSL3HandshakeLock(ss);
+    ssl_ReleaseRecvBufLock(ss);
 
     PORT_ZFree(val, valLen);
     return rv;