Code Review Request for 7146728 and 7130959

[Valerie (Yu-Ching) Peng]

I thought about this too when I noticed that other security sources also 
have codes just simply remove the leading sign byte without checking the 
overall length.

I will make an utility function for this in one of my other JDK 8 fixes 
which cover more providers.
For this bug, the current changes work, so I will proceed as planned.

Thanks for the feedback,
Valerie

On 03/14/12 18:58, Michael StJohns wrote:
> Sorry for the late comment here -
>
> It may make sense to make a utility function with the calling sequence similar to
>
> static byte[] getBytesFixedLength (BigInteger x, int numBits)
>          throws IllegalArgumentException
>
> some where in the security utility code.
>
> The length of the returned byte array is ceiling(numBits/8).
>
> Throws IAE if x is negative or zero or numBits is<  x.bitLength().
>
> Put the various comments in the utility function instead of spreading them throughout the code.
>
> This function should be used for both ECDH and DH.
>
> Rethrow the IAE as a ProviderException  if it occurs (it shouldn't for DH or ECDH generated secrets).
>
> Mike
>
>
> At 07:01 PM 3/14/2012, Brad Wetmore wrote:
>> Here's the comment I was trying to suggest in your office yesterday:
>>
>>         /*
>>          * NOTE: BigInteger.toByteArray() returns a byte array containing
>>          * the two's-complement representation of this BigInteger with
>>          * the most significant byte in the zero-th element. This
>>          * contains the minimum number of bytes required to represent
>>          * this BigInteger, including at least one sign bit whose value
>>          * is always 0.
>>          *
>>          * Keys are always positive, and the above sign bit isn't
>>          * actually used when representing keys.  (i.e. key = new
>>          * BigInteger(1, byteArray))  To obtain an array containing
>>          * exactly expectedLen bytes of magnitude, we strip any extra
>>          * leading 0's, or pad with 0's in case of a "short" secret.
>>          */
>>
>> Hope this helps,
>>
>> Brad
>>
>>
>>
>> On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
>>> Ok, more comments/changes added as suggested.
>>> Webrev updated at:
>>> http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
>>>
>>> Thanks,
>>> Valerie
>>>
>>> On 03/08/12 17:15, Brad Wetmore wrote:
>>>>>> DHKeyAgreement:
>>>>>> ===============
>>>>> I added more comments to both code blocks to explain what's going on.
>>>> A couple more requested comments.
>>>>
>>>> Around line 299, could please add that the sign bit is always 0.
>>>>
>>>> Around line 303, could you put in a comment that since there is enough
>>>> room here for the sign bit in this array, it just becomes a leading zero.
>>>>
>>>>> Good point, I modified the engineGenerateSecret() to leverage
>>>>> engineGenerateSecret(byte[], int) method.
>>>> Great, thanks!
>>>>
>>>>>> 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.
>>>> Much better, thanks!
>>>>
>>>> Minor nit (take or leave) you could adjust the logic around 208 to
>>>> only create newSecret if secret.length<  secretLen.
>>>>
>>>> if (secret.length>  secretLen) {
>>>> throw new ProviderException(...
>>>> }
>>>> byte [] newSecret = new byte[secretLen];
>>>> System.arraycopy(...);
>>>> return newSecret;
>>>>
>>>>
>>>>>> TestShort.java
>>>>>> ==============
>>>>>> New bugid
>>>>>>
>>>>>> @bug 4942494 7146728
>>>>> Done.
>>>>>
>>>>>> Indention problem around the try.
>>>>> Fixed.
>>>> Thanks for taking out the commented out code, I almost mentioned it
>>>> and then didn't.
>>>>
>>>> Brad
>