RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations
sha.jiang at oracle.com
sha.jiang at oracle.com
Sat May 19 23:13:54 UTC 2018
Hi Jamil,
On 19/05/2018 22:40, Jamil Nimeh wrote:
> Hi John, comments below:
>
> On 5/19/2018 1:31 AM, sha.jiang at 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.jiang at 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 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180520/33befaa1/attachment.htm>
More information about the security-dev
mailing list