[9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
Valerie Peng
valerie.peng at oracle.com
Fri Aug 19 00:48:06 UTC 2016
I share your view on most things. It's just that the APIs are there
before the PKCS11 provider is added.
So, there are some history reason as to why things are as they are today.
Re-structuring the public classes are almost impossible considering the
compatibility impact.
However, we can explore other functional fixes if necessary.
Thanks for the review and feedback, it's very helpful,
Valerie
On 8/18/2016 2:25 PM, Michael StJohns wrote:
> On 8/18/2016 4:49 PM, Valerie Peng wrote:
>> Hi Mike,
>>
>> Thanks for the feedback and the detailed write up.
>>
>> The scenario here is complicated by the sensitive/non-extractable
>> keys of PKCS#11 and the fact that java key and key specification
>> classes assume all relevant values being available.
> Um... I think that's true for any PublicKey, but not for Secret or
> Private keys. At worst, the Key object is a handle for the real key
> that contains all those items, but they might not be available. At
> best, most of those components will be available. I say at best,
> because of the language in RSAMultiPrimePrivateCRTKey for
> getOtherPrimeInfo() which says it can return null.
>
>> Only when all relevant values are available, then we will construct
>> the corresponding key objects. This is necessary as there are other
>> providers which may receive such keys and they can't handle keys like
>> this.
>
> Keys can't generally move across providers AIRC? You can try and use
> a key factory to convert them, but that's not guaranteed. A PKCS11
> derived key isn't going to be portable to another provider without
> extraction to a keyspec in any case.
>
> Hmm.. I went back and read the JDK8 p11 guide
> (https://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html)
> and section 3.2 gives the guidance that you only use the generic
> interfaces for unextractable keys. I actually think that's wrong,
> given the general guidance in the JCA documentation with respect to
> Opaque Keys vs transparent KeySpecs. (Hmm... I wonder if this
> guidance was in the originally submitted code package documentation).
>
> Then there's the point that even a generic Public or Private key has a
> "getEncoded()" method. *bleah*
>
>
>>
>> I am sure that the current PKCS11 provider code needs many
>> improvement/finer handlings. But I don't see a straightforward way of
>> "making CKA_PUBLIC_EXPONENT available" across various RSA Key
>> classes. This should be tracked in a different issue.
>
> That's just one of the items. As I mentioned in another email, I
> think the RSA key classes and interfaces need a bit more work and
> tweaking. I wouldn't try and accomplish that quite yet.
>
>>
>> Given the current release schedule, the deadline for this fix (P4) is
>> coming up in 10 days and I will be on vacation next week.
>>
>> If you agree with the value of addressing this with the proposed
>> changes for JDK 9, then we can proceed.
>> Otherwise, I will defer this bug to the update release and we can
>> spend more time to polish this.
>
> I think you might as well go ahead with this change. The fix you've
> got should work as long as someone who generates a RSA Key pair on a
> PKCS11 which is both sensitive and unextractable doesn't try to cast
> the keys to RSAPublic or RSAPrivate.
>
> Mike
>
>
>> Valerie
>>
>> On 8/18/2016 8:40 AM, Michael StJohns wrote:
>>> On 8/17/2016 11:36 PM, Valerie Peng wrote:
>>>> Regression tests are still running, but thought that I will send
>>>> the updated webrev out and see if there are more comments.
>>>>
>>>> Webrev is updated at:
>>>> http://cr.openjdk.java.net/~valeriep/8078661/webrev.01/
>>>>
>>>> Thanks,
>>>> Valerie
>>> Hi Valerie -
>>>
>>> You know - re-reading this code I'm reminding of why PKCS11 annoys
>>> me so much.
>>>
>>> At line 333 (of the "new" P11Key) you grab the Token, Sensitive and
>>> Extractable values and if the private data is sensitive or not
>>> extractable you create a generic P11PrivateKey and return that.
>>> However the contract for RSAKey requires that the public modulus be
>>> returned if available, and, since its not a sensitive attribute it
>>> probably should be available. Also, even if the key is sensitive -
>>> if its a sensitive CRT key, then CKA_PUBLIC_EXPONENT should be
>>> available.
>>>
>>> That's going to be a surprise if someone tries to cast this return
>>> to an (RSAKey) or (RSAPrivateKey). _This should be changed so a key
>>> of the appropriate type is always created._
>>>
>>> Also, checking for CKA_EXTRACTABLE being true, doesn't actually get
>>> you access to the clear text information. If a key is extractable,
>>> then it can be wrapped out under another key. The components
>>> themselves aren't available. It's possible to have a non-sensitive,
>>> non-extractable key where the components are retrievable, but the
>>> key can't be wrapped out.
>>>
>>>
>>> (Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey,
>>> but should probably be in RSAKey instead).
>>>
>>> So:
>>>
>>> All RSA keys - even the sensitive private ones - should return
>>> CKA_MODULUS.
>>> All RSA Private CRT Keys - even the sensitive ones - should also
>>> return CKA_PUBLIC_EXPONENT.
>>> All non-sensitive RSA Private keys - should also return
>>> CKA_PRIVATE_EXPONENT
>>> All non-sensitive RSA Private CRT Keys - should also return
>>> CKA_PRIME_1, CKA_PRIME_2, CKA_EXPONENT_1, CKA_EXPONENT_2 and
>>> CKA_COEFFICIENT.
>>>
>>> This is harder to do than it needs to be due to how
>>> p11_objmgt.c::Java_sun_security_pkcs11_wrapper_PKCS11_C_1GetAttributeValue
>>> is built. At lines 248 and 270, it does a check for an error return
>>> and throws an exception if any error occurs. However, for
>>> C_GetAttributeValue, there are a number of "non-fatal" errors that
>>> indicate either buffer size errors or sensitivity of one or more
>>> components or unavailability of one or more components.
>>>> Note that the error codes CKR_ATTRIBUTE_SENSITIVE,
>>>> CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote
>>>> true errors for *C_GetAttributeValue*. If a call to
>>>> *C_GetAttributeValue* returns any of these three values, then the
>>>> call must nonetheless have processed /every/ attribute in the
>>>> template supplied to *C_GetAttributeValue*. Each attribute in the
>>>> template whose value /can be/ returned by the call to
>>>> *C_GetAttributeValue* /will be/ returned by the call to
>>>> *C_GetAttributeValue*.
>>>
>>>
>>>
>>> If you updated this slightly - maybe by adding a new method to
>>> wrapper.PKCS11 (say GetAttributeValuesNoError) - to return the
>>> attributes it was able to get in the call with nulls elsewhere, then
>>> you could do all of the above in one pass.
>>>
>>> Sorry to complicate this. Mike
>>>
>>> ps - I don't have a current build environment set up for the JDK,
>>> otherwise I'd code it and test it myself. I'm happy to take a swing
>>> at it and provide you unverified code you can integrate.
>>>
>>>>
>>>> On 8/17/2016 9:55 AM, Michael StJohns wrote:
>>>>> On 8/16/2016 9:24 PM, Valerie Peng wrote:
>>>>>>
>>>>>> Anyone has time to review a straightforward fix? The current
>>>>>> PKCS11 code assume that if public exponent is available for RSA
>>>>>> Private Key, then it's a RSA CRT key. However, not all vendor
>>>>>> implementation works this way. Changing to a tighter check and
>>>>>> did minor code-refactoring to avoid re-retrieving the attribute
>>>>>> values.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8078661
>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Valerie
>>>>>
>>>>> Given that there's a change to PKCS11 for 2.40 that says that all
>>>>> RSA private key objects MUST also store CKA_PUBLIC_EXPONENT, some
>>>>> change needed to be made.
>>>>>
>>>>> Sorry - I don't think this fix will work. Or if its working on
>>>>> your version of PKCS11, your version of PKCS11 is doing it wrong.
>>>>> The problem is that if you specify attributes that don't exist on
>>>>> the object, the underlying PKCS11 library is supposed to return
>>>>> CKR_ATTRIBUTE_TYPE_INVALID. And that should trigger a thrown
>>>>> exception before you ever get anything copied to your attributes.
>>>>>
>>>>> 1) Get modulus and private exponent first. That gives you the
>>>>> stuff for a generic RSA private key - and if it fails, there's no
>>>>> reason to continue.
>>>>>
>>>>> 2) Then get the rest of the stuff. If that fails, then you
>>>>> already have the stuff you need for a normal private key.
>>>>>
>>>>>
>>>>>> boolean crtKey;
>>>>>> try {
>>>>>> session.token.p11.C_GetAttributeValue
>>>>>> (session.id(), keyID, attrs2);
>>>>>> - crtKey = (attrs2[0].pValue instanceof byte[]);
>>>>>> + crtKey = ((attrs2[1].pValue instanceof byte[]) &&
>>>>>> + (attrs2[3].pValue instanceof byte[]) &&
>>>>>> + (attrs2[4].pValue instanceof byte[]) &&
>>>>>> + (attrs2[5].pValue instanceof byte[]) &&
>>>>>> + (attrs2[6].pValue instanceof byte[]) &&
>>>>>> + (attrs2[7].pValue instanceof byte[])) ;
>>>>>> } catch (PKCS11Exception e) {
>>>>>> // ignore, assume not available
>>>>>> crtKey = false;
>>>>>> }
>>>>>
>>>>> // Change attrs2 so it only has the additional CRT attributes
>>>>> (e.g. delete CKA_MODULUS, CKA_PRIVATE_EXPONENT from the list
>>>>>
>>>>> Replace the above with
>>>>>
>>>>> CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {
>>>>> new CK_ATTRIBUTE(CKA_MODULUS),
>>>>> new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)
>>>>> };
>>>>> // no try block needed here - we want to throw the error if it occurs
>>>>> session.token.p11.C_GetAttributeValue (session.id(), keyID, attrs3);
>>>>>
>>>>> // So far so good - we have the base attributes, let's see if we
>>>>> can get the additional attributes;
>>>>>
>>>>> try {
>>>>> session.token.p11.C_GetAttributeValue(session.id(),keyID, attrs2);
>>>>> } catch (PKCS11Exception e) {
>>>>> // we really should check the return value for one of the
>>>>> non-fatal values, but let's just assume its not a CRT key
>>>>> return new P11RSAPrivateNonCRTKey (session, keyID, algorithm,
>>>>> keyLength, attrs2, attrs3);
>>>>> }
>>>>>
>>>>> // if we fall through then its a CRT key
>>>>> // -- we should check for byte[] ness of each of the components,
>>>>> and throw an error if they arent - but which error?
>>>>>
>>>>> return new P11RSAPrivateKey (session, keyID, algorithm, keyLength,
>>>>> attrs2, attrs3);
>>>>>
>>>>> // there are cleanups necessary in other places. I'd suggest
>>>>> rather than depending on the ordering of attributes, you do
>>>>> assignment by CKA_ values just so someone coming later doesn't
>>>>> mess things up by mistake. Also, a hell of a lot more readable.
>>>>>
>>>>> static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[] attrs, long
>>>>> attrType) {
>>>>> for (CK_ATTRIBUTE a : attrs) {
>>>>> if (a.type == attrType)
>>>>> return a;
>>>>> }
>>>>> return null; // or throw something?
>>>>> }
>>>>>
>>>>>
>>>>> coeff = getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();
>>>>>
>>>>>
>>>>> The other possibility is to change the C code for
>>>>> C_GetAttributeValues so it doesn't error out for the non-fatal
>>>>> error codes and instead returns a null value attribute when the
>>>>> attribute is missing.
>>>>>
>>>>> Mike
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160818/5311d79e/attachment.htm>
More information about the security-dev
mailing list