Bug 1370866 - Check return value of PK11Slot_GetNSSToken. r=djackson

Differential Revision: https://phabricator.services.mozilla.com/D139420

6 files changed, 156 insertions, 52 deletions

diff --git a/lib/pk11wrap/pk11auth.c b/lib/pk11wrap/pk11auth.c
index c633e53f7..42e64e1c9 100644
--- a/lib/pk11wrap/pk11auth.c
+++ b/lib/pk11wrap/pk11auth.c
@@ -4,6 +4,8 @@
 /*
  * This file deals with PKCS #11 passwords and authentication.
  */
+#include "dev.h"
+#include "dev3hack.h"
 #include "seccomon.h"
 #include "secmod.h"
 #include "secmodi.h"
@@ -637,8 +639,11 @@ PK11_DoPassword(PK11SlotInfo *slot, CK_SESSION_HANDLE session,
     }
     if (rv == SECSuccess) {
         if (!contextSpecific && !PK11_IsFriendly(slot)) {
-            nssTrustDomain_UpdateCachedTokenCerts(slot->nssToken->trustDomain,
-                                                  slot->nssToken);
+            NSSToken *token = PK11Slot_GetNSSToken(slot);
+            if (token) {
+                nssTrustDomain_UpdateCachedTokenCerts(token->trustDomain, token);
+                (void)nssToken_Destroy(token);
+            }
         }
     } else if (!attempt)
         PORT_SetError(SEC_ERROR_BAD_PASSWORD);
diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c
index 9c745d7b8..84d830035 100644
--- a/lib/pk11wrap/pk11cert.c
+++ b/lib/pk11wrap/pk11cert.c
@@ -242,16 +242,17 @@ pk11_fastCert(PK11SlotInfo *slot, CK_OBJECT_HANDLE certID,
     NSSCertificate *c;
     nssCryptokiObject *co = NULL;
     nssPKIObject *pkio;
-    NSSToken *token;
     NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
 
     /* Get the cryptoki object from the handle */
-    token = PK11Slot_GetNSSToken(slot);
-    if (token && token->defaultSession) {
-        co = nssCryptokiObject_Create(token, token->defaultSession, certID);
-    } else {
+    NSSToken *token = PK11Slot_GetNSSToken(slot);
+    if (!token || !token->defaultSession) {
+        (void)nssToken_Destroy(token); /* null token is ok */
         PORT_SetError(SEC_ERROR_NO_TOKEN);
+        return NULL;
     }
+    co = nssCryptokiObject_Create(token, token->defaultSession, certID);
+    (void)nssToken_Destroy(token);
     if (!co) {
         return NULL;
     }
@@ -754,7 +755,7 @@ find_certs_from_uri(const char *uriString, void *wincx)
             nssPKIObjectCollection_AddInstances(collection, instances, 0);
             nss_ZFreeIf(instances);
         }
-        nssToken_Destroy(*tok);
+        (void)nssToken_Destroy(*tok);
     }
     nss_ZFreeIf(tokens);
     nssList_Destroy(certList);
@@ -863,9 +864,7 @@ find_certs_from_nickname(const char *nickname, void *wincx)
     } else {
         slot = PK11_GetInternalKeySlot();
         token = PK11Slot_GetNSSToken(slot);
-        if (token) {
-            nssToken_AddRef(token);
-        } else {
+        if (!token) {
             PORT_SetError(SEC_ERROR_NO_TOKEN);
         }
     }
@@ -929,7 +928,7 @@ find_certs_from_nickname(const char *nickname, void *wincx)
     }
 loser:
     if (token) {
-        nssToken_Destroy(token);
+        (void)nssToken_Destroy(token);
     }
     if (slot) {
         PK11_FreeSlot(slot);
@@ -1129,15 +1128,15 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert,
     PRStatus status;
     NSSCertificate *c;
     nssCryptokiObject *keyobj, *certobj;
-    NSSToken *token = PK11Slot_GetNSSToken(slot);
-    SECItem *keyID = pk11_mkcertKeyID(cert);
+    NSSToken *token = NULL;
     char *emailAddr = NULL;
     nssCertificateStoreTrace lockTrace = { NULL, NULL, PR_FALSE, PR_FALSE };
     nssCertificateStoreTrace unlockTrace = { NULL, NULL, PR_FALSE, PR_FALSE };
-
+    SECItem *keyID = pk11_mkcertKeyID(cert);
     if (keyID == NULL) {
         goto loser; /* error code should be set already */
     }
+    token = PK11Slot_GetNSSToken(slot);
     if (!token) {
         PORT_SetError(SEC_ERROR_NO_TOKEN);
         goto loser;
@@ -1230,8 +1229,12 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert,
     (void)STAN_ForceCERTCertificateUpdate(c);
     nssCertificate_Destroy(c);
     SECITEM_FreeItem(keyID, PR_TRUE);
+    (void)nssToken_Destroy(token);
     return SECSuccess;
 loser:
+    if (token) {
+        (void)nssToken_Destroy(token);
+    }
     CERT_MapStanError();
     SECITEM_FreeItem(keyID, PR_TRUE);
     if (PORT_GetError() != SEC_ERROR_TOKEN_NOT_LOGGED_IN) {
@@ -1466,7 +1469,7 @@ PK11_FindCertByIssuerAndSNOnToken(PK11SlotInfo *slot,
     NSSCertificate *cert = NULL;
     NSSDER issuer, serial;
     NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
-    NSSToken *token = slot->nssToken;
+    NSSToken *token = NULL;
     nssSession *session;
     nssCryptokiObject *instance = NULL;
     nssPKIObject *object = NULL;
@@ -1481,12 +1484,18 @@ PK11_FindCertByIssuerAndSNOnToken(PK11SlotInfo *slot,
         return NULL;
     }
 
-    /* Paranoia */
-    if (token == NULL) {
+    token = PK11Slot_GetNSSToken(slot);
+    if (!token) {
         PORT_SetError(SEC_ERROR_NO_TOKEN);
         return NULL;
     }
 
+    session = nssToken_GetDefaultSession(token); /* non-owning */
+    if (!session) {
+        (void)nssToken_Destroy(token);
+        return NULL;
+    }
+
     /* PKCS#11 needs to use DER-encoded serial numbers.  Create a
      * CERTIssuerAndSN that actually has the encoded value and pass that
      * to PKCS#11 (and the crypto context).
@@ -1495,20 +1504,17 @@ PK11_FindCertByIssuerAndSNOnToken(PK11SlotInfo *slot,
                                    &issuerSN->serialNumber,
                                    SEC_ASN1_GET(SEC_IntegerTemplate));
     if (!derSerial) {
+        (void)nssToken_Destroy(token);
         return NULL;
     }
 
     NSSITEM_FROM_SECITEM(&issuer, &issuerSN->derIssuer);
     NSSITEM_FROM_SECITEM(&serial, derSerial);
 
-    session = nssToken_GetDefaultSession(token);
-    if (!session) {
-        goto loser;
-    }
-
     instance = nssToken_FindCertificateByIssuerAndSerialNumber(token, session,
                                                                &issuer, &serial, nssTokenSearchType_TokenForced, &status);
 
+    (void)nssToken_Destroy(token);
     SECITEM_FreeItem(derSerial, PR_TRUE);
 
     if (!instance) {
@@ -2177,16 +2183,22 @@ PK11_TraverseCertsForSubjectInSlot(CERTCertificate *cert, PK11SlotInfo *slot,
     td = STAN_GetDefaultTrustDomain();
     NSSITEM_FROM_SECITEM(&subject, &cert->derSubject);
     token = PK11Slot_GetNSSToken(slot);
+    if (!token) {
+        return SECSuccess;
+    }
     if (!nssToken_IsPresent(token)) {
+        (void)nssToken_Destroy(token);
         return SECSuccess;
     }
     collection = nssCertificateCollection_Create(td, NULL);
     if (!collection) {
+        (void)nssToken_Destroy(token);
         return SECFailure;
     }
     subjectList = nssList_Create(NULL, PR_FALSE);
     if (!subjectList) {
         nssPKIObjectCollection_Destroy(collection);
+        (void)nssToken_Destroy(token);
         return SECFailure;
     }
     (void)nssTrustDomain_GetCertsForSubjectFromCache(td, &subject,
@@ -2201,6 +2213,7 @@ PK11_TraverseCertsForSubjectInSlot(CERTCertificate *cert, PK11SlotInfo *slot,
     certs = nssPKIObjectCollection_GetCertificates(collection,
                                                    NULL, 0, NULL);
     nssPKIObjectCollection_Destroy(collection);
+    (void)nssToken_Destroy(token);
     if (certs) {
         CERTCertificate *oldie;
         NSSCertificate **cp;
@@ -2234,7 +2247,8 @@ PK11_TraverseCertsForNicknameInSlot(SECItem *nickname, PK11SlotInfo *slot,
     nssList *nameList = NULL;
     nssTokenSearchType tokenOnly = nssTokenSearchType_TokenOnly;
     token = PK11Slot_GetNSSToken(slot);
-    if (!nssToken_IsPresent(token)) {
+    if (!token || !nssToken_IsPresent(token)) {
+        (void)nssToken_Destroy(token);
         return SECSuccess;
     }
     if (nickname->data[nickname->len - 1] != '\0') {
@@ -2264,6 +2278,7 @@ PK11_TraverseCertsForNicknameInSlot(SECItem *nickname, PK11SlotInfo *slot,
     certs = nssPKIObjectCollection_GetCertificates(collection,
                                                    NULL, 0, NULL);
     nssPKIObjectCollection_Destroy(collection);
+    (void)nssToken_Destroy(token);
     if (certs) {
         CERTCertificate *oldie;
         NSSCertificate **cp;
@@ -2283,6 +2298,7 @@ PK11_TraverseCertsForNicknameInSlot(SECItem *nickname, PK11SlotInfo *slot,
         nss_ZFreeIf(nick);
     return (nssrv == PR_SUCCESS) ? SECSuccess : SECFailure;
 loser:
+    (void)nssToken_Destroy(token);
     if (created) {
         nss_ZFreeIf(nick);
     }
@@ -2308,16 +2324,22 @@ PK11_TraverseCertsInSlot(PK11SlotInfo *slot,
     NSSCertificate **certs;
     nssTokenSearchType tokenOnly = nssTokenSearchType_TokenOnly;
     tok = PK11Slot_GetNSSToken(slot);
+    if (!tok) {
+        return SECSuccess;
+    }
     if (!nssToken_IsPresent(tok)) {
+        (void)nssToken_Destroy(tok);
         return SECSuccess;
     }
     collection = nssCertificateCollection_Create(td, NULL);
     if (!collection) {
+        (void)nssToken_Destroy(tok);
         return SECFailure;
     }
     certList = nssList_Create(NULL, PR_FALSE);
     if (!certList) {
         nssPKIObjectCollection_Destroy(collection);
+        (void)nssToken_Destroy(tok);
         return SECFailure;
     }
     (void)nssTrustDomain_GetCertsFromCache(td, certList);
@@ -2330,6 +2352,7 @@ PK11_TraverseCertsInSlot(PK11SlotInfo *slot,
     certs = nssPKIObjectCollection_GetCertificates(collection,
                                                    NULL, 0, NULL);
     nssPKIObjectCollection_Destroy(collection);
+    (void)nssToken_Destroy(tok);
     if (certs) {
         CERTCertificate *oldie;
         NSSCertificate **cp;
@@ -2369,7 +2392,6 @@ PK11_FindCertFromDERCertItem(PK11SlotInfo *slot, const SECItem *inDerCert,
     SECStatus rv;
     CERTCertificate *cert = NULL;
 
-    tok = PK11Slot_GetNSSToken(slot);
     NSSITEM_FROM_SECITEM(&derCert, inDerCert);
     rv = pk11_AuthenticateUnfriendly(slot, PR_TRUE, wincx);
     if (rv != SECSuccess) {
@@ -2377,8 +2399,14 @@ PK11_FindCertFromDERCertItem(PK11SlotInfo *slot, const SECItem *inDerCert,
         return NULL;
     }
 
+    tok = PK11Slot_GetNSSToken(slot);
+    if (!tok) {
+        PK11_FreeSlot(slot);
+        return NULL;
+    }
     co = nssToken_FindCertificateByEncodedCertificate(tok, NULL, &derCert,
                                                       nssTokenSearchType_TokenOnly, NULL);
+    (void)nssToken_Destroy(tok);
 
     if (co) {
         cert = PK11_MakeCertFromHandle(slot, co->handle, NULL);
diff --git a/lib/pk11wrap/pk11nobj.c b/lib/pk11wrap/pk11nobj.c
index 80bc009f7..586ed80e3 100644
--- a/lib/pk11wrap/pk11nobj.c
+++ b/lib/pk11wrap/pk11nobj.c
@@ -413,12 +413,17 @@ PK11_FindCrlByName(PK11SlotInfo **slot, CK_OBJECT_HANDLE *crlHandle,
         nssPKIObjectCollection *collection;
         nssTokenSearchType tokenOnly = nssTokenSearchType_TokenOnly;
         NSSToken *token = PK11Slot_GetNSSToken(*slot);
+        if (!token) {
+            goto loser;
+        }
         collection = nssCRLCollection_Create(td, NULL);
         if (!collection) {
+            (void)nssToken_Destroy(token);
             goto loser;
         }
         instances = nssToken_FindCRLsBySubject(token, NULL, &subject,
                                                tokenOnly, 0, NULL);
+        (void)nssToken_Destroy(token);
         nssPKIObjectCollection_AddInstances(collection, instances, 0);
         nss_ZFreeIf(instances);
         crls = nssPKIObjectCollection_GetCRLs(collection, NULL, 0, NULL);
@@ -482,16 +487,21 @@ PK11_PutCrl(PK11SlotInfo *slot, SECItem *crl, SECItem *name,
             char *url, int type)
 {
     NSSItem derCRL, derSubject;
-    NSSToken *token = PK11Slot_GetNSSToken(slot);
+    NSSToken *token;
     nssCryptokiObject *object;
     PRBool isKRL = (type == SEC_CRL_TYPE) ? PR_FALSE : PR_TRUE;
     CK_OBJECT_HANDLE rvH;
 
     NSSITEM_FROM_SECITEM(&derSubject, name);
     NSSITEM_FROM_SECITEM(&derCRL, crl);
-
+    token = PK11Slot_GetNSSToken(slot);
+    if (!token) {
+        PORT_SetError(SEC_ERROR_NO_TOKEN);
+        return CK_INVALID_HANDLE;
+    }
     object = nssToken_ImportCRL(token, NULL,
                                 &derSubject, &derCRL, isKRL, url, PR_TRUE);
+    (void)nssToken_Destroy(token);
 
     if (object) {
         rvH = object->handle;
@@ -510,8 +520,8 @@ SECStatus
 SEC_DeletePermCRL(CERTSignedCrl *crl)
 {
     PRStatus status;
-    NSSToken *token;
     nssCryptokiObject *object;
+    NSSToken *token;
     PK11SlotInfo *slot = crl->slot;
 
     if (slot == NULL) {
@@ -520,13 +530,17 @@ SEC_DeletePermCRL(CERTSignedCrl *crl)
         PORT_SetError(SEC_ERROR_CRL_INVALID);
         return SECFailure;
     }
-    token = PK11Slot_GetNSSToken(slot);
 
+    token = PK11Slot_GetNSSToken(slot);
+    if (!token) {
+        return SECFailure;
+    }
     object = nss_ZNEW(NULL, nssCryptokiObject);
     if (!object) {
+        (void)nssToken_Destroy(token);
         return SECFailure;
     }
-    object->token = nssToken_AddRef(token);
+    object->token = token; /* object takes ownership */
     object->handle = crl->pkcs11ID;
     object->isTokenObject = PR_TRUE;
 
diff --git a/lib/pk11wrap/pk11slot.c b/lib/pk11wrap/pk11slot.c
index c320019f9..99be9528f 100644
--- a/lib/pk11wrap/pk11slot.c
+++ b/lib/pk11wrap/pk11slot.c
@@ -362,19 +362,24 @@ PK11_NewSlotInfo(SECMODModule *mod)
     PK11SlotInfo *slot;
 
     slot = (PK11SlotInfo *)PORT_Alloc(sizeof(PK11SlotInfo));
-    if (slot == NULL)
+    if (slot == NULL) {
         return slot;
-
-    slot->sessionLock = mod->isThreadSafe ? PZ_NewLock(nssILockSession) : mod->refLock;
-    if (slot->sessionLock == NULL) {
-        PORT_Free(slot);
-        return NULL;
     }
     slot->freeListLock = PZ_NewLock(nssILockFreelist);
     if (slot->freeListLock == NULL) {
-        if (mod->isThreadSafe) {
-            PZ_DestroyLock(slot->sessionLock);
-        }
+        PORT_Free(slot);
+        return NULL;
+    }
+    slot->nssTokenLock = PZ_NewLock(nssILockOther);
+    if (slot->nssTokenLock == NULL) {
+        PZ_DestroyLock(slot->freeListLock);
+        PORT_Free(slot);
+        return NULL;
+    }
+    slot->sessionLock = mod->isThreadSafe ? PZ_NewLock(nssILockSession) : mod->refLock;
+    if (slot->sessionLock == NULL) {
+        PZ_DestroyLock(slot->nssTokenLock);
+        PZ_DestroyLock(slot->freeListLock);
         PORT_Free(slot);
         return NULL;
     }
@@ -462,6 +467,10 @@ PK11_DestroySlot(PK11SlotInfo *slot)
         PZ_DestroyLock(slot->freeListLock);
         slot->freeListLock = NULL;
     }
+    if (slot->nssTokenLock) {
+        PZ_DestroyLock(slot->nssTokenLock);
+        slot->nssTokenLock = NULL;
+    }
 
     /* finally Tell our parent module that we've gone away so it can unload */
     if (slot->module) {
@@ -1260,6 +1269,7 @@ PK11_InitToken(PK11SlotInfo *slot, PRBool loadCerts)
     CK_RV crv;
     SECStatus rv;
     PRStatus status;
+    NSSToken *nssToken;
 
     /* set the slot flags to the current token values */
     if (!slot->isThreadSafe)
@@ -1297,7 +1307,9 @@ PK11_InitToken(PK11SlotInfo *slot, PRBool loadCerts)
     slot->maxPassword = slot->tokenInfo.ulMaxPinLen;
     PORT_Memcpy(slot->serial, slot->tokenInfo.serialNumber, sizeof(slot->serial));
 
-    nssToken_UpdateName(slot->nssToken);
+    nssToken = PK11Slot_GetNSSToken(slot);
+    nssToken_UpdateName(nssToken); /* null token is OK */
+    (void)nssToken_Destroy(nssToken);
 
     slot->defRWSession = (PRBool)((!slot->readOnly) &&
                                   (slot->tokenInfo.ulMaxSessionCount == 1));
@@ -1365,7 +1377,9 @@ PK11_InitToken(PK11SlotInfo *slot, PRBool loadCerts)
             PK11_ExitSlotMonitor(slot);
     }
 
-    status = nssToken_Refresh(slot->nssToken);
+    nssToken = PK11Slot_GetNSSToken(slot);
+    status = nssToken_Refresh(nssToken); /* null token is OK */
+    (void)nssToken_Destroy(nssToken);
     if (status != PR_SUCCESS)
         return SECFailure;
 
@@ -1598,8 +1612,11 @@ pk11_IsPresentCertLoad(PK11SlotInfo *slot, PRBool loadCerts)
         return PR_TRUE;
     }
 
-    if (slot->nssToken) {
-        return nssToken_IsPresent(slot->nssToken);
+    NSSToken *nssToken = PK11Slot_GetNSSToken(slot);
+    if (nssToken) {
+        PRBool present = nssToken_IsPresent(nssToken);
+        (void)nssToken_Destroy(nssToken);
+        return present;
     }
 
     /* removable slots have a flag that says they are present */
@@ -2649,20 +2666,44 @@ PK11_ResetToken(PK11SlotInfo *slot, char *sso_pwd)
         PORT_SetError(PK11_MapError(crv));
         return SECFailure;
     }
-    nssTrustDomain_UpdateCachedTokenCerts(slot->nssToken->trustDomain,
-                                          slot->nssToken);
+    NSSToken *token = PK11Slot_GetNSSToken(slot);
+    if (token) {
+        nssTrustDomain_UpdateCachedTokenCerts(token->trustDomain, token);
+        (void)nssToken_Destroy(token);
+    }
     return SECSuccess;
 }
+
 void
 PK11Slot_SetNSSToken(PK11SlotInfo *sl, NSSToken *nsst)
 {
+    NSSToken *old;
+    if (nsst) {
+        nsst = nssToken_AddRef(nsst);
+    }
+
+    PZ_Lock(sl->nssTokenLock);
+    old = sl->nssToken;
     sl->nssToken = nsst;
+    PZ_Unlock(sl->nssTokenLock);
+
+    if (old) {
+        (void)nssToken_Destroy(old);
+    }
 }
 
 NSSToken *
 PK11Slot_GetNSSToken(PK11SlotInfo *sl)
 {
-    return sl->nssToken;
+    NSSToken *rv = NULL;
+
+    PZ_Lock(sl->nssTokenLock);
+    if (sl->nssToken) {
+        rv = nssToken_AddRef(sl->nssToken);
+    }
+    PZ_Unlock(sl->nssTokenLock);
+
+    return rv;
 }
 
 PRBool
diff --git a/lib/pk11wrap/pk11util.c b/lib/pk11wrap/pk11util.c
index 08c793bf3..0862ee289 100644
--- a/lib/pk11wrap/pk11util.c
+++ b/lib/pk11wrap/pk11util.c
@@ -13,6 +13,7 @@
 #include "pki3hack.h"
 #include "secerr.h"
 #include "dev.h"
+#include "dev3hack.h"
 #include "utilpars.h"
 #include "pkcs11uri.h"
 
@@ -1266,8 +1267,14 @@ SECMOD_WaitForAnyTokenEvent(SECMODModule *mod, unsigned long flags,
     }
     /* if we are in the delay period for the "isPresent" call, reset
      * the delay since we know things have probably changed... */
-    if (slot && slot->nssToken && slot->nssToken->slot) {
-        nssSlot_ResetDelay(slot->nssToken->slot);
+    if (slot) {
+        NSSToken *nssToken = PK11Slot_GetNSSToken(slot);
+        if (nssToken) {
+            if (nssToken->slot) {
+                nssSlot_ResetDelay(nssToken->slot);
+            }
+            (void)nssToken_Destroy(nssToken);
+        }
     }
     return slot;
 
@@ -1500,8 +1507,12 @@ SECMOD_OpenNewSlot(SECMODModule *mod, const char *moduleSpec)
     if (slot) {
         /* if we are in the delay period for the "isPresent" call, reset
          * the delay since we know things have probably changed... */
-        if (slot->nssToken && slot->nssToken->slot) {
-            nssSlot_ResetDelay(slot->nssToken->slot);
+        NSSToken *nssToken = PK11Slot_GetNSSToken(slot);
+        if (nssToken) {
+            if (nssToken->slot) {
+                nssSlot_ResetDelay(nssToken->slot);
+            }
+            (void)nssToken_Destroy(nssToken);
         }
         /* force the slot info structures to properly reset */
         (void)PK11_IsPresent(slot);
@@ -1631,8 +1642,12 @@ SECMOD_CloseUserDB(PK11SlotInfo *slot)
     PR_smprintf_free(sendSpec);
     /* if we are in the delay period for the "isPresent" call, reset
      * the delay since we know things have probably changed... */
-    if (slot->nssToken && slot->nssToken->slot) {
-        nssSlot_ResetDelay(slot->nssToken->slot);
+    NSSToken *nssToken = PK11Slot_GetNSSToken(slot);
+    if (nssToken) {
+        if (nssToken->slot) {
+            nssSlot_ResetDelay(nssToken->slot);
+        }
+        (void)nssToken_Destroy(nssToken);
         /* force the slot info structures to properly reset */
         (void)PK11_IsPresent(slot);
     }
diff --git a/lib/pk11wrap/secmodti.h b/lib/pk11wrap/secmodti.h
index 04c63a869..5dca1e46c 100644
--- a/lib/pk11wrap/secmodti.h
+++ b/lib/pk11wrap/secmodti.h
@@ -107,6 +107,7 @@ struct PK11SlotInfoStr {
     unsigned int lastState;
     /* for Stan */
     NSSToken *nssToken;
+    PZLock *nssTokenLock;
     /* the tokeninfo struct */
     CK_TOKEN_INFO tokenInfo;
     /* fast mechanism lookup */