draft-13 part 1: New Session Ticket and PSK resumption as index

draft-13 part 1: New Session Ticket and PSK resumption as index

[mt, 2016-05-31 01:08:43]

I found a minor bug, but I think that you should write around it.  Otherwise,
this is good.

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

https://codereview.appspot.com/300200043/diff/1/lib/ssl/ssl3con.c#newcode10863
lib/ssl/ssl3con.c:10863: SECItem ticket_data = { 0, NULL, 0 };
s/ticket_data/ticket/ ?  Might save on wrapping on 10883-4

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

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2469
lib/ssl/tls13con.c:2469: ticket_data.len;
Nit: order these to match the order they appear.

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2514
lib/ssl/tls13con.c:2514: PRUint32 flags;
BUG: PRInt32 or you will never catch an error.

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2550
lib/ssl/tls13con.c:2550: * ever allocate the high bit. */
Thanks for the red flag (see above).

Rather than marking this TODO, just fix it and read this as a buffer. 
Presumably, we should ignore the high bit when it is set, so read this in two
pieces and ignore the first piece.

I would prefer that this value were only 16 bits, but I'm like that.

[ekr-rietveld, 2016-05-31 01:16:56]

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

https://codereview.appspot.com/300200043/diff/1/lib/ssl/ssl3con.c#newcode10863
lib/ssl/ssl3con.c:10863: SECItem ticket_data = { 0, NULL, 0 };
On 2016/05/31 01:08:43, mt wrote:
> s/ticket_data/ticket/ ?  Might save on wrapping on 10883-4

Willdo.

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

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2469
lib/ssl/tls13con.c:2469: ticket_data.len;
On 2016/05/31 01:08:43, mt wrote:
> Nit: order these to match the order they appear.

I think they are the same, as in line 2447 and following. What am I missing?

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2550
lib/ssl/tls13con.c:2550: * ever allocate the high bit. */
On 2016/05/31 01:08:43, mt wrote:
> Thanks for the red flag (see above).
> 
> Rather than marking this TODO, just fix it and read this as a buffer. 
> Presumably, we should ignore the high bit when it is set, so read this in two
> pieces and ignore the first piece.
> 
> I would prefer that this value were only 16 bits, but I'm like that.

I am assigning to tmp, which is PRint32, checking for < 0, and then and only
then assigning to flags. So if I am thinking this through correctly, there is a
bug, but it only happens when the TLS WG allocates the high bit. We should still
handle read errors correctly, unless I'm confused.

I think you're right about reading in blocks and then using PR_ntohl, and I'll
make that change, but I just wanted to double-check my understanding

[mt, 2016-05-31 01:21:06]

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

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2469
lib/ssl/tls13con.c:2469: ticket_data.len;
On 2016/05/31 01:16:56, ekr-webrtc wrote:
> On 2016/05/31 01:08:43, mt wrote:
> > Nit: order these to match the order they appear.
> 
> I think they are the same, as in line 2447 and following. What am I missing?

Ugh, extensions before the actual ticket.  My brain manages very poorly with
that sort of thing.

https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2550
lib/ssl/tls13con.c:2550: * ever allocate the high bit. */
On 2016/05/31 01:16:56, ekr-webrtc wrote:
> On 2016/05/31 01:08:43, mt wrote:
> > Thanks for the red flag (see above).
> > 
> > Rather than marking this TODO, just fix it and read this as a buffer. 
> > Presumably, we should ignore the high bit when it is set, so read this in
two
> > pieces and ignore the first piece.
> > 
> > I would prefer that this value were only 16 bits, but I'm like that.
> 
> I am assigning to tmp, which is PRint32, checking for < 0, and then and only
> then assigning to flags. So if I am thinking this through correctly, there is
a
> bug, but it only happens when the TLS WG allocates the high bit. We should
still
> handle read errors correctly, unless I'm confused.
> 
> I think you're right about reading in blocks and then using PR_ntohl, and I'll
> make that change, but I just wanted to double-check my understanding

Yes, reading into a 32-bit integer will flag an error when there isn't one. 
That means we will fail a connection when we shouldn't.

[ekr-rietveld, 2016-05-31 01:26:10]

On 2016/05/31 01:21:06, mt wrote:
> https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c
> File lib/ssl/tls13con.c (right):
> 
> https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2469
> lib/ssl/tls13con.c:2469: ticket_data.len;
> On 2016/05/31 01:16:56, ekr-webrtc wrote:
> > On 2016/05/31 01:08:43, mt wrote:
> > > Nit: order these to match the order they appear.
> > 
> > I think they are the same, as in line 2447 and following. What am I missing?
> 
> Ugh, extensions before the actual ticket.  My brain manages very poorly with
> that sort of thing.
> 
> https://codereview.appspot.com/300200043/diff/1/lib/ssl/tls13con.c#newcode2550
> lib/ssl/tls13con.c:2550: * ever allocate the high bit. */
> On 2016/05/31 01:16:56, ekr-webrtc wrote:
> > On 2016/05/31 01:08:43, mt wrote:
> > > Thanks for the red flag (see above).
> > > 
> > > Rather than marking this TODO, just fix it and read this as a buffer. 
> > > Presumably, we should ignore the high bit when it is set, so read this in
> two
> > > pieces and ignore the first piece.
> > > 
> > > I would prefer that this value were only 16 bits, but I'm like that.
> > 
> > I am assigning to tmp, which is PRint32, checking for < 0, and then and only
> > then assigning to flags. So if I am thinking this through correctly, there
is
> a
> > bug, but it only happens when the TLS WG allocates the high bit. We should
> still
> > handle read errors correctly, unless I'm confused.
> > 
> > I think you're right about reading in blocks and then using PR_ntohl, and
I'll
> > make that change, but I just wanted to double-check my understanding
> 
> Yes, reading into a 32-bit integer will flag an error when there isn't one. 
> That means we will fail a connection when we shouldn't.

OK, sounds like we're on the same page.

[ekr-rietveld, 2016-06-04 17:37:34]

PTAL