Code Review Request for 6996769: support AEAD ciphers

[Valerie (Yu-Ching) Peng]

FYI, I have filed 8005830 "Enhance GCM parameters generation"
Thanks,
Valerie


On 01/03/13 20:14, Michael StJohns wrote:
> Hi Valerie - Comments in line.
>
> At 09:17 PM 1/3/2013, Valerie (Yu-Ching) Peng wrote:
>> Michael,
>>
>> I thought this some more. I think a cleaner way to do what you wanted may be achieved by adding an AlgorithmParameterGenerator impl for GCM.
>> I've read through your email multiple times but yet I can't think of a good way to fit all the functionality that you wanted into the GCMParameterSpec class while maintaining the contract of AlgorithmParameterSpec and AlgorithmParameters.
> I've been trying to figure out if what I was looking for was a contract violation.  The problem is that the counter based encryption modes have a state requirement beyond that usually handled by the AlgorithmParam* and Cipher classes.  Each message (or rather, each init of the Cipher object) needs to get a unique IV.  Mostly the IV for message N+1 is based on a counter increment of the IV for message N.  That ensures you don't repeat the IV.  You can do this out of band to the Cipher and AlgorithmParam* objects by managing the IV as external data, but there are some guidances (especially in the case of GCM) which would really rather the IV uniqueness was enforced inside the cryptographic context.  In some ways what's needed is a CipherConext class which wraps a Cipher object and an IV generator
>
>> The GCMParameterSpec.generateNextIV() method also seems a bit strange to me since you need a random source and GCMParameterSpec is just a spec and isn't associated with any provider.
> Fair.  But in the end the IV is really just a formatting convention over a set of common (nonce) data and a variable part (which is constant for each Cipher.init() context).  There are only a few defined ways of generating an IV for GCM (and for that matter for CCM and CTR) based on the previous or first IV.
>
> I keep going back and forth on whether the spec or the parameters object should get the generateNextIv() function, but in the end, it may be both (to deal with how you implement this to support an IV generator inside an HSM).  I think you can build a Spec class that provides the right functionality to get started.
>
>
>> This also means that the conversion between GCMParameterSpec and GCM AlgorithmParameters object and vice versa, may not return you the same spec.
> I now think that the right answer is that generateNextIv() should return an immutable AlgorithmParam* object.  That should keep the relationship contract that currently exists.
>
>
>> I guess I have a hard time accepting the idea of an AlgorithmParameterSpec object with changing values. This are just my opinion, however.
> I was trying to avoid the garbage collection of a per-message AlgorithmParam* object.  Is there a design document that requires AlgorithmParameterSpec's to be immutable?   If so, just return a new object rather than updating the internal state of the Spec object.
>
>
>> I will file a separate RFE to keep track of this as I think more discussion are needed.
>> JDK8 feature deadline is approaching and we can address this as an RFE separately.
> I think that's reasonable.  I may take a whack at defining the three or four Spec classes (a CounterSpec and the three CTR, GCM and CCM mode specs derived from that spec).
>
> Thanks - Mike
>
>
>> Regards,
>> Valerie
>>
>> On 12/17/12 10:54, Michael StJohns wrote:
>>> Hi Valerie -
>>>
>>> Comments in line.
>>>
>>> At 08:32 PM 12/14/2012, Valerie (Yu-Ching) Peng wrote:
>>>> Mike,
>>>>
>>>> Thanks for the comments...
>>>> With the current API, it is still possible to have the IV generated at the provider level by calling Cipher.init(...) without GCM parameters.
>>>> This means that provider will generate IVs using provider-default iv length as well as provider-default value for tag length.
>>>> Of course, things may be a little awkward if you need to use custom tag length and a different iv length from what the provider uses as default.
>>> Right.  And that may be required by specific protocols.  Not saying it is now, but there needs to be support for this in the API.
>>>
>>>
>>>> SunJCE provider uses 128-bit tag length as well as 96-bit IVs as default. If there are demands to use other non-default values, then we should see if something can be done to facilitate these scenarios.
>>> It's perfectly acceptable for any given implementation to only permit 96 bit IVs,   But the AEAD algorithms have the possibility of some really unique initialization variables.
>>>
>>> Going back to just the 96 bit IV you've got two ways of constructing the IV - a deterministic and a random (8.2.1 and 8.2.2).  For the both of these you have a "fixed field" (aka the "free field" which may be empty in 8.2.2) which is specified by the user. For 8.2.1 you also have an invocation field which is either a linear counter or an LFSR based value.  For 8.2.2 you have a random field.
>>>
>>> You really need an API for the GCMParameterSpec which allows you to specify either the IV value or the generation parameters.
>>>
>>>
>>>
>>>> As for preventing the same IV from being re-used, I think it takes more than just adding a new constructor for GCMParameterSpec.
>>> Right - but the enforcement has to be in the cipher engine, and only for the ENCRYPT operation.
>>>
>>>> Generally, AlgorithmParameterSpec objects can be converted to AlgorithmParameters objects and vice versa. If the GCMParameterSpec only specifies iv length, would you expect the provider to automatically generate a new IV w/o explicit re-init? If yes, then what should the Cipher.getParameters() return, the new one or the old one?
>>> Depends on whether there is a contract of immutability for AlgorithmParameterSpec's - if no, then you update the state in the existing object and return that object.  If yes, you create a new object from the existing object with the generated IV.
>>>
>>>
>>>> Regardless which way we choose, it introduces a window on when Cipher.getParameters() should be called.
>>> If you pass in the GCMParameterSpec with just the IV format info, then when you call init, you cause that object to do a generate of the IV.  The GCMParameterSpec object should indicate whether the IV was passed or generated.
>>>
>>> Thinking this through, the GCMParameterSpec needs  "generateNextIV()" method.  If the GCMParameterSpec is an IV generator rather than an IV container, then generateNextIV() updates the internal state of the parameter spec and IV.  Every time you pass this is as an init() argument, you call generateNextIV() - the spec will either ignore the call (IV container) or update the IV (IV generator).  (Hmm... possibly a common interface method for AEAD and CTR mode parameter spec object).
>>>
>>> For the pass back and forth between the GCMAlgorithmParameter object and this, just reflect the state at the time called.
>>>
>>> I think this works for the ordering specified in the GCM spec.
>>>
>>> This isn't perfect - what you really want is some state to continue from Cipher.init to Cipher.init call without reference to an externally twiddleable object.  Cipher.updateIV which would do an internal reset and then update the parameter counter appropriately to get a new IV for the next message?
>>>
>>> Something similar will probably be needed for CCM as well.
>>>
>>>
>>>> Valerie
>>>>
>>>> On 12/13/12 20:07, Michael StJohns wrote:
>>>>> Sorry for the late comment -
>>>>>
>>>>> You might want to consider section 9.1, first paragraph of SP800-38D which defines the GCM mode.  Basically, for FIPS validated implementations, to prevent accidental reuse, the IV needs to be generated inside the cryptographic boundary using one of the defined mechanism.   Obviously, that's on the ENCRYPT side since you need to match the IV on the DECRYPT side.
>>>>>
>>>>> I think you may need to add a GCMParameterSpec that allows you to specify either the IV or the IV length as a subclass of IvParameterSpec.
>>>>>
>>>>> Mike
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> At 08:23 PM 12/7/2012, Valerie (Yu-Ching) Peng wrote:
>>>>>> Max,
>>>>>>
>>>>>> The webrev has been updated so that different key + iv values have to be used for AES/GCM encryption.
>>>>>> Latest version at: http://cr.openjdk.java.net/~valeriep/6996769/webrev.03/
>>>>>>
>>>>>> Please review and send me comments.
>>>>>> Thanks!
>>>>>> Valerie
>>>>>>
>>>>>> On 11/07/12 21:50, Valerie (Yu-Ching) Peng wrote:
>>>>>>> Max,
>>>>>>>
>>>>>>> Update: I removed the block (starting line 580 in CipherCore.java) for handling RC2 since it's never used.
>>>>>>>
>>>>>>> It turns out that the current impl in RC2Cipher always convert the AlgorithmParameters object to RC2ParameterSpec and only uses CipherCore.init(..., AlgorithmParameterSpec,...) method. Thus, I won't be adding a regression test, but rather simply document the current RC2Cipher behavior in CipherCore.java to clarify things up.
>>>>>>>
>>>>>>> The updated webrev is at:
>>>>>>> http://cr.openjdk.java.net/~valeriep/6996769/webrev.01/
>>>>>>>
>>>>>>> Xuelei brought up the issue of enforcing (Key+IV) uniqueness for GCM mode during this afternoon's meeting.
>>>>>>> I think more changes may be made after we decide what to do.
>>>>>>> So, there may be a webrev.02 coming... Just a heads up.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Valerie
>>>>>>>
>>>>>>> On 11/07/12 14:48, Valerie (Yu-Ching) Peng wrote:
>>>>>>>>> 580                  } else if (key.getAlgorithm().equals("RC2")) {
>>>>>>>>>
>>>>>>>>> This seems a bug fix. Is there a regression test for it?
>>>>>>>> I just noticed this problem when make the enhancement for GCM mode.
>>>>>>>> I will add a regression test for this.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/security-dev/attachments/20130107/9db6a5d0/attachment-0001.html