Bugzilla Bug 299197: define two bitflags for every PKCS #11 object

attribute with no exceptions. renamed PK11_ATTR_READONLY as
PK11_ATTR_UNMODIFIABLE.  In pk11_OpFlagsToAttributes, backed out a change
I made before.  Made pk11_AttrFlagsToAttributes table-driven. In
pk11_loadPrivKeyWithFlags, fixed the bug (always loading the public key as
a token object).  Other code cleanups. r=relyea,nelsonb.
Modified files: pk11akey.c pk11obj.c pk11pub.h pk11skey.c secmodt.h

security/nss/lib/pk11wrap/pk11akey.c

@@ -626,9 +626,9 @@ pk11_loadPrivKeyWithFlags(PK11SlotInfo *slot,SECKEYPrivateKey *privKey,
 	return NULL;
      }
 
-     /* try loading the public key as a token object */
+     /* try loading the public key */
      if (pubKey) {
-	PK11_ImportPublicKey(slot, pubKey, PR_TRUE);
+	PK11_ImportPublicKey(slot, pubKey, token);
 	if (pubKey->pkcs11Slot) {
 	    PK11_FreeSlot(pubKey->pkcs11Slot);
 	    pubKey->pkcs11Slot = NULL;
@@ -637,7 +637,7 @@ pk11_loadPrivKeyWithFlags(PK11SlotInfo *slot,SECKEYPrivateKey *privKey,
      }
 
      /* build new key structure */
-     return PK11_MakePrivKey(slot, privKey->keyType, (PRBool)!token, 
+     return PK11_MakePrivKey(slot, privKey->keyType, !token, 
 						objectID, privKey->wincx);
 }
 
@@ -649,7 +649,7 @@ pk11_loadPrivKey(PK11SlotInfo *slot,SECKEYPrivateKey *privKey,
     if (token) {
 	attrFlags |= (PK11_ATTR_TOKEN | PK11_ATTR_PRIVATE);
     } else {
-	attrFlags |= PK11_ATTR_PUBLIC;
+	attrFlags |= (PK11_ATTR_SESSION | PK11_ATTR_PUBLIC);
     }
     if (sensitive) {
 	attrFlags |= PK11_ATTR_SENSITIVE;
@@ -769,7 +769,10 @@ PK11_GenerateKeyPairWithFlags(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
     PRBool haslock = PR_FALSE;
     PRBool pubIsToken = PR_FALSE;
     PRBool token = ((attrFlags & PK11_ATTR_TOKEN) != 0);
-    PRBool readOnly = ((attrFlags & PK11_ATTR_READONLY) != 0);
+    /* subset of attrFlags applicable to the public key */
+    PK11AttrFlags pubKeyAttrFlags = attrFlags &
+	(PK11_ATTR_TOKEN | PK11_ATTR_SESSION
+	| PK11_ATTR_MODIFIABLE | PK11_ATTR_UNMODIFIABLE);
 
     if (pk11_BadAttrFlags(attrFlags)) {
 	PORT_SetError( SEC_ERROR_INVALID_ARGS );
@@ -935,8 +938,8 @@ PK11_GenerateKeyPairWithFlags(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
 	}
     }
     /* set the public key attributes */
-    PK11_SETATTRS(attrs, CKA_TOKEN, token ? &cktrue : &ckfalse,
+    attrs += pk11_AttrFlagsToAttributes(pubKeyAttrFlags, attrs,
-					 sizeof(CK_BBOOL)); attrs++;
+						&cktrue, &ckfalse);
     PK11_SETATTRS(attrs, CKA_DERIVE, 
 		mechanism_info.flags & CKF_DERIVE ? &cktrue : &ckfalse,
 					 sizeof(CK_BBOOL)); attrs++;
@@ -952,11 +955,6 @@ PK11_GenerateKeyPairWithFlags(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
     PK11_SETATTRS(attrs, CKA_ENCRYPT, 
 		mechanism_info.flags & CKF_ENCRYPT? &cktrue : &ckfalse,
 					 sizeof(CK_BBOOL)); attrs++;
-    if (readOnly) {
-	/* the default value of the CKA_MODIFIABLE attribute is CK_TRUE */
-	PK11_SETATTRS(attrs, CKA_MODIFIABLE, &ckfalse,
-					 sizeof(CK_BBOOL)); attrs++;
-    }
     /* set the private key attributes */
     PK11_SETATTRS(privattrs, CKA_DERIVE, 
 		mechanism_info.flags & CKF_DERIVE ? &cktrue : &ckfalse,
@@ -1075,7 +1073,7 @@ PK11_GenerateKeyPairWithFlags(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
 	return NULL;
     }
 
-    privKey = PK11_MakePrivKey(slot,keyType,(PRBool)!token,privID,wincx);
+    privKey = PK11_MakePrivKey(slot,keyType,!token,privID,wincx);
     if (privKey == NULL) {
 	SECKEY_DestroyPublicKey(*pubKey);
 	PK11_DestroyObject(slot,privID);
@@ -1095,6 +1093,8 @@ PK11_GenerateKeyPair(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
 
     if (token) {
 	attrFlags |= PK11_ATTR_TOKEN;
+    } else {
+	attrFlags |= PK11_ATTR_SESSION;
     }
     if (sensitive) {
 	attrFlags |= (PK11_ATTR_SENSITIVE | PK11_ATTR_PRIVATE);

security/nss/lib/pk11wrap/pk11obj.c

@@ -445,10 +445,9 @@ pk11_OpFlagsToAttributes(CK_FLAGS flags, CK_ATTRIBUTE *attrs, CK_BBOOL *ckTrue)
     for (; flags && test <= CKF_DERIVE; test <<= 1, ++pType) {
     	if (test & flags) {
 	    flags ^= test;
-	    if (*pType) {
+	    PR_ASSERT(*pType);
-		PK11_SETATTRS(attr, *pType, ckTrue, sizeof *ckTrue); 
+	    PK11_SETATTRS(attr, *pType, ckTrue, sizeof *ckTrue); 
-		++attr;
+	    ++attr;
-	    }
 	}
     }
     return (attr - attrs);
@@ -462,8 +461,8 @@ PRBool
 pk11_BadAttrFlags(PK11AttrFlags attrFlags)
 {
     PK11AttrFlags trueFlags = attrFlags & 0x55555555;
-    PK11AttrFlags falseFlags = attrFlags >> 1 & 0x55555555;
+    PK11AttrFlags falseFlags = (attrFlags >> 1) & 0x55555555;
-    return (trueFlags & falseFlags) != 0 ? PR_TRUE : PR_FALSE;
+    return ((trueFlags & falseFlags) != 0);
 }
 
 /*
@@ -474,64 +473,28 @@ unsigned int
 pk11_AttrFlagsToAttributes(PK11AttrFlags attrFlags, CK_ATTRIBUTE *attrs,
 				CK_BBOOL *ckTrue, CK_BBOOL *ckFalse)
 {
-    CK_ATTRIBUTE *attr = attrs;
+    const static CK_ATTRIBUTE_TYPE attrTypes[5] = {
+	CKA_TOKEN, CKA_PRIVATE, CKA_MODIFIABLE, CKA_SENSITIVE,
+	CKA_EXTRACTABLE
+    };
+
+    const CK_ATTRIBUTE_TYPE *pType	= attrTypes;
+          CK_ATTRIBUTE      *attr	= attrs;
+          PK11AttrFlags      test	= PK11_ATTR_TOKEN;
 
     PR_ASSERT(!pk11_BadAttrFlags(attrFlags));
 
-    /*
+    /* we test two related bitflags in each iteration */
-     * The default value of the CKA_TOKEN attribute is CK_FALSE,
+    for (; attrFlags && test <= PK11_ATTR_EXTRACTABLE; test <<= 2, ++pType) {
-     * so we only need to set this attribute for CK_TRUE.
+    	if (test & attrFlags) {
-     */
+	    attrFlags ^= test;
-    if (attrFlags & PK11_ATTR_TOKEN) {
+	    PK11_SETATTRS(attr, *pType, ckTrue, sizeof *ckTrue); 
-	PK11_SETATTRS(attr, CKA_TOKEN, ckTrue, sizeof *ckTrue);
+	    ++attr;
-	++attr;
+	} else if ((test << 1) & attrFlags) {
-    }
+	    attrFlags ^= (test << 1);
-
+	    PK11_SETATTRS(attr, *pType, ckFalse, sizeof *ckFalse); 
-    /*
+	    ++attr;
-     * The default value of the CKA_PRIVATE attribute is
+	}
-     * token-specific.
-     */
-    if (attrFlags & PK11_ATTR_PRIVATE) {
-	PK11_SETATTRS(attr, CKA_PRIVATE, ckTrue, sizeof *ckTrue);
-	++attr;
-    } else if (attrFlags & PK11_ATTR_PUBLIC) {
-	PK11_SETATTRS(attr, CKA_PRIVATE, ckFalse, sizeof *ckFalse);
-	++attr;
-    }
-
-    /*
-     * The default value of the CKA_MODIFIABLE attribute is CK_TRUE,
-     * so we only need to set this attribute for CK_FALSE.
-     */
-    if (attrFlags & PK11_ATTR_READONLY) {
-	PK11_SETATTRS(attr, CKA_MODIFIABLE, ckFalse, sizeof *ckFalse);
-	++attr;
-    }
-
-    /*
-     * For private keys, the default value of the CKA_SENSITIVE
-     * attribute is token-specific.  For secret keys, the default
-     * value of this attribute is CK_FALSE per PKCS #11 but in
-     * practice it is token-specific.
-     */
-    if (attrFlags & PK11_ATTR_SENSITIVE) {
-	PK11_SETATTRS(attr, CKA_SENSITIVE, ckTrue, sizeof *ckTrue);
-	++attr;
-    } else if (attrFlags & PK11_ATTR_INSENSITIVE) {
-	PK11_SETATTRS(attr, CKA_SENSITIVE, ckFalse, sizeof *ckFalse);
-	++attr;
-    }
-
-    /*
-     * The default value of the CKA_EXTRACTABLE attribute is
-     * token-specific.
-     */
-    if (attrFlags & PK11_ATTR_EXTRACTABLE) {
-	PK11_SETATTRS(attr, CKA_EXTRACTABLE, ckTrue, sizeof *ckTrue);
-	++attr;
-    } else if (attrFlags & PK11_ATTR_UNEXTRACTABLE) {
-	PK11_SETATTRS(attr, CKA_EXTRACTABLE, ckFalse, sizeof *ckFalse);
-	++attr;
     }
     return (attr - attrs);
 }

security/nss/lib/pk11wrap/pk11pub.h

@@ -353,8 +353,9 @@ PK11SlotInfo * PK11_GetSlotFromKey(PK11SymKey *symKey);
 void *PK11_GetWindow(PK11SymKey *symKey);
 /*
  * The attrFlags is the logical OR of the PK11_ATTR_XXX bitflags.
- * These flags apply to the private key.  The PK11_ATTR_TOKEN and
+ * These flags apply to the private key.  The PK11_ATTR_TOKEN,
- * PK11_ATTR_READONLY flags also apply to the public key.
+ * PK11_ATTR_SESSION, PK11_ATTR_MODIFIABLE, and PK11_ATTR_UNMODIFIABLE
+ * flags also apply to the public key.
  */
 SECKEYPrivateKey *PK11_GenerateKeyPairWithFlags(PK11SlotInfo *slot,
    CK_MECHANISM_TYPE type, void *param, SECKEYPublicKey **pubk,

security/nss/lib/pk11wrap/pk11skey.c

@@ -919,7 +919,6 @@ PK11_TokenKeyGen(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, SECItem *param,
 	keySize = 0;
     }
 
-    /* TNH: Isn't this redundant, since "handleKey" will set defaults? */
     opFlags |= weird ? CKF_DECRYPT : CKF_ENCRYPT;
 
     if (isToken) {

security/nss/lib/pk11wrap/secmodt.h

@@ -211,8 +211,7 @@ typedef PRUint32 PK11AttrFlags;
  * private keys or secret keys.  Some of these bitflags also apply
  * to the public keys associated with the private keys.
  *
- * Some of these PKCS #11 object attributes have a token-specific
+ * For each PKCS #11 object attribute, we need two bitflags to
- * default value.  For such attributes, we need two bitflags to
  * specify not only "true" and "false" but also "default".  For
  * example, PK11_ATTR_PRIVATE and PK11_ATTR_PUBLIC control the
  * CKA_PRIVATE attribute.  If PK11_ATTR_PRIVATE is set, we add
@@ -230,14 +229,22 @@ typedef PRUint32 PK11AttrFlags;
 
 /*
  * PK11_ATTR_TOKEN
+ * PK11_ATTR_SESSION
  *
- * If this flag is set, the object is a token object.  If this
+ * These two flags determine whether the object is a token or
- * flag is not set, the object is *by default* a session object.
+ * session object.
- * This flag specifies the value of the PKCS #11 CKA_TOKEN
+ *
+ * These two flags are related and cannot both be set.
+ * If the PK11_ATTR_TOKEN flag is set, the object is a token
+ * object.  If the PK11_ATTR_SESSION flag is set, the object is
+ * a session object.  If neither flag is set, the object is *by
+ * default* a session object.
+ *
+ * These two flags specify the value of the PKCS #11 CKA_TOKEN
  * attribute.
  */
 #define PK11_ATTR_TOKEN         0x00000001L
-/*      Reserved                0x00000002L */
+#define PK11_ATTR_SESSION       0x00000002L
 
 /*
  * PK11_ATTR_PRIVATE
@@ -262,18 +269,23 @@ typedef PRUint32 PK11AttrFlags;
 #define PK11_ATTR_PUBLIC        0x00000008L
 
 /*
- * PK11_ATTR_READONLY
+ * PK11_ATTR_MODIFIABLE
+ * PK11_ATTR_UNMODIFIABLE
  *
- * If this flag is set, the object is read-only.  If this flag is
+ * These two flags determine whether the object is modifiable or
- * not set, the object is *by default* modifiable.
+ * read-only.
  *
- * This flag specifies the value of the PKCS #11 CKA_MODIFIABLE
+ * These two flags are related and cannot both be set.
+ * If the PK11_ATTR_MODIFIABLE flag is set, the object can be
+ * modified.  If the PK11_ATTR_UNMODIFIABLE flag is set, the object
+ * is read-only.  If neither flag is set, the object is *by default*
+ * modifiable.
+ *
+ * These two flags specify the value of the PKCS #11 CKA_MODIFIABLE
  * attribute.
- *
- * XXX Should we name this flag PK11_ATTR_UNMODIFIABLE?
  */
-/*      Reserved                0x00000010L */
+#define PK11_ATTR_MODIFIABLE    0x00000010L
-#define PK11_ATTR_READONLY      0x00000020L
+#define PK11_ATTR_UNMODIFIABLE  0x00000020L
 
 /* Attributes for PKCS #11 key objects. */