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
