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

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

Precede each CBC encrypted application data record with an empty
application data record in order to randomize the IV in a backwards
compatible manner.

BUG=87159
TEST=HTTPS sites continue to work.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90632

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

LGTM.  Please note my comments below.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/README.chro...
File net/third_party/nss/README.chromium (right):

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/README.chro...
net/third_party/nss/README.chromium:46: * Preceed each CBC encrypted application
data record with an empty
Typo: Preceed => Precede

IMPORTANT: the patch does not do exactly what you described
here.  The patch adds an empty application data record for
each call to ssl3_SendApplicationData, which may send
multiple application data records if the data length is
greater than MAX_FRAGMENT_LENGTH.

I believe what the patch does is good enough, and we
should simply change the description here to match the code.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:1919: }
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)

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2072: &outlen, wrBuf->space -
SSL3_RECORD_HEADER_LENGTH, pIn, contentLen);
Please confirm that it is fine to pass pIn=NULL and
contentLen=0 to cwSpec->compressor().

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2326: PRBool    is_block_cipher;
Nit: name the variable isBlockCipher.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2356: if (is_block_cipher) {
We should also test if len > 0.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2364: }
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'd like you to confirm that you're ok with the "if (ss->pendingBuf.len)" stuff
before landing.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/README.chro...
File net/third_party/nss/README.chromium (right):

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/README.chro...
net/third_party/nss/README.chromium:46: * Preceed each CBC encrypted application
data record with an empty
On 2011/06/23 23:58:33, wtc wrote:
> Typo: Preceed => Precede
> 
> IMPORTANT: the patch does not do exactly what you described
> here.  The patch adds an empty application data record for
> each call to ssl3_SendApplicationData, which may send
> multiple application data records if the data length is
> greater than MAX_FRAGMENT_LENGTH.
> 
> I believe what the patch does is good enough, and we
> should simply change the description here to match the code.

Done.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:1919: }
On 2011/06/23 23:58:33, wtc wrote:
> 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.

Done

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2072: &outlen, wrBuf->space -
SSL3_RECORD_HEADER_LENGTH, pIn, contentLen);
On 2011/06/23 23:58:33, wtc wrote:
> Please confirm that it is fine to pass pIn=NULL and
> contentLen=0 to cwSpec->compressor().

Done.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2326: PRBool    is_block_cipher;
On 2011/06/23 23:58:33, wtc wrote:
> Nit: name the variable isBlockCipher.

Done.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2356: if (is_block_cipher) {
On 2011/06/23 23:58:33, wtc wrote:
> We should also test if len > 0.

Done.

http://codereview.chromium.org/7239002/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:2364: }
On 2011/06/23 23:58:33, wtc wrote:
> 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;
>         }

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.

[agl, 2011-06-24 19:04:44]

(appengine's email sending didn't appear to work last time.)

[wtc, 2011-06-27 18:11:55]

LGTM.

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3con.c:2368: }
I suspect what you described will indeed happen if we
simply fall into the while loop below.  But I think it's
better to deal with the if (ss->pendingBuf.len) condition
explicitly; at least it would help me convince myself the
new code is correct.

Because of your comment, I found that given totalSent = 0,
we can simplify it to avoid the goto statement:

        if (ss->pendingBuf.len) {
            /* must be a non-blocking socket */
            PORT_Assert(!ssl_SocketIsBlocking(ss));
            PORT_Assert(ss->lastWriteBlocked);
            PORT_SetError(PR_WOULD_BLOCK_ERROR);
            return SECFailure;
        }

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3con.c:2404: writeBlocked:
Nit: labels are not indented in the NSS source code.

[agl, 2011-06-27 19:05:30]

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3con.c:2368: }
On 2011/06/27 18:11:55, wtc wrote:
> I suspect what you described will indeed happen if we
> simply fall into the while loop below.  But I think it's
> better to deal with the if (ss->pendingBuf.len) condition
> explicitly; at least it would help me convince myself the
> new code is correct.
> 
> Because of your comment, I found that given totalSent = 0,
> we can simplify it to avoid the goto statement:
> 
>         if (ss->pendingBuf.len) {
>             /* must be a non-blocking socket */
>             PORT_Assert(!ssl_SocketIsBlocking(ss));
>             PORT_Assert(ss->lastWriteBlocked);
>             PORT_SetError(PR_WOULD_BLOCK_ERROR);
>             return SECFailure;
>         }

Done.

http://codereview.chromium.org/7239002/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3con.c:2404: writeBlocked:
On 2011/06/27 18:11:55, wtc wrote:
> Nit: labels are not indented in the NSS source code.

(mooted: line removed.)