RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

Jamil Nimeh jamil.j.nimeh at oracle.com
Wed May 2 15:23:02 UTC 2018



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


More information about the security-dev mailing list