RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Jamil Nimeh]

Hi Sean, many thanks for taking a detailed look at the code.  My 
comments are in-line

On 5/1/2018 1:53 PM, Sean Mullan wrote:
> On 5/1/18 1:20 PM, Sean Mullan wrote:
>> On 4/27/18 5: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/
>
> More comments:
>
> - Poly1305.java
>
>   44  * @since 10
>
> Not necessary, but should be 11 if so.
JN: Will fix
>
>   46 public final class Poly1305
>
> Can this be package-private?
JN: Not sure.  I will make it so and run through the tests.  It will 
probably work.
>
>   84             keyBytes.length);
>  122             BLOCK_LENGTH - blockOffset);
>
> indent at least 4 more spaces
JJN: Will fix.
>
> - ChaCha20Poly1305ParamTest.java
>
> When you test for exceptions, you should throw an exception if the 
> method does not throw an exception. For example:
>
>  171         try {
>  172             apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
>  173         } catch (IllegalArgumentException iae) {
>  174             System.out.println("Caught expected exception: " + iae);
>  175         }
>
> should be something like:
>
>     try {
>         apsBad.init(NONCE_OCTET_STR_12, "OraclePrivate");
>         throw new Exception("init passed unexpectedly");
>     } catch (IllegalArgumentException iae) {
>         System.out.println("Caught expected exception: " + iae);
>     }
>
> there are several other cases like this.
JN: Yikes!  I normally do put an exception in the happy-path when I'm 
expecting a failure.  Major oversight here.  I will fix this.
>
> - ChaCha20KAT.java
>
> looks good.
>
> - ChaCha20NoReuse.java
>
> looks good.
>
> - ChaCha20Cipher.java
>
> Exception messages say "bits" - should we use "bytes" to be consistent 
> with the API?
JN: Sure, I can do that.
>
>  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).
>
>  301                 if (newNonce.length != 12) {
>  302                     throw new InvalidAlgorithmParameterException(
>  303                         "ChaCha20 nonce must be 96 bits in length");
>  304                 }
>
> You don't need to check for this, since the ChaCha20ParamSpec class 
> does already (especially if you make it final).
JN: Yep, you're right.  A throwback to an earlier rev of the code before 
the ChaCha20ParameterSpec rigidly enforced this.  I can remove it.
>
>
>  375                         if (dv.tag == DerValue.tag_OctetString) {
>  376                             // Get the nonce value
>  377                             newNonce = dv.getOctetString();
>
> Similar to a previous comment, I would just call 
> DerValue.getOctetString() directly and let it throw an Exception if it 
> is the wrong tag.
JN: Except that I'll still have to catch the IOException and make that 
the cause of the InvalidAlgorithmParameterException.  I don't think 
CipherSpi.engineInit(int, Key, AlgorithmParameters, SecureRandom) can 
directly throw IOException.  If I have to wrap it inside IAPE, do you 
still prefer to do it that way or is the existing way OK?
>
>  395                 throw new UnsupportedOperationException("Invalid 
> mode: " +
>
> Seems inconsistent with 321 which throws RuntimeExc.
JN: So it is.  I'll change the UOE to RuntimeException.
>
>  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.
>
>  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.

Thank you for the detailed comments!
--Jamil