RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

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.

   46 public final class Poly1305

Can this be package-private?

   84             keyBytes.length);
  122             BLOCK_LENGTH - blockOffset);

indent at least 4 more spaces

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

- ChaCha20KAT.java

looks good.

- ChaCha20NoReuse.java

looks good.

- ChaCha20Cipher.java

Exception messages say "bits" - should we use "bytes" to be consistent 
with the API?

  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.

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

  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.

  395                 throw new UnsupportedOperationException("Invalid 
mode: " +

Seems inconsistent with 321 which throws RuntimeExc.

  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.

  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.

--Sean