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

net/third_party/nss/ssl/ssl3con.c

 /*
  * SSL3 Protocol
  *
  * ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
@@ skipping 1896 matching lines @@
     if (mac_def->mac == mac_null) {
         *outLength = 0;
         return SECSuccess;
     }
     if (! spec->bypassCiphers) {
         PK11Context *mac_context = 
             (useServerMacKey ? spec->server.write_mac_context
                              : 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 */
         const SECHashObject *hashObj = NULL;
         unsigned int       pad_bytes = 0;
         PRUint64           write_mac_context[MAX_MAC_CONTEXT_LLONGS];
 
         switch (mac_def->mac) {
         case ssl_mac_null:
             *outLength = 0;
@@ skipping 132 matching lines @@
 
     ssl_GetSpecReadLock(ss);    /********************************/
 
     cwSpec = ss->ssl3.cwSpec;
     cipher_def = cwSpec->cipher_def;
 
     if (cwSpec->compressor) {
         int outlen;
         rv = cwSpec->compressor(
             cwSpec->compressContext, wrBuf->buf + SSL3_RECORD_HEADER_LENGTH,
             &outlen, wrBuf->space - SSL3_RECORD_HEADER_LENGTH, pIn, contentLen);

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

Please confirm that it is fine to pass pIn=NULL and
contentLen=0 to cwSpec->compressor().

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

Done.

         if (rv != SECSuccess)
             return rv;
         pIn = wrBuf->buf + SSL3_RECORD_HEADER_LENGTH;
         contentLen = outlen;
     }
 
     /*
      * Add the MAC
      */
     rv = ssl3_ComputeRecordMAC( cwSpec, (PRBool)(ss->sec.isServer),
@@ skipping 141 matching lines @@
             return SECFailure;  /* ssl3_InitState has set the error code. */
         }
     }
 
     /* check for Token Presence */
     if (!ssl3_ClientAuthTokenPresent(ss->sec.ci.sid)) {
         PORT_SetError(SSL_ERROR_TOKEN_INSERTION_REMOVAL);
         return SECFailure;
     }
 
-    while (nIn > 0) {
+    do {
         PRUint32  contentLen = PR_MIN(nIn, MAX_FRAGMENT_LENGTH);
 
         if (wrBuf->space < contentLen + SSL3_BUFFER_FUDGE) {
             PRInt32 newSpace = PR_MAX(wrBuf->space * 2, contentLen);
             newSpace = PR_MIN(newSpace, MAX_FRAGMENT_LENGTH);
             newSpace += SSL3_BUFFER_FUDGE;
             rv = sslBuffer_Grow(wrBuf, newSpace);
             if (rv != SECSuccess) {
                 SSL_DBG(("%d: SSL3[%d]: SendRecord, tried to get %d bytes",
                          SSL_GETPID(), ss->fd, newSpace));
@@ skipping 56 matching lines @@
                  * append it to the buffer of previously unsent ciphertext.
                  */
                 rv = ssl_SaveWriteData(ss, wrBuf->buf + sent, wrBuf->len);
                 if (rv != SECSuccess) {
                     /* presumably a memory error, SEC_ERROR_NO_MEMORY */
                     return SECFailure;
                 }
             }
         }
         totalSent += contentLen;
-    }
+    } while (nIn > 0);
     return totalSent;
 }
 
 #define SSL3_PENDING_HIGH_WATER 1024
 
 /* Attempt to send the content of "in" in an SSL application_data record.
  * Returns "len" or SECFailure,   never SECWouldBlock, nor SECSuccess.
  */
 int
 ssl3_SendApplicationData(sslSocket *ss, const unsigned char *in,
                          PRInt32 len, PRInt32 flags)
 {
     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) {
         PORT_SetError(PR_INVALID_ARGUMENT_ERROR);
         return SECFailure;
     }
 
     if (ss->pendingBuf.len > SSL3_PENDING_HIGH_WATER &&
         !ssl_SocketIsBlocking(ss)) {
         PORT_Assert(!ssl_SocketIsBlocking(ss));
         PORT_SetError(PR_WOULD_BLOCK_ERROR);
         return SECFailure;
     }
 
     if (ss->appDataBuffered && len) {
         PORT_Assert (in[0] == (unsigned char)(ss->appDataBuffered));
         if (in[0] != (unsigned char)(ss->appDataBuffered)) {
             PORT_SetError(PR_INVALID_ARGUMENT_ERROR);
             return SECFailure;
         }
         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;
 
         if (totalSent > 0) {
             /*
              * The thread yield is intended to give the reader thread a
              * chance to get some cycles while the writer thread is in
              * the middle of a large application data write.  (See
              * Bugzilla bug 127740, comment #1.)
              */
@@ skipping 7491 matching lines @@
 
     ss->ssl3.initialized = PR_FALSE;
 
     if (ss->ssl3.nextProto.data) {
         PORT_Free(ss->ssl3.nextProto.data);
         ss->ssl3.nextProto.data = NULL;
     }
 }
 
 /* End of ssl3con.c */