Code review request, 7106773: 512 bits RSA key cannot work with SHA384 and SHA512

[Weijun Wang]

On 11/18/2011 08:39 PM, Xuelei Fan wrote:
> The VPN is awesomely bad today.
>
> The webrev in cr.openjdk is
> http://cr.openjdk.java.net/~xuelei/7106773/webrev.01/
>
> On 11/18/2011 7:39 PM, Weijun Wang wrote:
>> Oh, there seems to be much more non-trivial code changes then the
>> previous version of webrev.
>>
>> I now focus on the utility class KeyLength.java:
>>
>> In ReflectiveProvider, how about adding a "." at the end of package name?
>>
> It's safer.
>
>> Also, is the while loop really necessary in getKeyLengthMethod? Why not
>> directly call getDeclaredMethod on the the baseClassName? If you want to
>> make sure the current class is a subclass of the baseClassName, try
>> isAssignableFrom.
>>
> It's a little strange of the implement of getKeyLengthMethod() because
> of the following causes:
> 1. Class.getDeclaredMethod() cannot return inherited methods.
>
> 2. Class.forName("sun.security.pkcs11.P11Key") is not reliable.
>     The current class loader is not always able to find the P11Key class.

Oh, In which case?

>   As you saw in the previous webrev, I specified the class loader while
> searching for the class:
>     Class.forName("sun.security.pkcs11.P11Key", true,
>                        key.getClass().getClassLoader());
>
>     There is one assumption that every key will come from the same class
> loader during the JVM life. So we can cache the loaded P11Key class.
> It's a little risky.
>
>     And more, it's OK when there is only one PKCS11 provider.  However,
> while we adding MSCAPI in, we will have to look through both PKCS11
> provider and MSCAPI provider in order to select the proper cached key
> class. If we get a key from none of PKCS11 and MSCAPI, we will continue
> calling the Class.forName() repeatedly until we are able to encounter
> the PKCS11 key or MSCAPI key. It's very bad in performance.
>
>     The calling in the getKeyLengthMethod() is lightweight, and we don't
> have to worry about the class loader any more.

Calling getDeclaredMethod each time is a waste. How about just

         while (clazz != null) {
             if (clazz.getName().equals(baseClassName)) {
                 // do sth
                 break;
             }
             clazz = clazz.getSuperclass();
         }

Also, you're using methods in reflection. Is it faster to use fields?

Thanks
Max

>
>> As for the test, I was thinking that you can add a unit test on the
>> KeyLength class itself.
>>
> I may add some new and simple tests on KeyLength only.
>
> Thanks,
> Xuelei
>
>> Thanks
>> Max
>>
>>
>> On 11/17/2011 11:14 PM, Xuelei Fan wrote:
>>> Sean, could you also review the fix? I plan to backport the fix to JDK
>>> 7, so that the key size constraints for both JSSE and CertPath can work
>>> with PKCS11 and MSCPI.
>>>
>>> updated webrev:
>>> http://javaweb.us.oracle.com/~xf138604/bugbios/7106773/webrev.01/
>>>
>>> In the new webrev, I add a new KeyLength class under sun.security.util.
>>> I think it may be useful for other models.
>>>
>>> On 11/11/2011 2:33 PM, Weijun Wang wrote:
>>>> SSLContextVersion.java:
>>>>
>>>>      typo: suported ->   supported
>>>>
>>> Updated.
>>>
>>>> DisabledAlgorithmConstraints.java:
>>>>
>>>>      When can size == 0? Why it's not allowed?
>>>>
>>> It's a line of dummy code in case the get key size method return 0.
>>>
>>>>      Yes, I think the reflection calls should be wrapped in doPriv
>>>>
>>>>      Please also consider keys from MSCAPI
>>>>
>>> Support MSCAPI now.
>>>
>>>>      Try looking at sun.misc.SharedSecrets and see if it's better
>>>>      for you. Probably not. That class is used to share non-public
>>>>      methods in public classes. Not the same case here.
>>>>
>>>> SignatureAndHashAlgorithm.java:
>>>>
>>>>      Is it possible to combine expected and signingKey into a single
>>>>      signingKey? Of course that means it might need to be never null
>>>>      and all key alg call it in a consistent way. I wonder if this
>>>>      is OK for you.
>>>>
>>> It not always work. For example, the key algorithm may be "EC", but the
>>> expected algorithm may be "ECDSA".
>>>
>>>>      The RSAKey interface seems not used anywhere.
>>>>
>>> Yes, removed.
>>>
>>>>      How about changing
>>>>
>>>>       288 }   // Otherwise, cannot determine the key size, prefer the
>>>> most
>>>>       289     // perferable hash algorithm.
>>>>
>>>>      to
>>>>
>>>>       maxDigestLength = Integer.MAX_VALUE;
>>>>
>>>>      then you don't need to assign -1 and check for<   0 later.
>>>>
>>> Good point.
>>>
>>>>      I would be glad if lines 302-306 can be further indented 4 spaces
>>>>
>>>>      The comment on lines 268-273 is confusing, because you did calculate
>>>>      maxDigestAlgorithm for 512 bits key later.
>>>>
>>>>      Personally, I prefer comments on 274-281 and codes on 284-288
>>>>      to be from a smaller keySize to bigger ones, but everything is OK.
>>>>
>>> Updated.
>>>
>>>> Test:
>>>>
>>>>      I would like a unit test on
>>>> DisabledAlgorithmConstraints::getKeySize.
>>>>      Hopefully the test can cover all SecretKey and PrivateKey from all
>>>>      providers with all keysizes.
>>>>
>>> Added some new tests for PKCS11 and MSCAPI.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>>> Thanks
>>>> Max
>>>>
>>>> On 11/11/2011 01:09 PM, Xuelei Fan wrote:
>>>>> Hi Weijun,
>>>>>
>>>>> Are you available to review my fix for CR 7106773? The fix in JSSE part
>>>>> is straightforward in that the call to
>>>>> SignatureAndHashAlgorithm.getPreferableAlgorithm() needs an additional
>>>>> Key parameter for RSA key exchanges. The significant change is at
>>>>> sun/security/util/DisabledAlgorithmConstraints.java. I modified the
>>>>> code
>>>>> in order to get the key size of the unextractable key in PKCS#11
>>>>> device.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7106773/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>
>