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

[Michael StJohns]

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/105a109f/attachment.htm>