Split RC4-encrypted records.

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c	(revision 185536)
+++ net/third_party/nss/ssl/ssl3con.c	(working copy)
@@ -2188,6 +2188,11 @@
 }
 
 /* Caller must hold the spec read lock. */
+/* The function uses the three members of wrBuf as follows:
+ * - Reads wrBuf->buf and wrBuf->space. Note that it does not read wrBuf->len.
+ * - Writes data into the buffer that wrBuf->buf points to.
+ * - Sets wrBuf->len to the number of bytes written on successful return.
+ */
 SECStatus
 ssl3_CompressMACEncryptRecord(ssl3CipherSpec *   cwSpec,
 		              PRBool             isServer,
@@ -2416,6 +2421,7 @@
     PRINT_BUF(3, (ss, "Send record (plain text)", pIn, nIn));
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss) );
+    PORT_Assert( wrBuf->len == 0 );
 
     capRecordVersion = ((flags & ssl_SEND_FLAG_CAP_RECORD_VERSION) != 0);
 
@@ -2449,7 +2455,7 @@
     while (nIn > 0) {
 	PRUint32  contentLen = PR_MIN(nIn, MAX_FRAGMENT_LENGTH);
 	unsigned int spaceNeeded;
-	unsigned int numRecords;
+	unsigned int numRecords = 1;
 
 	ssl_GetSpecReadLock(ss);    /********************************/
 
@@ -2461,10 +2467,30 @@
 	     * as explained in the documentation for SSL_CBC_RANDOM_IV in ssl.h
 	     */
 	    numRecords = 2;
-	} else {
-	    numRecords = 1;
 	}

[agl, 2013/03/19 16:41:00]

This could be an else if to make it clear that the cases are mutually exclusive.

[wtc, 2013/03/22 01:33:29]

Done.

+	if (nIn > 1 && ss->opt.cbcRandomIV &&
+	    type == content_application_data &&
+	    ss->ssl3.cwSpec->cipher_def->calg == ssl_calg_rc4 &&
+	    ss->ssl3.rc4EncryptedBytes < SSL3_BIASED_RC4_BYTES) {

[wtc, 2013/03/16 01:15:38]

Reminder to self: add a comment to explain how we are splitting
the application data.

+	    PRInt32 inBytes = nIn;
 
+	    numRecords = 0;
+
+	    /* Count how many one-byte records are needed to exhaust the
+	     * biased RC4 keystream bytes. */
+	    while (inBytes > 0 &&
+		   ss->ssl3.rc4EncryptedBytes < SSL3_BIASED_RC4_BYTES) {
+		ss->ssl3.rc4EncryptedBytes += 1 + ss->ssl3.cwSpec->mac_size;

[wtc, 2013/03/16 01:15:38]

It is non-ideal that we are incrementing ss->ssl3.rc4EncryptedBytes
for application data records here and for handshake records
on lines 2552-2556 below. But this avoids repeating the
calculation.

Also, I am too lazy to come up with a branch-free way to
calculate numRecords.

+		inBytes--;
+		numRecords++;
+	    }
+
+	    /* Send any remaining bytes in one record. */
+	    if (inBytes > 0) {
+		numRecords++;
+	    }
+	}
+
 	spaceNeeded = contentLen + (numRecords * SSL3_BUFFER_FUDGE);
 	if (ss->ssl3.cwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_1 &&
 	    ss->ssl3.cwSpec->cipher_def->type == type_block) {
@@ -2479,32 +2505,30 @@
 	    }
 	}
 
-	if (numRecords == 2) {
-	    sslBuffer secondRecord;
+	if (numRecords > 1) {
+	    sslBuffer recordBuf;
+	    unsigned int i;
 
-	    rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
-					       ss->sec.isServer, IS_DTLS(ss),
-					       capRecordVersion, type, pIn,
-					       1, wrBuf);
-	    if (rv != SECSuccess)
-	        goto spec_locked_loser;
+	    wrBuf->len = 0;
+	    for (i = 0; i < numRecords; i++) {
+		recordBuf.buf = wrBuf->buf + wrBuf->len;
+		recordBuf.len = 0;
+		recordBuf.space = wrBuf->space - wrBuf->len;
 
-	    PRINT_BUF(50, (ss, "send (encrypted) record data [1/2]:",
-	                   wrBuf->buf, wrBuf->len));
+		rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
+						   ss->sec.isServer,
+						   IS_DTLS(ss),
+						   capRecordVersion, type,
+						   pIn + i,
+						   (i != numRecords - 1) ?
+						   1 : contentLen - i,
+						   &recordBuf);
+		if (rv != SECSuccess)
+		    break;
 
-	    secondRecord.buf = wrBuf->buf + wrBuf->len;
-	    secondRecord.len = 0;
-	    secondRecord.space = wrBuf->space - wrBuf->len;
-
-	    rv = ssl3_CompressMACEncryptRecord(ss->ssl3.cwSpec,
-	                                       ss->sec.isServer, IS_DTLS(ss),
-					       capRecordVersion, type,
-					       pIn + 1, contentLen - 1,
-	                                       &secondRecord);
-	    if (rv == SECSuccess) {
-	        PRINT_BUF(50, (ss, "send (encrypted) record data [2/2]:",
-	                       secondRecord.buf, secondRecord.len));
-	        wrBuf->len += secondRecord.len;
+		PRINT_BUF(50, (ss, "send (encrypted) record data:",
+			       recordBuf.buf, recordBuf.len));
+		wrBuf->len += recordBuf.len;
 	    }
 	} else {
 	    if (!IS_DTLS(ss)) {
@@ -2524,6 +2548,12 @@
 	    if (rv == SECSuccess) {
 	        PRINT_BUF(50, (ss, "send (encrypted) record data:",
 	                       wrBuf->buf, wrBuf->len));
+
+		if (ss->opt.cbcRandomIV && type == content_handshake &&
+		    ss->ssl3.cwSpec->cipher_def->calg == ssl_calg_rc4) {
+		    ss->ssl3.rc4EncryptedBytes +=
+			wrBuf->len - SSL3_RECORD_HEADER_LENGTH;

[wtc, 2013/03/16 01:15:38]

I could add an output argument to ssl3_CompressMACEncryptRecord
to return the ciphertext length. Here I just subtract
SSL3_RECORD_HEADER_LENGTH from wrBuf->len. I guess
the TLS record header length did not change in TLS 1.2?

[agl, 2013/03/19 16:41:00]

It doesn't. (I don't think it ever could if TLS is to be able to negotiate a
common version with previous implementations.)

+		}
 	    }
 	}
 
@@ -2591,6 +2621,7 @@
 		    /* presumably a memory error, SEC_ERROR_NO_MEMORY */
 		    return SECFailure;
 		}
+		wrBuf->len = 0;	/* All cipher text is saved away. */
 	    }
 	}
 	totalSent += contentLen;
@@ -2621,7 +2652,6 @@
 
     if (ss->pendingBuf.len > SSL3_PENDING_HIGH_WATER &&
         !ssl_SocketIsBlocking(ss)) {
-	PORT_Assert(!ssl_SocketIsBlocking(ss));
 	PORT_SetError(PR_WOULD_BLOCK_ERROR);
 	return SECFailure;
     }
@@ -4188,6 +4218,7 @@
     if (ss->ssl3.hs.cert_status.data) {
 	SECITEM_FreeItem(&ss->ssl3.hs.cert_status, PR_FALSE);
     }
+    ss->ssl3.rc4EncryptedBytes = 0;

[wtc, 2013/03/16 01:15:38]

I found the places where we need to reset
ss->ssl3.rc4EncryptedBytes to 0 by searching for
"PORT_Memset(&ss->xtnData" and the
"We might be starting a session renegotiation ..."
comment.

I believe we only need to reset
ss->ssl3.rc4EncryptedBytes to 0 in ssl3_SendClientHello
and ssl3_HandleClientHello. But to be safe I reset it
whenever we clear ss->xtnData.

[agl, 2013/03/19 16:41:00]

I think this would be taken care of naturally if the encrypted byte count was
part of cwSpec/crSpec. However, I don't think that it's a big difference.

[wtc, 2013/03/19 17:44:02]

That seems like a good idea because rc4EncryptedBytes is
closely related to the write_seq_num field in cwSpec.

This will also allow me to increment rc4EncryptedBytes
in ssl3_CompressMACEncryptRecord. Originally I wanted to
do that, but ssl3_CompressMACEncryptRecord only has access
to 'cwSpec' but not to 'ss', so I can't do that.

[wtc, 2013/03/22 01:33:29]

Done.

 
     SSL_TRC(30,("%d: SSL3[%d]: reset handshake hashes",
 	    SSL_GETPID(), ss->fd ));
@@ -6816,6 +6847,7 @@
      */
     PORT_Memset(&ss->xtnData, 0, sizeof(TLSExtensionData));
     ss->statelessResume = PR_FALSE;
+    ss->ssl3.rc4EncryptedBytes = 0;
 
     rv = ssl3_InitState(ss);
     if (rv != SECSuccess) {
@@ -7575,6 +7607,7 @@
     ssl_GetSSL3HandshakeLock(ss);
 
     PORT_Memset(&ss->xtnData, 0, sizeof(TLSExtensionData));
+    ss->ssl3.rc4EncryptedBytes = 0;
 
     rv = ssl3_InitState(ss);
     if (rv != SECSuccess) {
@@ -10846,7 +10879,9 @@
 #endif
     ssl_ReleaseSpecWriteLock(ss);
 
+    /* XXX: move xtnData into ss->ssl3? */
     PORT_Memset(&ss->xtnData, 0, sizeof(TLSExtensionData));
+    ss->ssl3.rc4EncryptedBytes = 0;
 
     if (IS_DTLS(ss)) {
 	ss->ssl3.hs.sendMessageSeq = 0;