RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

Jamil Nimeh jamil.j.nimeh at oracle.com
Sat May 19 14:40:16 UTC 2018 

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).

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/20180519/1147b955/attachment.htm>

More information about the security-dev mailing list