Curve25519 TLS integration

Curve25519 TLS integration

[mt, 2016-08-12 04:03:06]

So the big issue here is that we will prefer 25519 to the exclusion of P-256. 
We need new code to handle switching between the two based on what shares are
included.

This patch - modulo comments - is probably OK if you land it with curve25519
disabled by default.  If you want to tackle the equal preference stuff in a
follow-up, then that follow-up can turn 25519 on by default.  I'll send around
an email to that effect.

https://codereview.appspot.com/304400043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/304400043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:50: return
PR_MAX(SSL_RSASTRENGTH_TO_ECSTRENGTH(serverKeyBits), minKeaBits);
It might pay to revisit this once you rebase.

I think that you will find that we still favour P-256 due to 25519 having one
fewer bit of security.  More on that later.

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

https://codereview.appspot.com/304400043/diff/1/lib/ssl/ssl3ecc.c#newcode453
lib/ssl/ssl3ecc.c:453: ssl_GetECGroupForServerSocket(sslSocket *ss)
This function will pretty much guarantee that we never pick 25519 over P-256. 
We've been avoiding refactoring this for a while, but you might have to do
something about it.

The problem here is that with a 128-bit AEAD/symmetric cipher, this function
insists on an ECDH exchange that is twice as wide.  25519 is only more than
twice as wide as the worst of our cipher suites, so we'll avoid picking it.

We should chat about what we think the requirements are here so that we can pick
the right algorithm (for picking the right algorithm).

https://codereview.appspot.com/304400043/diff/1/lib/ssl/ssl3ecc.c#newcode734
lib/ssl/ssl3ecc.c:734: /* set up EC parameters in peerKey */
Fix the comment.

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

https://codereview.appspot.com/304400043/diff/1/lib/ssl/sslimpl.h#newcode1772
lib/ssl/sslimpl.h:1772: extern SECStatus ssl3_ImportECDHKeyShare(
I would remove the '3' here and name it ssl_ImportECDHKeyShare.

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

https://codereview.appspot.com/304400043/diff/1/lib/ssl/sslsock.c#newcode176
lib/ssl/sslsock.c:176: { 29, ssl_grp_ec_curve25519, 255, group_type_ec,
SEC_OID_CURVE25519, PR_TRUE }
Up the top, as we discussed on the list.

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

https://codereview.appspot.com/304400043/diff/1/lib/ssl/sslt.h#newcode342
lib/ssl/sslt.h:342: ssl_grp_ec_curve25519 = 29,
Please cite where this number is coming from.

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

https://codereview.appspot.com/304400043/diff/1/lib/ssl/tls13con.c#newcode323
lib/ssl/tls13con.c:323: tls13_SetupClientHello(sslSocket *ss)
This function needs to be modified so that we generate both P-256 and X25519
shares by default.

One way to deal with that is to introduce a new value for groupDef->type (which
would cover X448 too when we implement that), but then you have to go through
and check all the places where that group_type_ec is checked (and replace it
with a `PRBool ssl_IsEcGroup(const sslNamedGroupDef*)` function perhaps).

A better plan (I think) is to add a new column to the table of named groups that
identifies groups as equal strength.  For those, we will generate a share if the
group is "equal" to our most preferred group; we can also use this when picking
which group to use.  If a group is "equal" to our most preferred AND it has a
share, we should pick that instead.

BTW, we probably want to do a similar thing for cipher suites to allow clients
to express preferences for ChaCha20-Poly1305 over AES-GCM.

https://codereview.appspot.com/304400043/diff/1/lib/ssl/tls13con.c#newcode1130
lib/ssl/tls13con.c:1130: expectedGroup = ssl_GetECGroupForServerSocket(ss);
This needs to be modified as well so that we pick groups of equal "value" when
there are shares for that group.  If we move 25519 up, we will pick 25519 and
reject a ClientHello with P-256, which is unnecessary.

See above for more information on how I think you could fix this.

[franziskus, 2016-08-17 08:53:05]

I'll upload a new version later from home that should fix most issues and
disable 25519 by default. Everything else, i.e. equal priority with P256 and
enabling by default, I'll tackle separately as discussed.

> lib/ssl/ssl3ecc.c:453: ssl_GetECGroupForServerSocket(sslSocket *ss)
> This function will pretty much guarantee that we never pick 25519 over P-256. 
> We've been avoiding refactoring this for a while, but you might have to do
> something about it.
> 
> The problem here is that with a 128-bit AEAD/symmetric cipher, this function
> insists on an ECDH exchange that is twice as wide.  25519 is only more than
> twice as wide as the worst of our cipher suites, so we'll avoid picking it.
> 
> We should chat about what we think the requirements are here so that we can
pick
> the right algorithm (for picking the right algorithm).

That's not entirely correct. We search for the weakest link in the chain, which
is usually the certificate here. I added 4096 bit RSA certs to the gtests and
set the 25519 curve bits to 256 in ssl_named_groups to allow choosing it here as
well (for 2048 bit certs we require only 224 bit curves so that's non problem).
But we should probably revisit this EC group selection algorithm. I'm not sure
if we really want to make the curve selection dependent on the weakest link.
Choosing it depending on the cipher (which is always at least 128) sounds more
sensible to me.

[mt, 2016-08-17 10:03:08]

On 2016/08/17 08:53:05, franziskus wrote:
> I'll upload a new version later from home that should fix most issues and
> disable 25519 by default. Everything else, i.e. equal priority with P256 and
> enabling by default, I'll tackle separately as discussed.
> 
> > lib/ssl/ssl3ecc.c:453: ssl_GetECGroupForServerSocket(sslSocket *ss)
> > This function will pretty much guarantee that we never pick 25519 over
P-256. 
> > We've been avoiding refactoring this for a while, but you might have to do
> > something about it.
> > 
> > The problem here is that with a 128-bit AEAD/symmetric cipher, this function
> > insists on an ECDH exchange that is twice as wide.  25519 is only more than
> > twice as wide as the worst of our cipher suites, so we'll avoid picking it.
> > 
> > We should chat about what we think the requirements are here so that we can
> pick
> > the right algorithm (for picking the right algorithm).
> 
> That's not entirely correct. We search for the weakest link in the chain,
which
> is usually the certificate here. I added 4096 bit RSA certs to the gtests and
> set the 25519 curve bits to 256 in ssl_named_groups to allow choosing it here
as
> well (for 2048 bit certs we require only 224 bit curves so that's non
problem).
> But we should probably revisit this EC group selection algorithm. I'm not sure
> if we really want to make the curve selection dependent on the weakest link.
> Choosing it depending on the cipher (which is always at least 128) sounds more
> sensible to me.

I agree, we should choose based on the record protection.  We might have a way
to raise the strength to match the certificate, but we shouldn't reduce strength
if the certificate key is weak (that leads to bad properties for collected
traffic).  That's a separate change again though, one I'd be happy to help with.