<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 8/17/2016 11:36 PM, Valerie Peng
wrote:<br>
</div>
<blockquote
cite="mid:89d1e859-a988-627d-6b7c-c5ad6f062f18@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Regression tests are still running, but thought that I will send
the updated webrev out and see if there are more comments.<br>
<br>
Webrev is updated at: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/8078661/webrev.01/">http://cr.openjdk.java.net/~valeriep/8078661/webrev.01/</a><br>
<br>
Thanks,<br>
Valerie<br>
</blockquote>
Hi Valerie - <br>
<br>
You know - re-reading this code I'm reminding of why PKCS11 annoys
me so much.<br>
<br>
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.<br>
<br>
That's going to be a surprise if someone tries to cast this return
to an (RSAKey) or (RSAPrivateKey). <u>This should be changed so a
key of the appropriate type is always created.</u><br>
<br>
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.<br>
<br>
<br>
(Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey,
but should probably be in RSAKey instead).<br>
<br>
So:<br>
<br>
All RSA keys - even the sensitive private ones - should return
CKA_MODULUS.<br>
All RSA Private CRT Keys - even the sensitive ones - should also
return CKA_PUBLIC_EXPONENT.<br>
All non-sensitive RSA Private keys - should also return
CKA_PRIVATE_EXPONENT<br>
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.<br>
<br>
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.<br>
<blockquote type="cite">Note that the error codes
CKR_ATTRIBUTE_SENSITIVE,
CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not
denote true errors
for <b>C_GetAttributeValue</b>. If a call to <b>C_GetAttributeValue</b>
returns any of these three values, then the call must nonetheless
have
processed <i>every</i> attribute in the template supplied to <b>C_GetAttributeValue</b>.
Each attribute in the template whose value <i>can be</i> returned
by the call
to <b>C_GetAttributeValue</b> <i>will be</i> returned by the
call to <b>C_GetAttributeValue</b>.</blockquote>
<br>
<br>
<br>
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. <br>
<br>
Sorry to complicate this. Mike<br>
<br>
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.<br>
<br>
<blockquote
cite="mid:89d1e859-a988-627d-6b7c-c5ad6f062f18@oracle.com"
type="cite"> <br>
<div class="moz-cite-prefix">On 8/17/2016 9:55 AM, Michael StJohns
wrote:<br>
</div>
<blockquote
cite="mid:d605af02-3f3b-8fb5-4bbd-ebbd836457cc@comcast.net"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 8/16/2016 9:24 PM, Valerie Peng
wrote:<br>
</div>
<blockquote
cite="mid:f2065a91-b1bd-f55b-7e1f-055752fcdfe9@oracle.com"
type="cite"> <br>
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. <br>
<br>
Bug: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8078661">https://bugs.openjdk.java.net/browse/JDK-8078661</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/8078661/webrev.00/">http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/</a>
<br>
<br>
Thanks, <br>
Valerie <br>
</blockquote>
<p>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.</p>
<p>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.</p>
<p>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.</p>
<p>2) Then get the rest of the stuff. If that fails, then you
already have the stuff you need for a normal private key.<br>
</p>
<p> <br>
</p>
<p> </p>
<blockquote type="cite">
<pre> boolean crtKey;
try {
session.token.p11.C_GetAttributeValue
(session.id(), keyID, attrs2);
<span class="removed">- crtKey = (attrs2[0].pValue instanceof byte[]);</span>
<span class="new">+ crtKey = ((attrs2[1].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[3].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[4].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[5].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[6].pValue instanceof byte[]) &&</span>
<span class="new">+ (attrs2[7].pValue instanceof byte[])) ;</span>
} catch (PKCS11Exception e) {
// ignore, assume not available
crtKey = false;
}</pre>
</blockquote>
<p>// Change attrs2 so it only has the additional CRT attributes
(e.g. delete CKA_MODULUS, CKA_PRIVATE_EXPONENT from the list<br>
</p>
<p>Replace the above with <br>
</p>
<p>CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {<br>
new CK_ATTRIBUTE(CKA_MODULUS),<br>
new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)<br>
};<br>
// no try block needed here - we want to throw the error if it
occurs<br>
session.token.p11.C_GetAttributeValue (session.id(), keyID,
attrs3);</p>
<p>// So far so good - we have the base attributes, let's see if
we can get the additional attributes;</p>
<p>try { <br>
session.token.p11.C_GetAttributeValue(session.id(),keyID,
attrs2);<br>
} catch (PKCS11Exception e) {<br>
// we really should check the return value for one of the
non-fatal values, but let's just assume its not a CRT key<br>
return new P11RSAPrivateNonCRTKey (session, keyID,
algorithm, keyLength, attrs2, attrs3);<br>
}</p>
<p>// if we fall through then its a CRT key<br>
// -- we should check for byte[] ness of each of the
components, and throw an error if they arent - but which
error?</p>
<p>return new P11RSAPrivateKey (session, keyID, algorithm,
keyLength, attrs2, attrs3);<br>
</p>
<p>// 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.<br>
</p>
<p>static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[] attrs, long
attrType) {<br>
for (CK_ATTRIBUTE a : attrs) {<br>
if (a.type == attrType)<br>
return a;<br>
}<br>
return null; // or throw something?<br>
}<br>
</p>
<p><br>
coeff = getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();<br>
</p>
<br>
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.<br>
<br>
Mike<br>
<br>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</body>
</html>