[9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
Michael StJohns
mstjohns at comcast.net
Thu Aug 18 21:25:29 UTC 2016
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/685307c0/attachment.htm>
More information about the security-dev
mailing list