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

[Valerie Peng]

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

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/20160817/09e7f93b/attachment.htm>