Code Review Request for 7146728 and 7130959

Wed Mar 7 03:19:24 UTC 2012

Brad,

Thanks for the review.
Please find the update webrev at:
http://cr.openjdk.java.net/~valeriep/7146728/webrev.01/

On 03/02/12 17:31, Brad Wetmore wrote:
> DHKeyAGreement.java
> ===================
> Mainly just a request for some additional commenting so the reader 
> doesn't have to have a good understanding of the BigInteger logic to 
> understand the assumptions being made here:
>
>     For example, if the expected bits of the modulus is
>     1024 bits (128 bytes exactly)
>
>     secret-magnitude     secret.length           action
>     ================     =============           ======
>     1024                 1025 bits (129 bytes)   trim front byte
>     1023                 1024 bits (128 bytes)   return
>                                                  (sign bit (always 0)
>                                                   becomes leading 0)
>     ...
>     1015                 1016 bits (127 bytes)   shift to end of array
>                                                  (implied leading 0's)
>     ...
>
I added more comments to both code blocks to explain what's going on.

> In the second bit of code, you have the new ProviderException that you 
> don't have in the first.  I'm not sure how you could get into this 
> case, if you're % by modulus, but I guess it's good to be careful.  
> That said, you should either make consistent with the above or 
> remove.  You might also combine the actual copy logic into a single 
> function called from both.
Good point, I modified the engineGenerateSecret() to leverage 
engineGenerateSecret(byte[], int) method.

> P11KeyAgreement.java
> ====================
>
>     // Solaris
>
> Shouldn't this be "// Solaris/NSS", for the case in which there isn't 
> a leading zero?
Well, I think putting NSS in both places seems somewhat confusing. What 
I tried to point out is that NSS (at least in the older versions, I have 
not tried the newer version to see if they switched behavior) trims off 
the leading 0s whenever possible. I've made some modification hoping to 
make it clearer.

> For performance, it's a shame that engineGenerateSecret(byte[] 
> sharedSecret, int offset) is doing a second copy here.  Not a request 
> to do anything, just an observation.
Well, that's just how it is written from the very beginning. The current 
impl is more optimized for engineGenerateSecret().
Probably not worthwhile to re-write the code to get rid of the extra 
copying.

>
> DHKeyAgreement2.java
> ====================
> >    Security.addProvider(jce)
>
> Good grief!  ;)  Still finding these!
>
> I think you should include the new bugid.
>
>      @bug 7146728
Done.

> TestShort.java
> ==============
> New bugid
>
>     @bug 4942494 7146728
Done.

> Indention problem around the try.
Fixed.

> Was the change at 69 because the leading zeros were being trimmed?
Yes, SunPKCS11 provider was explicitly trimming off the leading 0s 
before this 7146728 fix.
Now that we decide to preserve the leading 0s, this test vector has to 
be updated.

Thanks,
Valerie

> Brad
>
>
>
>
>
>
> On 3/1/2012 2:03 PM, Valerie (Yu-Ching) Peng wrote:
>>
>> Yes, that's the section that mentioned the generated secret having the
>> same length as p.
>> I guess it depends on how it's interpreted. The way I look at it is that
>> if the generated secrets aren't of the same length, then whoever using
>> this variant for deriving the keying material will need to do the extra
>> work of checking the size of generated secret assuming that they know
>> the size of p to add back the leading 0s if needed.
>> I can't find any specification dictating trimming off the leading 0s for
>> the generated secret. Considering both models, preserving vs trimming, I
>> feel the former makes more sense since it is simpler to use and provides
>> more flexibility than the later. RFC 2631 has not be crystal clear on
>> this I am afraid, that's why there has been some inconsistency in
>> different vendors implementation since people interpret what they read
>> differently.
>> Valerie
>>
>> On 02/29/12 23:00, Brad Wetmore wrote:
>>>
>>>
>>> On 2/21/2012 5:33 PM, Valerie (Yu-Ching) Peng wrote:
>>>> Brad,
>>>>
>>>> Can you please review the fixes for the following 2 bugs:
>>>>
>>>> * 7146728: Inconsistent length for the generated secret using DH key
>>>> agreement impl from SunJCE and PKCS11
>>>> o http://cr.openjdk.java.net/~valeriep/7146728/webrev.00/
>>>>
>>>> This impacts both SunJCE provider and SunPKCS11 provider. The
>>>> implementations are inconsistent within SunJCE provider itself
>>>> between the engineGenerateSecret() and
>>>> engineGenerateSecret(byte[], int). Given that RFC 2631 specifies
>>>> the leading 0s must be preserved so the generated secret has as
>>>> many octets as the prime P,
>>>
>>> Just to be clear here, you're referring to Section 2.1.2 of 2631,
>>> which is just one of the DH Key agreement variants (based on X9.42)
>>> for generating Keying Material from secret keys obtained from a "raw"
>>> DH calculations, and is then subject later SHA1 manipulations, right?
>>> This method provides motivation/incentive to output our secret keys
>>> with the same lengths, but I don't think this RFC makes any claims
>>> that the general output of "raw" DH key agreement operation must be
>>> the same length.
>>>
>>> I'll take another look over the code tomorrow.
>>>
>>> Thanks,
>>>
>>> Brad
>>>
>>>
>>> I have changed both SunJCE and
>>>> SunPKCS11 provider to do so. When testing against Solaris and
>>>> NSS libraries, Solaris preserves the leading 0s while NSS trims
>>>> it off, thus similar handling is also needed in SunPKCS11 provider.
>>>>
>>>> * 7130959: Tweak 7058133 fix for JDK 8 (javah makefile changes)
>>>> o http://cr.openjdk.java.net/~valeriep/7130959/webrev.00/
>>>>
>>>> Instead of using the -Xbootclasspath, switching over to use
>>>> -boothclasspath for consistency with the backported changes in
>>>> the update releases for earlier JDK, e.g. 7u.
>>>>
>>>> Thanks,
>>>> Valerie
>>>>
>>>>
>>