net: Precede each CBC encrypted application data record with an empty one.

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
diff --git a/net/third_party/nss/ssl/ssl3con.c b/net/third_party/nss/ssl/ssl3con.c
index 2a564ace634e4a022b0a7f02dcc37cf4d25dc49a..8189d255793aa31901bbbce4cb6beed4576f4359 100644
--- a/net/third_party/nss/ssl/ssl3con.c
+++ b/net/third_party/nss/ssl/ssl3con.c
@@ -1914,7 +1914,9 @@ ssl3_ComputeRecordMAC(
 	                     : spec->client.write_mac_context);
 	rv  = PK11_DigestBegin(mac_context);
 	rv |= PK11_DigestOp(mac_context, temp, tempLen);
-	rv |= PK11_DigestOp(mac_context, input, inputLength);
+	if (inputLength > 0) {
+	    rv |= PK11_DigestOp(mac_context, input, inputLength);
+	}

[wtc, 2011/06/23 23:58:33]

It may be safe to pass inputLength=0 to PK11_DigestOp.
Did you get a crash without this check?  Or did you get a
failure because of this check in PK11_DigestOp?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap...

If the latter, you should be able to work around this check
by passing 'in' instead of NULL to the ssl3_SendRecord call
on line 2361.

Note: I think PK11_CipherOp should check
    if (!in && inLen)
instead of just
    if (!in)

[agl, 2011/06/24 18:50:18]

Done

 	rv |= PK11_DigestFinal(mac_context, outbuf, outLength, spec->mac_size);
     } else {
 	/* bypass version */
@@ -2229,7 +2231,7 @@ ssl3_SendRecord(   sslSocket *        ss,
 	return SECFailure;
     }
 
-    while (nIn > 0) {
+    do {
 	PRUint32  contentLen = PR_MIN(nIn, MAX_FRAGMENT_LENGTH);
 
 	if (wrBuf->space < contentLen + SSL3_BUFFER_FUDGE) {
@@ -2306,7 +2308,7 @@ ssl3_SendRecord(   sslSocket *        ss,
 	    }
 	}
 	totalSent += contentLen;
-    }
+    } while (nIn > 0);
     return totalSent;
 }
 
@@ -2321,6 +2323,7 @@ ssl3_SendApplicationData(sslSocket *ss, const unsigned char *in,
 {
     PRInt32   totalSent	= 0;
     PRInt32   discarded = 0;
+    PRBool    is_block_cipher;

[wtc, 2011/06/23 23:58:33]

Nit: name the variable isBlockCipher.

[agl, 2011/06/24 18:50:18]

Done.

 
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss) );
     if (len < 0 || !in) {
@@ -2345,6 +2348,22 @@ ssl3_SendApplicationData(sslSocket *ss, const unsigned char *in,
 	len--;
 	discarded = 1;
     }
+
+    ssl_GetSpecReadLock(ss);
+    is_block_cipher = ss->ssl3.cwSpec->cipher_def->type == type_block;
+    ssl_ReleaseSpecReadLock(ss);
+
+    if (is_block_cipher) {

[wtc, 2011/06/23 23:58:33]

We should also test if len > 0.

[agl, 2011/06/24 18:50:18]

Done.

+	// We assume that block ciphers are used in CBC mode and prepend an
+	// empty record. This effectively randomizes the IV in a backwards
+	// compatible way.
+	PRInt32 sent = ssl3_SendRecord(ss, content_application_data,
+				       NULL, 0 /* no payload */, flags);
+	if (sent < 0) {
+	    return SECFailure; /* error code set by ssl3_SendRecord */
+	}

[wtc, 2011/06/23 23:58:33]

I think we also need to duplicate the code from lines
2392-2397:

        if (ss->pendingBuf.len) {
            /* must be a non-blocking socket */
            PORT_Assert(!ssl_SocketIsBlocking(ss));
            PORT_Assert(ss->lastWriteBlocked);
            goto LINE 2399;
        }

[agl, 2011/06/24 18:50:18]

I didn't do this. Here's my reasoning:

The code in 2399 is a no-op. totalSent = 0 and discarded = 1. The only thing
that'll happen is that PR_WOULD_BLOCK_ERROR/SECFailure is returned.

But that will happen anyway, we'll fall into the while loop at 2367 and the
first write will be buffered and we'll hit that code anyway.

None the less, I'll admit that might be a false economy so I've added a goto.

+    }
+
     while (len > totalSent) {
 	PRInt32   sent, toSend;