From 5c3685a18e45362bef8db24a3b6192309b3c20fd Mon Sep 17 00:00:00 2001 From: "relyea%netscape.com" Date: Mon, 3 Oct 2005 22:01:57 +0000 Subject: [PATCH] Bug 272484 Certificate manager crashes [@ _PR_MD_ATOMIC_DECREMENT - PK11_FreeSymKey] r=wtc [part 3 of 3] --- security/nss/lib/pkcs12/p12d.c | 57 +++++++++++++++-------------- security/nss/lib/pkcs12/p12e.c | 15 +++----- security/nss/lib/pkcs7/p7decode.c | 12 +----- security/nss/lib/pkcs7/p7local.c | 26 +++++++------ security/nss/lib/smime/cmscipher.c | 28 ++++++-------- security/nss/lib/smime/cmsencdata.c | 12 +----- security/nss/lib/smime/cmsenvdata.c | 12 +----- security/nss/lib/smime/cmsrecinfo.c | 13 +++++-- 8 files changed, 73 insertions(+), 102 deletions(-) diff --git a/security/nss/lib/pkcs12/p12d.c b/security/nss/lib/pkcs12/p12d.c index 6d91d5347459..2d4d29cae396 100644 --- a/security/nss/lib/pkcs12/p12d.c +++ b/security/nss/lib/pkcs12/p12d.c @@ -110,7 +110,6 @@ struct SEC_PKCS12DecoderContextStr { /* state variables for decoding authenticated safes. */ SEC_PKCS7DecoderContext *currentASafeP7Dcx; - SEC_PKCS5KeyAndPassword *currentASafeKeyPwd; SEC_ASN1DecoderContext *aSafeDcx; SEC_PKCS7DecoderContext *aSafeP7Dcx; sec_PKCS12AuthenticatedSafe authSafe; @@ -173,25 +172,43 @@ sec_pkcs12_proper_version(sec_PKCS12PFXItem *pfx) static PK11SymKey * sec_pkcs12_decoder_get_decrypt_key(void *arg, SECAlgorithmID *algid) { - SEC_PKCS5KeyAndPassword *keyPwd = - (SEC_PKCS5KeyAndPassword *)arg; + SEC_PKCS12DecoderContext *p12dcx = + (SEC_PKCS12DecoderContext *) arg; + PK11SlotInfo *slot; + PK11SymKey *bulkKey; - if(!keyPwd) { + if(!p12dcx) { return NULL; } /* if no slot specified, use the internal key slot */ - if(!keyPwd->slot) { - keyPwd->slot = PK11_GetInternalKeySlot(); + if(p12dcx->slot) { + slot = PK11_ReferenceSlot(p12dcx->slot); + } else { + slot = PK11_GetInternalKeySlot(); } - /* retrieve the key */ - if(!keyPwd->key) { - keyPwd->key = PK11_PBEKeyGen(keyPwd->slot, algid, - keyPwd->pwitem, PR_FALSE, keyPwd->wincx); + bulkKey = PK11_PBEKeyGen(slot, algid, p12dcx->pwitem, + PR_FALSE, p12dcx->wincx); + /* some tokens can't generate PBE keys on their own, generate the + * key in the internal slot, and let the Import code deal with it, + * (if the slot can't generate PBEs, then we need to use the internal + * slot anyway to unwrap). */ + if (!bulkKey && !PK11_IsInternal(slot)) { + PK11_FreeSlot(slot); + slot = PK11_GetInternalKeySlot(); + bulkKey = PK11_PBEKeyGen(slot, algid, p12dcx->pwitem, + PR_FALSE, p12dcx->wincx); + } + PK11_FreeSlot(slot); + + /* set the password data on the key */ + if (bulkKey) { + PK11_SetSymKeyUserData(bulkKey,p12dcx->pwitem, NULL); } - return (PK11SymKey *)keyPwd; + + return bulkKey; } /* XXX this needs to be modified to handle enveloped data. most @@ -778,25 +795,12 @@ sec_pkcs12_decoder_asafes_notify(void *arg, PRBool before, void *dest, goto loser; } - /* set up password and encryption key information */ - p12dcx->currentASafeKeyPwd = - (SEC_PKCS5KeyAndPassword*)PORT_ArenaZAlloc(p12dcx->arena, - sizeof(SEC_PKCS5KeyAndPassword)); - if(!p12dcx->currentASafeKeyPwd) { - p12dcx->errorValue = SEC_ERROR_NO_MEMORY; - goto loser; - } - p12dcx->currentASafeKeyPwd->pwitem = p12dcx->pwitem; - p12dcx->currentASafeKeyPwd->slot = p12dcx->slot; - p12dcx->currentASafeKeyPwd->wincx = p12dcx->wincx; - /* initiate the PKCS7ContentInfo decode */ p12dcx->currentASafeP7Dcx = SEC_PKCS7DecoderStart( sec_pkcs12_decoder_safe_contents_callback, safeContentsCtx, p12dcx->pwfn, p12dcx->pwfnarg, - sec_pkcs12_decoder_get_decrypt_key, - p12dcx->currentASafeKeyPwd, + sec_pkcs12_decoder_get_decrypt_key, p12dcx, sec_pkcs12_decoder_decryption_allowed); if(!p12dcx->currentASafeP7Dcx) { p12dcx->errorValue = PORT_GetError(); @@ -818,9 +822,6 @@ sec_pkcs12_decoder_asafes_notify(void *arg, PRBool before, void *dest, p12dcx->currentASafeP7Dcx = NULL; } p12dcx->currentASafeP7Dcx = NULL; - if(p12dcx->currentASafeKeyPwd->key != NULL) { - p12dcx->currentASafeKeyPwd->key = NULL; - } } diff --git a/security/nss/lib/pkcs12/p12e.c b/security/nss/lib/pkcs12/p12e.c index 650fd175cf03..f0fbbcb2e764 100644 --- a/security/nss/lib/pkcs12/p12e.c +++ b/security/nss/lib/pkcs12/p12e.c @@ -1901,10 +1901,9 @@ sec_pkcs12_encoder_asafe_process(sec_PKCS12EncoderContext *p12ecx) { SEC_PKCS7EncoderContext *innerP7ecx; SEC_PKCS7ContentInfo *cinfo; - void *arg = NULL; + PK11SymKey *bulkKey = NULL; SEC_ASN1EncoderContext *innerA1ecx = NULL; SECStatus rv = SECSuccess; - SEC_PKCS5KeyAndPassword keyPwd; if(p12ecx->currentSafe < p12ecx->p12exp->authSafe.safeCount) { SEC_PKCS12SafeInfo *safeInfo; @@ -1923,15 +1922,11 @@ sec_pkcs12_encoder_asafe_process(sec_PKCS12EncoderContext *p12ecx) /* determine the safe type and set the appropriate argument */ switch(cinfoType) { case SEC_OID_PKCS7_DATA: - arg = NULL; + case SEC_OID_PKCS7_ENVELOPED_DATA: break; case SEC_OID_PKCS7_ENCRYPTED_DATA: - keyPwd.pwitem = &safeInfo->pwitem; - keyPwd.key = safeInfo->encryptionKey; - arg = &keyPwd; - break; - case SEC_OID_PKCS7_ENVELOPED_DATA: - arg = NULL; + bulkKey = safeInfo->encryptionKey; + PK11_SetSymKeyUserData(bulkKey, &safeInfo->pwitem, NULL); break; default: return SECFailure; @@ -1941,7 +1936,7 @@ sec_pkcs12_encoder_asafe_process(sec_PKCS12EncoderContext *p12ecx) /* start the PKCS7 encoder */ innerP7ecx = SEC_PKCS7EncoderStart(cinfo, sec_P12P7OutputCB_CallA1Update, - p12ecx->middleA1ecx, (PK11SymKey *)arg); + p12ecx->middleA1ecx, bulkKey); if(!innerP7ecx) { goto loser; } diff --git a/security/nss/lib/pkcs7/p7decode.c b/security/nss/lib/pkcs7/p7decode.c index 797527338c44..735e8c106626 100644 --- a/security/nss/lib/pkcs7/p7decode.c +++ b/security/nss/lib/pkcs7/p7decode.c @@ -38,7 +38,7 @@ /* * PKCS7 decoding, verification. * - * $Id: p7decode.c,v 1.19 2005/09/02 01:24:56 wtchang%redhat.com Exp $ + * $Id: p7decode.c,v 1.20 2005/10/03 22:01:56 relyea%netscape.com Exp $ */ #include "nssrenam.h" @@ -691,16 +691,6 @@ sec_pkcs7_decoder_start_decrypt (SEC_PKCS7DecoderContext *p7dcx, int depth, decryptobj = sec_PKCS7CreateDecryptObject (bulkkey, &(enccinfo->contentEncAlg)); - /* - * For PKCS5 Encryption Algorithms, the bulkkey is actually a different - * structure. Therefore, we need to set the bulkkey to the actual key - * prior to freeing it. - */ - if ( SEC_PKCS5IsAlgorithmPBEAlg(&(enccinfo->contentEncAlg)) && bulkkey ) { - SEC_PKCS5KeyAndPassword *keyPwd = (SEC_PKCS5KeyAndPassword *)bulkkey; - bulkkey = keyPwd->key; - } - /* * We are done with (this) bulkkey now. */ diff --git a/security/nss/lib/pkcs7/p7local.c b/security/nss/lib/pkcs7/p7local.c index c697f406a911..bc03271768ce 100644 --- a/security/nss/lib/pkcs7/p7local.c +++ b/security/nss/lib/pkcs7/p7local.c @@ -40,7 +40,7 @@ * encoding/creation side *and* the decoding/decryption side. Anything * else should be static routines in the appropriate file. * - * $Id: p7local.c,v 1.7 2004/04/25 15:03:13 gerv%gerv.net Exp $ + * $Id: p7local.c,v 1.8 2005/10/03 22:01:56 relyea%netscape.com Exp $ */ #include "p7local.h" @@ -118,11 +118,12 @@ sec_PKCS7CreateDecryptObject (PK11SymKey *key, SECAlgorithmID *algid) if (SEC_PKCS5IsAlgorithmPBEAlg(algid)) { CK_MECHANISM pbeMech, cryptoMech; SECItem *pbeParams, *pwitem; - SEC_PKCS5KeyAndPassword *keyPwd; - keyPwd = (SEC_PKCS5KeyAndPassword *)key; - key = keyPwd->key; - pwitem = keyPwd->pwitem; + pwitem = (SECItem *)PK11_GetSymKeyUserData(key); + if (!pwitem) { + PORT_Free(result); + return NULL; + } pbeMech.mechanism = PK11_AlgtagToMechanism(algtag); pbeParams = PK11_ParamFromAlgid(algid); @@ -211,25 +212,28 @@ sec_PKCS7CreateEncryptObject (PRArenaPool *poolp, PK11SymKey *key, ciphercx = NULL; if (SEC_PKCS5IsAlgorithmPBEAlg(algid)) { CK_MECHANISM pbeMech, cryptoMech; - SECItem *pbeParams; - SEC_PKCS5KeyAndPassword *keyPwd; + SECItem *pbeParams, *pwitem; PORT_Memset(&pbeMech, 0, sizeof(CK_MECHANISM)); PORT_Memset(&cryptoMech, 0, sizeof(CK_MECHANISM)); + pwitem = (SECItem *)PK11_GetSymKeyUserData(key); + if (!pwitem) { + PORT_Free(result); + return NULL; + } + pbeMech.mechanism = PK11_AlgtagToMechanism(algtag); pbeParams = PK11_ParamFromAlgid(algid); if(!pbeParams) { PORT_Free(result); return NULL; } - keyPwd = (SEC_PKCS5KeyAndPassword *)key; - key = keyPwd->key; pbeMech.pParameter = pbeParams->data; pbeMech.ulParameterLen = pbeParams->len; - if(PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, - keyPwd->pwitem, PR_FALSE) != CKR_OK) { + if(PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, pwitem, + PR_FALSE) != CKR_OK) { PORT_Free(result); SECITEM_ZfreeItem(pbeParams, PR_TRUE); return NULL; diff --git a/security/nss/lib/smime/cmscipher.c b/security/nss/lib/smime/cmscipher.c index 1ab29e88cc17..2299b506f861 100644 --- a/security/nss/lib/smime/cmscipher.c +++ b/security/nss/lib/smime/cmscipher.c @@ -37,7 +37,7 @@ /* * Encryption/decryption routines for CMS implementation, none of which are exported. * - * $Id: cmscipher.c,v 1.7 2004/04/25 15:03:16 gerv%gerv.net Exp $ + * $Id: cmscipher.c,v 1.8 2005/10/03 22:01:57 relyea%netscape.com Exp $ */ #include "cmslocal.h" @@ -93,17 +93,14 @@ NSS_CMSCipherContext_StartDecrypt(PK11SymKey *key, SECAlgorithmID *algid) /* set param and mechanism */ if (SEC_PKCS5IsAlgorithmPBEAlg(algid)) { CK_MECHANISM pbeMech, cryptoMech; - SECItem *pbeParams; - SEC_PKCS5KeyAndPassword *keyPwd; + SECItem *pbeParams, *pwitem; PORT_Memset(&pbeMech, 0, sizeof(CK_MECHANISM)); PORT_Memset(&cryptoMech, 0, sizeof(CK_MECHANISM)); - /* HACK ALERT! - * in this case, key is not actually a PK11SymKey *, but a SEC_PKCS5KeyAndPassword * - */ - keyPwd = (SEC_PKCS5KeyAndPassword *)key; - key = keyPwd->key; + pwitem = PK11_GetSymKeyUserData(key); + if (!pwitem) + return NULL; /* find correct PK11 mechanism and parameters to initialize pbeMech */ pbeMech.mechanism = PK11_AlgtagToMechanism(algtag); @@ -114,7 +111,7 @@ NSS_CMSCipherContext_StartDecrypt(PK11SymKey *key, SECAlgorithmID *algid) pbeMech.ulParameterLen = pbeParams->len; /* now map pbeMech to cryptoMech */ - if (PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, keyPwd->pwitem, + if (PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, pwitem, PR_FALSE) != CKR_OK) { SECITEM_ZfreeItem(pbeParams, PR_TRUE); return NULL; @@ -187,17 +184,14 @@ NSS_CMSCipherContext_StartEncrypt(PRArenaPool *poolp, PK11SymKey *key, SECAlgori /* set param and mechanism */ if (SEC_PKCS5IsAlgorithmPBEAlg(algid)) { CK_MECHANISM pbeMech, cryptoMech; - SECItem *pbeParams; - SEC_PKCS5KeyAndPassword *keyPwd; + SECItem *pbeParams, *pwitem; PORT_Memset(&pbeMech, 0, sizeof(CK_MECHANISM)); PORT_Memset(&cryptoMech, 0, sizeof(CK_MECHANISM)); - /* HACK ALERT! - * in this case, key is not actually a PK11SymKey *, but a SEC_PKCS5KeyAndPassword * - */ - keyPwd = (SEC_PKCS5KeyAndPassword *)key; - key = keyPwd->key; + pwitem = PK11_GetSymKeyUserData(key); + if (!pwitem) + return NULL; /* find correct PK11 mechanism and parameters to initialize pbeMech */ pbeMech.mechanism = PK11_AlgtagToMechanism(algtag); @@ -208,7 +202,7 @@ NSS_CMSCipherContext_StartEncrypt(PRArenaPool *poolp, PK11SymKey *key, SECAlgori pbeMech.ulParameterLen = pbeParams->len; /* now map pbeMech to cryptoMech */ - if (PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, keyPwd->pwitem, + if (PK11_MapPBEMechanismToCryptoMechanism(&pbeMech, &cryptoMech, pwitem, PR_FALSE) != CKR_OK) { SECITEM_ZfreeItem(pbeParams, PR_TRUE); return NULL; diff --git a/security/nss/lib/smime/cmsencdata.c b/security/nss/lib/smime/cmsencdata.c index e05fbd48c180..05079d2318c1 100644 --- a/security/nss/lib/smime/cmsencdata.c +++ b/security/nss/lib/smime/cmsencdata.c @@ -37,7 +37,7 @@ /* * CMS encryptedData methods. * - * $Id: cmsencdata.c,v 1.7 2004/04/25 15:03:16 gerv%gerv.net Exp $ + * $Id: cmsencdata.c,v 1.8 2005/10/03 22:01:57 relyea%netscape.com Exp $ */ #include "cmslocal.h" @@ -245,16 +245,6 @@ NSS_CMSEncryptedData_Decode_BeforeData(NSSCMSEncryptedData *encd) if (cinfo->ciphcx == NULL) goto loser; /* error has been set by NSS_CMSCipherContext_StartDecrypt */ - /* - * HACK ALERT!! - * For PKCS5 Encryption Algorithms, the bulkkey is actually a different - * structure. Therefore, we need to set the bulkkey to the actual key - * prior to freeing it. - */ - if (SEC_PKCS5IsAlgorithmPBEAlg(bulkalg)) { - SEC_PKCS5KeyAndPassword *keyPwd = (SEC_PKCS5KeyAndPassword *)bulkkey; - bulkkey = keyPwd->key; - } /* we are done with (this) bulkkey now. */ PK11_FreeSymKey(bulkkey); diff --git a/security/nss/lib/smime/cmsenvdata.c b/security/nss/lib/smime/cmsenvdata.c index 71fabdb9fbba..718844cd7084 100644 --- a/security/nss/lib/smime/cmsenvdata.c +++ b/security/nss/lib/smime/cmsenvdata.c @@ -37,7 +37,7 @@ /* * CMS envelopedData methods. * - * $Id: cmsenvdata.c,v 1.10 2004/04/25 15:03:16 gerv%gerv.net Exp $ + * $Id: cmsenvdata.c,v 1.11 2005/10/03 22:01:57 relyea%netscape.com Exp $ */ #include "cmslocal.h" @@ -384,16 +384,6 @@ NSS_CMSEnvelopedData_Decode_BeforeData(NSSCMSEnvelopedData *envd) if (cinfo->ciphcx == NULL) goto loser; /* error has been set by NSS_CMSCipherContext_StartDecrypt */ - /* - * HACK ALERT!! - * For PKCS5 Encryption Algorithms, the bulkkey is actually a different - * structure. Therefore, we need to set the bulkkey to the actual key - * prior to freeing it. - */ - if (SEC_PKCS5IsAlgorithmPBEAlg(bulkalg)) { - SEC_PKCS5KeyAndPassword *keyPwd = (SEC_PKCS5KeyAndPassword *)bulkkey; - bulkkey = keyPwd->key; - } rv = SECSuccess; diff --git a/security/nss/lib/smime/cmsrecinfo.c b/security/nss/lib/smime/cmsrecinfo.c index 9ca034c859cf..1804d5411ff9 100644 --- a/security/nss/lib/smime/cmsrecinfo.c +++ b/security/nss/lib/smime/cmsrecinfo.c @@ -37,7 +37,7 @@ /* * CMS recipientInfo methods. * - * $Id: cmsrecinfo.c,v 1.15 2005/09/16 17:52:37 wtchang%redhat.com Exp $ + * $Id: cmsrecinfo.c,v 1.16 2005/10/03 22:01:57 relyea%netscape.com Exp $ */ #include "cmslocal.h" @@ -63,8 +63,15 @@ nss_cmsrecipientinfo_usessubjectkeyid(NSSCMSRecipientInfo *ri) return PR_FALSE; } - -static SECOidData fakeContent = { 0 }; +/* + * NOTE: fakeContent marks CMSMessage structure which is only used as a carrier + * of pwfn_arg and arena pools. In an ideal world, NSSCMSMessage would not have + * been exported, and we would have added an ordinary enum to handle this + * check. Unfortunatly wo don't have that luxury so we are overloading the + * contentTypeTag field. NO code should every try to interpret this content tag + * as a real OID tag, or use any fields other than pwfn_arg or poolp of this + * CMSMessage for that matter */ +static const SECOidData fakeContent; NSSCMSRecipientInfo * nss_cmsrecipientinfo_create(NSSCMSMessage *cmsg, NSSCMSRecipientIDSelector type, CERTCertificate *cert, SECKEYPublicKey *pubKey,