Bug 272484 Certificate manager crashes [@ _PR_MD_ATOMIC_DECREMENT - PK11_FreeSymKey]

The problem only happens if we try to import a key into a token which then fails
to import. The basic issue was a hack in the pkcs 7 code to support PKCS 12, A
special structure was used to replace the SymKey structure, and the code 'knew'
the special structure existed before it dealt with the symkey. The fix addes a
new capability to symkeys, where applications can attach application specific
data to the key structure. PKCS 12 uses this to attache the PBE information
for CMS. (part 1 of 3)

This patch also improves the key's reuse of sessions, so sessions are not thrashed
when SSL is used with them.

r=wtc

security/nss/lib/pk11wrap/pk11cert.c

@@ -2392,3 +2392,41 @@ PK11_ListCertsInSlot(PK11SlotInfo *slot)
     return certs;
 }
 
+PK11SlotList *
+PK11_GetAllSlotsForCert(NSSCertificate *c, void *arg)
+{
+    /* add multiple instances to the cert list */
+    nssCryptokiObject **ip;
+    nssCryptokiObject **instances = nssPKIObject_GetInstances(&c->object);
+    PK11SlotList *slotList;
+    PRBool found = PR_FALSE;
+
+
+    if (!instances) {
+	PORT_SetError(SEC_ERROR_NO_TOKEN);
+	return NULL;
+    }
+
+    slotList = PK11_NewSlotList();
+    if (!slotList) {
+	nssCryptokiObjectArray_Destroy(instances);
+	return NULL;
+    }
+
+    for (ip = instances; *ip; ip++) {
+	nssCryptokiObject *instance = *ip;
+	PK11SlotInfo *slot = instance->token->pk11slot;
+	if (slot) {
+	    PK11_AddSlotToList(slotList, slot);
+	    found = PR_TRUE;
+	}
+    }
+    if (!found) {
+	PK11_FreeSlotList(slotList);
+	PORT_SetError(SEC_ERROR_NO_TOKEN);
+	slotList = NULL;
+    }
+
+    nssCryptokiObjectArray_Destroy(instances);
+    return slotList;
+}

security/nss/lib/pk11wrap/pk11pub.h

@@ -299,6 +299,39 @@ PK11SymKey * PK11_ListFixedKeysInSlot(PK11SlotInfo *slot, char *nickname,
 PK11SymKey *PK11_GetNextSymKey(PK11SymKey *symKey);
 CK_KEY_TYPE PK11_GetSymKeyType(PK11SymKey *key);
 
+/*
+ * PK11_SetSymKeyUserData
+ *   sets generic user data on keys (usually a pointer to a data structure)
+ * that can later be retrieved by PK11_GetSymKeyUserData().
+ *    symKey - key where data will be set.
+ *    data - data to be set.
+ *    freefunc - function used to free the data.
+ * Setting user data on symKeys with existing user data already set will cause 
+ * the existing user data to be freed before the new user data is set.
+ * Freeing user data is done by calling the user specified freefunc. 
+ * If freefunc is NULL, the user data is assumed to be global or static an 
+ * not freed. Passing NULL for user data to PK11_SetSymKeyUserData has the 
+ * effect of freeing any existing user data, and clearing the user data 
+ * pointer. If user data exists when the symKey is finally freed, that 
+ * data will be freed with freefunc.
+ *
+ * Applications should only use this function on keys which the application
+ * has created directly, as there is only one user data value per key.
+ */
+void PK11_SetSymKeyUserData(PK11SymKey *symKey, void *data, 
+                                 PK11FreeDataFunc freefunc);
+/* PK11_GetSymKeyUserData 
+ *   retrieves generic user data which was set on a key by 
+ * PK11_SetSymKeyUserData.
+ *    symKey - key with data to be fetched
+ *
+ * If no data exists, or the data has been cleared, PK11_GetSymKeyUserData
+ * will return NULL. Returned data is still owned and managed by the SymKey,
+ * the caller should not free the data.
+ *
+ */
+void *PK11_GetSymKeyUserData(PK11SymKey *symKey);
+
 SECStatus PK11_PubWrapSymKey(CK_MECHANISM_TYPE type, SECKEYPublicKey *pubKey,
 				PK11SymKey *symKey, SECItem *wrappedKey);
 SECStatus PK11_WrapSymKey(CK_MECHANISM_TYPE type, SECItem *params,

security/nss/lib/pk11wrap/pk11sdr.c

@@ -169,7 +169,8 @@ PK11SDR_Encrypt(SECItem *keyid, SECItem *data, SECItem *result, void *cx)
     paddedData.data = 0;
   
     arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
-  if (!arena) { rv = SECFailure; goto loser; }
+    if (!arena) { 
+	rv = SECFailure; goto loser; }
 
     /* 1. Locate the requested keyid, or the default key (which has a keyid)
      * 2. Create an encryption context
@@ -178,7 +179,10 @@ PK11SDR_Encrypt(SECItem *keyid, SECItem *data, SECItem *result, void *cx)
      */
 
     slot = PK11_GetInternalKeySlot();
-  if (!slot) { rv = SECFailure; goto loser; }
+    if (!slot) { 
+	rv = SECFailure; 
+	goto loser;
+    }
 
     /* Use triple-DES */
     type = CKM_DES3_CBC;
@@ -188,7 +192,9 @@ PK11SDR_Encrypt(SECItem *keyid, SECItem *data, SECItem *result, void *cx)
      * won't find it.
      */
     rv = PK11_Authenticate(slot, PR_TRUE, cx);
-  if (rv != SECSuccess) goto loser;
+    if (rv != SECSuccess) {
+	goto loser;
+    }
 
     /* Find the key to use */
     pKeyID = keyid;
@@ -200,18 +206,24 @@ PK11SDR_Encrypt(SECItem *keyid, SECItem *data, SECItem *result, void *cx)
 	   */
 
 	  if (pk11sdrLock) PR_Lock(pk11sdrLock);
-
 	  /* Try to find the key */
 	  key = PK11_FindFixedKey(slot, type, pKeyID, cx);
 	  
 	  /* If the default key doesn't exist yet, try to create it */
-	  if (!key) key = PK11_GenDES3TokenKey(slot, pKeyID, cx);
+	  if (!key) {
-	  if (pk11sdrLock) PR_Unlock(pk11sdrLock);
+		key = PK11_GenDES3TokenKey(slot, pKeyID, cx);
+	  }
+	  if (pk11sdrLock) {
+		PR_Unlock(pk11sdrLock);
+	  }
     } else {
 	  key = PK11_FindFixedKey(slot, type, pKeyID, cx);
     }
 
-  if (!key) { rv = SECFailure; goto loser; }
+    if (!key) { 
+	rv = SECFailure; 
+	goto loser; 
+    }
 
     params = PK11_GenerateNewParam(type, key);
     if (!params) { rv = SECFailure; goto loser; }
@@ -220,23 +232,34 @@ PK11SDR_Encrypt(SECItem *keyid, SECItem *data, SECItem *result, void *cx)
     if (!ctx) { rv = SECFailure; goto loser; }
 
     rv = padBlock(data, PK11_GetBlockSize(type, 0), &paddedData);
-  if (rv != SECSuccess) goto loser;
+    if (rv != SECSuccess) {
+	goto loser;
+    }
 
     sdrResult.data.len = paddedData.len;
-  sdrResult.data.data = (unsigned char *)PORT_ArenaAlloc(arena, sdrResult.data.len);
+    sdrResult.data.data = (unsigned char *)
+			PORT_ArenaAlloc(arena, sdrResult.data.len);
 
-  rv = PK11_CipherOp(ctx, sdrResult.data.data, (int*)&sdrResult.data.len, sdrResult.data.len,
+    rv = PK11_CipherOp(ctx, sdrResult.data.data, 
+		       (int*)&sdrResult.data.len, sdrResult.data.len,
                        paddedData.data, paddedData.len);
-  if (rv != SECSuccess) goto loser;
+    if (rv != SECSuccess) {
+	goto loser;
+    }
 
     PK11_Finalize(ctx);
 
     sdrResult.keyid = *pKeyID;
 
     rv = PK11_ParamToAlgid(SEC_OID_DES_EDE3_CBC, params, arena, &sdrResult.alg);
-  if (rv != SECSuccess) goto loser;
+    if (rv != SECSuccess) {
+	goto loser;
+    }
 
-  if (!SEC_ASN1EncodeItem(0, result, &sdrResult, template)) { rv = SECFailure; goto loser; }
+    if (!SEC_ASN1EncodeItem(0, result, &sdrResult, template)) {
+	rv = SECFailure;
+	goto loser;
+    }
 
 loser:
     SECITEM_ZfreeItem(&paddedData, PR_FALSE);

security/nss/lib/pk11wrap/pk11skey.c

@@ -69,39 +69,74 @@ pk11_ExitKeyMonitor(PK11SymKey *symKey) {
     	PK11_ExitSlotMonitor(symKey->slot);
 }
 
-
+/*
+ * pk11_getKeyFromList returns a symKey that has a session (if needSession
+ * was specified), or explicitly does not have a session (if needSession
+ * was not specified).
+ */
 static PK11SymKey *
-pk11_getKeyFromList(PK11SlotInfo *slot) {
+pk11_getKeyFromList(PK11SlotInfo *slot, PRBool needSession) {
     PK11SymKey *symKey = NULL;
 
     PZ_Lock(slot->freeListLock);
+    /* own session list are symkeys with sessions that the symkey owns.
+     * 'most' symkeys will own their own session. */
+    if (needSession) {
+	if (slot->freeSymKeysWithSessionHead) {
+    	    symKey = slot->freeSymKeysWithSessionHead;
+	    slot->freeSymKeysWithSessionHead = symKey->next;
+	    slot->keyCount--;
+	}
+    }
+    /* if we don't need a symkey with its own session, or we couldn't find
+     * one on the owner list, get one from the non-owner free list. */
+    if (!symKey) {
 	if (slot->freeSymKeysHead) {
     	    symKey = slot->freeSymKeysHead;
 	    slot->freeSymKeysHead = symKey->next;
 	    slot->keyCount--;
 	}
+    }
     PZ_Unlock(slot->freeListLock);
     if (symKey) {
 	symKey->next = NULL;
-	if ((symKey->series != slot->series) || (!symKey->sessionOwner))
+	if (!needSession) {
+	    return symKey;
+	}
+	/* if we are getting an owner key, make sure we have a valid session.
+         * session could be invalid if the token has been removed or because
+         * we got it from the non-owner free list */
+	if ((symKey->series != slot->series) || 
+			 (symKey->session == CK_INVALID_SESSION)) {
 	    symKey->session = pk11_GetNewSession(slot, &symKey->sessionOwner);
+	}
 	PORT_Assert(symKey->session != CK_INVALID_SESSION);
 	if (symKey->session != CK_INVALID_SESSION)
 	    return symKey;
 	PK11_FreeSymKey(symKey);
+	/* if we are here, we need a session, but couldn't get one, it's 
+	 * unlikely we pk11_GetNewSession will succeed if we call it a second
+	 * time. */
+	return NULL;
     }
 
     symKey = PORT_New(PK11SymKey);
     if (symKey == NULL) {
 	return NULL;
     }
+
     symKey->next = NULL;
+    if (needSession) {
 	symKey->session = pk11_GetNewSession(slot,&symKey->sessionOwner);
 	PORT_Assert(symKey->session != CK_INVALID_SESSION);
-    if (symKey->session != CK_INVALID_SESSION)
+        if (symKey->session == CK_INVALID_SESSION) {
-	return symKey;
 	    PK11_FreeSymKey(symKey);
-    return NULL;
+	    symKey = NULL;
+	}
+    } else {
+	symKey->session = CK_INVALID_SESSION;
+    }
+    return symKey;
 }
 
 /* Caller MUST hold slot->freeListLock (or ref count == 0?) !! */
@@ -110,14 +145,18 @@ PK11_CleanKeyList(PK11SlotInfo *slot)
 {
     PK11SymKey *symKey = NULL;
 
+    while (slot->freeSymKeysWithSessionHead) {
+    	symKey = slot->freeSymKeysWithSessionHead;
+	slot->freeSymKeysWithSessionHead = symKey->next;
+	pk11_CloseSession(slot, symKey->session, symKey->sessionOwner);
+	PORT_Free(symKey);
+    }
     while (slot->freeSymKeysHead) {
     	symKey = slot->freeSymKeysHead;
 	slot->freeSymKeysHead = symKey->next;
-/* XXX Perhaps this should be:
-**   if (symKey->sessionOwner)  */
 	pk11_CloseSession(slot, symKey->session, symKey->sessionOwner);
 	PORT_Free(symKey);
-    };
+    }
     return;
 }
 
@@ -125,19 +164,25 @@ PK11_CleanKeyList(PK11SlotInfo *slot)
  * create a symetric key:
  *      Slot is the slot to create the key in.
  *      type is the mechanism type 
+ *      owner is does this symKey structure own it's object handle (rare
+ *        that this is false).
+ *      needSession means the returned symKey will return with a valid session
+ *        allocated already.
  */
 static PK11SymKey *
-pk11_CreateSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, PRBool owner, 
+pk11_CreateSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, 
-								void *wincx)
+		  PRBool owner, PRBool needSession, void *wincx)
 {
 
-    PK11SymKey *symKey = pk11_getKeyFromList(slot);
+    PK11SymKey *symKey = pk11_getKeyFromList(slot, needSession);
-
 
     if (symKey == NULL) {
 	return NULL;
     }
-    if (symKey->session == CK_INVALID_SESSION) {
+    /* if needSession was specified, make sure we have a valid session.
+     * callers which specify needSession as false should do their own
+     * check of the session before returning the symKey */
+    if (needSession && symKey->session == CK_INVALID_SESSION) {
     	PK11_FreeSymKey(symKey);
 	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
 	return NULL;
@@ -156,6 +201,8 @@ pk11_CreateSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, PRBool owner,
     symKey->refCount = 1;
     symKey->origin = PK11_OriginNULL;
     symKey->parent = NULL;
+    symKey->freeFunc = NULL;
+    symKey->userData = NULL;
     PK11_ReferenceSlot(slot);
     return symKey;
 }
@@ -183,11 +230,33 @@ PK11_FreeSymKey(PK11SymKey *symKey)
 	    PORT_Memset(symKey->data.data, 0, symKey->data.len);
 	    PORT_Free(symKey->data.data);
 	}
+	/* free any existing data */
+	if (symKey->userData && symKey->freeFunc) {
+	    (*symKey->freeFunc)(symKey->userData);
+	}
         slot = symKey->slot;
         PZ_Lock(slot->freeListLock);
 	if (slot->keyCount < slot->maxKeyCount) {
+	    /* 
+             * freeSymkeysWithSessionHead contain a list of reusable
+	     *  SymKey structures with valid sessions.
+	     *    sessionOwner must be true.
+             *    session must be valid.
+             * freeSymKeysHead contain a list of SymKey structures without
+             *  valid session.
+             *    session must be CK_INVALID_SESSION.
+	     *    though sessionOwner is false, callers should not depend on
+	     *    this fact.
+	     */
+	    if (symKey->sessionOwner) {
+		PORT_Assert (symKey->session != CK_INVALID_SESSION);
+		symKey->next = slot->freeSymKeysWithSessionHead;
+		slot->freeSymKeysWithSessionHead = symKey;
+	    } else {
+		symKey->session = CK_INVALID_SESSION;
 		symKey->next = slot->freeSymKeysHead;
 		slot->freeSymKeysHead = symKey;
+	    }
 	    slot->keyCount++;
 	    symKey->slot = NULL;
 	    freeit = PR_FALSE;
@@ -254,6 +323,25 @@ PK11_SetSymKeyNickname(PK11SymKey *symKey, const char *nickname)
     return PK11_SetObjectNickname(symKey->slot,symKey->objectID,nickname);
 }
 
+void *
+PK11_GetSymKeyUserData(PK11SymKey *symKey)
+{
+    return symKey->userData;
+}
+
+void
+PK11_SetSymKeyUserData(PK11SymKey *symKey, void *userData, 
+                                              PK11FreeDataFunc freeFunc)
+{
+    /* free any existing data */
+    if (symKey->userData && symKey->freeFunc) {
+	(*symKey->freeFunc)(symKey->userData);
+    }
+    symKey->userData = userData;
+    symKey->freeFunc = freeFunc;
+    return;
+}
+
 /*
  * turn key handle into an appropriate key object
  */
@@ -262,12 +350,13 @@ PK11_SymKeyFromHandle(PK11SlotInfo *slot, PK11SymKey *parent, PK11Origin origin,
     CK_MECHANISM_TYPE type, CK_OBJECT_HANDLE keyID, PRBool owner, void *wincx)
 {
     PK11SymKey *symKey;
+    PRBool needSession = !(owner && parent);
 
     if (keyID == CK_INVALID_HANDLE) {
 	return NULL;
     }
 
-    symKey = pk11_CreateSymKey(slot,type,owner,wincx);
+    symKey = pk11_CreateSymKey(slot, type, owner, needSession, wincx);
     if (symKey == NULL) {
 	return NULL;
     }
@@ -279,11 +368,19 @@ PK11_SymKeyFromHandle(PK11SlotInfo *slot, PK11SymKey *parent, PK11Origin origin,
     /* This is only used by SSL. What we really want here is a session
      * structure with a ref count so  the session goes away only after all the
      * keys do. */
-    if (owner && parent) {
+    if (!needSession) {
-	pk11_CloseSession(symKey->slot, symKey->session,symKey->sessionOwner);
 	symKey->sessionOwner = PR_FALSE;
 	symKey->session = parent->session;
 	symKey->parent = PK11_ReferenceSymKey(parent);
+        /* This is the only case where pk11_CreateSymKey does not explicitly
+	 * check symKey->session. We need to assert here to make sure.
+	 * the session isn't invalid. */
+	PORT_Assert(parent->session != CK_INVALID_SESSION);
+	if (parent->session == CK_INVALID_SESSION) {
+	    PK11_FreeSymKey(symKey);
+	    PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+	    return NULL;
+	}
     }
 
     return symKey;
@@ -344,7 +441,7 @@ pk11_ImportSymKeyWithTempl(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
     PK11SymKey *    symKey;
     SECStatus	    rv;
 
-    symKey = pk11_CreateSymKey(slot,type,!isToken,wincx);
+    symKey = pk11_CreateSymKey(slot, type, !isToken, PR_TRUE, wincx);
     if (symKey == NULL) {
 	return NULL;
     }
@@ -843,11 +940,11 @@ PK11_TokenKeyGenWithFlags(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
 	    return NULL;
 	}
 
-        symKey = pk11_CreateSymKey(bestSlot, type, !isToken, wincx);
+        symKey = pk11_CreateSymKey(bestSlot, type, !isToken, PR_TRUE, wincx);
 
         PK11_FreeSlot(bestSlot);
     } else {
-	symKey = pk11_CreateSymKey(slot, type, !isToken, wincx);
+	symKey = pk11_CreateSymKey(slot, type, !isToken, PR_TRUE, wincx);
     }
     if (symKey == NULL) return NULL;
 
@@ -1327,7 +1424,7 @@ pk11_DeriveWithTemplate( PK11SymKey *baseKey, CK_MECHANISM_TYPE derive,
 
 
     /* get our key Structure */
-    symKey = pk11_CreateSymKey(slot,target,!isPerm,baseKey->cx);
+    symKey = pk11_CreateSymKey(slot, target, !isPerm, PR_TRUE, baseKey->cx);
     if (symKey == NULL) {
 	return NULL;
     }
@@ -1400,7 +1497,7 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
     }
 
     /* get our key Structure */
-    symKey = pk11_CreateSymKey(slot,target,PR_TRUE,wincx);
+    symKey = pk11_CreateSymKey(slot, target, PR_TRUE, PR_TRUE, wincx);
     if (symKey == NULL) {
 	return NULL;
     }
@@ -1562,7 +1659,7 @@ PK11_PubDeriveWithKDF(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
     CK_RV crv;
 
     /* get our key Structure */
-    symKey = pk11_CreateSymKey(slot,target,PR_TRUE,wincx);
+    symKey = pk11_CreateSymKey(slot, target, PR_TRUE, PR_TRUE, wincx);
     if (symKey == NULL) {
 	return NULL;
     }
@@ -1846,7 +1943,7 @@ pk11_AnyUnwrapKey(PK11SlotInfo *slot, CK_OBJECT_HANDLE wrappingKey,
     }
 
     /* get our key Structure */
-    symKey = pk11_CreateSymKey(slot,target,!isPerm,wincx);
+    symKey = pk11_CreateSymKey(slot, target, !isPerm, PR_TRUE, wincx);
     if (symKey == NULL) {
 	if (param_free) SECITEM_FreeItem(param_free,PR_TRUE);
 	return NULL;

security/nss/lib/pk11wrap/pk11slot.c

@@ -364,6 +364,7 @@ PK11_NewSlotInfo(SECMODModule *mod)
 	PORT_Free(slot);
 	return slot;
     }
+    slot->freeSymKeysWithSessionHead = NULL;
     slot->freeSymKeysHead = NULL;
     slot->keyCount = 0;
     slot->maxKeyCount = 0;

security/nss/lib/pk11wrap/secmodt.h

@@ -66,6 +66,7 @@ typedef struct PK11RSAGenParamsStr PK11RSAGenParams;
 typedef unsigned long SECMODModuleID;
 typedef struct PK11DefaultArrayEntryStr PK11DefaultArrayEntry;
 typedef struct PK11GenericObjectStr PK11GenericObject;
+typedef void (*PK11FreeDataFunc)(void *);
 
 struct SECMODModuleStr {
     PRArenaPool	*arena;

security/nss/lib/pk11wrap/secmodti.h

@@ -97,6 +97,7 @@ struct PK11SlotInfoStr {
      * still in use */
     PRInt32 refCount;    /* to be in/decremented by atomic calls ONLY! */
     PZLock *freeListLock;
+    PK11SymKey *freeSymKeysWithSessionHead;
     PK11SymKey *freeSymKeysHead;
     int keyCount;
     int maxKeyCount;
@@ -161,11 +162,14 @@ struct PK11SymKeyStr {
     PRInt32	      refCount;	/* number of references to this key */
     int		      size;	/* key size in bytes */
     PK11Origin	      origin;	/* where this key came from 
-						(see def in secmodt.h) */
+                                 * (see def in secmodt.h) */
-    PK11SymKey *parent;
+    PK11SymKey        *parent;  /* potential owner key of the session */
-    uint16 series;		/* break up the slot info into various groups of 
+    uint16 series;		/* break up the slot info into various groups 
-			 * inserted tokens so that keys and certs can be
+				 * of inserted tokens so that keys and certs 
-			 * invalidated */
+				 * can be invalidated */
+    void *userData;		/* random data the application can attach to
+                                 * this key */
+    PK11FreeDataFunc freeFunc;	/* function to free the user data */
 };