Don't resend payload after Snap Start misprediction.

Don't resend payload after Snap Start misprediction.

The Snap Start code in NSS worked like the prototype implementation in
tlsclient. This had the library take care of resending the application
data in the event of a mispredict. However, that was safe because it did
certificate verification as the message was received.

However, in Chrome, it's possible that a mispredict could be triggered by
the server having a different certificate and NSS would resend the
application data before Chrome verified the certificate.

This change removes that behaviour from NSS and makes the retransmission
the job of ssl_client_socket_nss.cc.

BUG=none
TEST=none

[wtc, 2010-10-28 22:09:04]

The code in ssl_client_socket_nss.cc is very subtle, so I
reviewed it carefully.  Please bear with me, as I think it's
important to get this right.

I'd like to see how you address my concern first.  Please
upload a new Patch Set after you make changes.  Thanks.

http://codereview.chromium.org/4213003/diff/2001/3001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2267
net/socket/ssl_client_socket_nss.cc:2267: // Likewise, if we merged a Write call
into the handshake we need to make the
Nit: I no longer know what this "Likewise" is referring to.
I suspect it's referring to the code at lines 1282-1285 above.

Perhaps we should just remove the word.

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2275
net/socket/ssl_client_socket_nss.cc:2275: if (result == OK &&
Nit: we only need to call SSL_GetSnapStartResult when
result == OK.  Since result == OK is the common case, this
may not be worth fixing.

IMPORTANT: It seems that if 'result' is not OK, we will need
to call
    DoWriteCallback(result);
to cause the merged Write call to fail asynchronously.
Otherwise, the merged Write call will get a successful
asynchronous completion callback due to the
DoWriteCallback(user_write_buf_len_) call on line 2289.

Then, we need to update http_network_transaction.cc
(or some other upper layer classes) to handle certificate
errors from the first socket->Write() call...

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2286
net/socket/ssl_client_socket_nss.cc:2286: int r = DoPayloadWrite();
Nit: r => rv_write or bytes_written

It seems that we don't need to DCHECK_EQ(r, user_write_buf_len_).
Instead, we can say
    int rv_write = DoPayloadWrite();
    if (rv_write != ERR_IO_PENDING)
      DoWriteCallback(rv_write);
    return result;

http://codereview.chromium.org/4213003/diff/2001/3002
File net/third_party/nss/ssl/snapstart.c (right):

http://codereview.chromium.org/4213003/diff/2001/3002#newcode821
net/third_party/nss/ssl/snapstart.c:821: ss->ssl3.snapStartApplicationData.data
= NULL;
Remove this line.  SECITEM_FreeItem sets |data| to NULL and
|len| to 0 after freeing |data|:

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

[agl, 2010-10-28 22:15:20]

On Thu, Oct 28, 2010 at 6:09 PM,  <wtc@chromium.org> wrote:
> I'd like to see how you address my concern first.  Please
> upload a new Patch Set after you make changes.  Thanks.

Thanks! Just a note to say that I'll be on vacation until Tue or Wed next week.


Cheers

AGL

[agl, 2010-11-02 19:26:26]

http://codereview.chromium.org/4213003/diff/2001/3001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2267
net/socket/ssl_client_socket_nss.cc:2267: // Likewise, if we merged a Write call
into the handshake we need to make the
On 2010/10/28 22:09:04, wtc wrote:
> Perhaps we should just remove the word.

Removed.

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2275
net/socket/ssl_client_socket_nss.cc:2275: if (result == OK &&
On 2010/10/28 22:09:04, wtc wrote:
> Nit: we only need to call SSL_GetSnapStartResult when
> result == OK.  Since result == OK is the common case, this
> may not be worth fixing.

I didn't think that the extra level of indentation was worthwhile, but I've
added it in case it makes the code clearer.

> IMPORTANT: It seems that if 'result' is not OK, we will need
> to call
>     DoWriteCallback(result);
> to cause the merged Write call to fail asynchronously.
> Otherwise, the merged Write call will get a successful
> asynchronous completion callback due to the
> DoWriteCallback(user_write_buf_len_) call on line 2289.

Yep, that's a good point. Have done, thanks.

> Then, we need to update http_network_transaction.cc
> (or some other upper layer classes) to handle certificate
> errors from the first socket->Write() call...

Indeed. I'm omitting this for now because I'll probably need willchan's help on
it. At the moment, --enable-snap-start disables all certificate validation
anyway so it's the lesser of two evils. But we will need to remember that Snap
Start is dangerous until that's done.

http://codereview.chromium.org/4213003/diff/2001/3001#newcode2286
net/socket/ssl_client_socket_nss.cc:2286: int r = DoPayloadWrite();
On 2010/10/28 22:09:04, wtc wrote:
> Nit: r => rv_write or bytes_written
> 
> It seems that we don't need to DCHECK_EQ(r, user_write_buf_len_).
> Instead, we can say
>     int rv_write = DoPayloadWrite();
>     if (rv_write != ERR_IO_PENDING)
>       DoWriteCallback(rv_write);
>     return result;

Done.

http://codereview.chromium.org/4213003/diff/2001/3002
File net/third_party/nss/ssl/snapstart.c (right):

http://codereview.chromium.org/4213003/diff/2001/3002#newcode821
net/third_party/nss/ssl/snapstart.c:821: ss->ssl3.snapStartApplicationData.data
= NULL;
On 2010/10/28 22:09:04, wtc wrote:
> Remove this line.  SECITEM_FreeItem sets |data| to NULL and
> |len| to 0 after freeing |data|:
> 
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/sec...

Done.

[wtc, 2010-11-02 22:29:17]

LGTM.  Note the important issue below.  Did you test this CL?

http://codereview.chromium.org/4213003/diff/9001/10001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4213003/diff/9001/10001#newcode2289
net/socket/ssl_client_socket_nss.cc:2289: }
IMPORTANT: I believe we need to add 'else' here, at line 2289, for the
snap-start correct prediction case:
      } else {
        DoWriteCallback(user_write_buf_len_);
      }

Do you agree?

Basically, inside the if (user_write_callback_) statement that starts on line
2270, we should always call DoWriteCallback except when the
DoPayloadWrite call on line 2286 returns ERR_IO_PENDING.