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

Weijun Wang weijun.wang at oracle.com
Fri Nov 18 14:43:16 UTC 2011 


On 11/18/2011 10:38 PM, Xuelei Fan wrote:
> On 11/18/2011 10:16 PM, Weijun Wang wrote:
>>
>> 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?
>>
> In the previous webrev, I found the case while running
> test/sun/security/pkcs11/KeyStore/ClientAuth.java while call
> Class.forName() with the current class loader.
>
>>>    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?
>>
> I also concerns about the override of the target method, although there
> is no such override at present. From some reports, it seems that fields
> reflection cost more cycles than method reflection.

Oh, I thought get a field is simpler.

>
> Performance is a big issue here. What do you think if we change the get
> key length method of pkcs11.P11Key and mscapi.Key for better performance?

How do you want to change?

-Max

>
> Thanks,
> Xuelei
>
>> 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
>>>>>
>>>
>

More information about the security-dev mailing list