Support ChaCha20+Poly1305 cipher suites.

Support ChaCha20+Poly1305 cipher suites.

Patch by Adam Langley.

Upstream NSS bug https://bugzilla.mozilla.org/show_bug.cgi?id=917571

R=agl@chromium.org
BUG=310768
TEST=none

[wtc, 2013-10-18 01:14:38]

Is there a server that I can test with?

https://codereview.chromium.org/27510015/diff/1/nss/lib/freebl/chacha20/chach...
File nss/lib/freebl/chacha20/chacha20.h (right):

https://codereview.chromium.org/27510015/diff/1/nss/lib/freebl/chacha20/chach...
nss/lib/freebl/chacha20/chacha20.h:17: const unsigned char nonce[32],

The compiler doesn't complain about this.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20.c:83: input[13] = counter64 >> 32;

On a 32-bit system, size_t is 32 bits, and the compiler does not allow us to
shift a size_t for 32 bits.

I fixed the problem by using a PRUint64 local variable. There are two
alternative fixes.

1. Change the |counter| parameter to be a PRUint64/uint64_t.

2. Since we only pass 0 or 1 as the value of |counter|, |counter| can be a
PRUint32/uint32_t, and we can just store 0 in input[13].

Which solution do you prefer?

[wtc, 2013-10-18 01:16:03]

Patch set 1 is the original files from you.

Patch set 2 contains two changes by me.

[wtc, 2013-10-19 01:09:42]

agl: this CL is now ready for review.

Patch set 1 is your original NSS patch.

Patch set 3 contains my changes, after I inspected every file.

Patch set 4 contains additional changes to the ChaCha20Poly1305 functions to
conform to the freebl API.

You can review the CL as follows.

First, review the diffs between patch sets 1 and 3. Then review the diffs
between patch sets 3 and 4.

Alternatively, you can just review the diffs between patch sets 1 and 4.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:153: ((uint32_t*)nonce)[1]

Ted Krovetz's code uses |np| here:

    vec s3 = NONCE(np);

You use |nonce|, which may not be aligned on a uint32_t boundary.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:184: x12 = counter+BPI*iters+(BPI-1); x13
= 0; x14 = np[0]; x15 = np[1];

BUG: I think Ted Krovetz's code has a bug here: |x13| is always 0, which means
|x12| is not overflowing into |x13|.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:235: op[13] = REVW_BE(REVW_BE(ip[13]) ^
(x13));

You also need to add |counter| to |x12|.

Note that nothing is added to |x13| (same bug as above).

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:828: const unsigned char *nonce, unsigned int nonceLen,

Do you think this function should assume |key| and |nonce| are the right lengths
and omit the |keyLen| and |nonceLen| parameters?

[wtc, 2013-10-19 01:15:39]

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:19: typedef unsigned vec __attribute__
((vector_size (16)));

This file and poly1305-donna-x64-sse2-incremental-source.c don't compile on
Windows when targeting x64.

This file seems within reach. I suspect we just need to find the Visual C++
equivalent of "__attribute__ ((vector_size (16)))" and the __SSE2__ and 
__SSSE3__ macros.

poly1305-donna-x64-sse2-incremental-source.c uses the __int128 type. I don't
know if Visual C++ has an equivalent.

[wtc, 2013-10-19 19:51:02]

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:839: const unsigned char *input, unsigned int inputLen);

I will go back to your original design and let the _Seal and _Open functions
take |iv| and |ad| inputs. This will allow us to perform multiple _Seal or _Open
operations using the same ChaCha20Poly1305Context.

I will change the _CreateContext function to calculate the Poly1305 key and
store it in the ChaCha20Poly1305Context.

In lib/softoken, I will resurrect the sftk_ChaCha20Poly1305 wrapper functions
that match the SFTKCipher function signature.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:13: #define ROTATE(v, c) (ROTL32(v, c))

Add parentheses around v and c?

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:19: { (p)[0] = (v >>  0) & 0xff; (p)[1] = (v
>>  8) & 0xff; \

Omit ">> 0"?

Add parentheses around v?

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:23: ((PRUint32)((p)[2]) << 16) |
((PRUint32)((p)[3]) << 24)   )

Omit the parentheses around (p)[i]? I think [] binds tighter than the (PRUint32)
typecast.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:37: for (i = 20; i > 0; i -= 2) {

20 => num_rounds

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:83: input[14] = U8TO32_LITTLE(nonce);

nonce + 0?

[wtc, 2013-10-19 20:15:23]

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:16: #define PLUSONE(v) (PLUS((v), 1))

Not used. Should we use this to increment the counter block?

[wtc, 2013-10-20 23:21:27]

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:839: const unsigned char *input, unsigned int inputLen);

On 2013/10/19 19:51:02, wtc wrote:
>
> I will change the _CreateContext function to calculate the Poly1305 key and
> store it in the ChaCha20Poly1305Context.

I just realized this won't work because Poly1305 keys are one-time keys.

[agl, 2013-10-21 21:53:38]

The change to move the "context" allocation into freebl does mean that it's no
longer possible to use the ChaCha code without an allocation for each
encryption.

Previously one could imagine some code using it directly without allocation,
although if everything is intended to go via PKCS#11 then that's moot.

LGTM. (And thanks for all the work!)

https://codereview.chromium.org/27510015/diff/1/nss/lib/freebl/chacha20/chach...
File nss/lib/freebl/chacha20/chacha20.h (right):

https://codereview.chromium.org/27510015/diff/1/nss/lib/freebl/chacha20/chach...
nss/lib/freebl/chacha20/chacha20.h:17: const unsigned char nonce[32],
On 2013/10/18 01:14:38, wtc wrote:
> 
> The compiler doesn't complain about this.

I don't think that it should. The "[123]" means the same as a pointer, but it
also documents the expected amount of data. Doesn't save you if you mess it up
though :(

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20.c:83: input[13] = counter64 >> 32;
On 2013/10/18 01:14:38, wtc wrote:
> I fixed the problem by using a PRUint64 local variable. There are two
> alternative fixes.
> 
> 1. Change the |counter| parameter to be a PRUint64/uint64_t.

I think that using a PRUint64/uint64_t is best.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:153: ((uint32_t*)nonce)[1]
On 2013/10/19 01:09:42, wtc wrote:
> 
> Ted Krovetz's code uses |np| here:
> 
>     vec s3 = NONCE(np);
> 
> You use |nonce|, which may not be aligned on a uint32_t boundary.

You're correct. I guess the code needs to memcpy from |nonce| to a uint32_t
|nonce_low| and |nonce_high|? (I've worked on x86 so long that alignment issues
often escape me.)

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:184: x12 = counter+BPI*iters+(BPI-1); x13
= 0; x14 = np[0]; x15 = np[1];
On 2013/10/19 01:09:42, wtc wrote:
> 
> BUG: I think Ted Krovetz's code has a bug here: |x13| is always 0, which means
> |x12| is not overflowing into |x13|.

That's correct. I think it's a performance optimisation rather than an omission.
In the OpenSSL code I test whether the input length is greater than (1ull <<
32)*64-64 to avoid this issue. Since that's 256GB, and it's a one-shot API, that
didn't seem to be a problem.

https://codereview.chromium.org/27510015/diff/12001/nss/lib/freebl/chacha20/c...
nss/lib/freebl/chacha20/chacha20_vec.c:235: op[13] = REVW_BE(REVW_BE(ip[13]) ^
(x13));
On 2013/10/19 01:09:42, wtc wrote:
> 
> You also need to add |counter| to |x12|.
> 
> Note that nothing is added to |x13| (same bug as above).

If the counter overflow were to be added there would need to be several other
changes but, as noted above, I don't think it's an omission.

https://codereview.chromium.org/27510015/diff/87001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/87001/nss/lib/freebl/blapi.h#ne...
nss/lib/freebl/blapi.h:828: const unsigned char *ad, unsigned int adLen,
I think this is fine, but it limits the API to handling 2**32 bytes in one shot.
This makes the lack of counter overflow in Ted's code unimportant.

https://codereview.chromium.org/27510015/diff/207001/nss.gyp
File nss.gyp (right):

https://codereview.chromium.org/27510015/diff/207001/nss.gyp#newcode1079
nss.gyp:1079: ['target_arch=="x64" and OS!="win"', {
That's a shame :(

What was the issue? I was hoping that, by avoiding asm code, that it would build
on Windows. If it's just a question of debugging-via-trybot then I think we
should disable the ciphersuites until we have this.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:828: const unsigned char *nonce, unsigned int nonceLen,
On 2013/10/19 01:09:42, wtc wrote:
> 
> Do you think this function should assume |key| and |nonce| are the right
lengths
> and omit the |keyLen| and |nonceLen| parameters?

I'm ok with that if you are. (There are tweaks to ChaCha20 that let it take a 16
byte key or a 24 byte nonce, but we don't support either. It's possible that we
could in the future however.)

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:13: #define ROTATE(v, c) (ROTL32(v, c))
On 2013/10/19 19:51:02, wtc wrote:
> 
> Add parentheses around v and c?

I think that's generally good style.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:16: #define PLUSONE(v) (PLUS((v), 1))
On 2013/10/19 20:15:23, wtc wrote:
> 
> Not used. Should we use this to increment the counter block?

I think it can be removed. It's a left-over from djb's original code.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:19: { (p)[0] = (v >>  0) & 0xff; (p)[1] = (v
>>  8) & 0xff; \
On 2013/10/19 19:51:02, wtc wrote:

> Omit ">> 0"?

Sure.

> Add parentheses around v?

Good idea.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:23: ((PRUint32)((p)[2]) << 16) |
((PRUint32)((p)[3]) << 24)   )
On 2013/10/19 19:51:02, wtc wrote:
> 
> Omit the parentheses around (p)[i]? I think [] binds tighter than the
(PRUint32)
> typecast.

I also think that the cast binds tighter, also the parens do remove the
possibility of being wrong. (This is, again, code taken from the public domain
sources.)

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:37: for (i = 20; i > 0; i -= 2) {
On 2013/10/19 19:51:02, wtc wrote:
> 
> 20 => num_rounds

Yes. (Note, you're commenting as if I can make the edit :)

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:19: typedef unsigned vec __attribute__
((vector_size (16)));
On 2013/10/19 01:15:39, wtc wrote:
> 
> This file and poly1305-donna-x64-sse2-incremental-source.c don't compile on
> Windows when targeting x64.
> 
> This file seems within reach. I suspect we just need to find the Visual C++
> equivalent of "__attribute__ ((vector_size (16)))" and the __SSE2__ and 
> __SSSE3__ macros.

Thanks. Let's land without building on Windows and I'll see what I can do.

> poly1305-donna-x64-sse2-incremental-source.c uses the __int128 type. I don't
> know if Visual C++ has an equivalent.

It doesn't. That's a shame. However, the plan is to bump up AES-GCM on platforms
where we have AES-NI and hopefully that includes most Windows machines. (And on
machines without AES-NI, I bet the fallback Poly1305 implementation is still
faster than AES-GCM.)

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
File nss/lib/freebl/chacha20poly1305.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
nss/lib/freebl/chacha20poly1305.c:129: if (ctx->tagLen == 0 || ctx->tagLen >
sizeof(tag)) {
This test shouldn't be needed now?

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
nss/lib/freebl/chacha20poly1305.c:166: if (ctx->tagLen == 0 || ctx->tagLen >
sizeof(tag)) {
This test shouldn't be needed now?

[wtc, 2013-10-22 22:36:42]

Thank you for your comments. Please review patch set 9.

You may review the diffs between patch sets 4 and 9.

https://codereview.chromium.org/27510015/diff/207001/nss.gyp
File nss.gyp (right):

https://codereview.chromium.org/27510015/diff/207001/nss.gyp#newcode1079
nss.gyp:1079: ['target_arch=="x64" and OS!="win"', {

On 2013/10/21 21:53:39, agl wrote:
> That's a shame :(
> 
> What was the issue?

I described the issue in another file. In short, chacha20_vec.c uses the GCC
__attribute__ vector_size extension, and
poly1305-donna-x64-sse2-incremental-source.c uses the __int128 type. Both of
them are unavailable in Visual C++.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:828: const unsigned char *nonce, unsigned int nonceLen,

I decided to leave the length arguments there.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:13: #define ROTATE(v, c) (ROTL32(v, c))
On 2013/10/21 21:53:39, agl wrote:
> On 2013/10/19 19:51:02, wtc wrote:
> > 
> > Add parentheses around v and c?
> 
> I think that's generally good style.

Done.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:16: #define PLUSONE(v) (PLUS((v), 1))
On 2013/10/21 21:53:39, agl wrote:
> On 2013/10/19 20:15:23, wtc wrote:
> > 
> > Not used. Should we use this to increment the counter block?
> 
> I think it can be removed. It's a left-over from djb's original code.

Done.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:19: { (p)[0] = (v >>  0) & 0xff; (p)[1] = (v
>>  8) & 0xff; \
On 2013/10/21 21:53:39, agl wrote:
> On 2013/10/19 19:51:02, wtc wrote:
> 
> > Omit ">> 0"?
> 
> Sure.
> 
> > Add parentheses around v?
> 
> Good idea.

Done.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:23: ((PRUint32)((p)[2]) << 16) |
((PRUint32)((p)[3]) << 24)   )
On 2013/10/21 21:53:39, agl wrote:
>
> I also think that the cast binds tighter, also the parens do remove the
> possibility of being wrong.

OK, I won't remove the parentheses.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20.c:37: for (i = 20; i > 0; i -= 2) {
On 2013/10/21 21:53:39, agl wrote:
> On 2013/10/19 19:51:02, wtc wrote:
> > 
> > 20 => num_rounds
> 
> Yes. (Note, you're commenting as if I can make the edit :)

Done.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:19: typedef unsigned vec __attribute__
((vector_size (16)));
On 2013/10/21 21:53:39, agl wrote:
> 
> Thanks. Let's land without building on Windows and I'll see what I can do.

I looked into the "__attribute__ ((vector_size (16)))" problem. It is a GCC
extension that allows us to use normal arithmetic and logical operators such as
+ and ^ on integer variables declared with this attribute, and the compiler will
generate vector instructions that operate on the four integers in one shot. It
is syntactic sugar, because there are equivalent compiler intrinsics.

Visual C++ doesn't support this syntactic sugar, so to port this file to Visual
C++ we will need to convert code such as:

  a += b; d ^= a;

to compiler intrinsics that operate on variables of the type __m128i.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
File nss/lib/freebl/chacha20poly1305.c (right):

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
nss/lib/freebl/chacha20poly1305.c:129: if (ctx->tagLen == 0 || ctx->tagLen >
sizeof(tag)) {
On 2013/10/21 21:53:39, agl wrote:
> This test shouldn't be needed now?

Done.

https://codereview.chromium.org/27510015/diff/207001/nss/lib/freebl/chacha20p...
nss/lib/freebl/chacha20poly1305.c:166: if (ctx->tagLen == 0 || ctx->tagLen >
sizeof(tag)) {
On 2013/10/21 21:53:39, agl wrote:
> This test shouldn't be needed now?

Done.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/blapi.h
File nss/lib/freebl/blapi.h (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/blapi.h#n...
nss/lib/freebl/blapi.h:833: unsigned int tagLen);

I added the ChaCha20Poly1305_InitContext function to initialize a
ChaCha20Poly1305Context structure allocated by the caller. This should address
your concern of an extra heap allocation.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:148: ((uint32_t*)nonce)[1]

Note: I didn't fix the alignment issue of |nonce|. There are too many ifdefs in
this file that I had a hard time understanding all the build configurations.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:179: x12 = (counter &
0xffffffff)+BPI*iters+(BPI-1); x13 = counter >> 32;

Could you please check my changes to x12 and x13 are correct?
Edit

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:231: op[13] = REVW_BE(REVW_BE(ip[13]) ^
(x13 + (counter >> 32)));

Similarly, please check my changes to these to lines are correct.

These changes can be simplified if we change |counter| back to an unsigned int.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/util/pkcs11n.h
File nss/lib/util/pkcs11n.h (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/util/pkcs11n.h#n...
nss/lib/util/pkcs11n.h:294: CK_ULONG     ulTagLen;

I changed this member to be a length in bytes. This will require a corresponding
change to net/third_party/nss/ssl.

[agl, 2013-10-23 14:24:27]

LGTM

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
File nss/lib/freebl/chacha20/chacha20_vec.c (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:179: x12 = (counter &
0xffffffff)+BPI*iters+(BPI-1); x13 = counter >> 32;
On 2013/10/22 22:36:42, wtc wrote:
> 
> Could you please check my changes to x12 and x13 are correct?
> Edit

Agreed. Although the counter doesn't overflow in the vector case so any users
who set the counter such that overflow was possible are going to have a bad
time. I believe this is fine because the plaintext length argument to the AEAD
is an unsigned int now and so cannot specify a length that can overflow.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/freebl/chacha20/...
nss/lib/freebl/chacha20/chacha20_vec.c:231: op[13] = REVW_BE(REVW_BE(ip[13]) ^
(x13 + (counter >> 32)));
On 2013/10/22 22:36:42, wtc wrote:
> 
> Similarly, please check my changes to these to lines are correct.
> 
> These changes can be simplified if we change |counter| back to an unsigned
int.

Done.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11c.c
File nss/lib/softoken/pkcs11c.c (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11c...
nss/lib/softoken/pkcs11c.c:479: sftk_ChaCha20Poly1305_CreateContext(const
unsigned char *key, unsigned int keyLen,
(not about about local style, but this spills over 80 chars as do several
others.)

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11i.h
File nss/lib/softoken/pkcs11i.h (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11i...
nss/lib/softoken/pkcs11i.h:404: /* SFTKChaCha20Poly1305Info saves the key, tag
length, nonce, and additional data
ditto about 80 chars.

[agl, 2013-10-23 14:45:33]

p.s. www.google.com etc are still running the old code where the lengths are
prefixes, not suffixes, until next week.

[wtc, 2013-10-23 20:04:20]

I'm going to commit patch set 11.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11c.c
File nss/lib/softoken/pkcs11c.c (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11c...
nss/lib/softoken/pkcs11c.c:479: sftk_ChaCha20Poly1305_CreateContext(const
unsigned char *key, unsigned int keyLen,
On 2013/10/23 14:24:28, agl wrote:
> (not about about local style, but this spills over 80 chars as do several
> others.)

Done.

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11i.h
File nss/lib/softoken/pkcs11i.h (right):

https://codereview.chromium.org/27510015/diff/437001/nss/lib/softoken/pkcs11i...
nss/lib/softoken/pkcs11i.h:404: /* SFTKChaCha20Poly1305Info saves the key, tag
length, nonce, and additional data
On 2013/10/23 14:24:28, agl wrote:
> ditto about 80 chars.

Done.

[commit-bot: I haz the power, 2013-10-23 20:04:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27510015/847001

[commit-bot: I haz the power, 2013-10-23 20:05:07]

Change committed as 230497