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
