Bug 1286140: HelloRetryRequest

Bug 1286140: HelloRetryRequest

[ekr-rietveld, 2016-08-29 17:21:18]

This is looking pretty good but I'm kind of sad about the null ciphersuite
jiggery-pokery. If we think hannes is right, can we make this less generic
looking (i.e., DTLS only) or perhaps just have DTLS fail

You seem to have introduced a pile of places where * is attached to the variable
not the type. This is the C convention for NSS but not the C++ convention.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_0rtt_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:164: // Remove the old ALPN value
and so the client will not offer ALPN.
This is a bad comment (mine, I think). It should say "early data"

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_damage_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_damage_unittest.cc:58:
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
Why did this change?

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:540: new
TlsExtensionCapture(ssl_tls13_pre_shared_key_xtn);
In C++, the * gets attached to the type.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:77:
!!(hrr_capture->buffer().len()));
len != 0 seems like it might be better here, but not willing to fight about it

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:24: SetupForZeroRtt();
This is a bit hidden, so maybe a comemnt.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_resumption_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_resumption_unittest.cc:332: new
TlsExtensionCapture(ssl_tls13_pre_shared_key_xtn);
Fix * lineup

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:374: DataBuffer* out) const;
* lineup again

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c#newcode5620
lib/ssl/ssl3con.c:5620: return #t;
Let's put this outside of the function just so it's less confusing

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c#newcode6203
lib/ssl/ssl3con.c:6203: }
Note: we won't need this if we do hello finished stuffing

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h#newcode761
lib/ssl/sslimpl.h:761: ssl_0rtt_ignored,
These need comments, please

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h#newcode767
lib/ssl/sslimpl.h:767: ssl_0rtt_ignore_basic,        /* until second flight */
Please improve this comemtn

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode978
lib/ssl/tls13con.c:978: tls13_ServerSetZeroRttState(sslSocket *ss, const
sslSessionID *sid)
Can we get some logging here.

Also, I think perhaps "NegotiateZeroRtt" would match up better with my new code

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode997
lib/ssl/tls13con.c:997: if (ss->opt.enable0RttData && sid &&
Put sid first

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1000
lib/ssl/tls13con.c:1000: PORT_Assert(ss->statelessResume);
Could this go outside the conditional

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1033
lib/ssl/tls13con.c:1033: goto free_sid;
free_sid seems to be misnamed.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1120
lib/ssl/tls13con.c:1120: if (sid) { /* Free the sid. */
Is there a way to have statelessResume and !sid?

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1213
lib/ssl/tls13con.c:1213: 2 +     /* version */
Indent.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1351
lib/ssl/tls13con.c:1351: rv = tls13_ComputeEarlySecrets(ss, zeroRttAccepted);
This seems like an unnecessary temporary. Can we just compute it in the
subroutine call or evaluate it inside.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1480
lib/ssl/tls13con.c:1480: /* Fool me once, shame on you; fool me twice... */
You ddn't like my version? :)

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1731
lib/ssl/tls13con.c:1731: }
Surely we can merge this and the previous lines somehow.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode2622
lib/ssl/tls13con.c:2622: } else if (ss->ssl3.hs.zeroRttState == ssl_0rtt_sent) {
A comment that this stays "sent" would be useful

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3159
lib/ssl/tls13con.c:3159: tls13_SetNullCipherSpec(ss, &ss->ssl3.cwSpec);
This seems hideous. If we're just screwing with the epoch anyway per HAnnes, can
we do something else here. or alternately, just set the epoch.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3831
lib/ssl/tls13con.c:3831: * though 0-RTT didn't happen. TODO: work out if this is
the best plan. */
I feel like the answer is "no" :)

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3877
lib/ssl/tls13con.c:3877: tls13_SetNullCipherSpec(ss, &ss->ssl3.crSpec);
Ugh #2

[mt, 2016-09-01 12:51:30]

Fixed, mostly

[ekr-rietveld, 2016-09-02 01:21:09]

I didn't see your comments on this bug, so I don't know what "Fixed, Mostly"
means. Provided none of the answers to my comments below reveal something
serious, LGTM.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_damage_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_damage_unittest.cc:58:
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Why did this change?

I don't see a response here.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:24: SetupForZeroRtt();
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> This is a bit hidden, so maybe a comemnt.

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c#newcode5620
lib/ssl/ssl3con.c:5620: return #t;
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Let's put this outside of the function just so it's less confusing
thanks

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1000
lib/ssl/tls13con.c:1000: PORT_Assert(ss->statelessResume);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Could this go outside the conditional

I don't see a response here. Not a big deal, but...

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1120
lib/ssl/tls13con.c:1120: if (sid) { /* Free the sid. */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Is there a way to have statelessResume and !sid?

Can you answer this?

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1213
lib/ssl/tls13con.c:1213: 2 +     /* version */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Indent.

Still looks busted.

https://codereview.appspot.com/301660044/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_drop_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_drop_unittest.cc:68: // This simulates a huge
number of drops on one side.
Assuming all of these are rebase artifacts.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/dtlscon.c#newcode1
lib/ssl/dtlscon.c:1: /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil;
c-basic-offset: 4 -*- */
This does not seem right.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c#newcode302
lib/ssl/ssl3con.c:302: #define MR_LOW (1ULL << 20)    /* For weak ciphers. */
They're not necessarily weak, just unsafe for lots of use.n

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c#newcode349
lib/ssl/ssl3con.c:349: #undef MR_LOW
I wouldn't bother with this.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/sslerr.h#newcode241
lib/ssl/sslerr.h:241: SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST =
(SSL_ERROR_BASE + 155),
Alert, this looks bad in Rietveld. Haven't checked the real code.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1159: sid = NULL;
Is this just belt and suspenders?

[mt, 2016-09-02 14:42:30]

Damn I'm not liking rietveld very much right now.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_0rtt_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:164: // Remove the old ALPN value
and so the client will not offer ALPN.
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> This is a bad comment (mine, I think). It should say "early data"

Done.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_damage_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_damage_unittest.cc:58:
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Why did this change?

In the old code, the server was able to read the alert that the client sent
because it sent the alert with the handshake traffic keys and the server was
expecting to see those keys.  Now that I pushed the cipher spec changes in the
client back to where it sends its second round, all the alert sending (which has
to happen while in "read mode" rather than "write mode" due to the arrangement
of locks), happens while the client is still writing cleartext. 

I could fix this, but it would mean having two different places where we set the
handshake cipher spec depending on whether we were doing 0-RTT or not.  That
would mean for the client:

 - with 0-RTT, we would send alerts with the 0-RTT keys and defer switching
cipher spec until we have read all the server's flight.
 - without 0-RTT, we would set the handshake cipher spec after receiving the
ServerHello.

(That this was a single tweak suggests that we don't have a lot of testing of
failures on the client's second round.  Maybe we need to work more on that.)

I decided not to complicate things further by setting cipher specs in two
places, but I'm less sure about it now, alerts are sort-of nice to be able to
read... WDYT?

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:540: new
TlsExtensionCapture(ssl_tls13_pre_shared_key_xtn);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> In C++, the * gets attached to the type.

Done.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:77:
!!(hrr_capture->buffer().len()));
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> len != 0 seems like it might be better here, but not willing to fight about it

Done.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:24: SetupForZeroRtt();
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> This is a bit hidden, so maybe a comemnt.

Done.

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:374: DataBuffer* out) const;
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> * lineup again

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c#newcode5620
lib/ssl/ssl3con.c:5620: return #t;
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Let's put this outside of the function just so it's less confusing

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/ssl3con.c#newcode6203
lib/ssl/ssl3con.c:6203: }
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Note: we won't need this if we do hello finished stuffing

Yes, I realized that and I'm much, much in favour of stuffing.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h#newcode761
lib/ssl/sslimpl.h:761: ssl_0rtt_ignored,
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> These need comments, please

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/sslimpl.h#newcode767
lib/ssl/sslimpl.h:767: ssl_0rtt_ignore_basic,        /* until second flight */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Please improve this comemtn

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode978
lib/ssl/tls13con.c:978: tls13_ServerSetZeroRttState(sslSocket *ss, const
sslSessionID *sid)
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Can we get some logging here.
> 
> Also, I think perhaps "NegotiateZeroRtt" would match up better with my new
code

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode997
lib/ssl/tls13con.c:997: if (ss->opt.enable0RttData && sid &&
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Put sid first

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1000
lib/ssl/tls13con.c:1000: PORT_Assert(ss->statelessResume);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Could this go outside the conditional

No, there are cases where we don't have stateless resumption for other reasons
(expired ticket, etc..., but we've got 0-RTT data inbound.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1033
lib/ssl/tls13con.c:1033: goto free_sid;
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> free_sid seems to be misnamed.

You mean it's really a loser block.  Yeah.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1120
lib/ssl/tls13con.c:1120: if (sid) { /* Free the sid. */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Is there a way to have statelessResume and !sid?

No.  It should always be the case that ending up with statelessResume being true
is a subset of the paths where sid is initially set.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1213
lib/ssl/tls13con.c:1213: 2 +     /* version */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Indent.

Done.  (Forgot to clang-format, bleargh).

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1351
lib/ssl/tls13con.c:1351: rv = tls13_ComputeEarlySecrets(ss, zeroRttAccepted);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> This seems like an unnecessary temporary. Can we just compute it in the
> subroutine call or evaluate it inside.

Done.  It fouls up the line wrapping though.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1480
lib/ssl/tls13con.c:1480: /* Fool me once, shame on you; fool me twice... */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> You ddn't like my version? :)

Bush is the best...

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1731
lib/ssl/tls13con.c:1731: }
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Surely we can merge this and the previous lines somehow.

I'll give you an alternative.  It's hard to preserve the error code correctly
though.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode2622
lib/ssl/tls13con.c:2622: } else if (ss->ssl3.hs.zeroRttState == ssl_0rtt_sent) {
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> A comment that this stays "sent" would be useful

Done.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3159
lib/ssl/tls13con.c:3159: tls13_SetNullCipherSpec(ss, &ss->ssl3.cwSpec);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> This seems hideous. If we're just screwing with the epoch anyway per HAnnes,
can
> we do something else here. or alternately, just set the epoch.

There is more than just the epoch, you have to manage the sequence numbers as
well.  It's horrible, but I would rather leave this working and then do a
wholesale switch to Hannes' scheme in a follow-up.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3831
lib/ssl/tls13con.c:3831: * though 0-RTT didn't happen. TODO: work out if this is
the best plan. */
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> I feel like the answer is "no" :)

Agreed.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode3877
lib/ssl/tls13con.c:3877: tls13_SetNullCipherSpec(ss, &ss->ssl3.crSpec);
On 2016/08/29 17:21:19, ekr-rietveld wrote:
> Ugh #2

Yessir.

[mt, 2016-09-09 01:28:46]

I have a couple of things that I'd like to get some feedback on before landing
this.

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/1/lib/ssl/tls13con.c#newcode1213
lib/ssl/tls13con.c:1213: 2 +     /* version */
On 2016/09/02 01:21:09, ekr-rietveld wrote:
> On 2016/08/29 17:21:19, ekr-rietveld wrote:
> > Indent.
> 
> Still looks busted.

Clang-format ftw.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c#newcode302
lib/ssl/ssl3con.c:302: #define MR_LOW (1ULL << 20)    /* For weak ciphers. */
On 2016/09/02 01:21:09, ekr-rietveld wrote:
> They're not necessarily weak, just unsafe for lots of use.n

Acknowledged.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/ssl3con.c#newcode349
lib/ssl/ssl3con.c:349: #undef MR_LOW
On 2016/09/02 01:21:09, ekr-rietveld wrote:
> I wouldn't bother with this.

Done.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/sslerr.h#newcode241
lib/ssl/sslerr.h:241: SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST =
(SSL_ERROR_BASE + 155),
On 2016/09/02 01:21:09, ekr-rietveld wrote:
> Alert, this looks bad in Rietveld. Haven't checked the real code.

Done.

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/301660044/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1159: sid = NULL;
On 2016/09/02 01:21:09, ekr-rietveld wrote:
> Is this just belt and suspenders?

Yes.  It prevents a double-free if someone decides to use loser in the code
below this line (I'm in two minds about whether to convert the failure returns
below...)

[mt, 2016-09-09 08:14:52]

Small tweaks

[ekr-rietveld, 2016-09-10 18:13:50]

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_damage_unittest.cc (right):

https://codereview.appspot.com/301660044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_damage_unittest.cc:58:
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
On 2016/09/02 14:42:29, mt wrote:
> On 2016/08/29 17:21:19, ekr-rietveld wrote:
> > Why did this change?
> 
> In the old code, the server was able to read the alert that the client sent
> because it sent the alert with the handshake traffic keys and the server was
> expecting to see those keys.  Now that I pushed the cipher spec changes in the
> client back to where it sends its second round, all the alert sending (which
has
> to happen while in "read mode" rather than "write mode" due to the arrangement
> of locks), happens while the client is still writing cleartext. 
> 
> I could fix this, but it would mean having two different places where we set
the
> handshake cipher spec depending on whether we were doing 0-RTT or not.  That
> would mean for the client:
> 
>  - with 0-RTT, we would send alerts with the 0-RTT keys and defer switching
> cipher spec until we have read all the server's flight.
>  - without 0-RTT, we would set the handshake cipher spec after receiving the
> ServerHello.
> 
> (That this was a single tweak suggests that we don't have a lot of testing of
> failures on the client's second round.  Maybe we need to work more on that.)
> 
> I decided not to complicate things further by setting cipher specs in two
> places, but I'm less sure about it now, alerts are sort-of nice to be able to
> read... WDYT?

I think it's fine for now.