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)
@@ -2416,6 +2416,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 +2450,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 +2462,26 @@
 	     * as explained in the documentation for SSL_CBC_RANDOM_IV in ssl.h
 	     */
 	    numRecords = 2;
-	} else {
-	    numRecords = 1;
 	}
+	if (nIn > 1 && ss->opt.cbcRandomIV &&

[agl, 2013/03/15 12:35:21]

Unlike the CBC case, we only need to split RC4 records at the beginning of the
connection, but I think this code will split all writes currently.

Also, do we wish to handle the case where an application makes multiple, short
writes at the beginning of the connection? (e.g. "GET ", followed by the URL,
followed by " HTTP/1.1")

In that case, we might want to keep track of how many keystream bytes we have
burnt over several writes. Since this is additional bookkeeping in the
connection struct, it may not be worthwhile.

Lastly, and most obscurely, this is technically a problem after a renegotiation
because the cipher gets rekeyed. However, imagining Javascript that's triggering
a renegotiation is tough and I feel that worrying about that might be too much
unless it's very easy to account for.

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

This code only splits the first RC4 application data record.
I use a bodge to do that, by setting ss->opt.cbcRandomIV to
false, on line 2482 below.
I wasn't planning to go out of my way to do that. But I
realized it isn't hard to do that, so patch set 2 handles
that correctly.
This is exactly the approach I take in patch set 2.  I
also handle renegotiations.

+	    type == content_application_data &&
+	    ss->ssl3.cwSpec->cipher_def->calg == ssl_calg_rc4) {
+	    /* Adjust these parameters. */
+	    enum {
+		BIASED_RC4_BYTES = 256,
+		MIN_RECORD_SIZE = 1 + MD5_LENGTH,
+		MAX_SPLIT_RECORDS =
+		    (BIASED_RC4_BYTES + MIN_RECORD_SIZE - 1) / MIN_RECORD_SIZE

[wtc, 2013/03/15 03:58:18]

Here I calculate a crude estimate of how many one-byte
records we need.  This needs to be fine tuned.

[agl, 2013/03/15 12:35:21]

A couple of ideas for tuning:

Obviously the MAC may be larger for a given connection. Additionally, we get a
free handshake protocol header, verify_data and MAC from the Finished record.

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

Done. With Google sites we also get NextProtocol and
EncryptedExtensions. With Twitter and Facebook sites we also
get NextProtocol (because they do SPDY/NPN).

+	    };
 
+	    numRecords = PR_MIN(nIn, MAX_SPLIT_RECORDS);
+	    /* Split only the first application data record.
+	     * NOTE: turning off ss->opt.cbcRandomIV is a hack.  We should
+	     * add a new boolean field to |ss|.
+	     */
+	    ss->opt.cbcRandomIV = PR_FALSE;
+	}
+
 	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 +2496,29 @@
 	    }
 	}
 
-	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;
+	    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;
 	    }

[wtc, 2013/03/15 03:58:18]

I generalized this code to be able to split the application
data into any number of records.

 	} else {
 	    if (!IS_DTLS(ss)) {
@@ -2591,6 +2605,7 @@
 		    /* presumably a memory error, SEC_ERROR_NO_MEMORY */
 		    return SECFailure;
 		}
+		wrBuf->len = 0;	/* All cipher text is saved away. */
 	    }
 	}
 	totalSent += contentLen;
@@ -2621,7 +2636,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;
     }