Bug 917571 - Add ChaCha20+Poly1305 cipher

Bug 917571 - Add ChaCha20+Poly1305 cipher

[mt, 2015-12-05 09:25:22]

LGTM

https://codereview.appspot.com/277040044/diff/1/cmd/bltest/blapitest.c
File cmd/bltest/blapitest.c (right):

https://codereview.appspot.com/277040044/diff/1/cmd/bltest/blapitest.c#newcod...
cmd/bltest/blapitest.c:848: case bltestCHACHA20:
why isn't AES-GCM an aead cipher?

https://codereview.appspot.com/277040044/diff/1/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/1/coreconf/Werror.mk#newcode13
coreconf/Werror.mk:13: ifndef CC_VERSION
If this is needed more generally, I would move it to a more central place
instead.

https://codereview.appspot.com/277040044/diff/1/external_tests/pk11_gtest/pk1...
File external_tests/pk11_gtest/pk11_chacha20poly1305_unittest.cc (right):

https://codereview.appspot.com/277040044/diff/1/external_tests/pk11_gtest/pk1...
external_tests/pk11_gtest/pk11_chacha20poly1305_unittest.cc:240: v ## CT,
sizeof(v ## CT));
Rather than use a horrible macro, can't you just put the key, data, aad, iv and
ciphertext into a struct?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/Makefile
File lib/freebl/Makefile (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/Makefile#newcode490
lib/freebl/Makefile:490: ifeq (,$(filter 0 1 2 3 4,$(word 1,$(CC_VERSION))))
This test isn't going to work for MSVC (i.e., it will likely pass because
CC_VERSION isn't set on Windows).  The same for some of the esoteric platforms. 
You need to check if CC_NAME is gcc (and maybe avoid gcc-* to avoid all the
crazy cross-compile stuff, I don't know).

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20.c
File lib/freebl/chacha20/chacha20.c (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
lib/freebl/chacha20/chacha20.c:22: #define XOR(v, w) ((v) ^ (w))
Is this macro really required?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
lib/freebl/chacha20/chacha20.c:23: #define PLUS(x, y) ((x) + (y))
And this one?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
lib/freebl/chacha20/chacha20.c:33: #define QUARTERROUND(a, b, c, d)             
                                 \
Can you make x an argument to the macro rather than rely on it being present in
the environment?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20_...
File lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20_...
lib/freebl/chacha20/chacha20_vec.c:9: /* clang-format off */
What happens if you don't do this here?  The code below is something of a mess,
how much damage would clang-format actually do?  Or do we simply accept that
this code is read-only?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20poly1305.h
File lib/freebl/chacha20poly1305.h (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20poly1305.h...
lib/freebl/chacha20poly1305.h:12: unsigned char tagLen;
Do we really need a configurable tag length?

https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305-...
File lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305-...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:11: /*
clang-format off */
This file is a bit of a WTF.

https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305.c
File lib/freebl/poly1305/poly1305.c (right):

https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305....
lib/freebl/poly1305/poly1305.c:9: /* clang-format off */
Same comment as before...

https://codereview.appspot.com/277040044/diff/1/lib/util/pkcs11n.h
File lib/util/pkcs11n.h (right):

https://codereview.appspot.com/277040044/diff/1/lib/util/pkcs11n.h#newcode294
lib/util/pkcs11n.h:294: CK_BYTE_PTR  pIv;  /* This is the nonce. */
pNonce is a better name.  Or don't we get to change that (I have no real
knowledge of PKCS#11).

[ttaubert, 2016-01-16 13:52:56]

On 2015/12/05 09:25:22, mt wrote:
>
https://codereview.appspot.com/277040044/diff/1/cmd/bltest/blapitest.c#newcod...
> cmd/bltest/blapitest.c:848: case bltestCHACHA20:
> why isn't AES-GCM an aead cipher?

Fixed.

> https://codereview.appspot.com/277040044/diff/1/coreconf/Werror.mk#newcode13
> coreconf/Werror.mk:13: ifndef CC_VERSION
> If this is needed more generally, I would move it to a more central place
> instead.

So far this is only used by lib/freebl/Makefile. I think we can move it
somewhere else when we need to?

>
https://codereview.appspot.com/277040044/diff/1/external_tests/pk11_gtest/pk1...
> external_tests/pk11_gtest/pk11_chacha20poly1305_unittest.cc:240: v ## CT,
> sizeof(v ## CT));
> Rather than use a horrible macro, can't you just put the key, data, aad, iv
and
> ciphertext into a struct?

I did that in sync with external_tests/pk11_gtest/pk11_rsapss_unittest.cc. I
started with a struct for all the test vector details there and I think you told
me to simply use a macro instead :)

> https://codereview.appspot.com/277040044/diff/1/lib/freebl/Makefile#newcode490
> lib/freebl/Makefile:490: ifeq (,$(filter 0 1 2 3 4,$(word 1,$(CC_VERSION))))
> This test isn't going to work for MSVC (i.e., it will likely pass because
> CC_VERSION isn't set on Windows).  The same for some of the esoteric
platforms. 
> You need to check if CC_NAME is gcc (and maybe avoid gcc-* to avoid all the
> crazy cross-compile stuff, I don't know).

Added a check for CC_NAME=gcc. I have no idea about the cross-compile stuff
either. Is there an easy way to check what breaks when checking this in?

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
> lib/freebl/chacha20/chacha20.c:22: #define XOR(v, w) ((v) ^ (w))
> Is this macro really required?

Nah, removed.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
> lib/freebl/chacha20/chacha20.c:23: #define PLUS(x, y) ((x) + (y))
> And this one?

Removed as well.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20....
> lib/freebl/chacha20/chacha20.c:33: #define QUARTERROUND(a, b, c, d)           
 
>                                  \
> Can you make x an argument to the macro rather than rely on it being present
in
> the environment?

Done.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20/chacha20_...
> lib/freebl/chacha20/chacha20_vec.c:9: /* clang-format off */
> What happens if you don't do this here?  The code below is something of a
mess,
> how much damage would clang-format actually do?  Or do we simply accept that
> this code is read-only?

I clang-formatted only parts of the code. Mostly macros would look ugly, that
seems to be a limitation of the formatter.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/chacha20poly1305.h...
> lib/freebl/chacha20poly1305.h:12: unsigned char tagLen;
> Do we really need a configurable tag length?

As CK_NSS_AEAD_PARAMS allow to specify |ulTagLen| and are meant to be generic
for AEAD constructions I think it makes sense to leave it in? People might use
ChaCha20/Poly1305 via sftk with a different tag size.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305-...
> lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:11: /*
> clang-format off */
> This file is a bit of a WTF.

Yeah... if it makes you feel any better, Chrome is shipping that same file too.

>
https://codereview.appspot.com/277040044/diff/1/lib/freebl/poly1305/poly1305....
> lib/freebl/poly1305/poly1305.c:9: /* clang-format off */
> Same comment as before...

Formatted only parts of the code here as well, some of the code really looked
worse after clang-format. I feel that implementations of crypto algorithms are
hard to format automatically.

> https://codereview.appspot.com/277040044/diff/1/lib/util/pkcs11n.h#newcode294
> lib/util/pkcs11n.h:294: CK_BYTE_PTR  pIv;  /* This is the nonce. */
> pNonce is a better name.  Or don't we get to change that (I have no real
> knowledge of PKCS#11).

Done. It's an NSS-specific type definition so I think we get to change that.

[wtc1, 2016-01-31 03:12:02]

Review comments on patch set 3: cmd/bltest/blapitest.c

I only had time to review cmd/bltest/blapitest.c today.
Since it is self-contained, I decided to send you the
comments first.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c
File cmd/bltest/blapitest.c (right):

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:603: unsigned int adLen);

Nit: please move the bltestAEADFn typedef to line 626 so that it is next
to the other function pointer typedefs.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:823: bltestAEADFn         aeadCipher;

Nit: list this right below symmkeyCipher because these two are most
similar to each other.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:848: case bltestAES_GCM:

IMPORTANT: remove the bltestAES_GCM case.

Details:

AES-GCM and ChaCha20 are the only two AEAD ciphers NSS support. However,
in blapitest.cc, AES-GCM is treated as a "symmkeyCipher", whereas
ChaCha20 is treated as an "aeadCipher". It took me a while to figure
out why.

To treat an AEAD cipher as a "symmkeyCipher", the nonce (IV) and the
AAD need to be passed to the cipher via the context (void *cx). That's
how it is done for AES-GCM. See bltest_aes_init().

Adam handled ChaCha20 as a new type of cipher, "aeadCipher". If we
want to preserve Adam's design, we must not treat AES-GCM as an
"aeadCipher".

Ideally, we should treat both AES-GCM and ChaCha20 the same way,
either as "symmkeyCipher" (the original design) or as "aeadCipher"
(the new design). I would probably try treating ChaCha20 as a
"symmkeyCipher" first, but it may be better to treat AES-GCM as an
"aeadCipher".

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:1648: bltestSymmKeyParams *sk = &cipherInfo->params.sk;

Nit: this can be a const pointer:

    const bltestSymmKeyParams *sk = &cipherInfo->params.sk;

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:2615:
ChaCha20Poly1305_DestroyContext((ChaCha20Poly1305Context *)cipherInfo->cx,

Nit: this line seems longer than 80 characters.

[wtc1, 2016-01-31 20:00:27]

Patch set 3 LGTM. I didn't review pk11_gtest.

Tim: thank you for updating the Chromium patch to the
new ChaCha20-Poly1305 spec. I noticed two changes:
1) the nonce is increased from 8 bytes to 12 bytes.
2) the associated data and ciphertext are both padded
with zeros to multiples of 16 bytes, and their lengths
are now at the end, rather than following them
immediately.

I didn't check the spec, so please verify I understood
the changes correctly.

I'm sorry about the late review. I didn't know you're waiting for me.

High-level comments:

1. The low-level ChaCha20 and Poly1305 implementations are
third-party code. I suggest that we not reformat them so
that it will be easier to update to new versions of the
code.

2. The Chromium patch uses pk11gcmtest.c for testing.
I assume you decided to use blapitest.c for testing.

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk#newcode14
coreconf/Werror.mk:14: CC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))

IMPORTANT: I understand Werror.mk is only used on Unix platforms and
today only GCC and clang are used in Unix platforms. It would be good
to avoid the GCC/clang-only assumption if we can. So the -dumpversion
command-line option should only be used when we know the compiler is
GCC or clang, and this variable should ideally be renamed GCC_VERSION.

Also, I would expect CC_VERSION to use the commas (for example, "4.9.1")
rather than spaces (for example, "4 9 0") as version component
separators.

It is fine to deal with this in follow-up work, but I think this
should be done, because in theory someone may want to compile NSS
on HP-UX, IBM AIX, or Solaris using the OS vendors' C compilers.

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk#newcode15
coreconf/Werror.mk:15: export CC_VERSION

I don't think we need to export CC_VERSION. In fact, I don't think we
need to export CC_NAME either (line 10 above).

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile
File lib/freebl/Makefile (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:485: have_int128_support = 1

have_int128_support => HAVE_INT128_SUPPORT

Our naming convention for makefile variables uses all capital letters.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:487: ifneq (,$(filter 4.6 4.7 4.8 4.9,$(word
1,$(CC_VERSION)).$(word 2,$(CC_VERSION))))

This assumes there is no GCC 4.10 release. I verified that.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:496: endif

Delete these three lines. Our convention for makefile variables is
to leave a variable undefined/unset rather than define it as 0.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:499: ifeq ($(have_int128_support),1)

If we leave have_int128_support undefined/unset rather than define it
as 0, then we can also just test ifdef have_int128_support here.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/blapi.h
File lib/freebl/blapi.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/blapi.h#newcod...
lib/freebl/blapi.h:1009: const unsigned char *nonce, unsigned int nonceLen,
const unsigned char *ad,

Nit: on this line and line 1016, it looks nicer to move |ad| to the
same line as the related |adLen|.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
File lib/freebl/chacha20/chacha20.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
lib/freebl/chacha20/chacha20.c:15: #pragma intrinsic(_lrotr, _lrotl)

Delete _lrotr because this file doesn't define the ROTR32 macro.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
File lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
lib/freebl/chacha20/chacha20_vec.c:131: nonce[3] = REVW_BE(((unsigned
*)nonce)[3]);

These four lines look wrong. The left-hand side of the assignments should
also be cast to (unsigned *), like the right-hand side:

    ((unsigned *)nonce)[0] = REVW_BE(((unsigned *)nonce)[0]);
    ((unsigned *)nonce)[1] = REVW_BE(((unsigned *)nonce)[1]);
    ((unsigned *)nonce)[2] = REVW_BE(((unsigned *)nonce)[2]);
    ((unsigned *)nonce)[3] = REVW_BE(((unsigned *)nonce)[3]);

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20poly13...
File lib/freebl/chacha20poly1305.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20poly13...
lib/freebl/chacha20poly1305.c:29: unsigned char zeros[15] = { 0 };

Add 'const'. Maybe also add 'static', in which case the initializer
can be omitted:

  const unsigned char zeros[15] = { 0 };

or

  static const unsigned char zeros[15];

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.c
File lib/freebl/loader.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.c#newco...
lib/freebl/loader.c:1976: }

Please add these functions to the end of the file.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h
File lib/freebl/loader.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h#newco...
lib/freebl/loader.h:710: SECStatus
(*p_ChaCha20Poly1305_InitContext)(ChaCha20Poly1305Context *ctx,

Nit/Optional: the original code all have a space between '*' and the
function pointer. It would be good to be consistent with that style.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h#newco...
lib/freebl/loader.h:725: const unsigned char *nonce, unsigned int nonceLen,
const unsigned char *ad,

Nit: on this line and on line 732, it looks nicer to move |ad| to
the same line as the related |adLen|.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/manifest.mn
File lib/freebl/manifest.mn (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/manifest.mn#ne...
lib/freebl/manifest.mn:58: chacha20poly1305.h \

Nit: let's put this after blapi.h so that the list is somewhat in
alphabetical order.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
File lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:9: * block
size. */

IMPORTANT: It seems that you reformatted this file (changed indentation
and folded long lines). For third-party code, it is usually better to
NOT touch it so that it will be easier to update to new versions of the
code.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:75: free
storage */

Nit: the second line of comment should start with a '*':

    poly1305_power P[2]; /* 288 bytes, top 32 bit halves unused = 144 bytes of
                          * free storage */

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:86: bytes raw
*/

Nit: the second line of comment should start with a '*':

} poly1305_state_internal; /* 448 bytes total + 63 bytes for alignment = 511
                            * bytes raw */

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
File lib/freebl/poly1305/poly1305.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305.c:7: * domain. */

IMPORTANT: similarly, this is also third-party code, so it is better
to NOT reformat it and not change memcpy to PORT_Memcpy.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305.c:268: U32TO8_LE(&mac[12], f3);

In Bugzilla bug 917571, Peter Kasting suggested we add (uint32_t) casts
to avoid MSVC warnings about possible value truncation:

        U32TO8_LE(&mac[ 0], (uint32_t)f0); f1 += (f0 >> 32);
	U32TO8_LE(&mac[ 4], (uint32_t)f1); f2 += (f1 >> 32);
	U32TO8_LE(&mac[ 8], (uint32_t)f2); f3 += (f2 >> 32);
	U32TO8_LE(&mac[12], (uint32_t)f3);

https://codereview.appspot.com/277040044/diff/40001/lib/softoken/pkcs11i.h
File lib/softoken/pkcs11i.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/softoken/pkcs11i.h#ne...
lib/softoken/pkcs11i.h:404: /* SFTKChaCha20Poly1305Info saves the key, tag
length, nonce, and additional data

Nit: this line seems longer than 80 characters.

https://codereview.appspot.com/277040044/diff/40001/tests/cipher/cipher.txt
File tests/cipher/cipher.txt (right):

https://codereview.appspot.com/277040044/diff/40001/tests/cipher/cipher.txt#n...
tests/cipher/cipher.txt:34: 0	chacha20_poly1305_-E	ChaCha20_Poly1305_Encrypt

IMPORTANT: why is there no ChaCha20_Poly1305_Decrypt?

[mt, 2016-01-31 23:40:08]

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk#newcode15
coreconf/Werror.mk:15: export CC_VERSION
On 2016/01/31 20:00:25, wtc1 wrote:
> 
> I don't think we need to export CC_VERSION. In fact, I don't think we
> need to export CC_NAME either (line 10 above).

When recursing, this saves a shell invocation.  That was important to stop this
stuff from causing gecko builds to slow down too much.  The original patch I
landed caused a pretty big slowdown without these.  Maybe we can add a NSS_
prefix if you are concerned about namespace pollution.

[ttaubert, 2016-02-01 15:49:30]

On 2016/01/31 20:00:27, wtc1 wrote:
> Tim: thank you for updating the Chromium patch to the
> new ChaCha20-Poly1305 spec. I noticed two changes:
> 1) the nonce is increased from 8 bytes to 12 bytes.

Yes, that's correct.

> 2) the associated data and ciphertext are both padded
> with zeros to multiples of 16 bytes, and their lengths
> are now at the end, rather than following them
> immediately.

Yes, correct as well per RFC 7539
https://tools.ietf.org/html/rfc7539#section-2.8

> I'm sorry about the late review. I didn't know you're waiting for me.

No worries, my fault. I'll communicate it better next time.

> 1. The low-level ChaCha20 and Poly1305 implementations are
> third-party code. I suggest that we not reformat them so
> that it will be easier to update to new versions of the
> code.

Agreed, reverted those changes.

> 2. The Chromium patch uses pk11gcmtest.c for testing.
> I assume you decided to use blapitest.c for testing.

Adam's original patch had tests for the freebl API and for pkcs11 (in
pk11gcmtest.c). I left the freebl tests but moved the pk11 tests to
pk11_gtests/.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c
File cmd/bltest/blapitest.c (right):

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:603: unsigned int adLen);
On 2016/01/31 03:12:01, wtc1 wrote:
> 
> Nit: please move the bltestAEADFn typedef to line 626 so that it is next
> to the other function pointer typedefs.

Done.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:823: bltestAEADFn         aeadCipher;
On 2016/01/31 03:12:01, wtc1 wrote:
> 
> Nit: list this right below symmkeyCipher because these two are most
> similar to each other.

Done.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:848: case bltestAES_GCM:
On 2016/01/31 03:12:01, wtc1 wrote:
> 
> IMPORTANT: remove the bltestAES_GCM case.

Done. I'll file a follow-up to clean up blapitest.c and make AES-GCM and
aeadCipher.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:1648: bltestSymmKeyParams *sk = &cipherInfo->params.sk;
On 2016/01/31 03:12:01, wtc1 wrote:
> 
> Nit: this can be a const pointer:
> 
>     const bltestSymmKeyParams *sk = &cipherInfo->params.sk;

Done.

https://codereview.appspot.com/277040044/diff/40001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:2615:
ChaCha20Poly1305_DestroyContext((ChaCha20Poly1305Context *)cipherInfo->cx,
On 2016/01/31 03:12:02, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk#newcode14
coreconf/Werror.mk:14: CC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
On 2016/01/31 20:00:26, wtc1 wrote:
> IMPORTANT: I understand Werror.mk is only used on Unix platforms and
> today only GCC and clang are used in Unix platforms. It would be good
> to avoid the GCC/clang-only assumption if we can. So the -dumpversion
> command-line option should only be used when we know the compiler is
> GCC or clang, and this variable should ideally be renamed GCC_VERSION.

I'll file a follow-up.

> Also, I would expect CC_VERSION to use the commas (for example, "4.9.1")
> rather than spaces (for example, "4 9 0") as version component
> separators.

I think this was done to allow for easier splitting later, using $(word
1,$(CC_VERSION)) for example.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile
File lib/freebl/Makefile (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:485: have_int128_support = 1
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> have_int128_support => HAVE_INT128_SUPPORT
> 
> Our naming convention for makefile variables uses all capital letters.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:496: endif
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Delete these three lines. Our convention for makefile variables is
> to leave a variable undefined/unset rather than define it as 0.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/Makefile#newco...
lib/freebl/Makefile:499: ifeq ($(have_int128_support),1)
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> If we leave have_int128_support undefined/unset rather than define it
> as 0, then we can also just test ifdef have_int128_support here.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/blapi.h
File lib/freebl/blapi.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/blapi.h#newcod...
lib/freebl/blapi.h:1009: const unsigned char *nonce, unsigned int nonceLen,
const unsigned char *ad,
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: on this line and line 1016, it looks nicer to move |ad| to the
> same line as the related |adLen|.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
File lib/freebl/chacha20/chacha20.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
lib/freebl/chacha20/chacha20.c:15: #pragma intrinsic(_lrotr, _lrotl)
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Delete _lrotr because this file doesn't define the ROTR32 macro.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
File lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20/chach...
lib/freebl/chacha20/chacha20_vec.c:131: nonce[3] = REVW_BE(((unsigned
*)nonce)[3]);
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> These four lines look wrong. The left-hand side of the assignments should
> also be cast to (unsigned *), like the right-hand side:
> 
>     ((unsigned *)nonce)[0] = REVW_BE(((unsigned *)nonce)[0]);
>     ((unsigned *)nonce)[1] = REVW_BE(((unsigned *)nonce)[1]);
>     ((unsigned *)nonce)[2] = REVW_BE(((unsigned *)nonce)[2]);
>     ((unsigned *)nonce)[3] = REVW_BE(((unsigned *)nonce)[3]);

Good spot! Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20poly13...
File lib/freebl/chacha20poly1305.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/chacha20poly13...
lib/freebl/chacha20poly1305.c:29: unsigned char zeros[15] = { 0 };
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Add 'const'. Maybe also add 'static', in which case the initializer
> can be omitted:
> 
>   const unsigned char zeros[15] = { 0 };
> 
> or
> 
>   static const unsigned char zeros[15];

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.c
File lib/freebl/loader.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.c#newco...
lib/freebl/loader.c:1976: }
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Please add these functions to the end of the file.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h
File lib/freebl/loader.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h#newco...
lib/freebl/loader.h:710: SECStatus
(*p_ChaCha20Poly1305_InitContext)(ChaCha20Poly1305Context *ctx,
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit/Optional: the original code all have a space between '*' and the
> function pointer. It would be good to be consistent with that style.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/loader.h#newco...
lib/freebl/loader.h:725: const unsigned char *nonce, unsigned int nonceLen,
const unsigned char *ad,
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: on this line and on line 732, it looks nicer to move |ad| to
> the same line as the related |adLen|.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/manifest.mn
File lib/freebl/manifest.mn (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/manifest.mn#ne...
lib/freebl/manifest.mn:58: chacha20poly1305.h \
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: let's put this after blapi.h so that the list is somewhat in
> alphabetical order.

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
File lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:9: * block
size. */
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> IMPORTANT: It seems that you reformatted this file (changed indentation
> and folded long lines). For third-party code, it is usually better to
> NOT touch it so that it will be easier to update to new versions of the
> code.

Reverted.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:75: free
storage */
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: the second line of comment should start with a '*':
> 
>     poly1305_power P[2]; /* 288 bytes, top 32 bit halves unused = 144 bytes of
>                           * free storage */

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305-donna-x64-sse2-incremental-source.c:86: bytes raw
*/
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: the second line of comment should start with a '*':
> 
> } poly1305_state_internal; /* 448 bytes total + 63 bytes for alignment = 511
>                             * bytes raw */

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
File lib/freebl/poly1305/poly1305.c (right):

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305.c:7: * domain. */
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> IMPORTANT: similarly, this is also third-party code, so it is better
> to NOT reformat it and not change memcpy to PORT_Memcpy.

Changed it back to the formatting as in Chrome's version. I couldn't find the
original version on GitHub, it's changed a lot since this was looked at for NSS.

https://codereview.appspot.com/277040044/diff/40001/lib/freebl/poly1305/poly1...
lib/freebl/poly1305/poly1305.c:268: U32TO8_LE(&mac[12], f3);
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> In Bugzilla bug 917571, Peter Kasting suggested we add (uint32_t) casts
> to avoid MSVC warnings about possible value truncation:
> 
>         U32TO8_LE(&mac[ 0], (uint32_t)f0); f1 += (f0 >> 32);
> 	U32TO8_LE(&mac[ 4], (uint32_t)f1); f2 += (f1 >> 32);
> 	U32TO8_LE(&mac[ 8], (uint32_t)f2); f3 += (f2 >> 32);
> 	U32TO8_LE(&mac[12], (uint32_t)f3);

Done.

https://codereview.appspot.com/277040044/diff/40001/lib/softoken/pkcs11i.h
File lib/softoken/pkcs11i.h (right):

https://codereview.appspot.com/277040044/diff/40001/lib/softoken/pkcs11i.h#ne...
lib/softoken/pkcs11i.h:404: /* SFTKChaCha20Poly1305Info saves the key, tag
length, nonce, and additional data
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/277040044/diff/40001/tests/cipher/cipher.txt
File tests/cipher/cipher.txt (right):

https://codereview.appspot.com/277040044/diff/40001/tests/cipher/cipher.txt#n...
tests/cipher/cipher.txt:34: 0	chacha20_poly1305_-E	ChaCha20_Poly1305_Encrypt
On 2016/01/31 20:00:26, wtc1 wrote:
> 
> IMPORTANT: why is there no ChaCha20_Poly1305_Decrypt?

Ah, looks like I forgot to add that. Thanks. Fixed a small bug in blapitest.c to
make it work, forgot to correct the output buffer's length after
encrypting/decrypting.

[wtc1, 2016-02-01 17:25:18]

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/40001/coreconf/Werror.mk#newcode15
coreconf/Werror.mk:15: export CC_VERSION

On 2016/01/31 23:40:08, mt wrote:
> 
> When recursing, this saves a shell invocation.  That was important to stop
this
> stuff from causing gecko builds to slow down too much.  The original patch I
> landed caused a pretty big slowdown without these.

Thanks for the explanation. I am surprised that using := (instead of
=) in the CC_NAME variable assignment was not enough. In any case,
I think a comment to point this out would be very helpful.

[wtc1, 2016-02-01 17:52:31]

Patch set 4 LGTM. Thanks!

I reviewed the diffs between patch sets 3 and 4 carefully.
I suggest just a few small changes.

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c
File cmd/bltest/blapitest.c (right):

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:614: unsigned int inputLen);

Nit: I suggest we put bltestAEADFn here because the prototype
of bltestSymmCipherFn and bltestAEADFn are very similar.
This will make it easier to compare their parameter lists.

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:2468: unsigned int outputLen = maxLen;

|outputLen| is an output argument. It doesn't need to be initialized.
To avoid the confusion that it needs to be initialized to the output
buffer size, we should either leave it uninitialized or initialize it
to 0.

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:2480: cipherInfo->output.pBuf.len = outputLen;

I reviewed this new line. I don't fully understand the code,but I
verified it matches what the is_symmkeyCipher case does.

https://codereview.appspot.com/277040044/diff/60001/tests/cipher/cipher.txt
File tests/cipher/cipher.txt (right):

https://codereview.appspot.com/277040044/diff/60001/tests/cipher/cipher.txt#n...
tests/cipher/cipher.txt:35: 0	chacha20_poly1305_-D	ChaCha20_Poly1305_Decrypt

Nit: maybe we should just use one space between
chacha20_poly1305_-D and ChaCha20_Poly1305_Decrypt?
We can't align ChaCha20_Poly1305_Decrypt anyway, so we don't
need to use multiple spaces.

[ttaubert, 2016-02-01 18:17:04]

On 2016/02/01 17:25:18, wtc1 wrote:
> Thanks for the explanation. I am surprised that using := (instead of
> =) in the CC_NAME variable assignment was not enough. In any case,
> I think a comment to point this out would be very helpful.

Will add a comment.

[ttaubert, 2016-02-01 18:21:13]

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c
File cmd/bltest/blapitest.c (right):

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:614: unsigned int inputLen);
On 2016/02/01 17:52:30, wtc1 wrote:
> 
> Nit: I suggest we put bltestAEADFn here because the prototype
> of bltestSymmCipherFn and bltestAEADFn are very similar.
> This will make it easier to compare their parameter lists.

Done.

https://codereview.appspot.com/277040044/diff/60001/cmd/bltest/blapitest.c#ne...
cmd/bltest/blapitest.c:2468: unsigned int outputLen = maxLen;
On 2016/02/01 17:52:30, wtc1 wrote:
> 
> |outputLen| is an output argument. It doesn't need to be initialized.
> To avoid the confusion that it needs to be initialized to the output
> buffer size, we should either leave it uninitialized or initialize it
> to 0.

Done.

https://codereview.appspot.com/277040044/diff/60001/tests/cipher/cipher.txt
File tests/cipher/cipher.txt (right):

https://codereview.appspot.com/277040044/diff/60001/tests/cipher/cipher.txt#n...
tests/cipher/cipher.txt:35: 0	chacha20_poly1305_-D	ChaCha20_Poly1305_Decrypt
On 2016/02/01 17:52:30, wtc1 wrote:
> 
> Nit: maybe we should just use one space between
> chacha20_poly1305_-D and ChaCha20_Poly1305_Decrypt?
> We can't align ChaCha20_Poly1305_Decrypt anyway, so we don't
> need to use multiple spaces.

Done.

[wtc1, 2016-02-01 18:48:10]

Patch set 5 LGTM.

https://codereview.appspot.com/277040044/diff/80001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/80001/coreconf/Werror.mk#newcode11
coreconf/Werror.mk:11: # recursing. This was added to not slow Gecko builds down
too much.

Please remove "Gecko", or remove the the entire second sentence.

[ttaubert, 2016-02-01 18:52:14]

On 2016/02/01 18:48:10, wtc1 wrote:
>
https://codereview.appspot.com/277040044/diff/80001/coreconf/Werror.mk#newcode11
> coreconf/Werror.mk:11: # recursing. This was added to not slow Gecko builds
down
> too much.
> 
> Please remove "Gecko", or remove the the entire second sentence.

Done. The latest patchset contains a few changes that were pulled in by
rebasing.

[mt, 2016-02-07 21:47:33]

A few suggestions for improvements.

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk#newco...
coreconf/Werror.mk:25: ifeq ($(CC_NAME),gcc)
The problem here is that some versions of gcc report a name of gcc-blah-blah, so
this will have a false positive.  Also, I believe that clang is able to compile
those same files.  Though I'd double check that if I were you.  Why not use
CC_IS_GCC here?

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk#newco...
coreconf/Werror.mk:26: GCC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
I didn't think of this before, but in the case that -dumpversion writes an error
message to stderr you will be sad.  I think that the easiest change here is:
GCC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion || echo x.x.x))

That way, an error that causes you to run this command will catch the return
code from -dumpversion and set the version to "x x x". All those ifeq statements
that rely on the output being empty will then fail properly.

https://codereview.appspot.com/277040044/diff/100001/lib/freebl/Makefile
File lib/freebl/Makefile (right):

https://codereview.appspot.com/277040044/diff/100001/lib/freebl/Makefile#newc...
lib/freebl/Makefile:489: else ifeq ($(CC_NAME),gcc)
Same comment as in Werror.mk.  I think that you should try CC_IS_GCC.

https://codereview.appspot.com/277040044/diff/100001/lib/freebl/Makefile#newc...
lib/freebl/Makefile:493: ifeq (,$(filter 0 1 2 3 4,$(word 1,$(GCC_VERSION))))
There is a chance that GCC_VERSION is empty...

[mt, 2016-02-08 02:48:20]

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk#newco...
coreconf/Werror.mk:25: ifeq ($(CC_NAME),gcc)
On 2016/02/07 21:47:33, mt wrote:
> The problem here is that some versions of gcc report a name of gcc-blah-blah,
so
> this will have a false positive.  Also, I believe that clang is able to
compile
> those same files.  Though I'd double check that if I were you.  Why not use
> CC_IS_GCC here?

I see that there is a separate test for clang.  So clang compilation isn't a
concern.  Though I believe that need better detection for clang.  When I get my
linux box back I have a patch that should improve both of these.

[ttaubert, 2016-02-08 14:51:17]

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk
File coreconf/Werror.mk (right):

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk#newco...
coreconf/Werror.mk:25: ifeq ($(CC_NAME),gcc)
On 2016/02/07 21:47:33, mt wrote:
> The problem here is that some versions of gcc report a name of gcc-blah-blah,
so
> this will have a false positive.  Also, I believe that clang is able to
compile
> those same files.  Though I'd double check that if I were you.  Why not use
> CC_IS_GCC here?

Done.

https://codereview.appspot.com/277040044/diff/100001/coreconf/Werror.mk#newco...
coreconf/Werror.mk:26: GCC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
On 2016/02/07 21:47:33, mt wrote:
> I didn't think of this before, but in the case that -dumpversion writes an
error
> message to stderr you will be sad.  I think that the easiest change here is:
> GCC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion || echo x.x.x))
> 
> That way, an error that causes you to run this command will catch the return
> code from -dumpversion and set the version to "x x x". All those ifeq
statements
> that rely on the output being empty will then fail properly.

Done.

https://codereview.appspot.com/277040044/diff/100001/lib/freebl/Makefile
File lib/freebl/Makefile (right):

https://codereview.appspot.com/277040044/diff/100001/lib/freebl/Makefile#newc...
lib/freebl/Makefile:489: else ifeq ($(CC_NAME),gcc)
On 2016/02/07 21:47:33, mt wrote:
> Same comment as in Werror.mk.  I think that you should try CC_IS_GCC.

Done.

[ttaubert, 2016-02-08 14:53:22]

The latest patch set uses CC_IS_GCC to cover those versions that report a name
of gcc-foo-bar and keeps the CC_NAME==clang checks to properly cover clang that
reports itself an older version of gcc.

[ttaubert, 2016-02-08 19:00:38]

I'll wait for (or work on if needed) a final "LGTM" for the Makefile changes to
land the cipher and the suite. It's the last thing we need modulo the small
follow-ups.

[mt, 2016-02-08 21:37:53]

Patch set 7 LGTM