Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
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
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
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
*
