TLS 1.3 draft-14 changes

TLS 1.3 draft-14 changes

[ekr-rietveld, 2016-08-13 21:19:19]

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

https://codereview.appspot.com/307030043/diff/1/lib/ssl/ssl3ext.c#newcode3702
lib/ssl/ssl3ext.c:3702: rv = ssl3_AppendHandshakeNumber(ss, 0, 4);
Nice placeholder but should be age.

https://codereview.appspot.com/307030043/diff/1/lib/ssl/ssl3ext.c#newcode3732
lib/ssl/ssl3ext.c:3732: }
Need to read this as block, not an int.

[ekr-rietveld, 2016-08-13 21:24:06]

PTAL. This is missing the HRR changes because they are gated on you.

[mt, 2016-08-15 03:53:50]

I think that this is pretty good.  I have a small suggestion about the alerts.

This should land before the HRR changes, especially if you manage to fix bug
1281249 at the same time.

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:249: SSL3_SendAlert(ss, level,
type);
You should check the return code of this one and return false if it isn't
SECSuccess.

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

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:202:
WAIT_(client_->error_code() == SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT, 2000);
Are the status codes right here?  This one seems like it might also need to be
RX_UNKNOWN_ALERT.

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3855: * treated as fatal. */
Why not generate a different error if the alert is at the wrong level?

That is, after the alert_fatal block and the end_of_early_data blocks below, add
a check:

if (ss->version >= SSL_...1_3 &&
    desc != close_notify &&
    desc != user_canceled) {
    if (!ss->opt.noCache && ss->sec.uncache)
        ss->sec.uncache(ss->sec.ci.sid);
    PORT_SetError(SSL_ERROR_RX_INVALID_WARNING_ALERT);
    return SECFailure;
}

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:7170: SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
We need a PR_STATIC_ASSERT that checks that sizeof(tls13_downgrade_random) ==
sizeof(tls12_downgrade_random)

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8962: * client_version indicating TLS 1.2 or below MUST set
the first eight
s/first/last (and below)

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:11131: NewSessionTicket nticket = { 0 };
note to self, check on this

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3729: /* Obfuscated ticket age. Ignore. */
Can you open a bug where we check this one?  That probably means having a new
public function where we set the timer slack.

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1390: * wrong in draft-13. Bug 1281249. */
We should fix this bug here at the same time since you fixed it.  This will make
the HRR patch much more pleasant.

[mt, 2016-08-15 03:55:09]

Ooops, I forgot: LGTM on condition that you fix Bug 1281249

On 2016/08/15 03:53:50, mt wrote:
> I think that this is pretty good.  I have a small suggestion about the alerts.
> 
> This should land before the HRR changes, especially if you manage to fix bug
> 1281249 at the same time.
> 
>
https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
> File external_tests/ssl_gtest/libssl_internals.c (right):
> 
>
https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/libssl_internals.c:249: SSL3_SendAlert(ss, level,
> type);
> You should check the return code of this one and return false if it isn't
> SECSuccess.
> 
>
https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
> File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):
> 
>
https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_loopback_unittest.cc:202:
> WAIT_(client_->error_code() == SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT, 2000);
> Are the status codes right here?  This one seems like it might also need to be
> RX_UNKNOWN_ALERT.
> 
> https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c
> File lib/ssl/ssl3con.c (right):
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:3855: * treated as fatal. */
> Why not generate a different error if the alert is at the wrong level?
> 
> That is, after the alert_fatal block and the end_of_early_data blocks below,
add
> a check:
> 
> if (ss->version >= SSL_...1_3 &&
>     desc != close_notify &&
>     desc != user_canceled) {
>     if (!ss->opt.noCache && ss->sec.uncache)
>         ss->sec.uncache(ss->sec.ci.sid);
>     PORT_SetError(SSL_ERROR_RX_INVALID_WARNING_ALERT);
>     return SECFailure;
> }
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:7170: SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
> We need a PR_STATIC_ASSERT that checks that sizeof(tls13_downgrade_random) ==
> sizeof(tls12_downgrade_random)
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:8962: * client_version indicating TLS 1.2 or below MUST set
> the first eight
> s/first/last (and below)
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:11131: NewSessionTicket nticket = { 0 };
> note to self, check on this

Checked, didn't delete comment :)

> https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3ext.c
> File lib/ssl/ssl3ext.c (right):
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3ext.c#newcode...
> lib/ssl/ssl3ext.c:3729: /* Obfuscated ticket age. Ignore. */
> Can you open a bug where we check this one?  That probably means having a new
> public function where we set the timer slack.
> 
> https://codereview.appspot.com/307030043/diff/20001/lib/ssl/tls13con.c
> File lib/ssl/tls13con.c (right):
> 
>
https://codereview.appspot.com/307030043/diff/20001/lib/ssl/tls13con.c#newcod...
> lib/ssl/tls13con.c:1390: * wrong in draft-13. Bug 1281249. */
> We should fix this bug here at the same time since you fixed it.  This will
make
> the HRR patch much more pleasant.

[ekr-rietveld, 2016-08-15 04:51:01]

I take it from your comments that you trust me to move the early_data entry in
the Known_Extensions table without review?

[mt, 2016-08-15 06:25:10]

On 2016/08/15 04:51:01, ekr-webrtc wrote:
> I take it from your comments that you trust me to move the early_data entry in
> the Known_Extensions table without review?

Yes, I think that you can handle that :)

[mt, 2016-08-15 06:31:15]

On 2016/08/15 06:25:10, mt wrote:
> On 2016/08/15 04:51:01, ekr-webrtc wrote:
> > I take it from your comments that you trust me to move the early_data entry
in
> > the Known_Extensions table without review?
> 
> Yes, I think that you can handle that :)

Oh, the thought occurred...

You might want to move this:
http://searchfox.org/nss/rev/3abb8f87bfef1efeef110e6fa48fb86e93c3e84d/lib/ssl...

to tls13_SendEncryptedServerSequence() (that's not on searchfox right now, but
it will be soon).

[ekr-rietveld, 2016-08-15 16:01:01]

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:249: SSL3_SendAlert(ss, level,
type);
On 2016/08/15 03:53:50, mt wrote:
> You should check the return code of this one and return false if it isn't
> SECSuccess.

Done.

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

https://codereview.appspot.com/307030043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:202:
WAIT_(client_->error_code() == SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT, 2000);
On 2016/08/15 03:53:50, mt wrote:
> Are the status codes right here?  This one seems like it might also need to be
> RX_UNKNOWN_ALERT.

This reflects the alert the other side sent. We just make it fatal

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3855: * treated as fatal. */
On 2016/08/15 03:53:50, mt wrote:
> Why not generate a different error if the alert is at the wrong level?
> 
> That is, after the alert_fatal block and the end_of_early_data blocks below,
add
> a check:
> 
> if (ss->version >= SSL_...1_3 &&
>     desc != close_notify &&
>     desc != user_canceled) {
>     if (!ss->opt.noCache && ss->sec.uncache)
>         ss->sec.uncache(ss->sec.ci.sid);
>     PORT_SetError(SSL_ERROR_RX_INVALID_WARNING_ALERT);
>     return SECFailure;
> }

I hear you here, but I think this is an inferior result because it hides the
other side's alert.

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:7170: SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
On 2016/08/15 03:53:50, mt wrote:
> We need a PR_STATIC_ASSERT that checks that sizeof(tls13_downgrade_random) ==
> sizeof(tls12_downgrade_random)

Done.

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8962: * client_version indicating TLS 1.2 or below MUST set
the first eight
On 2016/08/15 03:53:50, mt wrote:
> s/first/last (and below)

Done.

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:11131: NewSessionTicket nticket = { 0 };
On 2016/08/15 03:53:50, mt wrote:
> note to self, check on this

Done.

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3729: /* Obfuscated ticket age. Ignore. */
On 2016/08/15 03:53:50, mt wrote:
> Can you open a bug where we check this one?  That probably means having a new
> public function where we set the timer slack.

Done.

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

https://codereview.appspot.com/307030043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1390: * wrong in draft-13. Bug 1281249. */
On 2016/08/15 03:53:50, mt wrote:
> We should fix this bug here at the same time since you fixed it.  This will
make
> the HRR patch much more pleasant.

Done.