Eliminate numerous potential causes of sending invalid (zero) session

handles down to a PKCS11 module. Bug 292049. r=relyea. Modified Files: pk11akey.c pk11auth.c pk11obj.c pk11skey.c pk11slot.c pk11util.c
2005-08-03 01:22:07 +00:00 · 2005-08-03 01:22:07 +00:00 · 13863082f7
--- a/security/nss/lib/pk11wrap/pk11akey.c
+++ b/security/nss/lib/pk11wrap/pk11akey.c
@ -959,12 +959,17 @@ PK11_GenerateKeyPair(PK11SlotInfo *slot,CK_MECHANISM_TYPE type,
 	haslock = PK11_RWSessionHasLock(slot,session_handle);
 	restore = PR_TRUE;
    } else {
-        PK11_EnterSlotMonitor(slot); /* gross!! */
 	session_handle = slot->session;
+	if (session_handle != CK_INVALID_SESSION)
+	    PK11_EnterSlotMonitor(slot);
 	restore = PR_FALSE;
 	haslock = PR_TRUE;
    }

+    if (session_handle == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+	return NULL;
+    }
    crv = PK11_GETTAB(slot)->C_GenerateKeyPair(session_handle, &mechanism,
 	pubTemplate,pubCount,privTemplate,privCount,&pubID,&privID);

@ -1640,6 +1645,10 @@ PK11_ConvertSessionPrivKeyToTokenPrivKey(SECKEYPrivateKey *privk, void* wincx)

    PK11_Authenticate(slot, PR_TRUE, wincx);
    rwsession = PK11_GetRWSession(slot);
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+	return NULL;
+    }
    crv = PK11_GETTAB(slot)->C_CopyObject(rwsession, privk->pkcs11ID,
        template, 1, &newKeyID);
    PK11_RestoreROSession(slot, rwsession);
@ -1652,6 +1661,7 @@ PK11_ConvertSessionPrivKeyToTokenPrivKey(SECKEYPrivateKey *privk, void* wincx)
    return PK11_MakePrivKey(slot, nullKey /*KeyType*/, PR_FALSE /*isTemp*/,
        newKeyID, NULL /*wincx*/);
 }
+
 /*
 * destroy a private key if there are no matching certs.
 * this function also frees the privKey structure.
--- a/security/nss/lib/pk11wrap/pk11auth.c
+++ b/security/nss/lib/pk11wrap/pk11auth.c
@ -316,7 +316,10 @@ PK11_CheckSSOPassword(PK11SlotInfo *slot, char *ssopw)

    /* get a rwsession */
    rwsession = PK11_GetRWSession(slot);
-    if (rwsession == CK_INVALID_SESSION) return rv;
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+    	return rv;
+    }

    if (slot->protectedAuthPath) {
 	len = 0;
@ -383,7 +386,11 @@ PK11_InitPin(PK11SlotInfo *slot,char *ssopw, char *userpw)

    /* get a rwsession */
    rwsession = PK11_GetRWSession(slot);
-    if (rwsession == CK_INVALID_SESSION) goto done;
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+	slot->lastLoginCheck = 0;
+    	return rv;
+    }

    if (slot->protectedAuthPath) {
 	len = 0;
@ -443,6 +450,10 @@ PK11_ChangePW(PK11SlotInfo *slot,char *oldpw, char *newpw)

    /* get a rwsession */
    rwsession = PK11_GetRWSession(slot);
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+    	return rv;
+    }

    crv = PK11_GETTAB(slot)->C_SetPIN(rwsession,
 		(unsigned char *)oldpw,oldLen,(unsigned char *)newpw,newLen);
--- a/security/nss/lib/pk11wrap/pk11obj.c
+++ b/security/nss/lib/pk11wrap/pk11obj.c
@ -97,6 +97,10 @@ PK11_DestroyTokenObject(PK11SlotInfo *slot,CK_OBJECT_HANDLE object) {

    
    rwsession = PK11_GetRWSession(slot);
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+    	return SECFailure;
+    }

    crv = PK11_GETTAB(slot)->C_DestroyObject(rwsession,object);
    if (crv != CKR_OK) {
@ -210,6 +214,9 @@ PK11_GetAttributes(PRArenaPool *arena,PK11SlotInfo *slot,
    /* make pedantic happy... note that it's only used arena != NULL */ 
    void *mark = NULL; 
    CK_RV crv;
+    PORT_Assert(slot->session != CK_INVALID_SESSION);
+    if (slot->session == CK_INVALID_SESSION)
+	return CKR_SESSION_HANDLE_INVALID;

    /*
     * first get all the lengths of the parameters.
@ -319,6 +326,10 @@ PK11_SetObjectNickname(PK11SlotInfo *slot, CK_OBJECT_HANDLE id,

    PK11_SETATTRS(&setTemplate, CKA_LABEL, (CK_CHAR *) nickname, len);
    rwsession = PK11_GetRWSession(slot);
+    if (rwsession == CK_INVALID_SESSION) {
+    	PORT_SetError(SEC_ERROR_BAD_DATA);
+    	return SECFailure;
+    }
    crv = PK11_GETTAB(slot)->C_SetAttributeValue(rwsession, id,
 			&setTemplate, 1);
    PK11_RestoreROSession(slot, rwsession);
@ -389,8 +400,13 @@ PK11_CreateNewObject(PK11SlotInfo *slot, CK_SESSION_HANDLE session,
 	    rwsession =  PK11_GetRWSession(slot);
 	} else if (rwsession == CK_INVALID_SESSION) {
 	    rwsession =  slot->session;
+	    if (rwsession != CK_INVALID_SESSION)
 		PK11_EnterSlotMonitor(slot);
 	}
+	if (rwsession == CK_INVALID_SESSION) {
+	    PORT_SetError(SEC_ERROR_BAD_DATA);
+	    return SECFailure;
+	}
 	crv = PK11_GETTAB(slot)->C_CreateObject(rwsession, theTemplate,
 							count,objectID);
 	if(crv != CKR_OK) {
@ -924,12 +940,18 @@ PK11_UnwrapPrivKey(PK11SlotInfo *slot, PK11SymKey *wrappingKey,
    if (newKey) {
 	if (perm) {
 	    /* Get RW Session will either lock the monitor if necessary, 
-	     *  or return a thread safe session handle. */ 
+	     *  or return a thread safe session handle, or fail. */ 
 	    rwsession = PK11_GetRWSession(slot);
 	} else {
 	    rwsession = slot->session;
+	    if (rwsession != CK_INVALID_SESSION) 
 		PK11_EnterSlotMonitor(slot);
 	}
+	if (rwsession == CK_INVALID_SESSION) {
+	    PK11_FreeSymKey(newKey);
+	    PORT_SetError(SEC_ERROR_BAD_DATA);
+	    return NULL;
+	}
 	crv = PK11_GETTAB(slot)->C_UnwrapKey(rwsession, &mechanism, 
 					 newKey->objectID,
 					 wrappedKey->data, 
@ -1105,7 +1127,7 @@ PK11_FindGenericObjects(PK11SlotInfo *slot, CK_OBJECT_CLASS objClass)
    CK_ATTRIBUTE template[1];
    CK_ATTRIBUTE *attrs = template;
    CK_OBJECT_HANDLE *objectIDs = NULL;
-    PK11GenericObject *lastObj, *obj;
+    PK11GenericObject *lastObj = NULL, *obj;
    PK11GenericObject *firstObj = NULL;
    int i, count = 0;

--- a/security/nss/lib/pk11wrap/pk11skey.c
+++ b/security/nss/lib/pk11wrap/pk11skey.c
@ -85,18 +85,26 @@ pk11_getKeyFromList(PK11SlotInfo *slot) {
 	symKey->next = NULL;
 	if ((symKey->series != slot->series) || (!symKey->sessionOwner))
    	    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);
    }

    symKey = PORT_New(PK11SymKey);
    if (symKey == NULL) {
 	return NULL;
    }
-    symKey->session = pk11_GetNewSession(slot,&symKey->sessionOwner);
    symKey->next = NULL;
+    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);
+    return NULL;
 }

+/* Caller MUST hold slot->freeListLock (or ref count == 0?) !! */
 void
 PK11_CleanKeyList(PK11SlotInfo *slot)
 {
@ -105,6 +113,8 @@ PK11_CleanKeyList(PK11SlotInfo *slot)
    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);
    };
@ -127,6 +137,11 @@ pk11_CreateSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, PRBool owner,
    if (symKey == NULL) {
 	return NULL;
    }
+    if (symKey->session == CK_INVALID_SESSION) {
+    	PK11_FreeSymKey(symKey);
+	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+	return NULL;
+    }

    symKey->type = type;
    symKey->data.type = siBuffer;
@ -851,12 +866,19 @@ PK11_TokenKeyGenWithFlags(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
    /* Get session and perform locking */
    if (isToken) {
 	PK11_Authenticate(symKey->slot,PR_TRUE,wincx);
-        session = PK11_GetRWSession(symKey->slot);  /* Should always be original slot */
+	/* Should always be original slot */
+        session = PK11_GetRWSession(symKey->slot);  
 	symKey->owner = PR_FALSE;
    } else {
        session = symKey->session;
+	if (session != CK_INVALID_SESSION) 
 	    pk11_EnterKeyMonitor(symKey);
    }
+    if (session == CK_INVALID_SESSION) {
+	PK11_FreeSymKey(symKey);
+	PORT_SetError(SEC_ERROR_BAD_DATA);
+	return NULL;
+    }

    crv = PK11_GETTAB(symKey->slot)->C_GenerateKey(session,
 			 &mechanism, genTemplate, count, &symKey->objectID);
@ -931,6 +953,10 @@ PK11_ConvertSessionSymKeyToTokenSymKey(PK11SymKey *symk, void *wincx)

    PK11_Authenticate(slot, PR_TRUE, wincx);
    rwsession = PK11_GetRWSession(slot);
+    if (rwsession == CK_INVALID_SESSION) {
+	PORT_SetError(SEC_ERROR_BAD_DATA);
+	return NULL;
+    }
    crv = PK11_GETTAB(slot)->C_CopyObject(rwsession, symk->objectID,
        template, 1, &newKeyID);
    PK11_RestoreROSession(slot, rwsession);
@ -1274,7 +1300,8 @@ pk11_DeriveWithTemplate( PK11SymKey *baseKey, CK_MECHANISM_TYPE derive,
        newBaseKey = pk11_CopyToSlot (newSlot, derive, CKA_DERIVE, 
 				     baseKey);
 	PK11_FreeSlot(newSlot);
-	if (newBaseKey == NULL) return NULL;	
+	if (newBaseKey == NULL) 
+	    return NULL;	
 	baseKey = newBaseKey;
 	slot = baseKey->slot;
    }
@ -1304,6 +1331,11 @@ pk11_DeriveWithTemplate( PK11SymKey *baseKey, CK_MECHANISM_TYPE derive,
        pk11_EnterKeyMonitor(symKey);
 	session = symKey->session;
    }
+    if (session == CK_INVALID_SESSION) {
+	if (!isPerm)
+	    pk11_ExitKeyMonitor(symKey);
+	crv = CKR_SESSION_HANDLE_INVALID;
+    } else {
 	crv = PK11_GETTAB(slot)->C_DeriveKey(session, &mechanism,
 	     baseKey->objectID, keyTemplate, templateCount, &symKey->objectID);
 	if (isPerm) {
@ -1311,8 +1343,9 @@ pk11_DeriveWithTemplate( PK11SymKey *baseKey, CK_MECHANISM_TYPE derive,
 	} else {
 	    pk11_ExitKeyMonitor(symKey);
 	}
-
-    if (newBaseKey) PK11_FreeSymKey(newBaseKey);
+    }
+    if (newBaseKey) 
+    	PK11_FreeSymKey(newBaseKey);
    if (crv != CKR_OK) {
 	PK11_FreeSymKey(symKey);
 	return NULL;
@ -1821,10 +1854,15 @@ pk11_AnyUnwrapKey(PK11SlotInfo *slot, CK_OBJECT_HANDLE wrappingKey,
        pk11_EnterKeyMonitor(symKey);
 	rwsession = symKey->session;
    }
+    PORT_Assert(rwsession != CK_INVALID_SESSION);
+    if (rwsession == CK_INVALID_SESSION) 
+    	crv = CKR_SESSION_HANDLE_INVALID;
+    else
 	crv = PK11_GETTAB(slot)->C_UnwrapKey(rwsession,&mechanism,wrappingKey,
 		wrappedKey->data, wrappedKey->len, keyTemplate, templateCount, 
 							  &symKey->objectID);
    if (isPerm) {
+	if (rwsession != CK_INVALID_SESSION)
 	    PK11_RestoreROSession(slot, rwsession);
    } else {
        pk11_ExitKeyMonitor(symKey);
--- a/security/nss/lib/pk11wrap/pk11slot.c
+++ b/security/nss/lib/pk11wrap/pk11slot.c
@ -666,36 +666,53 @@ CK_SESSION_HANDLE PK11_GetRWSession(PK11SlotInfo *slot)
 {
    CK_SESSION_HANDLE rwsession;
    CK_RV crv;
+    PRBool haveMonitor = PR_FALSE;

-    if (!slot->isThreadSafe || slot->defRWSession) PK11_EnterSlotMonitor(slot);
-    if (slot->defRWSession) return slot->session;
+    if (!slot->isThreadSafe || slot->defRWSession) {
+    	PK11_EnterSlotMonitor(slot);
+	haveMonitor = PR_TRUE;
+    }
+    if (slot->defRWSession) {
+	PORT_Assert(slot->session != CK_INVALID_SESSION);
+	if (slot->session != CK_INVALID_SESSION) 
+	    return slot->session;
+    }

    crv = PK11_GETTAB(slot)->C_OpenSession(slot->slotID,
 				CKF_RW_SESSION|CKF_SERIAL_SESSION,
 				  	  slot, pk11_notify,&rwsession);
-    if (crv == CKR_SESSION_COUNT) {
-	PK11_GETTAB(slot)->C_CloseSession(slot->session);
-	slot->session = CK_INVALID_SESSION;
-    	crv = PK11_GETTAB(slot)->C_OpenSession(slot->slotID,
-				CKF_RW_SESSION|CKF_SERIAL_SESSION,
-				  	  slot,pk11_notify,&rwsession);
-    }
-    if (crv != CKR_OK) {
+    PORT_Assert(rwsession != CK_INVALID_SESSION || crv != CKR_OK);
+    if (crv != CKR_OK || rwsession == CK_INVALID_SESSION) {
+	if (crv == CKR_OK) 
+	    crv = CKR_DEVICE_ERROR;
+	if (haveMonitor)
+	    PK11_ExitSlotMonitor(slot);
 	PORT_SetError(PK11_MapError(crv));
-	if (slot->session == CK_INVALID_SESSION) {
-    	    PK11_GETTAB(slot)->C_OpenSession(slot->slotID,CKF_SERIAL_SESSION,
-				  	 slot,pk11_notify,&slot->session);
-	}
-	if (!slot->isThreadSafe) PK11_ExitSlotMonitor(slot);
 	return CK_INVALID_SESSION;
    }
-
+    if (slot->defRWSession) { /* we have the monitor */
+    	slot->session = rwsession;
+    }
    return rwsession;
 }

 PRBool
-PK11_RWSessionHasLock(PK11SlotInfo *slot,CK_SESSION_HANDLE session_handle) {
-    return (PRBool)(!slot->isThreadSafe || slot->defRWSession);
+PK11_RWSessionHasLock(PK11SlotInfo *slot,CK_SESSION_HANDLE session_handle) 
+{
+    PRBool hasLock;
+    hasLock = (PRBool)(!slot->isThreadSafe || 
+    	      (slot->defRWSession && slot->session != CK_INVALID_SESSION));
+    return hasLock;
+}
+
+static PRBool
+pk11_RWSessionIsDefault(PK11SlotInfo *slot,CK_SESSION_HANDLE rwsession)
+{
+    PRBool isDefault;
+    isDefault = (PRBool)(slot->session == rwsession &&
+    	                 slot->defRWSession && 
+			 slot->session != CK_INVALID_SESSION);
+    return isDefault;
 }

 /*
@ -706,16 +723,14 @@ PK11_RWSessionHasLock(PK11SlotInfo *slot,CK_SESSION_HANDLE session_handle) {
 void
 PK11_RestoreROSession(PK11SlotInfo *slot,CK_SESSION_HANDLE rwsession)
 {
-    if (slot->defRWSession) {
-	PK11_ExitSlotMonitor(slot);
-	return;
-    }
+    PORT_Assert(rwsession != CK_INVALID_SESSION);
+    if (rwsession != CK_INVALID_SESSION) {
+    	PRBool doExit = PK11_RWSessionHasLock(slot, rwsession);
+	if (!pk11_RWSessionIsDefault(slot, rwsession))
 	    PK11_GETTAB(slot)->C_CloseSession(rwsession);
-    if (slot->session == CK_INVALID_SESSION) {
-    	 PK11_GETTAB(slot)->C_OpenSession(slot->slotID,CKF_SERIAL_SESSION,
-				  	 slot,pk11_notify,&slot->session);
+	if (doExit)
+	    PK11_ExitSlotMonitor(slot);
    }
-    if (!slot->isThreadSafe) PK11_ExitSlotMonitor(slot);
 }

 /************************************************************
--- a/security/nss/lib/pk11wrap/pk11util.c
+++ b/security/nss/lib/pk11wrap/pk11util.c
@ -861,7 +861,7 @@ SECMOD_UpdateSlotList(SECMODModule *mod)
    CK_ULONG count;
    int i, oldCount;
    PRBool freeRef = PR_FALSE;
-    void *mark;
+    void *mark = NULL;
    CK_ULONG *slotIDs = NULL;
    PK11SlotInfo **newSlots = NULL;
    PK11SlotInfo **oldSlots = NULL;