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

Xuelei Fan xuelei.fan at oracle.com
Sat Nov 19 02:26:32 UTC 2011


>> 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?

I thought to expose the package private class and method. The update
looks like:

  package sun.security.pkcs11;
  ...
- abstract class P11Key implements Key {
+ abstract public class P11Key implements Key {
  ...
-     int keyLength() {
+     public int keyLength() {
  ...
  }


It is similar for MSCAPI. Then we may be able to call the keyLength()
method directly.

But as MSCPI is only available for Windows, we may run into compiling
problem on non-windows platforms.


Another solution is to define a new interface:
    package sun.security.util;
    public interface Measurable {
        public int getLength();
    }

And updated P11Key and MSCPI Key accordingly to implement the
Measureable interface:
  package sun.security.pkcs11;
  ...
- abstract class P11Key implements Key, Measurable {
+ abstract class P11Key implements Key {
  ...
-     int keyLength() {
+     public int getLength() {
  ...
  }

It is similar for MSCAPI Key.

Xuelei

On 11/18/2011 10:43 PM, Weijun Wang wrote:
> 
> 
> 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