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
