[RFC PATCH] Fix use-after-free of token in NSSTrustDomain_FindTokenByName()

When NSSTrustDomain_FindTokenByName() returns 'tok', what guarantees
that it still exists, the moment after the locks are dropped?

Aren't we supposed to use nssToken_AddRef() before dropping the locks,
to ensure that it stays around long enough for our caller to actually
use it?

So we'd want something like the patch below?

I ask because we're adding a new NSSTrustDomain_FindTokensByURI()
function and won't want to make the same mistake. I've been drumming
into Varun that he needs to make his new functions look at much like
*existing* similar functions as possible... but perhaps *not* to the
extent of preserving their bugs :)

diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c
index 1b9be63..e4443c5 100644
--- a/lib/pk11wrap/pk11cert.c
+++ b/lib/pk11wrap/pk11cert.c
@@ -731,20 +731,23 @@ find_certs_from_nickname(const char *nickname, void *wincx)
     if ((delimit = PORT_Strchr(nickCopy,':')) != NULL) {
 	tokenName = nickCopy;
 	nickname = delimit + 1;
 	*delimit = '\0';
 	/* find token by name */
+	/* We got a ref on the token here... */
 	token = NSSTrustDomain_FindTokenByName(defaultTD, (NSSUTF8 *)tokenName);
 	if (token) {
 	    slot = PK11_ReferenceSlot(token->pk11slot);
 	} else {
 	    PORT_SetError(SEC_ERROR_NO_TOKEN);
 	}
 	*delimit = ':';
     } else {
 	slot = PK11_GetInternalKeySlot();
 	token = PK11Slot_GetNSSToken(slot);
+	/* ... so we need one here too... */
+	nssToken_AddRef(token);
     }
     if (token) {
 	nssList *certList;
 	nssCryptokiObject **instances;
 	nssTokenSearchType tokenOnly = nssTokenSearchType_TokenOnly;
@@ -803,10 +806,14 @@ find_certs_from_nickname(const char *nickname, void *wincx)
     }
  loser:
     if (collection) {
 	nssPKIObjectCollection_Destroy(collection);
     }
+    if (token) {
+	/* ... and we drop it here */
+	nssToken_Destroy(token);
+    }
     if (slot) {
 	PK11_FreeSlot(slot);
     }
     if (nickCopy) PORT_Free(nickCopy);
     return certs;
diff --git a/lib/pki/trustdomain.c b/lib/pki/trustdomain.c
index 87ea6da..dd72347 100644
--- a/lib/pki/trustdomain.c
+++ b/lib/pki/trustdomain.c
@@ -294,11 +294,14 @@ NSSTrustDomain_FindTokenByName (
          tok != (NSSToken *)NULL;
          tok  = (NSSToken *)nssListIterator_Next(td->tokens))
     {
 	if (nssToken_IsPresent(tok)) {
 	    myName = nssToken_GetName(tok);
-	    if (nssUTF8_Equal(tokenName, myName, &nssrv)) break;
+	    if (nssUTF8_Equal(tokenName, myName, &nssrv)) {
+		tok = nssToken_AddRef(tok);
+		break;
+	    }
 	}
     }
     nssListIterator_Finish(td->tokens);
     NSSRWLock_UnlockRead(td->tokensLock);
     return tok;

(Note that there is no find_certs_from_nickname() in HEAD at the
moment; there are two almost identical copies of the same code,
duplicated in PK11_FindCertsFromNickname() and
PK11_FindCertFromNickname() with the occasional bug fix applied to one
but not the other. I fixed that already in my tree and will submit that
cleanup separately. Again, I didn't want Varun doing the same thing
with the equivalent FromURI() functions.)

-- 
David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

-- 
dev-tech-crypto mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-tech-crypto