net: add ChaCha20+Poly1305 support to libssl.

net: add ChaCha20+Poly1305 support to libssl.

(No sementic change without the matching libnss work.)

BUG=none
R=wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223615

[wtc, 2013-09-13 17:29:14]

Patch set 3 LGTM.

I think it is safe to use the 0xCC** cipher suite numbers
for experiments. IANA would probably assign numbers from
0xC0**, so there should be no risk of collision. (IANA seems
to assign numbers from 0x00** for Standards Track RFCs and
from 0xC0** from Informational RFCs.)

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:53: } CK_AEAD_PARAMS;

This differs from CK_GCM_PARAMS only in the ulTagBits field. Do you think the
a ulTagBits field could be useful?

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:118: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,  
SSL_NOT_ALLOWED, PR_TRUE, PR_FALSE},

The |enabled| setting for the two CHACHA20_POLY1305 ciphers should be PR_FALSE
because these are ECC ciphers.

The CHACHA20_POLY1305 ciphers are listed before AES_128_GCM ciphers. I assume
this is because CHACHA20_POLY1305 uses 256-bit keys.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:420: {TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
cipher_c20p1305, mac_null, kea_ecdhe_rsa},

Use mac_aead instead of mac_null.

Nit: list ECDHE_RSA before ECDHE_ECDSA, matching the {0xcc, 0x13}, {0xcc, 0x14}
order.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2057: static const int   tagSize = 16;

Delete tagSize. It's not used.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2062: param.len = sizeof(CK_AEAD_PARAMS);

Nit: use sizeof(aeadParams).

Nit: SECItem has just three fields. In NSS we usually just assign them without
using a memset to zero the struct first:

  param.type = siBuffer;
  param.data = (unsigned char *) &aeadParams;
  param.len = sizeof(aeadParams);

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2115: if (calg == calg_aes_gcm || calg ==
calg_c20p1305) {

We should have a more systematic way to set the pwSpec->aead function pointer
if AEAD ciphers proliferate.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:298: #define ssl_V3_SUITES_IMPLEMENTED 39

This should be 37 (unchanged) because the two new ciphers are ECC ciphers.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:478: cipher_c20p1305,

I think we should at least say "cha". "c" seems too short :-)

But I can see it may make some names too long.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslinfo.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslinfo.c:140:
{0,CS(TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305),S_ECDSA,K_ECDHE,C_C20P1305,B_256,M_AEAD_128,$0,
0, 0, },

There is a "$0" in this line. It should be "0".

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslinfo.c:141:
{0,CS(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305), 
S_RSA,K_ECDHE,C_C20P1305,B_256,M_AEAD_128, 0, 0, 0, },

Move these to the ECDHE_ECDSA and ECDHE_RSA sections, respectively.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslproto.h (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslproto.h:169: #define
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305	0xCC14

Move these after 0xC031 (line 216).

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:114: { 
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },

Move this to the ECDHE_ECDSA section.

[agl, 2013-09-13 20:51:45]

Thanks.

There's one substantive reply about merging the CK_AEAD_PARAMS and
CK_GCM_PARAMS.

As for the ciphersuite values, it's ChaCha20+Poly1305, thus 0xcc for ChaCha and
0x13 for 1305.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:53: } CK_AEAD_PARAMS;
On 2013/09/13 17:29:14, wtc wrote:
> This differs from CK_GCM_PARAMS only in the ulTagBits field. Do you think the
> a ulTagBits field could be useful?

I don't plan on truncating the MAC, but others might. Even so, it would seem odd
to use CK_GCM_PARAMS with other algorithms. Would you prefer that the code
support ulTagBits in order to keep the structs the same? If so, you would like
to use CK_GCM_PARAMS for ChaCha, or typedef CK_GCM_PARAMS CK_AEAD_PARAMS?

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:118: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,  
SSL_NOT_ALLOWED, PR_TRUE, PR_FALSE},
On 2013/09/13 17:29:14, wtc wrote:
> The |enabled| setting for the two CHACHA20_POLY1305 ciphers should be PR_FALSE
> because these are ECC ciphers.

Done.

> The CHACHA20_POLY1305 ciphers are listed before AES_128_GCM ciphers. I assume
> this is because CHACHA20_POLY1305 uses 256-bit keys.

Initially it's because I wanted to test but I do think that the ChaCha ciphers
are safer than AES_GCM because not all systems have AES-NI. With optimisation
enabled, they run at about 50% the speed of AES-GCM with AES-NI.

In the future I would like the AES_GCM ciphers to be listed first in cases where
the system has AES-NI. (Perhaps AES256 as, with AES-NI, that's not a major
performance issue.) Servers would need to be able to select different ciphers
depending on the client's cipher preference without always respecting the client
cipher preference, so it's a little complex. For now however, ChaCha is a safe
choice as a first-preference cipher.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:420: {TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
cipher_c20p1305, mac_null, kea_ecdhe_rsa},
On 2013/09/13 17:29:14, wtc wrote:
> 
> Use mac_aead instead of mac_null.
> 
> Nit: list ECDHE_RSA before ECDHE_ECDSA, matching the {0xcc, 0x13}, {0xcc,
0x14}
> order.

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2057: static const int   tagSize = 16;
On 2013/09/13 17:29:14, wtc wrote:
> 
> Delete tagSize. It's not used.

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2062: param.len = sizeof(CK_AEAD_PARAMS);
On 2013/09/13 17:29:14, wtc wrote:
> 
> Nit: use sizeof(aeadParams).
> 
> Nit: SECItem has just three fields. In NSS we usually just assign them without
> using a memset to zero the struct first:
> 
>   param.type = siBuffer;
>   param.data = (unsigned char *) &aeadParams;
>   param.len = sizeof(aeadParams);

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:298: #define ssl_V3_SUITES_IMPLEMENTED 39
On 2013/09/13 17:29:14, wtc wrote:
> 
> This should be 37 (unchanged) because the two new ciphers are ECC ciphers.

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:478: cipher_c20p1305,
On 2013/09/13 17:29:14, wtc wrote:
> 
> I think we should at least say "cha". "c" seems too short :-)
> 
> But I can see it may make some names too long.

I've changed to cipher_chacha20, which is less precise but also less confusing.
These names appear in tables and it's very difficult to maintain a limited
line-length with the full name. Let me know whatever you prefer.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslinfo.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslinfo.c:140:
{0,CS(TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305),S_ECDSA,K_ECDHE,C_C20P1305,B_256,M_AEAD_128,$0,
0, 0, },
On 2013/09/13 17:29:14, wtc wrote:
> 
> There is a "$0" in this line. It should be "0".

Thanks! A vim movement command that slipped in.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslinfo.c:141:
{0,CS(TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305), 
S_RSA,K_ECDHE,C_C20P1305,B_256,M_AEAD_128, 0, 0, 0, },
On 2013/09/13 17:29:14, wtc wrote:
> 
> Move these to the ECDHE_ECDSA and ECDHE_RSA sections, respectively.

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslproto.h (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslproto.h:169: #define
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305	0xCC14
On 2013/09/13 17:29:14, wtc wrote:
> 
> Move these after 0xC031 (line 216).

Done.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:114: { 
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
On 2013/09/13 17:29:14, wtc wrote:
> 
> Move this to the ECDHE_ECDSA section.

Done.

[wtc, 2013-09-13 21:32:25]

Patch set 4 LGTM.

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:53: } CK_AEAD_PARAMS;

For the struct to be named CK_AEAD_PARAMS, it should be
general enough to support all AEAD algorithms. This is why
I compared CK_AEAD_PARAMS with CK_GCM_PARAMS and noticed
that it is missing ulTagBit.

I just checked CK_CCM_PARAMS and found it also has a
ulDataLen field (the other fields seem equivalent to the
fields of CK_GCM_PARAMS even though the names are different).

1517 /* CK_CCM_PARAMS is new for version 2.30 */
1518 typedef struct CK_CCM_PARAMS {
1519   CK_ULONG     ulDataLen;
1520   CK_BYTE_PTR  pNonce;
1521   CK_ULONG     ulNonceLen;
1522   CK_BYTE_PTR  pAAD;
1523   CK_ULONG     ulAADLen;
1524   CK_ULONG     ulMACLen;
1525 } CK_CCM_PARAMS;

So, I would prefer that we either have a CK_AEAD_PARAMS
that is general, or just have a CK_CHACHA20_POLY1305_PARAMS.

https://codereview.chromium.org/23619044/diff/13001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslinfo.c (right):

https://codereview.chromium.org/23619044/diff/13001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslinfo.c:121: #define C_C20P1305 "CHACHA20POLY1305",
calg_chacha20

Nit: C_C20P1305 => C_CHACHA20

[agl, 2013-09-16 22:19:07]

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23619044/diff/6001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:53: } CK_AEAD_PARAMS;
On 2013/09/13 21:32:26, wtc wrote:
> For the struct to be named CK_AEAD_PARAMS, it should be
> general enough to support all AEAD algorithms. This is why
> I compared CK_AEAD_PARAMS with CK_GCM_PARAMS and noticed
> that it is missing ulTagBit.

I've added the ulTagBit member.

https://codereview.chromium.org/23619044/diff/13001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslinfo.c (right):

https://codereview.chromium.org/23619044/diff/13001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslinfo.c:121: #define C_C20P1305 "CHACHA20POLY1305",
calg_chacha20
On 2013/09/13 21:32:26, wtc wrote:
> 
> Nit: C_C20P1305 => C_CHACHA20

Done.

[commit-bot: I haz the power, 2013-09-16 22:26:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/23619044/31001

[commit-bot: I haz the power, 2013-09-17 04:27:18]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[agl, 2013-09-17 14:52:34]

Committed patchset #5 manually as r223615 (presubmit successful).