[9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation

Thu Aug 18 15:40:36 UTC 2016

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/66babbb8/attachment.htm>