RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

Sean Mullan sean.mullan at oracle.com
Wed May 2 14:35:59 UTC 2018


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



More information about the security-dev mailing list