308242 - Expose new key generation functions in JSS for key export

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Thomas Kwan]

Need to expose new NSS functionalities to 
JSS for generating keys that can be 
exported from hardware tokens.

We are proposing to expose the following 
new functions in JSS:

  PK11KeyPairGenerator.generateKeyPairWithFlags(int attrflags);
  PK11KeyGenerator.generate(jint opflags, jint attrflags);

Please see also the following NSS bug for reference

https://bugzilla.mozilla.org/show_bug.cgi?id=299197

Comment 1

[Thomas Kwan]

Attachment: Proposed patch

Comment 2

[Thomas Kwan]

Attachment: Proposed patch with changes in jss.def

Comment 3

[Thomas Kwan]

Attachment: proposed patch with constant defines

Comment 4

[Thomas Kwan]

Attachment: Patch with function overloading

Comment 5

[Wan-Teh Chang]

Comment on attachment 196221 [details] [diff] [review]
Patch with function overloading 

Thomas, this patch looks good, but it has a serious problem.

In security/jss/org/mozilla/jss/crypto/KeyGenerator.java, we have:

>+    /* opflags */
>+    public static int CKA_SENSITIVE = 0x00000103;
>+    public static int CKA_ENCRYPT = 0x00000104;
>+    public static int CKA_DECRYPT = 0x00000105;
>+    public static int CKA_WRAP = 0x00000106;
>+    public static int CKA_UNWRAP = 0x00000107;
>+    public static int CKA_SIGN = 0x00000108;
>+    public static int CKA_SIGN_RECOVER = 0x00000109;
>+    public static int CKA_VERIFY = 0x0000010A;
>+    public static int CKA_VERIFY_RECOVER = 0x0000010B;
>+    public static int CKA_DERIVE = 0x0000010C;

CKA_SENSITIVE should be removed.  The remaining flags should have
the CKF_ prefix, and their values should match the same-named
macros defined in mozilla/security/nss/lib/softoken/pkcs11t.h:
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/pkcs11t.h#960

A suggestion: The C functions ..._PK11_KeyGenerator_generateWithFlags
and ..._PK11KeyPairGenerator_generateRSAKeyPairWithFlags are almost the
same as the existing without-flags functions.  I urge you to define
the similar functions to share code.  You could do that by defining
the existing functions in terms of the new functions.  If this code
refactoring is not as easy as it seems, we can do this later.

I will give you suggested changes to the comments in person.

Comment 6

[Thomas Kwan]

Attachment: Included wtc's review comments

Comment 7

[Wan-Teh Chang]

Attachment: Thomas Kwan's patch after wtc's editing

I have a question: is KeyGenerator the right interface
in which to define the PK11_ATTR_XXX and CKF_XXX flags?
Those flags are also used by KeyPairGenerator.

Comment 8

[Thomas Kwan]

We can create IOpAttr.java and IPK11Attr.java that provide the defines.
For example,

public interface IOpAttr
{
  public static final int CKF_ENCRYPT = 0x00000100;
  ...
}

This is not too object oriented. But we do not need to pass in any
Java object into the JNI.

For a more object-oriented way, we can create the following classes

public class OpAttr
{
  public final static OpAttr CKF_ENRYPT = new OpAttr(0x0000100);
  ...

  private int _i;
  private OpAttr(int i)
  {
    _i = i;
  }
  
  public int intValue()
  {
    return _i;
  }
}

public class PK11Attr
{
 public final static OpAttr PK11_ATTR_TOKEN = new OpAttr(0x0000001);
  ...

  private int _i;
  private OpAttr(int i)
  {
    _i = i;
  }
  
  public int intValue()
  {
    return _i;
  }
}

Comment 9

[Wan-Teh Chang]

Attachment: Thomas Kwan's patch after wtc's editing, v2

Improved the comments.

Comment 10

[Wan-Teh Chang]

Attachment: Export the extractable/unextractable attributes of PK11_GenerateKeyPairWithFlags

I found that the KeyPairGenerator class already has a
temporaryPairs method for specifying whether temporary
(session) or permanent (token) keypairs should be generated.
So, to be in line with this method, I added an
extractablePairs method for specifying extractable/unextractable
keypairs.

See
http://lxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11akey.c#1089

for how PK11_GenerateKeyPair has been reimplemented by
calling PK11_GenerateKeyPairWithFlags.	I used the same
code in this patch to maintain backward compatibility
with the PK11_GenerateKeyPair calls in the original code.

I changed the prototype of the private (native) methods
generateRSAKeyPair and generateDSAKeyPair.  My reading of
the Java Language Specification says this change is binary
compatible.  Glen, please verify this.
(http://java.sun.com/docs/books/jls/third_edition/html/binaryComp.html)

Comment 11

[Wan-Teh Chang]

Comment on attachment 200152 [details] [diff] [review]
Export the extractable/unextractable attributes of PK11_GenerateKeyPairWithFlags

Thomas, Glen, please review this JSS patch today.  Thanks.

Comment 12

[Wan-Teh Chang]

Attachment: Diffs between PK11_GenerateKeyPair and PK11_GenerateKeyPairWithFlags

To help you review the JSS patch, here are the diffs
between PK11_GenerateKeyPair and PK11_GenerateKeyPairWithFlags.

The most important part is this part in the middle:

@@ -146,12 +157,8 @@
 
     /* set up the private key template */
     privattrs = privTemplate;
-    PK11_SETATTRS(privattrs, CKA_SENSITIVE, sensitive ? &cktrue : &ckfalse, 
-					sizeof(CK_BBOOL)); privattrs++;
-    PK11_SETATTRS(privattrs, CKA_TOKEN, token ? &cktrue : &ckfalse,
-					 sizeof(CK_BBOOL)); privattrs++;
-    PK11_SETATTRS(privattrs, CKA_PRIVATE, sensitive ? &cktrue : &ckfalse,
-					 sizeof(CK_BBOOL)); privattrs++;
+    privattrs += pk11_AttrFlagsToAttributes(attrFlags, privattrs,
+						&cktrue, &ckfalse);
 
     /* set up the mechanism specific info */
     switch (type) {

This shows how the 'sensitive' parameter of PK11_GenerateKeyPair
controls both the CKA_SENSITIVE and CKA_PRIVIATE attributes.  This
explains the following code in the JSS patch that replaces the
original PK11_GenerateKeyPair call with an equivalent
PK11_GenerateKeyPairWithFlags call (note that 'temporary' is
the negation of 'token'):

@@ -162,13 +163,31 @@
     /**************************************************
      * generate the key pair on the token
      *************************************************/
-    privk = PK11_GenerateKeyPair(   slot,
-                                    CKM_RSA_PKCS_KEY_PAIR_GEN,
-                                    (void*) &params, /* params is not a ptr */
-                                    &pubk,
-                                    !temporary, /* token (permanent) object */
-                                    sensitive,
-                                    NULL /* default PW callback */ );
+    if( temporary ) {
+        attrFlags |= PK11_ATTR_SESSION;
+    } else {
+        attrFlags |= PK11_ATTR_TOKEN;
+    }
+    if( extractable == 1 ) {
+        attrFlags |= PK11_ATTR_EXTRACTABLE;
+    } else if( extractable == 0 ) {
+        attrFlags |= PK11_ATTR_UNEXTRACTABLE;
+    }
+    /*
+     * The PRIVATE/PUBLIC attributes are set this way to be backward
+     * compatible with the original PK11_GenerateKeyPair call.
+     */
+    if( sensitive ) {
+        attrFlags |= (PK11_ATTR_SENSITIVE | PK11_ATTR_PRIVATE);
+    } else {
+        attrFlags |= (PK11_ATTR_INSENSITIVE | PK11_ATTR_PUBLIC);
+    }
+    privk = PK11_GenerateKeyPairWithFlags(slot,
+                                          CKM_RSA_PKCS_KEY_PAIR_GEN,
+                                          &params, /* params is not a ptr */
+                                          &pubk,
+                                          attrFlags,
+                                          NULL /* default PW callback */ );
     if( privk == NULL ) {
         int errLength;
         char *errBuf;

Comment 13

[glen beasley]

Comment on attachment 200152 [details] [diff] [review]
Export the extractable/unextractable attributes of PK11_GenerateKeyPairWithFlags

binary compatiblity is good and the patch looks good, with the clause that I did not run any tests on my end.

Comment 14

[Thomas Kwan]

looks good , thanks!

Comment 15

[Wan-Teh Chang]

I've checked in the patch on the JSS tip (JSS 4.1.1)
and JSS_3_X_BRANCH (JSS 3.7).

Comment 16

[Thomas Kwan]

Wan-Teh,

Turned out that I need a similar function for org.mozilla.jss.crypto.KeyGenerator.java also.

Currently, when I generate a symmetric key by
calling 

  kg = token.getKeyGenerator(KeyGenAlgorithm.DES);
  Symmetric sk = kg.generate();

The corresponding NSS performs

122219440[b591f240]: C_GenerateKey
122219440[b591f240]:   hSession = 0xa
122219440[b591f240]:   pMechanism = 0x748dbbc
122219440[b591f240]:   pTemplate = 0x748dbdc
122219440[b591f240]:   ulCount = 2
122219440[b591f240]:   phKey = 0xb592d34c
122219440[b591f240]:     CKA_ENCRYPT = CK_TRUE [1]
122219440[b591f240]:     CKA_SIGN = CK_TRUE [1]
122219440[b591f240]:       mechanism = 0x131
122219440[b591f240]:   *phKey = 0x1d
122219440[b591f240]:   rv = 0x0

In order to make Luna happy, I need

-1218582208[9c52548]: C_GenerateKey
-1218582208[9c52548]:   hSession = 0xa
-1218582208[9c52548]:   pMechanism = 0xbfff9bc0
-1218582208[9c52548]:   pTemplate = 0xbfff9be0
-1218582208[9c52548]:   ulCount = 5
-1218582208[9c52548]:   phKey = 0x9fe9b0c
-1218582208[9c52548]:     CKA_TOKEN = CK_TRUE [1]
-1218582208[9c52548]:     CKA_PRIVATE = CK_TRUE [1]
-1218582208[9c52548]:     CKA_ENCRYPT = CK_TRUE [1]
-1218582208[9c52548]:     CKA_WRAP = CK_TRUE [1]
-1218582208[9c52548]:     CKA_UNWRAP = CK_TRUE [1]
-1218582208[9c52548]:       mechanism = 0x131
-1218582208[9c52548]:   *phKey = 0x15
-1218582208[9c52548]:   rv = 0x0

Other, a later call to Wrap Key will fail with

122219440[b591f240]: C_WrapKey
122219440[b591f240]:   hSession = 0x8
122219440[b591f240]:   pMechanism = 0x748dc20
122219440[b591f240]:   hWrappingKey = 0x1d
122219440[b591f240]:   hKey = 0x24
122219440[b591f240]:   pWrappedKey = 0xb583c198
122219440[b591f240]:   pulWrappedKeyLen = 0x748dc3c
122219440[b591f240]:       mechanism = 0x136
122219440[b591f240]:   *pulWrappedKeyLen = 0x1000
122219440[b591f240]:   rv = 0x63

Comment 17

[Wan-Teh Chang]

Attachment: Expose the opFlags and the temporary/permanent attributes of PK11_TokenKeyGenWithFlags

Please test and review this patch.

This patch tries to reuse existing JSS facilities and
imitate the style.  So the opFlags are specified using
an array of the existing SymmetricKey.Usage objects,
and the temporary/permanent attributes are specified
using a new KeyGenerator.temporaryKeys() method (in the
style of the KeyPairGenerator.temporaryPairs() method).

The default opFlags are CKF_SIGN | CKF_ENCRYPT for
backward compatibility.  The default temporaryKeyMode
is true (temporary keys), again for backward compatibilty.
Note that for permanent (token) keys I also make them
"private" PKCS #11 objects so that they can only be used
after the user has authenticated to the token.

To review this patch, you need to see with LXR how PK11_KeyGen
is implemented with PK11_TokenKeyGen, and how PK11_TokenKeyGen
is implemented with PK11_TokenKeyGenWithFlags:
http://lxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11skey.c#1044
http://lxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11skey.c#1015
This will motivate the default values chosen to maintain backward
compatibility.

It is unusual for a boolean field 'temporaryKeyMode' to have
the default value 'true'.  (Usually booleans default to false.)
That field could be named 'permanentKeyMode' with the default
value 'false'.  It is named 'temporaryKeyMode' to be consistent
with PK11KeyPairGenerator.temporaryPairMode.  If you think
'permanentKeyMode' looks better, please speak up.

Comment 18

[Wan-Teh Chang]

Comment on attachment 201479 [details] [diff] [review]
Expose the opFlags and the temporary/permanent attributes of PK11_TokenKeyGenWithFlags

Here is an example of how the new methods would be used
to make Luna happy:

         // DES key
         KeyGenerator kg = token.getKeyGenerator(KeyGenAlgorithm.DES);
+        SymmetricKey.Usage usages[] = new SymmetricKey.Usage[3];
+        usages[0] = SymmetricKey.Usage.ENCRYPT;
+        usages[1] = SymmetricKey.Usage.WRAP;
+        usages[2] = SymmetricKey.Usage.UNWRAP;
+        kg.setKeyUsages(usages);
+        kg.temporaryKeys(false);
         SymmetricKey key = kg.generate();

Comment 19

[glen beasley]

Comment on attachment 201479 [details] [diff] [review]
Expose the opFlags and the temporary/permanent attributes of PK11_TokenKeyGenWithFlags

Look good. A nice to have would be if you added some test code to org.mozilla.jss.tests.SymKeyGen

Comment 20

[Wan-Teh Chang]

Comment on attachment 201479 [details] [diff] [review]
Expose the opFlags and the temporary/permanent attributes of PK11_TokenKeyGenWithFlags

I've checked in this patch on the JSS tip (JSS 4.1.1)
and JSS_3_X_BRANCH (JSS 3.7).

I made some changes to SymKeyGen.java to test the
changes.  The reason I didn't include those changes
in this patch is that I'm waiting for Sandeep's changes
to that test.  (I assume Sandeep has been improving
that test to test the other symmetric ciphers and
that's how he discovered the problems with RC4 and
AES.)  If Sandeep can attach his current patch to
a bug report, I'll backport his changes to the
JSS_3_X_BRANCH.

Comment 21

[Wan-Teh Chang]

Attachment: Expose the opFlags and the temporary/permanent attributes of PK11_TokenKeyGenWithFlags (as checked in)

This is what I actually checked in.  I made a simple change
to test for null elements in the 'usages' array.

Comment 22

[Thomas Kwan]

One very minor thing we can do is to check to see if usages is null (1).
But other than that, looks ok.

    public void setKeyUsages(SymmetricKey.Usage[] usages)
    {
        this.opFlags = 0;
        if (usages != null) { <------------ (1)
          for( int i = 0; i < usages.length; i++ ) {
            if( usages[i] != null ) {
               ...
            }
          }
        }
    }

Comment 23

[Wan-Teh Chang]

Thomas: thanks for the code review.  I made exactly the change
you suggested when I checked in the patch :-)

Please click on the "Edit" links to the right of the two patches
I asked you to review and set "review+".  Thanks.

Comment 24

[Wan-Teh Chang]

Thomas: I misunderstood your suggestion.   My checkin
doesn't test 'usages' for null.  Sorry about the
confusion.

A null 'usages' should be treated as either an invalid
input argument or "use the default key usages (ENCRYPT+SIGN)".
I'd prefer to treat it as an invalid input argument.  What's
the Java way to do that?

Comment 25

[Thomas Kwan]

Java supports zero size array. For example, the caller of
setKeyUsages can create

  SymmetricKey.Usage usages = new SymmetricKey.Usage[0];

The "usages" created above is not null, yet it represents
an empty array. If the pre-condition for setKeyUsages
requires an empty array, then the function will not break.

Otherwise, if the caller accidentially passes in null,
then setKeyUsages will raise a runtime exception of 
NullPointerException.

One possible design could be to add the following 
member functions to the key generator class

        kg.enableKeyUsage(usage);
        kg.disableKeyUsage(usage);
        kg.isKeyUsageSet(usage);

If we need to add multiple usages, we can create a
class that represents a set of usages. 

It probably is not efficient to check for null in setKeyUsages
as I previously suggested. What we can do is to javadoc
the fact that this function does not accept null.

Comment 26

[Robert Relyea]

Comment on attachment 200152 [details] [diff] [review]
Export the extractable/unextractable attributes of PK11_GenerateKeyPairWithFlags

NSS portions look good
r=relyea