Attachment 637191 Details for Bug 872124 – Fix locking issue. Also authenticate on the correct session

[patch] Fix locking issue. Also authenticate on the correct session

patch-pk11wrap-locking (text/plain), 15.46 KB, created by Bob Relyea on 2012-11-02 18:17:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Bob Relyea

Created: 2012-11-02 18:17:14 UTC

Size: 15.46 KB

patch

obsolete

>Index: lib/cryptohi/keythi.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v
>retrieving revision 1.17
>diff -u -p -r1.17 keythi.h
>--- lib/cryptohi/keythi.h	16 May 2012 12:34:15 -0000	1.17
>+++ lib/cryptohi/keythi.h	2 Nov 2012 18:09:53 -0000
>@@ -214,7 +214,12 @@ typedef struct SECKEYPublicKeyStr SECKEY
> #define SECKEY_HAS_ATTRIBUTE_SET(key,attribute) \
>     (0 != (key->staticflags & SECKEY_Attributes_Cached)) ? \
>     (0 != (key->staticflags & SECKEY_##attribute)) : \
>-    PK11_HasAttributeSet(key->pkcs11Slot,key->pkcs11ID,attribute)
>+    PK11_HasAttributeSet(key->pkcs11Slot,key->pkcs11ID,attribute, PR_FALSE)
>+
>+#define SECKEY_HAS_ATTRIBUTE_SET_LOCK(key,attribute, haslock) \
>+    (0 != (key->staticflags & SECKEY_Attributes_Cached)) ? \
>+    (0 != (key->staticflags & SECKEY_##attribute)) : \
>+    PK11_HasAttributeSet(key->pkcs11Slot,key->pkcs11ID,attribute, haslock)
> 
> /*
> ** A generic key structure
>Index: lib/cryptohi/seckey.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v
>retrieving revision 1.68
>diff -u -p -r1.68 seckey.c
>--- lib/cryptohi/seckey.c	25 Jun 2012 21:48:39 -0000	1.68
>+++ lib/cryptohi/seckey.c	2 Nov 2012 18:09:53 -0000
>@@ -1918,7 +1918,7 @@ loser:
> }
> 
> #define SECKEY_CacheAttribute(key, attribute) \
>-    if (CK_TRUE == PK11_HasAttributeSet(key->pkcs11Slot, key->pkcs11ID, attribute)) { \
>+    if (CK_TRUE == PK11_HasAttributeSet(key->pkcs11Slot, key->pkcs11ID, attribute, PR_FALSE)) { \
>         key->staticflags |= SECKEY_##attribute; \
>     } else { \
>         key->staticflags &= (~SECKEY_##attribute); \
>Index: lib/pk11wrap/pk11akey.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v
>retrieving revision 1.36
>diff -u -p -r1.36 pk11akey.c
>--- lib/pk11wrap/pk11akey.c	25 Apr 2012 14:50:04 -0000	1.36
>+++ lib/pk11wrap/pk11akey.c	2 Nov 2012 18:09:54 -0000
>@@ -740,7 +740,7 @@ PK11_MakePrivKey(PK11SlotInfo *slot, Key
> 	CK_KEY_TYPE pk11Type = CKK_RSA;
> 
> 	pk11Type = PK11_ReadULongAttribute(slot,privID,CKA_KEY_TYPE);
>-	isTemp = (PRBool)!PK11_HasAttributeSet(slot,privID,CKA_TOKEN);
>+	isTemp = (PRBool)!PK11_HasAttributeSet(slot,privID,CKA_TOKEN,PR_FALSE);
> 	switch (pk11Type) {
> 	case CKK_RSA: keyType = rsaKey; break;
> 	case CKK_DSA: keyType = dsaKey; break;
>@@ -754,7 +754,7 @@ PK11_MakePrivKey(PK11SlotInfo *slot, Key
> 
>     /* if the key is private, make sure we are authenticated to the
>      * token before we try to use it */
>-    isPrivate = (PRBool)PK11_HasAttributeSet(slot,privID,CKA_PRIVATE);
>+    isPrivate = (PRBool)PK11_HasAttributeSet(slot,privID,CKA_PRIVATE,PR_FALSE);
>     if (isPrivate) {
> 	rv = PK11_Authenticate(slot, PR_TRUE, wincx);
>  	if (rv != SECSuccess) {
>@@ -1432,7 +1432,7 @@ PK11_GenerateKeyPairWithOpFlags(PK11Slot
> 
>     /* set the ID to the public key so we can find it again */
>     cka_id = pk11_MakeIDFromPublicKey(*pubKey);
>-    pubIsToken = (PRBool)PK11_HasAttributeSet(slot,pubID, CKA_TOKEN);
>+    pubIsToken = (PRBool)PK11_HasAttributeSet(slot,pubID, CKA_TOKEN,PR_FALSE);
> 
>     PK11_SETATTRS(&setTemplate, CKA_ID, cka_id->data, cka_id->len);
> 
>Index: lib/pk11wrap/pk11auth.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v
>retrieving revision 1.16
>diff -u -p -r1.16 pk11auth.c
>--- lib/pk11wrap/pk11auth.c	16 May 2012 12:34:19 -0000	1.16
>+++ lib/pk11wrap/pk11auth.c	2 Nov 2012 18:09:54 -0000
>@@ -45,8 +45,9 @@ static struct PK11GlobalStruct {
>  * Check the user's password. Log into the card if it's correct.
>  * succeed if the user is already logged in.
>  */
>-SECStatus
>-pk11_CheckPassword(PK11SlotInfo *slot,char *pw,PRBool contextSpecific)
>+static SECStatus
>+pk11_CheckPassword(PK11SlotInfo *slot, CK_SESSION_HANDLE session,
>+			char *pw, PRBool alreadyLocked, PRBool contextSpecific)
> {
>     int len = 0;
>     CK_RV crv;
>@@ -66,13 +67,13 @@ pk11_CheckPassword(PK11SlotInfo *slot,ch
>     }
> 
>     do {
>-	PK11_EnterSlotMonitor(slot);
>-	crv = PK11_GETTAB(slot)->C_Login(slot->session,
>+	if (!alreadyLocked) PK11_EnterSlotMonitor(slot);
>+	crv = PK11_GETTAB(slot)->C_Login(session,
> 		contextSpecific ? CKU_CONTEXT_SPECIFIC : CKU_USER,
> 						(unsigned char *)pw,len);
> 	slot->lastLoginCheck = 0;
> 	mustRetry = PR_FALSE;
>-	PK11_ExitSlotMonitor(slot);
>+	if (!alreadyLocked) PK11_ExitSlotMonitor(slot);
> 	switch (crv) {
> 	/* if we're already logged in, we're good to go */
> 	case CKR_OK:
>@@ -91,10 +92,19 @@ pk11_CheckPassword(PK11SlotInfo *slot,ch
> 	 * if the token is still there. */
> 	case CKR_SESSION_HANDLE_INVALID:
> 	case CKR_SESSION_CLOSED:
>+	    if (session != slot->session) {
>+		/* don't bother retrying, we were in a middle of an operation,
>+		 * which is now lost. Just fail. */
>+	        PORT_SetError(PK11_MapError(crv));
>+	        rv = SECFailure; 
>+		break;
>+	    }
> 	    if (retry++ == 0) {
> 		rv = PK11_InitToken(slot,PR_FALSE);
> 		if (rv == SECSuccess) {
> 		    if (slot->session != CK_INVALID_SESSION) {
>+			session = slot->session; /* we should have 
>+						  * a new session now */
> 			mustRetry = PR_TRUE;
> 		    } else {
> 			PORT_SetError(PK11_MapError(crv));
>@@ -242,7 +252,8 @@ PK11_HandlePasswordCheck(PK11SlotInfo *s
> 	    NeedAuth = PR_TRUE;
> 	}
>     }
>-    if (NeedAuth) PK11_DoPassword(slot,PR_TRUE,wincx,PR_FALSE);
>+    if (NeedAuth) PK11_DoPassword(slot, slot->session, PR_TRUE,
>+			wincx, PR_FALSE, PR_FALSE);
> }
> 
> void
>@@ -301,7 +312,8 @@ pk11_LoginStillRequired(PK11SlotInfo *sl
> SECStatus
> PK11_Authenticate(PK11SlotInfo *slot, PRBool loadCerts, void *wincx) {
>     if (pk11_LoginStillRequired(slot,wincx)) {
>-	return PK11_DoPassword(slot,loadCerts,wincx,PR_FALSE);
>+	return PK11_DoPassword(slot, slot->session, loadCerts, wincx,
>+				PR_FALSE, PR_FALSE);
>     }
>     return SECSuccess;
> }
>@@ -532,7 +544,8 @@ PK11_SetIsLoggedInFunc(PK11IsLoggedInFun
>  * of the PKCS 11 module.
>  */
> SECStatus
>-PK11_DoPassword(PK11SlotInfo *slot, PRBool loadCerts, void *wincx,
>+PK11_DoPassword(PK11SlotInfo *slot, CK_SESSION_HANDLE session,
>+			PRBool loadCerts, void *wincx, PRBool alreadyLocked,
> 			PRBool contextSpecific)
> {
>     SECStatus rv = SECFailure;
>@@ -602,7 +615,8 @@ PK11_DoPassword(PK11SlotInfo *slot, PRBo
> 		break;
> 	    }
> 	}
>-	rv = pk11_CheckPassword(slot,password,contextSpecific);
>+	rv = pk11_CheckPassword(slot, session, password, 
>+				alreadyLocked, contextSpecific);
> 	PORT_Memset(password, 0, PORT_Strlen(password));
> 	PORT_Free(password);
> 	if (rv != SECWouldBlock) break;
>Index: lib/pk11wrap/pk11merge.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11merge.c,v
>retrieving revision 1.10
>diff -u -p -r1.10 pk11merge.c
>--- lib/pk11wrap/pk11merge.c	25 Apr 2012 14:50:05 -0000	1.10
>+++ lib/pk11wrap/pk11merge.c	2 Nov 2012 18:09:54 -0000
>@@ -169,15 +169,15 @@ pk11_getPrivateKeyUsage(PK11SlotInfo *sl
> {
>     unsigned int usage = 0;
> 
>-    if ((PK11_HasAttributeSet(slot, id, CKA_UNWRAP) || 
>-			PK11_HasAttributeSet(slot,id, CKA_DECRYPT))) {
>+    if ((PK11_HasAttributeSet(slot, id, CKA_UNWRAP,PR_FALSE) || 
>+			PK11_HasAttributeSet(slot,id, CKA_DECRYPT,PR_FALSE))) {
> 	usage |= KU_KEY_ENCIPHERMENT;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_DERIVE)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_DERIVE, PR_FALSE)) {
> 	usage |= KU_KEY_AGREEMENT;
>     }
>-    if ((PK11_HasAttributeSet(slot, id, CKA_SIGN_RECOVER) || 
>-			PK11_HasAttributeSet(slot, id, CKA_SIGN))) {
>+    if ((PK11_HasAttributeSet(slot, id, CKA_SIGN_RECOVER, PR_FALSE) || 
>+			PK11_HasAttributeSet(slot, id, CKA_SIGN, PR_FALSE))) {
> 	usage |= KU_DIGITAL_SIGNATURE;
>     }
>     return usage;
>@@ -373,31 +373,31 @@ pk11_getSecretKeyFlags(PK11SlotInfo *slo
> {
>     CK_FLAGS flags = 0;
> 
>-    if (PK11_HasAttributeSet(slot, id, CKA_UNWRAP)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_UNWRAP, PR_FALSE)) {
> 	flags |= CKF_UNWRAP;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_WRAP)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_WRAP, PR_FALSE)) {
> 	flags |= CKF_WRAP;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_ENCRYPT)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_ENCRYPT, PR_FALSE)) {
> 	flags |= CKF_ENCRYPT;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_DECRYPT)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_DECRYPT, PR_FALSE)) {
> 	flags |= CKF_DECRYPT;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_DERIVE)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_DERIVE, PR_FALSE)) {
> 	flags |= CKF_DERIVE;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_SIGN)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_SIGN, PR_FALSE)) {
> 	flags |= CKF_SIGN;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_SIGN_RECOVER)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_SIGN_RECOVER, PR_FALSE)) {
> 	flags |= CKF_SIGN_RECOVER;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_VERIFY)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_VERIFY, PR_FALSE)) {
> 	flags |= CKF_VERIFY;
>     }
>-    if (PK11_HasAttributeSet(slot, id, CKA_VERIFY_RECOVER)) {
>+    if (PK11_HasAttributeSet(slot, id, CKA_VERIFY_RECOVER, PR_FALSE)) {
> 	flags |= CKF_VERIFY_RECOVER;
>     }
>     return flags;
>Index: lib/pk11wrap/pk11obj.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v
>retrieving revision 1.27
>diff -u -p -r1.27 pk11obj.c
>--- lib/pk11wrap/pk11obj.c	29 Jun 2012 17:46:25 -0000	1.27
>+++ lib/pk11wrap/pk11obj.c	2 Nov 2012 18:09:54 -0000
>@@ -146,7 +146,7 @@ PK11_ReadULongAttribute(PK11SlotInfo *sl
>  */
> CK_BBOOL
> PK11_HasAttributeSet( PK11SlotInfo *slot, CK_OBJECT_HANDLE id,
>-				                      CK_ATTRIBUTE_TYPE type )
>+				 CK_ATTRIBUTE_TYPE type, PRBool haslock )
> {
>     CK_BBOOL ckvalue = CK_FALSE;
>     CK_ATTRIBUTE theTemplate;
>@@ -156,10 +156,10 @@ PK11_HasAttributeSet( PK11SlotInfo *slot
>     PK11_SETATTRS( &theTemplate, type, &ckvalue, sizeof( CK_BBOOL ) );
> 
>     /* Retrieve attribute value. */
>-    PK11_EnterSlotMonitor(slot);
>+    if (!haslock) PK11_EnterSlotMonitor(slot);
>     crv = PK11_GETTAB( slot )->C_GetAttributeValue( slot->session, id,
>                                                     &theTemplate, 1 );
>-    PK11_ExitSlotMonitor(slot);
>+    if (!haslock) PK11_ExitSlotMonitor(slot);
>     if( crv != CKR_OK ) {
>         PORT_SetError( PK11_MapError( crv ) );
>         return CK_FALSE;
>@@ -254,7 +254,7 @@ PK11_GetAttributes(PRArenaPool *arena,PK
> PRBool
> PK11_IsPermObject(PK11SlotInfo *slot, CK_OBJECT_HANDLE handle)
> {
>-    return (PRBool) PK11_HasAttributeSet(slot, handle, CKA_TOKEN);
>+    return (PRBool) PK11_HasAttributeSet(slot, handle, CKA_TOKEN, PR_FALSE);
> }
> 
> char *
>@@ -735,6 +735,7 @@ PK11_Sign(SECKEYPrivateKey *key, SECItem
>     CK_MECHANISM mech = {0, NULL, 0 };
>     PRBool owner = PR_TRUE;
>     CK_SESSION_HANDLE session;
>+    PRBool haslock = PR_FALSE;
>     CK_ULONG len;
>     CK_RV crv;
> 
>@@ -745,24 +746,27 @@ PK11_Sign(SECKEYPrivateKey *key, SECItem
>     }
> 
>     session = pk11_GetNewSession(slot,&owner);
>-    if (!owner || !(slot->isThreadSafe)) PK11_EnterSlotMonitor(slot);
>+    haslock = (!owner || !(slot->isThreadSafe));
>+    if (haslock) PK11_EnterSlotMonitor(slot);
>     crv = PK11_GETTAB(slot)->C_SignInit(session,&mech,key->pkcs11ID);
>     if (crv != CKR_OK) {
>-	if (!owner || !(slot->isThreadSafe)) PK11_ExitSlotMonitor(slot);
>+	if (haslock) PK11_ExitSlotMonitor(slot);
> 	pk11_CloseSession(slot,session,owner);
> 	PORT_SetError( PK11_MapError(crv) );
> 	return SECFailure;
>     }
>-	/* PKCS11 2.20 says if CKA_ALWAYS_AUTHENTICATE then 
>-	 * do C_Login with CKU_CONTEXT_SPECIFIC 
>-	 * between C_SignInit and C_Sign */
>-	if (SECKEY_HAS_ATTRIBUTE_SET(key,CKA_ALWAYS_AUTHENTICATE)) {
>-		PK11_DoPassword(slot, PR_FALSE, key->wincx, PR_TRUE);
>-	}
>+
>+    /* PKCS11 2.20 says if CKA_ALWAYS_AUTHENTICATE then 
>+     * do C_Login with CKU_CONTEXT_SPECIFIC 
>+     * between C_SignInit and C_Sign */
>+    if (SECKEY_HAS_ATTRIBUTE_SET_LOCK(key, CKA_ALWAYS_AUTHENTICATE, haslock)) {
>+	PK11_DoPassword(slot, session, PR_FALSE, key->wincx, haslock, PR_TRUE);
>+    }
>+
>     len = sig->len;
>     crv = PK11_GETTAB(slot)->C_Sign(session,hash->data,
> 					hash->len, sig->data, &len);
>-    if (!owner || !(slot->isThreadSafe)) PK11_ExitSlotMonitor(slot);
>+    if (haslock) PK11_ExitSlotMonitor(slot);
>     pk11_CloseSession(slot,session,owner);
>     sig->len = len;
>     if (crv != CKR_OK) {
>@@ -788,6 +792,7 @@ pk11_PrivDecryptRaw(SECKEYPrivateKey *ke
>     CK_ULONG out = maxLen;
>     PRBool owner = PR_TRUE;
>     CK_SESSION_HANDLE session;
>+    PRBool haslock = PR_FALSE;
>     CK_RV crv;
> 
>     if (key->keyType != rsaKey) {
>@@ -803,23 +808,26 @@ pk11_PrivDecryptRaw(SECKEYPrivateKey *ke
> 	PK11_HandlePasswordCheck(slot, key->wincx);
>     }
>     session = pk11_GetNewSession(slot,&owner);
>-    if (!owner || !(slot->isThreadSafe)) PK11_EnterSlotMonitor(slot);
>+    haslock = (!owner || !(slot->isThreadSafe));
>+    if (haslock) PK11_EnterSlotMonitor(slot);
>     crv = PK11_GETTAB(slot)->C_DecryptInit(session, mech, key->pkcs11ID);
>     if (crv != CKR_OK) {
>-	if (!owner || !(slot->isThreadSafe)) PK11_ExitSlotMonitor(slot);
>+	if (haslock) PK11_ExitSlotMonitor(slot);
> 	pk11_CloseSession(slot,session,owner);
> 	PORT_SetError( PK11_MapError(crv) );
> 	return SECFailure;
>     }
>-	/* PKCS11 2.20 says if CKA_ALWAYS_AUTHENTICATE then 
>-	 * do C_Login with CKU_CONTEXT_SPECIFIC 
>-	 * between C_DecryptInit and C_Decrypt */
>-	/* But see note above about servers */
>-	if (SECKEY_HAS_ATTRIBUTE_SET(key,CKA_ALWAYS_AUTHENTICATE)) {
>-		PK11_DoPassword(slot, PR_FALSE, key->wincx, PR_TRUE);
>-	}
>+
>+    /* PKCS11 2.20 says if CKA_ALWAYS_AUTHENTICATE then 
>+     * do C_Login with CKU_CONTEXT_SPECIFIC 
>+     * between C_DecryptInit and C_Decrypt
>+     * ... But see note above about servers */
>+     if (SECKEY_HAS_ATTRIBUTE_SET_LOCK(key, CKA_ALWAYS_AUTHENTICATE, haslock)) {
>+	PK11_DoPassword(slot, session, PR_FALSE, key->wincx, haslock, PR_TRUE);
>+    }
>+
>     crv = PK11_GETTAB(slot)->C_Decrypt(session,enc, encLen, data, &out);
>-    if (!owner || !(slot->isThreadSafe)) PK11_ExitSlotMonitor(slot);
>+    if (haslock) PK11_ExitSlotMonitor(slot);
>     pk11_CloseSession(slot,session,owner);
>     *outLen = out;
>     if (crv != CKR_OK) {
>Index: lib/pk11wrap/pk11priv.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v
>retrieving revision 1.18
>diff -u -p -r1.18 pk11priv.h
>--- lib/pk11wrap/pk11priv.h	16 May 2012 12:34:20 -0000	1.18
>+++ lib/pk11wrap/pk11priv.h	2 Nov 2012 18:09:55 -0000
>@@ -59,8 +59,9 @@ void PK11_CleanKeyList(PK11SlotInfo *slo
> /************************************************************
>  *  Slot Password Management
>  ************************************************************/
>-SECStatus PK11_DoPassword(PK11SlotInfo *slot, PRBool loadCerts, void *wincx,
>-				PRBool contextSpecific);
>+SECStatus PK11_DoPassword(PK11SlotInfo *slot, CK_SESSION_HANDLE session,
>+			PRBool loadCerts, void *wincx, PRBool alreadyLocked,
>+			PRBool contextSpecific);
> SECStatus PK11_VerifyPW(PK11SlotInfo *slot,char *pw);
> void PK11_HandlePasswordCheck(PK11SlotInfo *slot,void *wincx);
> void PK11_SetVerifyPasswordFunc(PK11VerifyPasswordFunc func);
>@@ -121,7 +122,8 @@ CK_OBJECT_HANDLE PK11_MatchItem(PK11Slot
> 						CK_OBJECT_CLASS o_class); 
> CK_BBOOL PK11_HasAttributeSet( PK11SlotInfo *slot,
> 			       CK_OBJECT_HANDLE id,
>-			       CK_ATTRIBUTE_TYPE type );
>+			       CK_ATTRIBUTE_TYPE type,
>+			       PRBool haslock );
> CK_RV PK11_GetAttributes(PLArenaPool *arena,PK11SlotInfo *slot,
> 			 CK_OBJECT_HANDLE obj,CK_ATTRIBUTE *attr, int count);
> int PK11_NumberCertsForCertSubject(CERTCertificate *cert);

Flags:

emaldona: review+

Actions: View | Diff

Attachments on bug 872124: 637038 | 637191