Re: RFR 6913047: SunPKCS11 memory leak

[Michael StJohns]

Hi Valerie -

I may be speaking only for myself, but there are probably a number of 
folk using sun.security.pkcs11.wrapper.* in lieu of the PKCS11 provider 
for a number of reasons including an ability to manage the key storage 
and wrapping efficiently.   Looking at this I'm thinking there will be a 
large number of things breaking because of the way you refactored things.


While I understand that none of the sun.security.* classes are "public" 
- these were mostly taken directly from the IAIK contribution way back 
when and the folks I worked with used them in lieu of external libraries 
to have a consistent PKCS11 interface java-to-java.


Perhaps instead you'd consider doing something like leaving the Java to 
Native public methods intact?


Mike




On 8/31/2018 7:53 PM, Valerie Peng wrote:


Hi Martin,

With the new model of "creating the key handle as needed", I think we 
should not allow the direct access of keyID field outside of P11Key 
class. This field should be made private and accessed through methods. 
In addition, existing PKCS11.C_XXX() methods with P11 keyID arguments 
can be made private and we can add wrapper methods with P11Key object 
argument to ensure proper usage of the P11Key object and its keyID field.


I have also refactored the keyID management code into a separate 
holder class. The prototype code can be found at: 
http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.00/


The main changes that I made are in P11Key.java and PKCS11.java. The 
other java classes are updated to call the wrapper methods in PKCS11.java.


Thanks & have a great Labor day weekend,

Valerie


On 8/16/2018 5:28 PM, Valerie Peng wrote:


Hi Martin,


Since there is no regression test for this test, you will need to add 
a "noreg-xxx" label to the bug record. For this issue, it'll probably 
be "noreg-hard".


To understand the changes better, please find my questions and some 
review comments below:



- line 397: It's incorrect to change initialize() to 
ensureInitialized(). If the cipher object were to be initialized 
twice with two different keys, you need to re-initialize again.

- line 471: Why do you change it to Throwable from PKCS11Exception?


- line 99: comment is somewhat unclear. typo: "temporal" should be 
"temporary".
- line 148-149: JDK-8165996 has been fixed. This does not apply now, 
does it?
- You changed the constructors of all the P11Key-related classes to 
take an additional boolean argument "tmpNativeKey". However, this 
boolean argument is only used when the underlying token is "NSS". Why 
limiting to NSS only? It seems that all callers except for PKCS11 
KeyStore pass true for "tmpNativeKey". When "tmpNativeKey" is true, 
the keyID handle first destroyed in constructor and later created 
again (ref count == 1) and destroyed (when ref count drops to 0). 
This replaced the PhantomReference approach in SessionKeyRef class, 
right? Now both approaches seem to be used and key destruction is 
taking places at different methods. I think we should clarify the 
cleanup model and perhaps refactor the code so it's easier which 
approach is in use. Or grouping all these cleanup-related 
fields/variables into a separate class for readability/clarity.
- line 157-175: nativeKeyWrapperKeyID is a static variable, shouldn't 
it be synchronized on a class level object?
- line 1343: the impl doesn't look right. Why do you call both 
removeObject() and addObject() here with the same check?
- line 1363: the change seems incorrect, you should still call 
session.removeObject(). the call setKeyIDAndSession(0, null) does not 
lower the session object count.


Thanks,
Valerie

On 8/7/2018 5:41 PM, Valerie Peng wrote:


Hi Martin,

Thanks for the update, I will resume the review on this one with 
your latest webrev.


BTW, there is no webrev.07 for your other fix, i.e. JDK-8029661, 
right? Just checking.


Regards,
Valerie

On 8/3/2018 2:13 PM, Martin Balao wrote:

Hi Valerie,

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.10.zip 



New in webrev 10:

 * Bug fix when private DSA/EC keys are sensitive
  * I found this bug removing "attributes = compatibility" entry in 
NSS configuration file so keys were CKA_SENSITIVE.
  * This is really an NSS bug when unwrapping ephemeral keys 
(NSC_UnwrapKey function), because CKA_NETSCAPE_DB attribute is 
required but not used/needed. There was a similar bug when creating 
objects (NSC_CreateObject function), fixed many years ago [1].
  * In those cases in which the bug occurs (private keys, of DSA/EC 
type, sensitive and without CKA_NETSCAPE_DB attribute set), I store 
an empty CKA_NETSCAPE_DB attribute in the buffer that will later be 
used for key unwrapping. I'm not doing a C_SetAttributeValue call 
because keys 

Re: Conceptual feedback on new ECC JEP

[Michael StJohns]

On 8/23/2018 1:50 PM, Adam Petcher wrote:
It will only support a subset of the API that is supported by the 
implementation in SunEC. In particular, it will reject any private 
keys with scalar values specified using BigInteger (as in 
ECPrivateKeySpec), and its private keys will not return scalar values 
as BigInteger (as in ECPrivateKey.getS()). 


Um... why?   EC Private keys are integers I've said this multiple 
times and - with the single exception of EDDSA keys because of a very 
idiosyncratic (and IMHO short-sighted) RFC specification - all of the EC 
private keys of whatever curve can be expressed as integers.


Mike