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