Bug 998524 - Improve SNI handling for NameType

Bug 998524 - Improve SNI handling for NameType

[mt, 2015-10-02 21:21:45]

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

https://codereview.appspot.com/269100043/diff/1/lib/ssl/ssl3ext.c#newcode424
lib/ssl/ssl3ext.c:424: goto alert_loser;
This is going to generate two alerts in the case that
ssl3_ConsumeHandshakeNumber fails.  I think that you need to goto loser when
(listLenBytes < 0).

https://codereview.appspot.com/269100043/diff/1/lib/ssl/ssl3ext.c#newcode435
lib/ssl/ssl3ext.c:435: goto loser;
Add comment: /* alert sent in ConsumeHandshakeNumber */

https://codereview.appspot.com/269100043/diff/1/lib/ssl/ssl3ext.c#newcode437
lib/ssl/ssl3ext.c:437: /* Record the first host_name(0) value, ignore the rest.
*/
   The ServerNameList MUST NOT contain more than one name of the same
   name_type. -- RFC 6066

I'm happy with the code ignoring duplicates for name_type != 0, but we should
not permit duplicates.

https://codereview.appspot.com/269100043/diff/1/lib/ssl/ssl3ext.c#newcode458
lib/ssl/ssl3ext.c:458: * moment, we continue parsing the whole list to check its
validity.
Except that we don't detect duplicates...

[mt, 2015-10-07 05:25:35]

LGTM

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

https://codereview.appspot.com/269100043/diff/20001/lib/ssl/ssl3ext.c#newcode464
lib/ssl/ssl3ext.c:464: }
I think that the loop here could be revised to:
while (data->len > 0) {
  ...
  listLenBytes -= dest->len + 3;
}

This check is actually redundant with the one on line 426, but you could leave
the check outside the loop:

if (listLenBytes > 0) {
  goto alert_loser;
}

[ttaubert, 2015-10-07 05:38:13]

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

https://codereview.appspot.com/269100043/diff/20001/lib/ssl/ssl3ext.c#newcode464
lib/ssl/ssl3ext.c:464: }
On 2015/10/07 05:25:35, mt wrote:
> I think that the loop here could be revised to:
> while (data->len > 0) {
>   ...
>   listLenBytes -= dest->len + 3;
> }
> 
> This check is actually redundant with the one on line 426, but you could leave
> the check outside the loop:
> 
> if (listLenBytes > 0) {
>   goto alert_loser;
> }

Done.

[mt, 2015-10-12 18:01:48]

LGTM

[ekr-rietveld, 2016-02-26 14:51:11]

It would be nice if you fixed up the memory semantics here so that sniNameArr[0]
wasn't pointing to data in the packet

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

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode460
lib/ssl/ssl3ext.c:460: /* Read Name Type. */
whitespace here.

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode466
lib/ssl/ssl3ext.c:466: /* Record the host_name(0) value. */
Record the value for host_name(0)

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode467
lib/ssl/ssl3ext.c:467: if (type == 0) {
Please make a constnat for this.

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode494
lib/ssl/ssl3ext.c:494: }

You have verified that listLenBytes == data->len and then
you loop as long as data->len > 0. Doesn't this guarantee that
you either read all the bytes or that you will have a decode
error (when you try to read beyond the end)

[ekr-rietveld, 2016-03-03 19:00:15]

ttaubert: when can we get this landed?

[ttaubert, 2016-03-03 19:55:02]

On 2016/03/03 19:00:15, ekr-webrtc wrote:
> ttaubert: when can we get this landed?

Sorry, wasn't aware that you wanted to get this landed soon. I prioritized the
SSLv2 and export cipher suite removal, will take a look at this tomorrow and
address your review comments.

[ttaubert, 2016-03-03 21:07:28]

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

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode460
lib/ssl/ssl3ext.c:460: /* Read Name Type. */
On 2016/02/26 14:51:11, ekr-webrtc wrote:
> whitespace here.

Done.

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode466
lib/ssl/ssl3ext.c:466: /* Record the host_name(0) value. */
On 2016/02/26 14:51:11, ekr-webrtc wrote:
> Record the value for host_name(0)

Done.

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode467
lib/ssl/ssl3ext.c:467: if (type == 0) {
On 2016/02/26 14:51:11, ekr-webrtc wrote:
> Please make a constnat for this.

Done.

https://codereview.appspot.com/269100043/diff/60001/lib/ssl/ssl3ext.c#newcode494
lib/ssl/ssl3ext.c:494: }
On 2016/02/26 14:51:11, ekr-webrtc wrote:
> 
> You have verified that listLenBytes == data->len and then
> you loop as long as data->len > 0. Doesn't this guarantee that
> you either read all the bytes or that you will have a decode
> error (when you try to read beyond the end)

You're right, removed the check and the accounting in the loop.

[ttaubert, 2016-03-03 21:07:47]

On 2016/02/26 14:51:11, ekr-webrtc wrote:
> It would be nice if you fixed up the memory semantics here so that
sniNameArr[0]
> wasn't pointing to data in the packet

Done.

[ttaubert, 2016-03-03 21:11:36]

On 2016/03/03 19:55:02, ttaubert wrote:
> On 2016/03/03 19:00:15, ekr-webrtc wrote:
> > ttaubert: when can we get this landed?
> 
> Sorry, wasn't aware that you wanted to get this landed soon. I prioritized the
> SSLv2 and export cipher suite removal, will take a look at this tomorrow and
> address your review comments.

Actually found some time to update the patch now, let me know what you think.
Thanks!

[ttaubert, 2016-04-26 09:20:08]

(Gentle review ping?)

[mt, 2016-04-26 11:20:22]

On 2016/04/26 09:20:08, ttaubert wrote:
> (Gentle review ping?)

I assume that's for ekr.  This still LGTM.

[ekr-rietveld, 2016-04-26 12:46:02]

I don't understand this code. The array can only have a length of 0 or 1 and is
only referenced a few places. Why don't you just have an sniValue instead and
use extension_negotiated to indicate whether it was supplied or not.

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

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8519: * the callers (*HandleClientHelloPart2) and the callers
This comment appears to no longer be correct, since this is now heap data.

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9257: }
Do you need to condition this on the extension being negotiated? When would the
name array be valid otherwise?

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

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:477: const PRUint8 NAMETYPE_HOSTNAME = 0;
Convention here is to make this all lowercase and an enum.

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:492: goto loser;
Comment that the alert was already sent.

[ttaubert, 2016-04-26 13:45:41]

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

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8519: * the callers (*HandleClientHelloPart2) and the callers
On 2016/04/26 12:46:02, ekr-webrtc wrote:
> This comment appears to no longer be correct, since this is now heap data.

Removed.

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9257: }
On 2016/04/26 12:46:02, ekr-webrtc wrote:
> Do you need to condition this on the extension being negotiated? When would
the
> name array be valid otherwise?

Right, we don't need that. Removed.

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

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:477: const PRUint8 NAMETYPE_HOSTNAME = 0;
On 2016/04/26 12:46:02, ekr-webrtc wrote:
> Convention here is to make this all lowercase and an enum.

Done.

https://codereview.appspot.com/269100043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:492: goto loser;
On 2016/04/26 12:46:02, ekr-webrtc wrote:
> Comment that the alert was already sent.

Done.

[ttaubert, 2016-04-26 13:45:49]

On 2016/04/26 12:46:02, ekr-webrtc wrote:
> I don't understand this code. The array can only have a length of 0 or 1 and
is
> only referenced a few places. Why don't you just have an sniValue instead and
> use extension_negotiated to indicate whether it was supplied or not.

We kept sniNameArr{Size} to not break existing APIs. A server currently cycles
through the given SNI names in the callback passed to |SSL_SNISocketConfigHook|
and returns the index of the entry chosen for the current connection.

[ekr-rietveld, 2016-04-26 16:07:34]

LGTM