Bug 1227905 - Support ChaCha20+Poly1305 cipher suites

Bug 1227905 - Support ChaCha20+Poly1305 cipher suites

[mt, 2015-11-25 22:23:19]

This looks fine to me overall.

I'm not sure that this is enough test coverage.  Can you have a look at
tests/ssl/sslcov.txt and maybe add the suites to the lists there?  You might
have to add new flags to the tstclnt and sslserv tools, but I think that you can
just use the hex values for the identifiers.

I don't see TLS_DHE_RSA_WITH_CHACHA20_POLY1305 here, which would be trivial to
implement (it's just a matter of adding the various table entries).  I think we
should do that too.

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

https://codereview.appspot.com/277090043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:501: SendReceive();
Please check that the cipher suite is correct.

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

https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode99
lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,    SSL_ALLOWED,
PR_TRUE, PR_FALSE},
Can you send an email to the nss-dev mailing list explaining why you think that
these should be prioritized over AES_128_GCM suites?  I'm not sure that we need
to reorder like this (though I'm on the fence).

https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2043: static const int tagSize = 16;
These constants are now in two places: here and in bulk_cipher_defs.  Any reason
not to index that table?  Or maybe #define something.

https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2051: *(uint64_t *)(nonce + 4) ^= *(uint64_t *)additionalData;
PRUint64 ... at least until we decide that ctypes.h is permitted.

[ekr, 2015-11-25 22:27:27]

On Wed, Nov 25, 2015 at 2:23 PM, <martin.thomson@gmail.com> wrote:

> This looks fine to me overall.
>
> I'm not sure that this is enough test coverage.  Can you have a look at
> tests/ssl/sslcov.txt and maybe add the suites to the lists there?  You
> might have to add new flags to the tstclnt and sslserv tools, but I
> think that you can just use the hex values for the identifiers.
>
> I don't see TLS_DHE_RSA_WITH_CHACHA20_POLY1305 here, which would be
> trivial to implement (it's just a matter of adding the various table
> entries).  I think we should do that too.
>
>
>
>
https://codereview.appspot.com/277090043/diff/20001/external_tests/ssl_gtest/...
> File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):
>
>
>
https://codereview.appspot.com/277090043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_loopback_unittest.cc:501: SendReceive();
> Please check that the cipher suite is correct.
>
> https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c
> File lib/ssl/ssl3con.c (right):
>
>
>
https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode99
> lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
> SSL_ALLOWED, PR_TRUE, PR_FALSE},
> Can you send an email to the nss-dev mailing list explaining why you
> think that these should be prioritized over AES_128_GCM suites?  I'm not
> sure that we need to reorder like this (though I'm on the fence).
>

I generally think we should not be prioritizing over GCM. Remember that most
people who run Firefox probably have AES-NI and so GCM is faster.

-Ekr

[ttaubert, 2015-11-30 17:48:07]

On 2015/11/25 22:23:19, mt wrote:
> I'm not sure that this is enough test coverage.  Can you have a look at
> tests/ssl/sslcov.txt and maybe add the suites to the lists there?  You might
> have to add new flags to the tstclnt and sslserv tools, but I think that you
can
> just use the hex values for the identifiers.

Good idea, done. Extended to gtests to cover all three cipher suites too.

> I don't see TLS_DHE_RSA_WITH_CHACHA20_POLY1305 here, which would be trivial to
> implement (it's just a matter of adding the various table entries).  I think
we
> should do that too.

Added.

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

https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode99
lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,    SSL_ALLOWED,
PR_TRUE, PR_FALSE},
On 2015/11/25 22:23:19, mt wrote:
> Can you send an email to the nss-dev mailing list explaining why you think
that
> these should be prioritized over AES_128_GCM suites?  I'm not sure that we
need
> to reorder like this (though I'm on the fence).

This change was carried over from Adam's version. I changed it back based on
your and Eric's comments, I think you're right that we don't want to prioritize
ChaCha over AES-GCM for now.

Should we have a follow-up bug that takes care of that for Firefox for Android?
And maybe Firefox OS?

https://codereview.appspot.com/277090043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2043: static const int tagSize = 16;
On 2015/11/25 22:23:19, mt wrote:
> These constants are now in two places: here and in bulk_cipher_defs.  Any
reason
> not to index that table?  Or maybe #define something.

Using the table now.

[ttaubert, 2015-12-01 10:16:20]

Updated cipher suite numbers according to the latest draft at
https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-03

[mt, 2015-12-02 04:25:11]

That looks fine to me.

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

https://codereview.appspot.com/277090043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:91: void
ConnectSendReceive(PRInt32 cipher_suite)
I think that int16_t would be best here to avoid the static_cast.

[ttaubert, 2015-12-02 07:36:32]

On 2015/12/02 04:25:11, mt wrote:
>
https://codereview.appspot.com/277090043/diff/60001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_loopback_unittest.cc:91: void
> ConnectSendReceive(PRInt32 cipher_suite)
> I think that int16_t would be best here to avoid the static_cast.

Fixed.

BTW, please ignore the ssl3con.c changes in patch set 4, guess I shouldn't have
rebased my branch :/

[ttaubert, 2015-12-02 07:44:41]

On 2015/12/02 07:36:32, ttaubert wrote:
> BTW, please ignore the ssl3con.c changes in patch set 4, guess I shouldn't
have
> rebased my branch :/

FWIW, these changes are from
https://hg.mozilla.org/projects/nss/rev/ddfaae48f249

[mt, 2015-12-08 20:55:50]

LGTM

Lost the last one apparently.

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

https://codereview.appspot.com/277090043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2123: }
Might be worth a switch statement where you can fail on the default.

[ttaubert, 2015-12-08 21:08:08]

On 2015/12/08 20:55:50, mt wrote:
>
https://codereview.appspot.com/277090043/diff/80001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:2123: }
> Might be worth a switch statement where you can fail on the default.

Done.

[ttaubert, 2016-01-12 15:57:27]

Updated to draft-ietf-tls-chacha20-poly1305-04 and had to rebase due to Bob's
changes. It would be good to look at the SEC_OID_ changes I think.

Unfortunately Rietveld included a few other changes due to the rebase that
aren't mine. What do we usually do? Should I open a new issue here and ask for
review again?

[wtc1, 2016-01-12 18:33:16]

On 2016/01/12 15:57:27, ttaubert wrote:
> Updated to draft-ietf-tls-chacha20-poly1305-04 and had to rebase due to Bob's
> changes. It would be good to look at the SEC_OID_ changes I think.
> 
> Unfortunately Rietveld included a few other changes due to the rebase that
> aren't mine. What do we usually do? Should I open a new issue here and ask for
> review again?

I didn't review the earlier patch sets, but everything in patch set 7 is
related to Chacha20+Poly1305 cipher suites. I don't understand why you
said Rietveld included some other changes. If they are only visible in
the diffs between patch set 7 and an earlier patch set, that is to be
expected after a rebase.

As long as patch set 7 contains the expected changes, you don't need to
open a new issue.

[wtc1, 2016-01-12 18:34:42]

Review comments on patch set 7:

This isn't a real review. I was just skimming through the
CL to see if patch set 7 contains unrelated changes.

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

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:134: { TLS_DHE_DSS_WITH_RC4_128_SHA,              SSL_ALLOWED,
PR_FALSE, PR_FALSE},

I suggest we not adjust the indentation of all other lines because
they are aligned with the table column heading on line 95. It is OK
for the new CHACHA20_POLY1305 lines to be misaligned.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2048: const int tagSize =
bulk_cipher_defs[cipher_aes_128_gcm].tag_size;

BUG: cipher_aes_128_gcm => cipher_chacha20

tag_size is 16 for both cipher_aes_128_gcm and cipher_chacha20, which is
why this bug doesn't break things.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2051: *
https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-01#section-2

IMPORTANT: this URL should be updated to the -04 draft.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2056: *(PRUint64 *)(nonce + 4) ^= *(PRUint64 *)additionalData;

1. IMPORTANT: we need to verify additionalDataLen >= sizeof(PRUint64).

Since additional_data is defined as:

      additional_data = seq_num + TLSCompressed.type +
                        TLSCompressed.version + TLSCompressed.length;

we should be able to perform a stricter test of additionalDataLen.

2. IMPORTANT: this kind of unaligned access is not portable. We need to XOR byte
by byte.

Note: we can define |nonce| as a union of unsigned char and PRUint64 arrays for
ensure alignment, but |additionalData| is still unaligned.

[ttaubert, 2016-01-12 19:00:41]

Thanks for your comments Wan-Teh! I addressed them all in the latest patch set.

[wtc1, 2016-01-12 19:26:40]

Patch set 7 LGTM. I completed a careful review. Although the
CL needs work, all of my suggested changes are
straightforward, so you can check in the CL without waiting
for a new review by me.

The only thing I'm worried about is the mysterious OID for
chacha20_poly1305. Please document the source of that OID.
If you are not sure, changes related to that OID should be
excluded from this CL.

https://codereview.appspot.com/277090043/diff/120001/cmd/ssltap/ssltap.c
File cmd/ssltap/ssltap.c (right):

https://codereview.appspot.com/277090043/diff/120001/cmd/ssltap/ssltap.c#newc...
cmd/ssltap/ssltap.c:448: case 0x00CCA9:    cs_str =
"TLS/DHE-RSA/CHACHA20-POLY1305/SHA256"; break;

BUG: these are 0x00CCA8 - 0x00CCAA in the -04 draft:
https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-04#section-3

https://codereview.appspot.com/277090043/diff/120001/lib/pk11wrap/pk11pars.c
File lib/pk11wrap/pk11pars.c (right):

https://codereview.appspot.com/277090043/diff/120001/lib/pk11wrap/pk11pars.c#...
lib/pk11wrap/pk11pars.c:357: {CIPHER_NAME("CAMELLIA256-CBC"),
SEC_OID_CAMELLIA_256_CBC, NSS_USE_ALG_IN_SSL},

Nit: maybe list the new item here, so that things are mostly sorted in
alphabetical order?

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

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:101: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,  
SSL_ALLOWED, PR_TRUE, PR_FALSE},

IMPORTANT: please confirm with Eric and Martin that we want to list
ChaCha20+Poly1305 after AES-GCM in our preference order.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:431: {TLS_DHE_DSS_WITH_AES_256_CBC_SHA256, cipher_aes_256,
hmac_sha256, kea_dhe_dss},

Please move the three new ChaCha20+Poly13 entries here, so that the AES entries
are contiguous.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2056: *(PRUint64 *)(nonce + 4) ^= *(PRUint64 *)additionalData;

On 2016/01/12 18:34:42, wtc1 wrote:
> 
> 1. IMPORTANT: we need to verify additionalDataLen >= sizeof(PRUint64).

I found that the original Chromium patch doesn't check this, but it is a good
idea to check or at least assert this.

https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2127: return SECFailure;

IMPORTANT: set the error code. I don't know what's the right error code.
For now, you can call PORT_SetError(SEC_ERROR_LIBRARY_FAILURE).

https://codereview.appspot.com/277090043/diff/120001/lib/util/secoid.c
File lib/util/secoid.c (right):

https://codereview.appspot.com/277090043/diff/120001/lib/util/secoid.c#newcod...
lib/util/secoid.c:203: CONST_OID chacha20_poly1305[]                = {
ALGORITHM, 0x1e };

IMPORTANT: where does this OID come from?

https://codereview.appspot.com/277090043/diff/120001/tests/ssl/ssl.sh
File tests/ssl/ssl.sh (right):

https://codereview.appspot.com/277090043/diff/120001/tests/ssl/ssl.sh#newcode92
tests/ssl/ssl.sh:92: CSHORT="-c
ABCDEF:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2:CCAAcdefgijklmnvyz"

IMPORTANT: CCAA (DHE_RSA) is the least interesting of the three new cipher
suites. I suggest we test CCA8 (ECDHE_RSA) instead.

https://codereview.appspot.com/277090043/diff/120001/tests/ssl/sslcov.txt
File tests/ssl/sslcov.txt (right):

https://codereview.appspot.com/277090043/diff/120001/tests/ssl/sslcov.txt#new...
tests/ssl/sslcov.txt:104: noECC  TLS12 :CCAA 
TLS12_DHE_RSA_WITH_CHACHA20_POLY1305

Typo: TLS12_DHE_RSA_WITH_CHACHA20_POLY1305 =>
TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

https://codereview.appspot.com/277090043/diff/120001/tests/ssl/sslcov.txt#new...
tests/ssl/sslcov.txt:175: ECC   TLS12  :CCA8
TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305

1. Typo: same here: add _SHA256 to the cipher suite names.

2. Nit: I would reverse these two cipher suites so that they are
tested in ascending cipher suite numbers.

[wtc1, 2016-01-12 19:35:22]

Review comments on patch set 8:

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

https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode99
lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,        
SSL_ALLOWED, PR_TRUE, PR_FALSE},

Nit; please restore the original indentation of these two lines.

https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:119: { TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256, SSL_ALLOWED,
PR_TRUE,  PR_FALSE},

Nit: We only need one space before PR_FALSE.

Optional: You can delete the spaces after the first two commas so that we regain
alignment of the later fields.

https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2057: PORT_Assert(additionalDataLen >= sizeof(PRUint64));

Let's just use "8" instead of sizeof(PRUint64) here, because we no longer use
PRUint64 below.

https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2058: for (i=0; i < 8; ++i) {

Nit: add spaces before and after the equal sign '=':
  i = 0

[mt, 2016-01-13 03:08:44]

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

https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2123: switch (calg) {
Tabs => spaces

[ttaubert, 2016-01-14 14:19:50]

On 2016/01/13 03:08:44, mt wrote:
>
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:2123: switch (calg) {
> Tabs => spaces

Done.

[ttaubert, 2016-01-14 14:27:33]

On 2016/01/12 19:35:22, wtc1 wrote:
>
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode99
> lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,        
> SSL_ALLOWED, PR_TRUE, PR_FALSE},
> 
> Nit; please restore the original indentation of these two lines.

Done.

>
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:119: { TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
SSL_ALLOWED,
> PR_TRUE,  PR_FALSE},
> 
> Nit: We only need one space before PR_FALSE.
> 
> Optional: You can delete the spaces after the first two commas so that we
regain
> alignment of the later fields.

Done.

>
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:2057: PORT_Assert(additionalDataLen >= sizeof(PRUint64));
> 
> Let's just use "8" instead of sizeof(PRUint64) here, because we no longer use
> PRUint64 below.

Done.

>
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:2058: for (i=0; i < 8; ++i) {
> 
> Nit: add spaces before and after the equal sign '=':
>   i = 0

Done.

[ttaubert, 2016-01-15 14:03:04]

On 2016/01/12 19:26:40, wtc1 wrote:
>
https://codereview.appspot.com/277090043/diff/120001/cmd/ssltap/ssltap.c#newc...
> cmd/ssltap/ssltap.c:448: case 0x00CCA9:    cs_str =
> "TLS/DHE-RSA/CHACHA20-POLY1305/SHA256"; break;
> 
> BUG: these are 0x00CCA8 - 0x00CCAA in the -04 draft:
> https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-04#section-3

Good catch, thanks. Forgot to update this part of the code.

>
https://codereview.appspot.com/277090043/diff/120001/lib/pk11wrap/pk11pars.c#...
> lib/pk11wrap/pk11pars.c:357: {CIPHER_NAME("CAMELLIA256-CBC"),
> SEC_OID_CAMELLIA_256_CBC, NSS_USE_ALG_IN_SSL},
> 
> Nit: maybe list the new item here, so that things are mostly sorted in
> alphabetical order?

Done.

>
https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:101: { TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,  
> SSL_ALLOWED, PR_TRUE, PR_FALSE},
> 
> IMPORTANT: please confirm with Eric and Martin that we want to list
> ChaCha20+Poly1305 after AES-GCM in our preference order.

Wrote a separate email to ekr and mt to have this confirmed.

>
https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:431: {TLS_DHE_DSS_WITH_AES_256_CBC_SHA256, cipher_aes_256,
> hmac_sha256, kea_dhe_dss},
> 
> Please move the three new ChaCha20+Poly13 entries here, so that the AES
entries
> are contiguous.

Done.

>
https://codereview.appspot.com/277090043/diff/120001/lib/ssl/ssl3con.c#newcod...
> lib/ssl/ssl3con.c:2127: return SECFailure;
> 
> IMPORTANT: set the error code. I don't know what's the right error code.
> For now, you can call PORT_SetError(SEC_ERROR_LIBRARY_FAILURE).

Done.

>
https://codereview.appspot.com/277090043/diff/120001/tests/ssl/ssl.sh#newcode92
> tests/ssl/ssl.sh:92: CSHORT="-c
>
ABCDEF:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2:CCAAcdefgijklmnvyz"
> 
> IMPORTANT: CCAA (DHE_RSA) is the least interesting of the three new cipher
> suites. I suggest we test CCA8 (ECDHE_RSA) instead.

I didn't change that because CSHORT is only used with NSS_DISABLE_ECC=1, so we
need to test the only non-EC ChaCha cipher suite.

>
https://codereview.appspot.com/277090043/diff/120001/tests/ssl/sslcov.txt#new...
> tests/ssl/sslcov.txt:104: noECC  TLS12 :CCAA 
> TLS12_DHE_RSA_WITH_CHACHA20_POLY1305
> 
> Typo: TLS12_DHE_RSA_WITH_CHACHA20_POLY1305 =>
> TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

Done.

>
https://codereview.appspot.com/277090043/diff/120001/tests/ssl/sslcov.txt#new...
> tests/ssl/sslcov.txt:175: ECC   TLS12  :CCA8
> TLS12_ECDHE_RSA_WITH_CHACHA20_POLY1305
> 
> 1. Typo: same here: add _SHA256 to the cipher suite names.
> 
> 2. Nit: I would reverse these two cipher suites so that they are
> tested in ascending cipher suite numbers.

Done.

[ttaubert, 2016-01-15 14:04:32]

>
https://codereview.appspot.com/277090043/diff/120001/lib/util/secoid.c#newcod...
> lib/util/secoid.c:203: CONST_OID chacha20_poly1305[]                = {
> ALGORITHM, 0x1e };
> 
> IMPORTANT: where does this OID come from?

So the problem is that it seems as with Bob's patch from bug 1009429 we need an
OID for ChaCha20/Poly1305 but there is none. How do we deal with that,
can/should we create a temporary one until an official one is assigned?

[ttaubert, 2016-01-15 14:39:48]

Would it be better to use

CONST_OID chacha20_poly1305[] = { NETSCAPE_ALGS, 0x02 };

as long as ChaCha20/Poly1305 is still a "Netscape-defined" algorithm?

[wtc1, 2016-01-15 18:19:56]

Tim: could you please upload a new patch set, so I can
review how you addressed my comments on patch set 7?
Thanks.

[wtc1, 2016-01-15 18:27:12]

On 2016/01/15 14:04:32, ttaubert wrote:
> 
> So the problem is that it seems as with Bob's patch from bug 1009429 we need
an
> OID for ChaCha20/Poly1305 but there is none. How do we deal with that,
> can/should we create a temporary one until an official one is assigned?

I suggest we deal with the OID in a separate patch because libSSL
doesn't need it.

Please ask Bob, Richard, and Ryan about how we should allocate an
OID for ChaCha20/Poly1305. ChaCha20/Poly1305 is not a Netscape or
NSS-defined algorithm. It is standardized in RFC 7539.

[ttaubert, 2016-01-16 12:35:03]

On 2016/01/15 18:27:12, wtc1 wrote:
> On 2016/01/15 14:04:32, ttaubert wrote:
> > 
> > So the problem is that it seems as with Bob's patch from bug 1009429 we need
> an
> > OID for ChaCha20/Poly1305 but there is none. How do we deal with that,
> > can/should we create a temporary one until an official one is assigned?
> 
> I suggest we deal with the OID in a separate patch because libSSL
> doesn't need it.

Removed the invalid OID.

> Please ask Bob, Richard, and Ryan about how we should allocate an
> OID for ChaCha20/Poly1305. ChaCha20/Poly1305 is not a Netscape or
> NSS-defined algorithm. It is standardized in RFC 7539.

Will do.

[wtc1, 2016-01-20 22:04:20]

Patch set 12 LGTM!

https://codereview.appspot.com/277090043/diff/220001/tests/ssl/ssl.sh
File tests/ssl/ssl.sh (right):

https://codereview.appspot.com/277090043/diff/220001/tests/ssl/ssl.sh#newcode93
tests/ssl/ssl.sh:93: CLONG="-c
ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C023:C027:C02B:C02F:CCA8:CCA9:CCAA:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2cdefgijklmnvyz"

Perhaps the definition of CSHORT and CLONG should be moved
into the if-then-else statement above (lines 86-90) to make
it clear they are only used under certain conditions?

At least, we should add comments to explain what CSHORT and
CLONG are. I thought CSHORT was just a shorter list of
cipher suites.

[ttaubert, 2016-01-21 13:42:34]

On 2016/01/20 22:04:20, wtc1 wrote:
> Perhaps the definition of CSHORT and CLONG should be moved
> into the if-then-else statement above (lines 86-90) to make
> it clear they are only used under certain conditions?

Moved the definitions into the if-then-else statement and unified both under a
single variable called CIPHER_SUITES. Removed all other CLONG/CSHORT references
in the file.

> At least, we should add comments to explain what CSHORT and
> CLONG are. I thought CSHORT was just a shorter list of
> cipher suites.

Added.

[wtc1, 2016-01-21 18:45:48]

Patch set 13 LGTM. I like your solution very much!