Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

Looks fine to me.

--Sean

On 5/30/18 5:39 PM, Jamil Nimeh wrote:

Hopefully the final set of updates before committing this feature:

  * Updated the ChaChaEngine implementations in ChaCha20Cipher.java to
properly conform to Cipher's specification when the output buffer
used in doUpdate is too small.  Instead of throwing
IndexOutOfBoundsException like it was doing, it will now throw a
ShortBufferException per the spec.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.09

Thanks,

--Jamil


On 05/22/2018 01:43 PM, Jamil Nimeh wrote:

A couple updates:

  * Touched up the comments surrounding init/wrap/unwrap methods. 
None of these affect public javadocs.

  * Added an implementation for engineGetParameters.  This was
something that just got forgotten from the initial development of
the cipher when there were no AlgorithmParameter implementations.
  o There are a couple tests added to ChaCha20Poly1305ParamTest to
exercise these new code paths.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.08

Thanks,

--Jamil


On 5/21/2018 11:36 AM, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ).  Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hopefully the final set of updates before committing this feature:

 * Updated the ChaChaEngine implementations in ChaCha20Cipher.java to
   properly conform to Cipher's specification when the output buffer
   used in doUpdate is too small.  Instead of throwing
   IndexOutOfBoundsException like it was doing, it will now throw a
   ShortBufferException per the spec.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.09

Thanks,

--Jamil


On 05/22/2018 01:43 PM, Jamil Nimeh wrote:

A couple updates:

  * Touched up the comments surrounding init/wrap/unwrap methods. 
None of these affect public javadocs.
  * Added an implementation for engineGetParameters.  This was
something that just got forgotten from the initial development of
the cipher when there were no AlgorithmParameter implementations.
  o There are a couple tests added to ChaCha20Poly1305ParamTest to
exercise these new code paths.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.08

Thanks,

--Jamil


On 5/21/2018 11:36 AM, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ).  Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

The updated webrev looks good. I have no further comments.

--Sean

On 5/22/18 4:43 PM, Jamil Nimeh wrote:

A couple updates:

  * Touched up the comments surrounding init/wrap/unwrap methods. None
of these affect public javadocs.
  * Added an implementation for engineGetParameters.  This was something
that just got forgotten from the initial development of the cipher
when there were no AlgorithmParameter implementations.
  o There are a couple tests added to ChaCha20Poly1305ParamTest to
exercise these new code paths.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.08

Thanks,

--Jamil


On 5/21/2018 11:36 AM, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the 
specification, if a null AP or APS is provided for these ciphers, a 
random nonce will be generated and the counter will be set to 1, just 
as is currently done with valid forms of init that don't take an AP 
or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the 
spec in Cipher, this is only true for ENCRYPT_MODE and will throw 
InvalidKeyException when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it 
would fail in checks in javax.crypto.Cipher before it ever made it 
down to my code. I'm pretty confident the init sequence is OK.  I 
did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

A couple updates:

 * Touched up the comments surrounding init/wrap/unwrap methods. None
   of these affect public javadocs.
 * Added an implementation for engineGetParameters.  This was something
   that just got forgotten from the initial development of the cipher
   when there were no AlgorithmParameter implementations.
 o There are a couple tests added to ChaCha20Poly1305ParamTest to
   exercise these new code paths.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.08

Thanks,

--Jamil


On 5/21/2018 11:36 AM, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the 
specification, if a null AP or APS is provided for these ciphers, a 
random nonce will be generated and the counter will be set to 1, just 
as is currently done with valid forms of init that don't take an AP 
or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the 
spec in Cipher, this is only true for ENCRYPT_MODE and will throw 
InvalidKeyException when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it 
would fail in checks in javax.crypto.Cipher before it ever made it 
down to my code. I'm pretty confident the init sequence is OK.  I 
did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Wow, good catch.  This is something that got left by the wayside, 
probably from one of the earliest revisions of the code when we didn't 
have an AlgorithmParameters implementation.  I'll get this fixed up 
today.  Shouldn't take too long to get this working.  I think it should 
return null if it is ChaCha20 since there's no AlgorithmParameters 
definition for ChaCha20, and return a ChaCha20Poly1305Parameters for 
ChaCha20-Poly1305.


For the latter Cipher, it shouldn't matter how you init the Cipher.  We 
can return the AP with the nonce value, however it came into being 
(random, AP or APS).


--Jamil


On 05/22/2018 10:29 AM, Sean Mullan wrote:

Question on engineGetParameters() in ChaCha20Cipher.java:

 231 protected AlgorithmParameters engineGetParameters() {
 232 return null;
 233 }

Shouldn't this return the params that are passed into engineInit()? 
Also, when null is passed in and the params are generated using 
default values, this method should return those params. From 
CipherSpi.engineInit(int opmode, Key key, SecureRandom):


"The generated parameters can be retrieved using engineGetParameters 
or engineGetIV (if the parameter is an IV)."


--Sean

On 5/21/18 2:36 PM, Jamil Nimeh wrote:

And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ). Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code. I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
    recommending callers reinitialize the Cipher to use different
    nonces after each complete encryption or decryption (similar
    language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
    cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

Question on engineGetParameters() in ChaCha20Cipher.java:

 231 protected AlgorithmParameters engineGetParameters() {
 232 return null;
 233 }

Shouldn't this return the params that are passed into engineInit()? 
Also, when null is passed in and the params are generated using default 
values, this method should return those params. From 
CipherSpi.engineInit(int opmode, Key key, SecureRandom):


"The generated parameters can be retrieved using engineGetParameters or 
engineGetIV (if the parameter is an IV)."


--Sean

On 5/21/18 2:36 PM, Jamil Nimeh wrote:

And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme in 
RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods will 
throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the specification, 
if a null AP or APS is provided for these ciphers, a random nonce will 
be generated and the counter will be set to 1, just as is currently 
done with valid forms of init that don't take an AP or APS object 
(e.g. Cipher.init(int, Key, SecureRandom) ).  Per the spec in Cipher, 
this is only true for ENCRYPT_MODE and will throw InvalidKeyException 
when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the 
initial state, which is then used for generating an authentication 
key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi John, comments in-line

On 5/21/2018 8:30 PM, sha.ji...@oracle.com wrote:


Hi Jamil,

-- ChaCha20Cipher.java
 497 /**
 498  * Perform additional initialization actions based on the key 
and operation

 499  * type.
 500  *
 501  * @param opmode the type of operation to do.  This value 
must be either
 502  *  {@code Cipher.ENCRYPT_MODE} or {@code 
Cipher.DECRYPT_MODE}

 503  * @param key a 256-bit key suitable for ChaCha20
 504  * @param newNonce the new nonce value for this initialization.
 505  *
 506  * @throws UnsupportedOperationException if the {@code 
opmode} parameter

 507  *  is {@code Cipher.WRAP_MODE} or {@code Cipher.UNWRAP_MODE}
 508  *  (currently unsupported).
 509  * @throws InvalidKeyException if the {@code opmode} 
parameter is not
 510  *  {@code Cipher.ENCRYPT_MODE} or {@code 
Cipher.DECRYPT_MODE}, or

 511  *  if the key format is not {@code RAW}.
 512  */
 513 private void init(int opmode, Key key, byte[] newNonce)
 514 throws InvalidKeyException {
 515 if ((opmode == Cipher.WRAP_MODE) || (opmode == 
Cipher.UNWRAP_MODE)) {

 516 throw new UnsupportedOperationException(
 517 "WRAP_MODE and UNWRAP_MODE are not currently 
supported");

 518 } else if ((opmode != Cipher.ENCRYPT_MODE) &&
 519 (opmode != Cipher.DECRYPT_MODE)) {
 520 throw new InvalidKeyException("Unknown opmode: " + 
opmode);

 521 }
With my understanding, WRAP_MODE and UNWRAP_MODE can be allowed, 
though they are not supported yet. But the doc on param opmode stats 
the allowed modes are only ENCRYPT_MODE and DECRYPT_MODE.
In fact, the doc on param opmode in method engineInit(int opmode, Key 
key, SecureRandom random) implies that WRAP_MODE can be accepted.


JN: Given that Cipher.init and CipherSpi.init both state that UOE should 
be thrown if WRAP/UNWRAP_MODE is provided and the underlying CipherSpi 
does not support those modes, you should NOT accept those values for the 
opmode parameter.  If and when we do support those modes of operation, 
then I will loosen the check and allow it.  But you shouldn't allow a 
Cipher object to be initialized with a mode that you know isn't 
supported.  I would much rather see things behave in a fail-fast manner 
where it is plausible, and this is one of those cases.


I will clean up the method header comments for engineInit to reflect the 
change I made down in the private init common method.


 242  * @param opmode the type of operation to do.  This value may 
not be
 243  *  {@code Cipher.DECRYPT_MODE} or {@code Cipher.UNWRAP} 
mode because

 244  *  it must generate random parameters like the nonce.
BTW, Cipher.UNWRAP or Cipher.UNWRAP_MODE?


It should be UNWRAP_MODE, good catch.



Best regards,
John Jiang

On 22/05/2018 02:36, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ).  Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[sha . jiang]

Hi Jamil,

-- ChaCha20Cipher.java
 497 /**
 498  * Perform additional initialization actions based on the key 
and operation

 499  * type.
 500  *
 501  * @param opmode the type of operation to do.  This value must 
be either

 502  *  {@code Cipher.ENCRYPT_MODE} or {@code Cipher.DECRYPT_MODE}
 503  * @param key a 256-bit key suitable for ChaCha20
 504  * @param newNonce the new nonce value for this initialization.
 505  *
 506  * @throws UnsupportedOperationException if the {@code opmode} 
parameter

 507  *  is {@code Cipher.WRAP_MODE} or {@code Cipher.UNWRAP_MODE}
 508  *  (currently unsupported).
 509  * @throws InvalidKeyException if the {@code opmode} parameter 
is not
 510  *  {@code Cipher.ENCRYPT_MODE} or {@code 
Cipher.DECRYPT_MODE}, or

 511  *  if the key format is not {@code RAW}.
 512  */
 513 private void init(int opmode, Key key, byte[] newNonce)
 514 throws InvalidKeyException {
 515 if ((opmode == Cipher.WRAP_MODE) || (opmode == 
Cipher.UNWRAP_MODE)) {

 516 throw new UnsupportedOperationException(
 517 "WRAP_MODE and UNWRAP_MODE are not currently 
supported");

 518 } else if ((opmode != Cipher.ENCRYPT_MODE) &&
 519 (opmode != Cipher.DECRYPT_MODE)) {
 520 throw new InvalidKeyException("Unknown opmode: " + 
opmode);

 521 }
With my understanding, WRAP_MODE and UNWRAP_MODE can be allowed, though 
they are not supported yet. But the doc on param opmode stats the 
allowed modes are only ENCRYPT_MODE and DECRYPT_MODE.
In fact, the doc on param opmode in method engineInit(int opmode, Key 
key, SecureRandom random) implies that WRAP_MODE can be accepted.
 242  * @param opmode the type of operation to do.  This value may 
not be
 243  *  {@code Cipher.DECRYPT_MODE} or {@code Cipher.UNWRAP} 
mode because

 244  *  it must generate random parameters like the nonce.
BTW, Cipher.UNWRAP or Cipher.UNWRAP_MODE?

Best regards,
John Jiang

On 22/05/2018 02:36, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the 
specification, if a null AP or APS is provided for these ciphers, a 
random nonce will be generated and the counter will be set to 1, just 
as is currently done with valid forms of init that don't take an AP 
or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the 
spec in Cipher, this is only true for ENCRYPT_MODE and will throw 
InvalidKeyException when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it 
would fail in checks in javax.crypto.Cipher before it ever made it 
down to my code. I'm pretty confident the init sequence is OK.  I 
did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme in 
RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods will 
throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the specification, 
if a null AP or APS is provided for these ciphers, a random nonce will 
be generated and the counter will be set to 1, just as is currently 
done with valid forms of init that don't take an AP or APS object 
(e.g. Cipher.init(int, Key, SecureRandom) ).  Per the spec in Cipher, 
this is only true for ENCRYPT_MODE and will throw InvalidKeyException 
when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the 
initial state, which is then used for generating an authentication 
key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the characteristics and behavior of 
the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[sha . jiang]

Hi Jamil,

On 19/05/2018 22:40, Jamil Nimeh wrote:

Hi John, comments below:

On 5/19/2018 1:31 AM, sha.ji...@oracle.com wrote:


Hi Jamil,

On 19/05/2018 07:27, Jamil Nimeh wrote:


Hi John,

Yes, the second call must throw IllegalStateException.  See the 
class description in javax.crypto.Cipher where it talks about AEAD 
modes and AAD processing.  It states that all AAD has to be supplied 
before update and/or doFinal methods are invoked.  This constraint 
is also talked about in Cipher.updateAAD's javadoc.



I see.

I was confused by a corner case that passing an empty byte[] (or a 
ByteBuffer that has no remaining byte) to the second call on 
Cipher.updateAAD().
In fact, in this case, the second call doesn't invoke the underlying 
engineUpdateAAD(), so IllegalStateException won't be thrown.
I see what you're talking about, but I can't protect against that (if 
it even needs protecting).  That zero-length check is in Cipher so my 
code is never invoked (as you stated).  The upside is the behavior of 
ChaCha20 in this case would be consistent with any other AEAD cipher.  
Changing that behavior (and I'm not convinced we should change it) is 
something that goes beyond the scope of this JEP.


I think it is safe in this case to not throw ISE since you're not 
adding any AAD data, so for ChaCha20 you would not affect the AAD 
length or padding, and any buffer that would affect it will most 
definitely cause ISE to be thrown.  I think we're OK here (as with any 
other AEAD cipher).

Never mind, I have no concern on this point.

Best regards,
John Jiang


Thanks,
--Jamil


Best regards,
John Jiang


Thanks for the catch on the double-space.

--Jamil

On 05/18/2018 04:06 PM, sha.ji...@oracle.com wrote:


Hi Jamil,

-- ChaCha20Cipher.java
 430 } else if (aadDone) {
 431 // No AAD updates allowed after the PT/CT update 
method  is called
 432 throw new IllegalStateException("Attempted to 
update AAD on " +

 433 "Cipher after plaintext/ciphertext update");

Please consider the below case,
cipher.updateAAD();
cipher.update();
cipher.updateAAD();
Should the second call on updateAAD() throw IllegalStateException?

Minor: Two spaces between "method" and "is" on line 431.

Best regards,
John Jiang

On 17/05/2018 03:05, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. 
Previously, a non-null AP or APS object was required. In order to 
better conform to the specification, if a null AP or APS is 
provided for these ciphers, a random nonce will be generated and 
the counter will be set to 1, just as is currently done with valid 
forms of init that don't take an AP or APS object (e.g. 
Cipher.init(int, Key, SecureRandom) ).  Per the spec in Cipher, 
this is only true for ENCRYPT_MODE and will throw 
InvalidKeyException when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in 
the ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever 
made it down to my code.  I'm pretty confident the init sequence 
is OK. I did move the setting of a few data members toward the 
end of the init sequence but setting the key and nonce is 
necessary before creating the initial state, which is then used 
for generating an authentication key for AEAD mode and generating 
keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor 
stuff mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use
different nonces after each complete encryption or
decryption (similar language to what exists already for
AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 
implementations and only retain the key bytes as an object 
field (thanks Thomas for catching this)


* Added additional protections against key/nonce reuse.  This 
is a behavioral change to ChaCha20 and ChaCha20-Poly1305.  
Instances of these ciphers will no 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi John, comments below:

On 5/19/2018 1:31 AM, sha.ji...@oracle.com wrote:


Hi Jamil,

On 19/05/2018 07:27, Jamil Nimeh wrote:


Hi John,

Yes, the second call must throw IllegalStateException.  See the class 
description in javax.crypto.Cipher where it talks about AEAD modes 
and AAD processing.  It states that all AAD has to be supplied before 
update and/or doFinal methods are invoked.  This constraint is also 
talked about in Cipher.updateAAD's javadoc.



I see.

I was confused by a corner case that passing an empty byte[] (or a 
ByteBuffer that has no remaining byte) to the second call on 
Cipher.updateAAD().
In fact, in this case, the second call doesn't invoke the underlying 
engineUpdateAAD(), so IllegalStateException won't be thrown.
I see what you're talking about, but I can't protect against that (if it 
even needs protecting).  That zero-length check is in Cipher so my code 
is never invoked (as you stated).  The upside is the behavior of 
ChaCha20 in this case would be consistent with any other AEAD cipher.  
Changing that behavior (and I'm not convinced we should change it) is 
something that goes beyond the scope of this JEP.


I think it is safe in this case to not throw ISE since you're not adding 
any AAD data, so for ChaCha20 you would not affect the AAD length or 
padding, and any buffer that would affect it will most definitely cause 
ISE to be thrown.  I think we're OK here (as with any other AEAD cipher).


Thanks,
--Jamil


Best regards,
John Jiang


Thanks for the catch on the double-space.

--Jamil

On 05/18/2018 04:06 PM, sha.ji...@oracle.com wrote:


Hi Jamil,

-- ChaCha20Cipher.java
 430 } else if (aadDone) {
 431 // No AAD updates allowed after the PT/CT update 
method  is called
 432 throw new IllegalStateException("Attempted to 
update AAD on " +

 433 "Cipher after plaintext/ciphertext update");

Please consider the below case,
cipher.updateAAD();
cipher.update();
cipher.updateAAD();
Should the second call on updateAAD() throw IllegalStateException?

Minor: Two spaces between "method" and "is" on line 431.

Best regards,
John Jiang

On 17/05/2018 03:05, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter 
will be set to 1, just as is currently done with valid forms of 
init that don't take an AP or APS object (e.g. Cipher.init(int, 
Key, SecureRandom) ).  Per the spec in Cipher, this is only true 
for ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is 
OK.  I did move the setting of a few data members toward the end 
of the init sequence but setting the key and nonce is necessary 
before creating the initial state, which is then used for 
generating an authentication key for AEAD mode and generating 
keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor 
stuff mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse.  This is 
a behavioral change to ChaCha20 and ChaCha20-Poly1305.  
Instances of these ciphers will no longer allow you to do 
subsequent doUpdate/doFinal calls after the first doFinal 
without re-initializing the cipher with 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[sha . jiang]

Hi Jamil,

On 19/05/2018 07:27, Jamil Nimeh wrote:


Hi John,

Yes, the second call must throw IllegalStateException.  See the class 
description in javax.crypto.Cipher where it talks about AEAD modes and 
AAD processing.  It states that all AAD has to be supplied before 
update and/or doFinal methods are invoked.  This constraint is also 
talked about in Cipher.updateAAD's javadoc.



I see.

I was confused by a corner case that passing an empty byte[] (or a 
ByteBuffer that has no remaining byte) to the second call on 
Cipher.updateAAD().
In fact, in this case, the second call doesn't invoke the underlying 
engineUpdateAAD(), so IllegalStateException won't be thrown.


Best regards,
John Jiang


Thanks for the catch on the double-space.

--Jamil

On 05/18/2018 04:06 PM, sha.ji...@oracle.com wrote:


Hi Jamil,

-- ChaCha20Cipher.java
 430 } else if (aadDone) {
 431 // No AAD updates allowed after the PT/CT update 
method  is called
 432 throw new IllegalStateException("Attempted to update 
AAD on " +

 433 "Cipher after plaintext/ciphertext update");

Please consider the below case,
cipher.updateAAD();
cipher.update();
cipher.updateAAD();
Should the second call on updateAAD() throw IllegalStateException?

Minor: Two spaces between "method" and "is" on line 431.

Best regards,
John Jiang

On 17/05/2018 03:05, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ).  Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi John,

Yes, the second call must throw IllegalStateException.  See the class 
description in javax.crypto.Cipher where it talks about AEAD modes and 
AAD processing.  It states that all AAD has to be supplied before update 
and/or doFinal methods are invoked.  This constraint is also talked 
about in Cipher.updateAAD's javadoc.


Thanks for the catch on the double-space.

--Jamil

On 05/18/2018 04:06 PM, sha.ji...@oracle.com wrote:


Hi Jamil,

-- ChaCha20Cipher.java
 430 } else if (aadDone) {
 431 // No AAD updates allowed after the PT/CT update 
method  is called
 432 throw new IllegalStateException("Attempted to update 
AAD on " +

 433 "Cipher after plaintext/ciphertext update");

Please consider the below case,
cipher.updateAAD();
cipher.update();
cipher.updateAAD();
Should the second call on updateAAD() throw IllegalStateException?

Minor: Two spaces between "method" and "is" on line 431.

Best regards,
John Jiang

On 17/05/2018 03:05, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the 
specification, if a null AP or APS is provided for these ciphers, a 
random nonce will be generated and the counter will be set to 1, just 
as is currently done with valid forms of init that don't take an AP 
or APS object (e.g. Cipher.init(int, Key, SecureRandom) ). Per the 
spec in Cipher, this is only true for ENCRYPT_MODE and will throw 
InvalidKeyException when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it 
would fail in checks in javax.crypto.Cipher before it ever made it 
down to my code. I'm pretty confident the init sequence is OK.  I 
did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[sha . jiang]

Hi Jamil,

-- ChaCha20Cipher.java
 430 } else if (aadDone) {
 431 // No AAD updates allowed after the PT/CT update 
method  is called
 432 throw new IllegalStateException("Attempted to update 
AAD on " +

 433 "Cipher after plaintext/ciphertext update");

Please consider the below case,
cipher.updateAAD();
cipher.update();
cipher.updateAAD();
Should the second call on updateAAD() throw IllegalStateException?

Minor: Two spaces between "method" and "is" on line 431.

Best regards,
John Jiang

On 17/05/2018 03:05, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS 
object was required.  In order to better conform to the specification, 
if a null AP or APS is provided for these ciphers, a random nonce will 
be generated and the counter will be set to 1, just as is currently 
done with valid forms of init that don't take an AP or APS object 
(e.g. Cipher.init(int, Key, SecureRandom) ).  Per the spec in Cipher, 
this is only true for ENCRYPT_MODE and will throw InvalidKeyException 
when done in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the 
initial state, which is then used for generating an authentication 
key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the characteristics and behavior of 
the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with Cipher's 
specification when forms of init that take AlgorithmParameters or 
AlgorithmParameterSpec are used. Previously, a non-null AP or APS object 
was required.  In order to better conform to the specification, if a 
null AP or APS is provided for these ciphers, a random nonce will be 
generated and the counter will be set to 1, just as is currently done 
with valid forms of init that don't take an AP or APS object (e.g. 
Cipher.init(int, Key, SecureRandom) ).  Per the spec in Cipher, this is 
only true for ENCRYPT_MODE and will throw InvalidKeyException when done 
in DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the initial 
state, which is then used for generating an authentication key for 
AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations and 
only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse.  This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be used 
to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev and 
the JEP which outlines the characteristics and behavior of the 
ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Okay, let me fix the code and the CSR and make them match.  I'll put the 
CSR back in draft and make a note in the comments about what is 
changing.  This is an innocuous change, but if Sean feels that code-wise 
the final class should not have final methods, then making the CSR match 
the fixed code sounds like the right way to go.


--Jamil

On 5/8/2018 11:03 AM, Roger Riggs wrote:

Hi,

"final" is a important modifier of the method signature and if the CSR 
and the implementation are
different it might raise a question about which is the correct 
signature when the JCK folks write tests.


It is pretty lightweight process to return the CSR to draft and update 
it and finalize it again.

It should be readily approved.  (or leave the implementation as final).

$.02, Roger

On 5/8/2018 1:58 PM, Sean Mullan wrote:

On 5/8/18 1:52 PM, Jamil Nimeh wrote:
I'll make those fixes.  One question with respect to the final 
methods: the CSR had those methods in the description and they were 
marked as final.  That CSR is now complete.  Will removing the final 
qualifier in the methods be an issue?  In terms of effect on the 
code it doesn't matter. It's more of a dot-i/cross-t kind of question.


I don't think so as the behavior is the same either way.

--Sean



--Jamil

On 5/8/2018 10:49 AM, Sean Mullan wrote:

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the 
default encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code. I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor 
stuff mostly:


  * Added words in the description of javax.crypto.Cipher 
recommending
    callers reinitialize the Cipher to use different nonces after 
each
    complete encryption or decryption (similar language to what 
exists

    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and 
cleaned

    up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is 
a behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException. This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, 
key and random, the nonce will always be random even if the 
random parameter is null.  A default instance of SecureRandom 
will be used to create the nonce in this case, instead of all 
zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly. It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

To clarify, both the impl and the CSR mark the ChaCha20ParameterSpec 
class as final, but the CSR also (accidentally) marked the methods of 
the class as final. If that qualifies as an API change, then we should 
fix the CSR, but I don't know for sure. It's an odd case.


--Sean

On 5/8/18 2:03 PM, Roger Riggs wrote:

Hi,

"final" is a important modifier of the method signature and if the CSR 
and the implementation are
different it might raise a question about which is the correct signature 
when the JCK folks write tests.


It is pretty lightweight process to return the CSR to draft and update 
it and finalize it again.

It should be readily approved.  (or leave the implementation as final).

$.02, Roger

On 5/8/2018 1:58 PM, Sean Mullan wrote:

On 5/8/18 1:52 PM, Jamil Nimeh wrote:
I'll make those fixes.  One question with respect to the final 
methods: the CSR had those methods in the description and they were 
marked as final.  That CSR is now complete.  Will removing the final 
qualifier in the methods be an issue?  In terms of effect on the code 
it doesn't matter. It's more of a dot-i/cross-t kind of question.


I don't think so as the behavior is the same either way.

--Sean



--Jamil

On 5/8/2018 10:49 AM, Sean Mullan wrote:

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the 
default encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code. I'm pretty confident the init sequence is OK. I 
did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher 
recommending
    callers reinitialize the Cipher to use different nonces after 
each
    complete encryption or decryption (similar language to what 
exists

    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
    up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException. This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly. It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Roger Riggs]

Hi,

"final" is a important modifier of the method signature and if the CSR 
and the implementation are
different it might raise a question about which is the correct signature 
when the JCK folks write tests.


It is pretty lightweight process to return the CSR to draft and update 
it and finalize it again.

It should be readily approved.  (or leave the implementation as final).

$.02, Roger

On 5/8/2018 1:58 PM, Sean Mullan wrote:

On 5/8/18 1:52 PM, Jamil Nimeh wrote:
I'll make those fixes.  One question with respect to the final 
methods: the CSR had those methods in the description and they were 
marked as final.  That CSR is now complete.  Will removing the final 
qualifier in the methods be an issue?  In terms of effect on the code 
it doesn't matter. It's more of a dot-i/cross-t kind of question.


I don't think so as the behavior is the same either way.

--Sean



--Jamil

On 5/8/2018 10:49 AM, Sean Mullan wrote:

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the 
default encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code. I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher 
recommending
    callers reinitialize the Cipher to use different nonces after 
each
    complete encryption or decryption (similar language to what 
exists

    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
    up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException. This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly. It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the characteristics and behavior of 
the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 5/8/18 1:52 PM, Jamil Nimeh wrote:
I'll make those fixes.  One question with respect to the final methods: 
the CSR had those methods in the description and they were marked as 
final.  That CSR is now complete.  Will removing the final qualifier in 
the methods be an issue?  In terms of effect on the code it doesn't 
matter.  It's more of a dot-i/cross-t kind of question.


I don't think so as the behavior is the same either way.

--Sean



--Jamil

On 5/8/2018 10:49 AM, Sean Mullan wrote:

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the default 
encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the 
initial state, which is then used for generating an authentication 
key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher recommending
    callers reinitialize the Cipher to use different nonces after each
    complete encryption or decryption (similar language to what exists
    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
    up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException. This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the characteristics and behavior of 
the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

I'll make those fixes.  One question with respect to the final methods: 
the CSR had those methods in the description and they were marked as 
final.  That CSR is now complete.  Will removing the final qualifier in 
the methods be an issue?  In terms of effect on the code it doesn't 
matter.  It's more of a dot-i/cross-t kind of question.


--Jamil

On 5/8/2018 10:49 AM, Sean Mullan wrote:

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the default 
encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case 
on init where a half-baked object would fail.  In most cases it would 
fail in checks in javax.crypto.Cipher before it ever made it down to 
my code.  I'm pretty confident the init sequence is OK.  I did move 
the setting of a few data members toward the end of the init sequence 
but setting the key and nonce is necessary before creating the 
initial state, which is then used for generating an authentication 
key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher recommending
    callers reinitialize the Cipher to use different nonces after each
    complete encryption or decryption (similar language to what exists
    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
    up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException. This is similar to the behavior of 
AES-GCM in encrypt mode, but for ChaCha20 it needs to be done for 
both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be 
used to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev 
and the JEP which outlines the characteristics and behavior of 
the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

- ChaCha20ParameterSpec.java

The methods don't need to be final now that the class is final.

- ChaCha20Poly1305Parameters.java

 122 if (decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {
 123 engineInit(encoded);

The spec for engineInit() says if decodingMethod is null, the default 
encoding should be used, so the code above should be:


 122 if (decodingMethod == null || 
decodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

 123 engineInit(encoded);

Looks good otherwise.

--Sean

On 5/4/18 10:06 AM, Jamil Nimeh wrote:

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case on 
init where a half-baked object would fail.  In most cases it would fail 
in checks in javax.crypto.Cipher before it ever made it down to my 
code.  I'm pretty confident the init sequence is OK.  I did move the 
setting of a few data members toward the end of the init sequence but 
setting the key and nonce is necessary before creating the initial 
state, which is then used for generating an authentication key for AEAD 
mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:

  * Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations and 
only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse.  This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. Attempting 
to reuse the cipher without a new initialization will throw an 
IllegalStateException.  This is similar to the behavior of AES-GCM in 
encrypt mode, but for ChaCha20 it needs to be done for both encrypt 
and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Round 5.

This adds Sean's comments.  Sean, I was never able to execute a case on 
init where a half-baked object would fail.  In most cases it would fail 
in checks in javax.crypto.Cipher before it ever made it down to my 
code.  I'm pretty confident the init sequence is OK.  I did move the 
setting of a few data members toward the end of the init sequence but 
setting the key and nonce is necessary before creating the initial 
state, which is then used for generating an authentication key for AEAD 
mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:

  * Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned
up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations and 
only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse.  This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305. Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. Attempting 
to reuse the cipher without a new initialization will throw an 
IllegalStateException.  This is similar to the behavior of AES-GCM in 
encrypt mode, but for ChaCha20 it needs to be done for both encrypt 
and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 5/2/18 11:23 AM, Jamil Nimeh wrote:
Ok. I think as long as these fields are never dependent on previous 
values if you call engineInit again, then it is ok. In other words, 
they should be the same even if the previous call to engineInit throws 
an Exception. It might be safer as the first thing you do to 
re-initialize these each time engineInit is called.


JN: Yeah, earlier this morning I found a sequence that the half-baked 
state of the object was a problem.  So I'm shuffling around the order 
that these parameters get set in the ChaCha20 object so it happens later 
in the method as you suggested earlier. Basically if you init the object 
with an unsupported mode like Cipher.UNWRAP_MODE, that gets pinned to 
the object and subsequent inits where the mode is corrected fail.  The 
stack trace on the second init never makes it into ChaCha20's code, so 
I'm not sure if something is set in Cipher itself that persists from 
init to init. But setting these fields at the end of the init process is 
a better way to do it regardless.


Another (I think simpler) option is to have a private method like 
initState() or initFields() which initializes all these fields to their 
default values and always call it as the first thing engineInit does.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

On 05/02/2018 07:35 AM, Sean Mullan wrote:

On 5/1/18 6:55 PM, Jamil Nimeh wrote:

262 counter = 1;

Here you set the counter field to 1, but this method can still throw 
exceptions after that. Is there any risk of leaving the object in an 
inconsistent state by setting counter=1 if this method fails to 
complete successfully? Same comment on line 305.
JN: I don't think so, since any Cipher.init call that would drop into 
these init routines expects the counter to be set to 1 (or to 
whatever value came in from the AlgorithmParameterSpec).  If an 
exception is thrown during the init codepath, the object will remain 
in an uninitialized state and cannot be used, so I don't see much 
danger there.  The only thing the user could do with an object in 
this situation would be to try to initialize it again (hopefully with 
parameters that don't cause it to fail).


Ok.


 401 if (newNonce == null) {
 402 newNonce = new byte[12];
 403 if (random != null) {
 404 random.nextBytes(newNonce);
 405 }

It looks like there is a possibility of a non-random nonce being 
used if random is null at this point, which it could be if null is 
passed as the random and params parameters. I don't think that is 
what is intended. I think the right thing to do is throw an 
InvalidAlgParamException if you get to this point and random is null.
JN: Well, the method comment is old/outdated, another throwback to an 
older version that said it would use all zeroes in the null/null case 
and I don't want that to behave that way any longer.  I think if I do 
a similar approach to the init actions in 258-260 we should be in 
better shape.  Then when params is null there will always be a random 
12-byte nonce generated, regardless of the null/non-null status of 
the "random" parameter.


Ok.


 508 this.keyBytes = newKeyBytes;
 509 nonce = newNonce;
 510
 511 aadDone = false;
 512 direction = opmode;

Here also you are setting various fields but the method may still 
throw an Exception - any issues about leaving the object in an 
inconsistent state? Preferably you should set these as the last 
thing you do before returning.
JN: As before.  The object is in an inconsistent state, but unusable 
because the initialized flag is still false (and that's the absolute 
last thing that gets set to true during init).  If that flag is 
false, the update, updateAAD and doFinal methods will throw an 
execption.  The only way to make it usable is to init successfully. I 
think we're OK here.


Ok. I think as long as these fields are never dependent on previous 
values if you call engineInit again, then it is ok. In other words, 
they should be the same even if the previous call to engineInit throws 
an Exception. It might be safer as the first thing you do to 
re-initialize these each time engineInit is called.


JN: Yeah, earlier this morning I found a sequence that the half-baked 
state of the object was a problem.  So I'm shuffling around the order 
that these parameters get set in the ChaCha20 object so it happens later 
in the method as you suggested earlier. Basically if you init the object 
with an unsupported mode like Cipher.UNWRAP_MODE, that gets pinned to 
the object and subsequent inits where the mode is corrected fail.  The 
stack trace on the second init never makes it into ChaCha20's code, so 
I'm not sure if something is set in Cipher itself that persists from 
init to init. But setting these fields at the end of the init process is 
a better way to do it regardless.


--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 5/1/18 6:55 PM, Jamil Nimeh wrote:

262 counter = 1;

Here you set the counter field to 1, but this method can still throw 
exceptions after that. Is there any risk of leaving the object in an 
inconsistent state by setting counter=1 if this method fails to 
complete successfully? Same comment on line 305.
JN: I don't think so, since any Cipher.init call that would drop into 
these init routines expects the counter to be set to 1 (or to whatever 
value came in from the AlgorithmParameterSpec).  If an exception is 
thrown during the init codepath, the object will remain in an 
uninitialized state and cannot be used, so I don't see much danger 
there.  The only thing the user could do with an object in this 
situation would be to try to initialize it again (hopefully with 
parameters that don't cause it to fail).


Ok.


 401 if (newNonce == null) {
 402 newNonce = new byte[12];
 403 if (random != null) {
 404 random.nextBytes(newNonce);
 405 }

It looks like there is a possibility of a non-random nonce being used 
if random is null at this point, which it could be if null is passed 
as the random and params parameters. I don't think that is what is 
intended. I think the right thing to do is throw an 
InvalidAlgParamException if you get to this point and random is null.
JN: Well, the method comment is old/outdated, another throwback to an 
older version that said it would use all zeroes in the null/null case 
and I don't want that to behave that way any longer.  I think if I do a 
similar approach to the init actions in 258-260 we should be in better 
shape.  Then when params is null there will always be a random 12-byte 
nonce generated, regardless of the null/non-null status of the "random" 
parameter.


Ok.


 508 this.keyBytes = newKeyBytes;
 509 nonce = newNonce;
 510
 511 aadDone = false;
 512 direction = opmode;

Here also you are setting various fields but the method may still 
throw an Exception - any issues about leaving the object in an 
inconsistent state? Preferably you should set these as the last thing 
you do before returning.
JN: As before.  The object is in an inconsistent state, but unusable 
because the initialized flag is still false (and that's the absolute 
last thing that gets set to true during init).  If that flag is false, 
the update, updateAAD and doFinal methods will throw an execption.  The 
only way to make it usable is to init successfully. I think we're OK here.


Ok. I think as long as these fields are never dependent on previous 
values if you call engineInit again, then it is ok. In other words, they 
should be the same even if the previous call to engineInit throws an 
Exception. It might be safer as the first thing you do to re-initialize 
these each time engineInit is called.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 5/1/18 5:45 PM, Jamil Nimeh wrote:

134 throw new IllegalArgumentException(
 135 "Unsupported parameter format: " + 
decodingMethod);


Although this seems like a reasonable exception to throw, 
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I 
think you should throw an IOE instead.


 202 throw new IllegalArgumentException(
 203 "Unsupported encoding format: " + 
encodingMethod);


Similar comment, AlgorithmParametersSpi.engineGetEncoded is not 
spec'ed to throw this, so I think you should throw an IOE instead.
JN: Fair enough.  I had figured IAE would be OK since it's an unchecked 
exception and seemed more appropriate to the situation. But IOE is fine 
too.  Will fix.


I checked all of the other JDK AlgorithmParametersSpi implementations, 
and they actually ignore the format parameter completely and always 
encode using ASN.1. But I don't think that is right. So, I think we 
should throw an Exception and maybe file a bug to add a clarification to 
the spec for engineGetEncoded.


One other issue. The spec for engineGetEncoded says that if the format 
is null, the primary encoding format for parameters is used, so the code 
for 199-200 should be:


199 if (encodingMethod == null || 
encodingMethod.equalsIgnoreCase(DEFAULT_FMT)) {

200 return engineGetEncoded();

--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Update: forget my comment on this finding, Sean.  I'm already wrapping 
IOE in IAPE when IOE gets thrown so it's better to go the route you 
suggested.  I didn't read the method carefully enough.





 375 if (dv.tag == DerValue.tag_OctetString) {
 376 // Get the nonce value
 377 newNonce = dv.getOctetString();

Similar to a previous comment, I would just call 
DerValue.getOctetString() directly and let it throw an Exception if 
it is the wrong tag.
JN: Except that I'll still have to catch the IOException and make that 
the cause of the InvalidAlgorithmParameterException.  I don't think 
CipherSpi.engineInit(int, Key, AlgorithmParameters, SecureRandom) can 
directly throw IOException.  If I have to wrap it inside IAPE, do you 
still prefer to do it that way or is the existing way OK?


--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi Sean, many thanks for taking a detailed look at the code.  My 
comments are in-line


On 5/1/2018 1:53 PM, Sean Mullan wrote:

On 5/1/18 1:20 PM, Sean Mullan wrote:

On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher recommending
    callers reinitialize the Cipher to use different nonces after each
    complete encryption or decryption (similar language to what exists
    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and 
cleaned up

    the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/


More comments:

- Poly1305.java

  44  * @since 10

Not necessary, but should be 11 if so.

JN: Will fix


  46 public final class Poly1305

Can this be package-private?
JN: Not sure.  I will make it so and run through the tests.  It will 
probably work.


  84 keyBytes.length);
 122 BLOCK_LENGTH - blockOffset);

indent at least 4 more spaces

JJN: Will fix.


- ChaCha20Poly1305ParamTest.java

When you test for exceptions, you should throw an exception if the 
method does not throw an exception. For example:


 171 try {
 172 apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
 173 } catch (IllegalArgumentException iae) {
 174 System.out.println("Caught expected exception: " + iae);
 175 }

should be something like:

    try {
    apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
    throw new Exception("init passed unexpectedly");
    } catch (IllegalArgumentException iae) {
    System.out.println("Caught expected exception: " + iae);
    }

there are several other cases like this.
JN: Yikes!  I normally do put an exception in the happy-path when I'm 
expecting a failure.  Major oversight here.  I will fix this.


- ChaCha20KAT.java

looks good.

- ChaCha20NoReuse.java

looks good.

- ChaCha20Cipher.java

Exception messages say "bits" - should we use "bytes" to be consistent 
with the API?

JN: Sure, I can do that.


 262 counter = 1;

Here you set the counter field to 1, but this method can still throw 
exceptions after that. Is there any risk of leaving the object in an 
inconsistent state by setting counter=1 if this method fails to 
complete successfully? Same comment on line 305.
JN: I don't think so, since any Cipher.init call that would drop into 
these init routines expects the counter to be set to 1 (or to whatever 
value came in from the AlgorithmParameterSpec).  If an exception is 
thrown during the init codepath, the object will remain in an 
uninitialized state and cannot be used, so I don't see much danger 
there.  The only thing the user could do with an object in this 
situation would be to try to initialize it again (hopefully with 
parameters that don't cause it to fail).


 301 if (newNonce.length != 12) {
 302 throw new InvalidAlgorithmParameterException(
 303 "ChaCha20 nonce must be 96 bits in length");
 304 }

You don't need to check for this, since the ChaCha20ParamSpec class 
does already (especially if you make it final).
JN: Yep, you're right.  A throwback to an earlier rev of the code before 
the ChaCha20ParameterSpec rigidly enforced this.  I can remove it.



 375 if (dv.tag == DerValue.tag_OctetString) {
 376 // Get the nonce value
 377 newNonce = dv.getOctetString();

Similar to a previous comment, I would just call 
DerValue.getOctetString() directly and let it throw an Exception if it 
is the wrong tag.
JN: Except that I'll still have to catch the IOException and make that 
the cause of the InvalidAlgorithmParameterException.  I don't think 
CipherSpi.engineInit(int, Key, AlgorithmParameters, SecureRandom) can 
directly throw IOException.  If I have to wrap it inside IAPE, do you 
still prefer to do it that way or is the existing way OK?


 395 throw new UnsupportedOperationException("Invalid 
mode: " +


Seems inconsistent with 321 which throws RuntimeExc.

JN: So it is.  I'll change the UOE to RuntimeException.


 401 if (newNonce == null) {
 402 newNonce = new byte[12];
 403 if (random != null) {
 404 random.nextBytes(newNonce);
 405 }

It looks like there is a possibility of a non-random nonce being used 
if random is null at this point, which it could be if null is passed 
as the random and params parameters. I don't think that is what is 
intended. I think the right thing to do is throw an 
InvalidAlgParamException if you get to this point and random is null.
JN: Well, the method comment is old/outdated, another throwback to an 
older version that said it would use all zeroes in the 

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Comments in-line:

On 5/1/2018 10:20 AM, Sean Mullan wrote:

On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher recommending
    callers reinitialize the Cipher to use different nonces after each
    complete encryption or decryption (similar language to what exists
    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned up
    the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/


Some comments so far, will continue my review later ...

- Cipher.java

After the paragraph, I would add a reference for more info, ex: 
"Please see http://www.ietf.org/rfc/rfc7539.txt;> RFC 7539 
 for more information on the ChaCha20 and ChaCha20-Poly1305 
algorithms."

JN: Sounds good.  I'll do that.


- SunJCE.java

81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2, 
PBE, "


Should add the ChaCha20 algorithm to the provider information String 
above.

JN: Yep, missed that.  Will fix.


- ChaCha20ParameterSpec.java

Thinking more about this -- if you don't think there is any good 
reason that a provider would want to subclass this, I would just make 
the whole class final.
JN: Given what ChaCha20 takes as variable inputs, I cannot see what 
anyone could gain from subclassing it.  I can set it to final and update 
the CSR.


  60  * @throws IllegalArgumentException if {@code nonce} is not 
12 bytes

  61  *  in length.

Remove period since this is a fragment that is not followed by a 
sentence.


  87  * @return the counter value.

Same as above.

JN: Will fix.


- ChaCha20Poly1305Parameters.java

52  * @since 11

We don't typically put @since lines in impl. classes.

JN: Consider it gone.


111 "ChaCha20-Poly1305 Parameter ASN.1 encoding 
error");


 102 if (val.tag == DerValue.tag_OctetString) {
 103 // Get the nonce value
 104 nonce = val.getOctetString();

I would probably just call val.getOctetString() directly and not 
proactively check the tag, since it will throw IOE if it is the wrong 
tag anyway. Then you don't need these lines:


 109 } else {
 110 throw new IOException(
 111 "ChaCha20-Poly1305 Parameter ASN.1 encoding 
error");

 112 }

There are other reasons an IOException can be thrown, so it doesn't 
seem necessary to have a special-case for one type of error.

JN: Sure, that seems reasonable.  Will fix.


 134 throw new IllegalArgumentException(
 135 "Unsupported parameter format: " + 
decodingMethod);


Although this seems like a reasonable exception to throw, 
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I 
think you should throw an IOE instead.


 202 throw new IllegalArgumentException(
 203 "Unsupported encoding format: " + 
encodingMethod);


Similar comment, AlgorithmParametersSpi.engineGetEncoded is not 
spec'ed to throw this, so I think you should throw an IOE instead.
JN: Fair enough.  I had figured IAE would be OK since it's an unchecked 
exception and seemed more appropriate to the situation. But IOE is fine 
too.  Will fix.


--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 5/1/18 1:20 PM, Sean Mullan wrote:

On 4/27/18 5:21 PM, Jamil Nimeh wrote:
Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher recommending
    callers reinitialize the Cipher to use different nonces after each
    complete encryption or decryption (similar language to what exists
    already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned up
    the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/


More comments:

- Poly1305.java

  44  * @since 10

Not necessary, but should be 11 if so.

  46 public final class Poly1305

Can this be package-private?

  84 keyBytes.length);
 122 BLOCK_LENGTH - blockOffset);

indent at least 4 more spaces

- ChaCha20Poly1305ParamTest.java

When you test for exceptions, you should throw an exception if the 
method does not throw an exception. For example:


 171 try {
 172 apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
 173 } catch (IllegalArgumentException iae) {
 174 System.out.println("Caught expected exception: " + iae);
 175 }

should be something like:

try {
apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
throw new Exception("init passed unexpectedly");
} catch (IllegalArgumentException iae) {
System.out.println("Caught expected exception: " + iae);
}

there are several other cases like this.

- ChaCha20KAT.java

looks good.

- ChaCha20NoReuse.java

looks good.

- ChaCha20Cipher.java

Exception messages say "bits" - should we use "bytes" to be consistent 
with the API?


 262 counter = 1;

Here you set the counter field to 1, but this method can still throw 
exceptions after that. Is there any risk of leaving the object in an 
inconsistent state by setting counter=1 if this method fails to complete 
successfully? Same comment on line 305.


 301 if (newNonce.length != 12) {
 302 throw new InvalidAlgorithmParameterException(
 303 "ChaCha20 nonce must be 96 bits in length");
 304 }

You don't need to check for this, since the ChaCha20ParamSpec class does 
already (especially if you make it final).


 375 if (dv.tag == DerValue.tag_OctetString) {
 376 // Get the nonce value
 377 newNonce = dv.getOctetString();

Similar to a previous comment, I would just call 
DerValue.getOctetString() directly and let it throw an Exception if it 
is the wrong tag.


 395 throw new UnsupportedOperationException("Invalid 
mode: " +


Seems inconsistent with 321 which throws RuntimeExc.

 401 if (newNonce == null) {
 402 newNonce = new byte[12];
 403 if (random != null) {
 404 random.nextBytes(newNonce);
 405 }

It looks like there is a possibility of a non-random nonce being used if 
random is null at this point, which it could be if null is passed as the 
random and params parameters. I don't think that is what is intended. I 
think the right thing to do is throw an InvalidAlgParamException if you 
get to this point and random is null.


 508 this.keyBytes = newKeyBytes;
 509 nonce = newNonce;
 510
 511 aadDone = false;
 512 direction = opmode;

Here also you are setting various fields but the method may still throw 
an Exception - any issues about leaving the object in an inconsistent 
state? Preferably you should set these as the last thing you do before 
returning.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 4/27/18 5:21 PM, Jamil Nimeh wrote:

Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:

  * Added words in the description of javax.crypto.Cipher recommending
callers reinitialize the Cipher to use different nonces after each
complete encryption or decryption (similar language to what exists
already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and cleaned up
the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/


Some comments so far, will continue my review later ...

- Cipher.java

After the paragraph, I would add a reference for more info, ex: "Please 
see http://www.ietf.org/rfc/rfc7539.txt;> RFC 7539  for 
more information on the ChaCha20 and ChaCha20-Poly1305 algorithms."


- SunJCE.java

81 "(implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2, 
PBE, "


Should add the ChaCha20 algorithm to the provider information String above.

- ChaCha20ParameterSpec.java

Thinking more about this -- if you don't think there is any good reason 
that a provider would want to subclass this, I would just make the whole 
class final.


  60  * @throws IllegalArgumentException if {@code nonce} is not 12 
bytes

  61  *  in length.

Remove period since this is a fragment that is not followed by a sentence.

  87  * @return the counter value.

Same as above.

- ChaCha20Poly1305Parameters.java

52  * @since 11

We don't typically put @since lines in impl. classes.

111 "ChaCha20-Poly1305 Parameter ASN.1 encoding error");

 102 if (val.tag == DerValue.tag_OctetString) {
 103 // Get the nonce value
 104 nonce = val.getOctetString();

I would probably just call val.getOctetString() directly and not 
proactively check the tag, since it will throw IOE if it is the wrong 
tag anyway. Then you don't need these lines:


 109 } else {
 110 throw new IOException(
 111 "ChaCha20-Poly1305 Parameter ASN.1 encoding 
error");

 112 }

There are other reasons an IOException can be thrown, so it doesn't seem 
necessary to have a special-case for one type of error.


 134 throw new IllegalArgumentException(
 135 "Unsupported parameter format: " + 
decodingMethod);


Although this seems like a reasonable exception to throw, 
AlgorithmParametersSpi.engineInit is not spec'ed to throw this, so I 
think you should throw an IOE instead.


 202 throw new IllegalArgumentException(
 203 "Unsupported encoding format: " + encodingMethod);

Similar comment, AlgorithmParametersSpi.engineGetEncoded is not spec'ed 
to throw this, so I think you should throw an IOE instead.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff mostly:

 * Added words in the description of javax.crypto.Cipher recommending
   callers reinitialize the Cipher to use different nonces after each
   complete encryption or decryption (similar language to what exists
   already for AES-GCM encryption).
 * Added an additional test case for ChaCha20NoReuse
 * Made accessor methods for ChaCha20ParameterSpec final and cleaned up
   the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations and 
only retain the key bytes as an object field (thanks Thomas for 
catching this)


* Added additional protections against key/nonce reuse.  This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of 
these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without re-initializing 
the cipher with either a new key or nonce. Attempting to reuse the 
cipher without a new initialization will throw an 
IllegalStateException.  This is similar to the behavior of AES-GCM in 
encrypt mode, but for ChaCha20 it needs to be done for both encrypt 
and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Agreed.  I will make that change.

--Jamil


On 04/26/2018 11:20 AM, Sean Mullan wrote:

On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do.  I made some wording changes there to move away from 
"96-bit" to "12-byte" also since that probably is more useful to 
developers.


One other comment is that I think the get methods of 
ChaCha20ParameterSpec should be final. There should be no reason to 
override those methods and it will help ensure that the contract of 
those methods are not being overridden by a subclass incorrectly.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do.  I made some wording changes there to move away from "96-bit" 
to "12-byte" also since that probably is more useful to developers.


One other comment is that I think the get methods of 
ChaCha20ParameterSpec should be final. There should be no reason to 
override those methods and it will help ensure that the contract of 
those methods are not being overridden by a subclass incorrectly.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

On 4/26/18 11:57 AM, Sean Mullan wrote:
The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.


Also:

  65 if (nonce.length == 12) {
  66 this.nonce = nonce.clone();
  67 } else {
  68 throw new IllegalArgumentException(
  69 "Nonce must be 96-bits in length");
  70 }

You should clone nonce before you check the length and check the length 
on the copy, not the parameter passed in. Also, you should use 
NONCE_LENGTH instead of 12 since it is already defined as a constant in 
the class.


--Sean



--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

The NONCE_LENGTH I fixed last night in response to JVT comments.  I will do the 
clone also.  Thanks for the comments!


--Jamil
 Original message From: Sean Mullan <sean.mul...@oracle.com> 
Date: 4/26/18  10:22 AM  (GMT-08:00) To: Jamil Nimeh 
<jamil.j.ni...@oracle.com>, OpenJDK Dev list <security-dev@openjdk.java.net> 
Subject: Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations 
On 4/26/18 11:57 AM, Sean Mullan wrote:
> The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
> on it.

Also:

   65 if (nonce.length == 12) {
   66 this.nonce = nonce.clone();
   67 } else {
   68 throw new IllegalArgumentException(
   69 "Nonce must be 96-bits in length");
   70 }

You should clone nonce before you check the length and check the length 
on the copy, not the parameter passed in. Also, you should use 
NONCE_LENGTH instead of 12 since it is already defined as a constant in 
the class.

--Sean

> 
> --Sean
> 
> On 3/26/18 3:08 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
>> cipher implementations.  Links to the webrev and the JEP which 
>> outlines the characteristics and behavior of the ciphers are listed 
>> below.
>>
>> http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
>> http://openjdk.java.net/jeps/329
>>
>> Thanks,
>> --Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Will do.  I made some wording changes there to move away from "96-bit" to 
"12-byte" also since that probably is more useful to developers.


--Jamil
 Original message From: Sean Mullan <sean.mul...@oracle.com> 
Date: 4/26/18  8:57 AM  (GMT-08:00) To: Jamil Nimeh <jamil.j.ni...@oracle.com>, 
OpenJDK Dev list <security-dev@openjdk.java.net> Subject: Re: RFR: ChaCha20 and 
ChaCha20/Poly1305 Cipher implementations 
The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.

--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:
> Hello all,
> 
> This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
> cipher implementations.  Links to the webrev and the JEP which outlines 
> the characteristics and behavior of the ciphers are listed below.
> 
> http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
> http://openjdk.java.net/jeps/329
> 
> Thanks,
> --Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.


--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which outlines 
the characteristics and behavior of the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations and 
only retain the key bytes as an object field (thanks Thomas for catching 
this)


* Added additional protections against key/nonce reuse.  This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances of these 
ciphers will no longer allow you to do subsequent doUpdate/doFinal calls 
after the first doFinal without re-initializing the cipher with either a 
new key or nonce. Attempting to reuse the cipher without a new 
initialization will throw an IllegalStateException.  This is similar to 
the behavior of AES-GCM in encrypt mode, but for ChaCha20 it needs to be 
done for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter classes 
in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Thomas Lußnig]

Hi,

ok that is an good point that if we have more values in the structure 
than we use this can make some confusion.
I was only looking from the coding point of view and saw that if i can 
use the same structure for booth cases this

can reduce the coding overhead. But i can fully understand your point.

Gruß Thomas

On 4/10/2018 11:37 PM, Jamil Nimeh wrote:

Hello Thomas, et al.,

On 3/26/2018 1:49 PM, Jamil Nimeh wrote:

Hi Thomas, thanks for the feedback

 1. Were there guidelines?  Not really, though I looked at other
parameter definitions in com.sun.crypto.provider and tried to
follow along the same lines that they do.  One thing that should
be changed is the LINE_SEP assignment shouldn't be an explicit
getProperty call.  I noticed most are doing
System.lineSeparator() so I'll change my implementation to match
that.  None of these params appear to stringify as json, so I'll
probably keep things consistent with the other parameter output.
 2. You make a fair point with respect to a null SecureRandom.  I can
make that spec change.
 3. Let me think on this one - I shied away from
ChaCha20ParameterSpec for AEAD mode only because you have this
nonce field that is set but gets ignored.  But making
ChaCha20ParameterSpec an IvParameterSpec potentially runs into
the same issue were it used for a ChaCha20-Poly1305 cipher.  If I
had to choose between the two I think I'd go with allowing
ChaCha20ParameterSpec to be used with CC20-P1305 rather than
making it a subclass of IvParameterSpec.  Doing the former helps
from a type safety perspective that you couldn't use a
ChaCha20ParameterSpec with other Ciphers that require an
IvParameterSpec.  I know I had some discussions early on in the
design where we talked about this, I need to refresh my memory as
to why we didn't allow it.

Finally getting back to #3.  Took me a while to find early discussions 
on this.  The primary objection to ChaCha20ParameterSpec being used 
with ChaCha20-Poly1305 (as opposed to plain old ChaCha20) has to do 
with the configurable block counter.  You have this parameter that is 
not used, and consumption of this type of AlgorithmParameterSpec then 
leaves it to documentation to define what happens (is it ignored?  
Used despite what the spec says?  Set to some default value regardless 
of what the caller sets there?).  Using IvParameterSpec with 
ChaCha20-Poly1305 is more clear because it only allows the caller what 
they need to get CC20/P1305 going, the nonce.  Respectfully, I would 
like to keep this as-is.


--Jamil


On 3/26/2018 12:45 PM, Thomas Lußnig wrote:


Hi Jamil,

1) where there any guidelines about how the engineToString should be 
formatted ?
I ask because i wondering why we need two new lines with access to 
the System property.
If it is represented as single line json no need to line break would 
be needed.


Gruß Thomas


/** * Creates a formatted string describing the parameters. * * 
@return a string representation of the ChaCha20 parameters. */ 
@Override protected String engineToString() { String LINE_SEP = 
System.getProperty("line.separator"); HexDumpEncoder encoder = new 
HexDumpEncoder(); StringBuilder sb = new StringBuilder(LINE_SEP + 
"nonce:" + LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"); 
return sb.toString(); }

2) I do not think it is an good idea to say no secureRandom=null will cause IV 
to be null.
I see here the risk of weak implementations. I would suggest to throw an 
Exception to
enforce secure usages. If someone really want an insecure IV he can provide 
am SecureRandom
implementation retuning 0 only or an matching IV.

  * @param random a {@code SecureRandom} implementation.  If {@code null}
  *  is used for the random object, then a nonce consisting of all
  *  zero bytes will be used.  Otherwise a random nonce will be
  *  used.

3) If ChaCha20ParameterSpec would extends IvParameterSpec if would be valid for 
booth modes in engineInit.
 Even if the counter is not needed.
 As an alternative i would allow ChaCha20ParameterSpec also for AEAD mode.

Grup Thomas
On 3/26/2018 9:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Thomas Lußnig]

Hi,

same with key/keyBytes is with the class Poly1305 also here the key is 
not used.


Gruß Thomas

On 4/11/2018 5:54 PM, Jamil Nimeh wrote:

Yes, that does appear to be the case, good catch!  I'll make that change.

--Jamil

On 4/11/2018 7:18 AM, Thomas Lußnig wrote:

Hi,

i found another point. The "key" field can be removed from 
ChaCha20Cipher.

1) This field is only set once and later checked if it was initialized.
 But we do not want to knew is the key exists but if key bytes 
exists.
2) So if two lines are changed from key to keyBytes we can remove 
this unused field.



Gruß Thomas

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java.html 


Lines 426 , 461 (change to keyBytes)
    if (key == null
Line 75+507 (remove)
    private Key key;
    this.key = key;


On 4/11/2018 12:34 AM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Thomas Lußnig]

Hi,

i found another point. The "key" field can be removed from ChaCha20Cipher.
1) This field is only set once and later checked if it was initialized.
 But we do not want to knew is the key exists but if key bytes exists.
2) So if two lines are changed from key to keyBytes we can remove this 
unused field.



Gruß Thomas

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java.html
Lines 426 , 461 (change to keyBytes)
    if (key == null
Line 75+507 (remove)
    private Key key;
    this.key = key;


On 4/11/2018 12:34 AM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter classes 
in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Okay, I will check that out and fix as appropriate.  Thanks for the 
comments!


--Jamil

On 4/11/2018 10:56 AM, Thomas Lußnig wrote:

Hi,

same with key/keyBytes is with the class Poly1305 also here the key is 
not used.


Gruß Thomas

On 4/11/2018 5:54 PM, Jamil Nimeh wrote:
Yes, that does appear to be the case, good catch!  I'll make that 
change.


--Jamil

On 4/11/2018 7:18 AM, Thomas Lußnig wrote:

Hi,

i found another point. The "key" field can be removed from 
ChaCha20Cipher.

1) This field is only set once and later checked if it was initialized.
 But we do not want to knew is the key exists but if key bytes 
exists.
2) So if two lines are changed from key to keyBytes we can remove 
this unused field.



Gruß Thomas

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java.html 


Lines 426 , 461 (change to keyBytes)
    if (key == null
Line 75+507 (remove)
    private Key key;
    this.key = key;


On 4/11/2018 12:34 AM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random, the nonce will always be random even if the random 
parameter is null.  A default instance of SecureRandom will be used 
to create the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and 
ChaCha20-Poly1305 cipher implementations.  Links to the webrev and 
the JEP which outlines the characteristics and behavior of the 
ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Yes, that does appear to be the case, good catch!  I'll make that change.

--Jamil

On 4/11/2018 7:18 AM, Thomas Lußnig wrote:

Hi,

i found another point. The "key" field can be removed from 
ChaCha20Cipher.

1) This field is only set once and later checked if it was initialized.
 But we do not want to knew is the key exists but if key bytes 
exists.
2) So if two lines are changed from key to keyBytes we can remove this 
unused field.



Gruß Thomas

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java.html 


Lines 426 , 461 (change to keyBytes)
    if (key == null
Line 75+507 (remove)
    private Key key;
    this.key = key;


On 4/11/2018 12:34 AM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter 
is null.  A default instance of SecureRandom will be used to create 
the nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line 
separator from a System property directly.  It calls 
System.lineSeparator() similar to how other AlgorithmParameter 
classes in com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key and 
random, the nonce will always be random even if the random parameter is 
null.  A default instance of SecureRandom will be used to create the 
nonce in this case, instead of all zeroes.


* Unused debug code was removed from the ChaCha20Cipher.java file

* ChaCha20Parameters.engineToString no longer obtains the line separator 
from a System property directly.  It calls System.lineSeparator() 
similar to how other AlgorithmParameter classes in 
com.sun.crypto.provider do it.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.02/

Thanks,

--Jamil


On 03/26/2018 12:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hello Thomas, et al.,

On 3/26/2018 1:49 PM, Jamil Nimeh wrote:

Hi Thomas, thanks for the feedback

 1. Were there guidelines?  Not really, though I looked at other
parameter definitions in com.sun.crypto.provider and tried to
follow along the same lines that they do.  One thing that should
be changed is the LINE_SEP assignment shouldn't be an explicit
getProperty call.  I noticed most are doing System.lineSeparator()
so I'll change my implementation to match that.  None of these
params appear to stringify as json, so I'll probably keep things
consistent with the other parameter output.
 2. You make a fair point with respect to a null SecureRandom. I can
make that spec change.
 3. Let me think on this one - I shied away from ChaCha20ParameterSpec
for AEAD mode only because you have this nonce field that is set
but gets ignored.  But making ChaCha20ParameterSpec an
IvParameterSpec potentially runs into the same issue were it used
for a ChaCha20-Poly1305 cipher. If I had to choose between the two
I think I'd go with allowing ChaCha20ParameterSpec to be used with
CC20-P1305 rather than making it a subclass of IvParameterSpec. 
Doing the former helps from a type safety perspective that you
couldn't use a ChaCha20ParameterSpec with other Ciphers that
require an IvParameterSpec.  I know I had some discussions early
on in the design where we talked about this, I need to refresh my
memory as to why we didn't allow it.

Finally getting back to #3.  Took me a while to find early discussions 
on this.  The primary objection to ChaCha20ParameterSpec being used with 
ChaCha20-Poly1305 (as opposed to plain old ChaCha20) has to do with the 
configurable block counter.  You have this parameter that is not used, 
and consumption of this type of AlgorithmParameterSpec then leaves it to 
documentation to define what happens (is it ignored?  Used despite what 
the spec says?  Set to some default value regardless of what the caller 
sets there?). Using IvParameterSpec with ChaCha20-Poly1305 is more clear 
because it only allows the caller what they need to get CC20/P1305 
going, the nonce.  Respectfully, I would like to keep this as-is.


--Jamil


On 3/26/2018 12:45 PM, Thomas Lußnig wrote:


Hi Jamil,

1) where there any guidelines about how the engineToString should be 
formatted ?
I ask because i wondering why we need two new lines with access to 
the System property.
If it is represented as single line json no need to line break would 
be needed.


Gruß Thomas


/** * Creates a formatted string describing the parameters. * * 
@return a string representation of the ChaCha20 parameters. */ 
@Override protected String engineToString() { String LINE_SEP = 
System.getProperty("line.separator"); HexDumpEncoder encoder = new 
HexDumpEncoder(); StringBuilder sb = new StringBuilder(LINE_SEP + 
"nonce:" + LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"); 
return sb.toString(); }

2) I do not think it is an good idea to say no secureRandom=null will cause IV 
to be null.
I see here the risk of weak implementations. I would suggest to throw an 
Exception to
enforce secure usages. If someone really want an insecure IV he can provide 
am SecureRandom
implementation retuning 0 only or an matching IV.

  * @param random a {@code SecureRandom} implementation.  If {@code null}
  *  is used for the random object, then a nonce consisting of all
  *  zero bytes will be used.  Otherwise a random nonce will be
  *  used.

3) If ChaCha20ParameterSpec would extends IvParameterSpec if would be valid for 
booth modes in engineInit.
 Even if the counter is not needed.
 As an alternative i would allow ChaCha20ParameterSpec also for AEAD mode.

Grup Thomas
On 3/26/2018 9:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

I think I could pull in Convert.hexStringToByteArray.  I might try 
pulling in that test class locally just to make sure things still run 
before you commit the field arithmetic stuff.  I also noticed I left in 
some debug routines in ChaCha20Cipher that need to get the axe, so I'll 
cut those out today.


--Jamil

On 3/28/2018 6:59 AM, Adam Petcher wrote:

On 3/28/2018 2:48 AM, sha.ji...@oracle.com wrote:

Would you like to move this method to a test lib class, like 
test/lib/jdk/test/lib/Utils.java? In fact, this class has a method, 
named toHexString, for converting bin to hex.


This method appears to be the same as Convert.hexStringToByteArray 
that I added to jdk.test.lib along with the field arithmetic 
implementation. That code is not incorporated yet, but the Poly1305 
code depends on the field arithmetic code, anyway. So adding this 
dependency may be reasonable.

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Adam Petcher]

On 3/28/2018 2:48 AM, sha.ji...@oracle.com wrote:

Would you like to move this method to a test lib class, like 
test/lib/jdk/test/lib/Utils.java? In fact, this class has a method, 
named toHexString, for converting bin to hex.


This method appears to be the same as Convert.hexStringToByteArray that 
I added to jdk.test.lib along with the field arithmetic implementation. 
That code is not incorporated yet, but the Poly1305 code depends on the 
field arithmetic code, anyway. So adding this dependency may be reasonable.

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[sha . jiang]

Hi Jamil,
I have a minor point on your tests.

-- com/sun/crypto/provider/Cipher/ChaCha20/ChaCha20KAT.java
 505 private static byte[] hex2bin(String hex) {
 506 int i;
 507 int len = hex.length();
 508 byte[] data = new byte [len / 2];
 509 for (i = 0; i < len; i += 2) {
 510 data[i / 2] = (byte)((Character.digit(hex.charAt(i), 
16) << 4) +

 511 Character.digit(hex.charAt(i + 1), 16));
 512 }
 513 return data;
 514 }
Would you like to move this method to a test lib class, like 
test/lib/jdk/test/lib/Utils.java? In fact, this class has a method, 
named toHexString, for converting bin to hex.
I think your method will be reused by other tests, including your 
another test 
com/sun/crypto/provider/Cipher/ChaCha20/ChaCha20Poly1305ParamTest.java.
In addition, you may want to declare the local variable "i" in the 
initialization expression in the for-loop.


Best regards,
John Jiang

On 27/03/2018 03:08, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Thomas Lußnig]

Hi,

this choice is even better than the current version. Because than the 
default system wide

secure random provider is used.

Gruß Thomas

On 3/27/2018 12:23 AM, Jamil Nimeh wrote:


Another thought on #2: Another way we could go with this is to create 
a new SecureRandom() or use JceSecurity.RANDOM when the random 
parameter is null.  That would make init(op, key, random) and init(op, 
key) behave the same when random is null.  You would always get a 
random nonce in these two forms.  I may go that direction since 
there's an established behavior for when no SecureRandom is provided 
through Cipher.init(int, Key).


--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Thomas Lußnig]

Hi Jamil,

1) where there any guidelines about how the engineToString should be 
formatted ?
I ask because i wondering why we need two new lines with access to the 
System property.
If it is represented as single line json no need to line break would be 
needed.


Gruß Thomas


/** * Creates a formatted string describing the parameters. * * @return 
a string representation of the ChaCha20 parameters. */ @Override 
protected String engineToString() { String LINE_SEP = 
System.getProperty("line.separator"); HexDumpEncoder encoder = new 
HexDumpEncoder(); StringBuilder sb = new StringBuilder(LINE_SEP + 
"nonce:" + LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"); return 
sb.toString(); }


2) I do not think it is an good idea to say no secureRandom=null will cause IV 
to be null.
   I see here the risk of weak implementations. I would suggest to throw an 
Exception to
   enforce secure usages. If someone really want an insecure IV he can provide 
am SecureRandom
   implementation retuning 0 only or an matching IV.

 * @param random a {@code SecureRandom} implementation.  If {@code null}
 *  is used for the random object, then a nonce consisting of all
 *  zero bytes will be used.  Otherwise a random nonce will be
 *  used.

3) If ChaCha20ParameterSpec would extends IvParameterSpec if would be valid for 
booth modes in engineInit.
Even if the counter is not needed.
As an alternative i would allow ChaCha20ParameterSpec also for AEAD mode.

Grup Thomas

On 3/26/2018 9:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Another thought on #2: Another way we could go with this is to create a 
new SecureRandom() or use JceSecurity.RANDOM when the random parameter 
is null.  That would make init(op, key, random) and init(op, key) behave 
the same when random is null.  You would always get a random nonce in 
these two forms.  I may go that direction since there's an established 
behavior for when no SecureRandom is provided through Cipher.init(int, Key).


--Jamil

On 03/26/2018 01:49 PM, Jamil Nimeh wrote:

Hi Thomas, thanks for the feedback

 1. Were there guidelines?  Not really, though I looked at other
parameter definitions in com.sun.crypto.provider and tried to
follow along the same lines that they do.  One thing that should
be changed is the LINE_SEP assignment shouldn't be an explicit
getProperty call.  I noticed most are doing System.lineSeparator()
so I'll change my implementation to match that.  None of these
params appear to stringify as json, so I'll probably keep things
consistent with the other parameter output.
 2. You make a fair point with respect to a null SecureRandom. I can
make that spec change.
 3. Let me think on this one - I shied away from ChaCha20ParameterSpec
for AEAD mode only because you have this nonce field that is set
but gets ignored.  But making ChaCha20ParameterSpec an
IvParameterSpec potentially runs into the same issue were it used
for a ChaCha20-Poly1305 cipher. If I had to choose between the two
I think I'd go with allowing ChaCha20ParameterSpec to be used with
CC20-P1305 rather than making it a subclass of IvParameterSpec. 
Doing the former helps from a type safety perspective that you

couldn't use a ChaCha20ParameterSpec with other Ciphers that
require an IvParameterSpec.  I know I had some discussions early
on in the design where we talked about this, I need to refresh my
memory as to why we didn't allow it.

--Jamil


On 3/26/2018 12:45 PM, Thomas Lußnig wrote:


Hi Jamil,

1) where there any guidelines about how the engineToString should be 
formatted ?
I ask because i wondering why we need two new lines with access to 
the System property.
If it is represented as single line json no need to line break would 
be needed.


Gruß Thomas


/** * Creates a formatted string describing the parameters. * * 
@return a string representation of the ChaCha20 parameters. */ 
@Override protected String engineToString() { String LINE_SEP = 
System.getProperty("line.separator"); HexDumpEncoder encoder = new 
HexDumpEncoder(); StringBuilder sb = new StringBuilder(LINE_SEP + 
"nonce:" + LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"); 
return sb.toString(); }

2) I do not think it is an good idea to say no secureRandom=null will cause IV 
to be null.
I see here the risk of weak implementations. I would suggest to throw an 
Exception to
enforce secure usages. If someone really want an insecure IV he can provide 
am SecureRandom
implementation retuning 0 only or an matching IV.

  * @param random a {@code SecureRandom} implementation.  If {@code null}
  *  is used for the random object, then a nonce consisting of all
  *  zero bytes will be used.  Otherwise a random nonce will be
  *  used.

3) If ChaCha20ParameterSpec would extends IvParameterSpec if would be valid for 
booth modes in engineInit.
 Even if the counter is not needed.
 As an alternative i would allow ChaCha20ParameterSpec also for AEAD mode.

Grup Thomas
On 3/26/2018 9:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi Thomas, thanks for the feedback

1. Were there guidelines?  Not really, though I looked at other
   parameter definitions in com.sun.crypto.provider and tried to follow
   along the same lines that they do.  One thing that should be changed
   is the LINE_SEP assignment shouldn't be an explicit getProperty
   call.  I noticed most are doing System.lineSeparator() so I'll
   change my implementation to match that.  None of these params appear
   to stringify as json, so I'll probably keep things consistent with
   the other parameter output.
2. You make a fair point with respect to a null SecureRandom.  I can
   make that spec change.
3. Let me think on this one - I shied away from ChaCha20ParameterSpec
   for AEAD mode only because you have this nonce field that is set but
   gets ignored.  But making ChaCha20ParameterSpec an IvParameterSpec
   potentially runs into the same issue were it used for a
   ChaCha20-Poly1305 cipher.  If I had to choose between the two I
   think I'd go with allowing ChaCha20ParameterSpec to be used with
   CC20-P1305 rather than making it a subclass of IvParameterSpec. 
   Doing the former helps from a type safety perspective that you
   couldn't use a ChaCha20ParameterSpec with other Ciphers that require
   an IvParameterSpec.  I know I had some discussions early on in the
   design where we talked about this, I need to refresh my memory as to
   why we didn't allow it.

--Jamil


On 3/26/2018 12:45 PM, Thomas Lußnig wrote:


Hi Jamil,

1) where there any guidelines about how the engineToString should be 
formatted ?
I ask because i wondering why we need two new lines with access to the 
System property.
If it is represented as single line json no need to line break would 
be needed.


Gruß Thomas


/** * Creates a formatted string describing the parameters. * * 
@return a string representation of the ChaCha20 parameters. */ 
@Override protected String engineToString() { String LINE_SEP = 
System.getProperty("line.separator"); HexDumpEncoder encoder = new 
HexDumpEncoder(); StringBuilder sb = new StringBuilder(LINE_SEP + 
"nonce:" + LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"); return 
sb.toString(); }

2) I do not think it is an good idea to say no secureRandom=null will cause IV 
to be null.
I see here the risk of weak implementations. I would suggest to throw an 
Exception to
enforce secure usages. If someone really want an insecure IV he can provide 
am SecureRandom
implementation retuning 0 only or an matching IV.

  * @param random a {@code SecureRandom} implementation.  If {@code null}
  *  is used for the random object, then a nonce consisting of all
  *  zero bytes will be used.  Otherwise a random nonce will be
  *  used.

3) If ChaCha20ParameterSpec would extends IvParameterSpec if would be valid for 
booth modes in engineInit.
 Even if the counter is not needed.
 As an alternative i would allow ChaCha20ParameterSpec also for AEAD mode.

Grup Thomas
On 3/26/2018 9:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which outlines 
the characteristics and behavior of the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil