When storing new CRL, Find old CRL and if it can be decoded, delete it.

Bug 363749. r=wtchang,alexei.volkov

2 files changed, 51 insertions, 41 deletions

diff --git a/security/nss/lib/certdb/crl.c b/security/nss/lib/certdb/crl.c
index e3bc50309..0c8bcc505 100644
--- a/security/nss/lib/certdb/crl.c
+++ b/security/nss/lib/certdb/crl.c
@@ -463,13 +463,21 @@ CERT_DecodeDERCrlWithFlags(PRArenaPool *narena, SECItem *derSignedCrl,
     SECStatus rv;
     OpaqueCRLFields* extended = NULL;
     const SEC_ASN1Template* crlTemplate = CERT_SignedCrlTemplate;
+    PRInt32 testOptions = options;
 
-    if (!derSignedCrl ||
-        ( (options & CRL_DECODE_ADOPT_HEAP_DER) && /* adopting DER requires
-                                                      not copying it */
-          (!(options & CRL_DECODE_DONT_COPY_DER))
-        )
-       ) {
+    PORT_Assert(derSignedCrl);
+    if (!derSignedCrl) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return NULL;
+    }
+
+    /* Adopting DER requires not copying it.  Code that sets ADOPT flag 
+     * but doesn't set DONT_COPY probably doesn't know What it is doing.  
+     * That condition is a programming error in the caller.
+     */
+    testOptions &= (CRL_DECODE_ADOPT_HEAP_DER | CRL_DECODE_DONT_COPY_DER);
+    PORT_Assert(testOptions != CRL_DECODE_ADOPT_HEAP_DER);
+    if (testOptions == CRL_DECODE_ADOPT_HEAP_DER) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return NULL;
     }
@@ -601,8 +609,9 @@ CERT_DecodeDERCrl(PRArenaPool *narena, SECItem *derSignedCrl, int type)
  *  caching stuff used by certificates....?
  * return values :
  *
- * SECSuccess means we got a valid DER CRL (passed in "decoded"), or no CRL at
- * all
+ * SECSuccess means we got a valid decodable DER CRL, or no CRL at all.
+ * Caller may distinguish those cases by the value returned in "decoded".
+ * When DER CRL is not found, error code will be SEC_ERROR_CRL_NOT_FOUND.
  *
  * SECFailure means we got a fatal error - most likely, we found a CRL,
  * and it failed decoding, or there was an out of memory error. Do NOT ignore
@@ -620,7 +629,6 @@ SEC_FindCrlByKeyOnSlot(PK11SlotInfo *slot, SECItem *crlKey, int type,
     SECItem *derCrl = NULL;
     CK_OBJECT_HANDLE crlHandle = 0;
     char *url = NULL;
-    int nsserror;
 
     PORT_Assert(decoded);
     if (!decoded) {
@@ -628,15 +636,12 @@ SEC_FindCrlByKeyOnSlot(PK11SlotInfo *slot, SECItem *crlKey, int type,
         return SECFailure;
     }
 
-    /* XXX it would be really useful to be able to fetch the CRL directly into
-       an arena. This would avoid a copy later on in the decode step */
-    PORT_SetError(0);
     derCrl = PK11_FindCrlByName(&slot, &crlHandle, crlKey, type, &url);
     if (derCrl == NULL) {
 	/* if we had a problem other than the CRL just didn't exist, return
 	 * a failure to the upper level */
-	nsserror = PORT_GetError();
-	if ((nsserror != 0) && (nsserror != SEC_ERROR_CRL_NOT_FOUND)) {
+	int nsserror = PORT_GetError();
+	if (nsserror != SEC_ERROR_CRL_NOT_FOUND) {
 	    rv = SECFailure;
 	}
 	goto loser;
@@ -644,15 +649,16 @@ SEC_FindCrlByKeyOnSlot(PK11SlotInfo *slot, SECItem *crlKey, int type,
     PORT_Assert(crlHandle != CK_INVALID_HANDLE);
     /* PK11_FindCrlByName obtained a slot reference. */
     
-    if (!(decodeoptions & CRL_DECODE_DONT_COPY_DER) ) {
-        /* force adoption of the DER from the heap - this will cause it to be
-           automatically freed when SEC_DestroyCrl is invoked */
-        decodeoptions |= CRL_DECODE_ADOPT_HEAP_DER;
-    }
+    /* derCRL is a fresh HEAP copy made for us by PK11_FindCrlByName.
+       Force adoption of the DER CRL from the heap - this will cause it 
+       to be automatically freed when SEC_DestroyCrl is invoked */
+    decodeoptions |= (CRL_DECODE_ADOPT_HEAP_DER | CRL_DECODE_DONT_COPY_DER);
+
     crl = CERT_DecodeDERCrlWithFlags(NULL, derCrl, type, decodeoptions);
     if (crl) {
         crl->slot = slot;
         slot = NULL; /* adopt it */
+	derCrl = NULL; /* adopted by the crl struct */
         crl->pkcs11ID = crlHandle;
         if (url) {
             crl->url = PORT_ArenaStrdup(crl->arena,url);
@@ -671,10 +677,7 @@ SEC_FindCrlByKeyOnSlot(PK11SlotInfo *slot, SECItem *crlKey, int type,
 
 loser:
     if (derCrl) {
-        /* destroy the DER if it was copied to the CRL */
-        if (crl && (!(decodeoptions & CRL_DECODE_DONT_COPY_DER)) ) {
-            SECITEM_FreeItem(derCrl, PR_TRUE);
-        }
+	SECITEM_FreeItem(derCrl, PR_TRUE);
     }
 
     *decoded = crl;
@@ -682,7 +685,6 @@ loser:
     return rv;
 }
 
-SECStatus SEC_DestroyCrl(CERTSignedCrl *crl);
 
 CERTSignedCrl *
 crl_storeCRL (PK11SlotInfo *slot,char *url,
@@ -691,15 +693,15 @@ crl_storeCRL (PK11SlotInfo *slot,char *url,
     CERTSignedCrl *oldCrl = NULL, *crl = NULL;
     PRBool deleteOldCrl = PR_FALSE;
     CK_OBJECT_HANDLE crlHandle = CK_INVALID_HANDLE;
+    SECStatus rv;
 
     PORT_Assert(newCrl);
     PORT_Assert(derCrl);
 
     /* we can't use the cache here because we must look in the same
        token */
-    SEC_FindCrlByKeyOnSlot(slot, &newCrl->crl.derName, type,
+    rv = SEC_FindCrlByKeyOnSlot(slot, &newCrl->crl.derName, type,
                                 &oldCrl, CRL_DECODE_SKIP_ENTRIES);
-
     /* if there is an old crl on the token, make sure the one we are
        installing is newer. If not, exit out, otherwise delete the
        old crl.
diff --git a/security/nss/lib/pk11wrap/pk11nobj.c b/security/nss/lib/pk11wrap/pk11nobj.c
index 3fac66128..3a88ef4ae 100644
--- a/security/nss/lib/pk11wrap/pk11nobj.c
+++ b/security/nss/lib/pk11wrap/pk11nobj.c
@@ -429,12 +429,15 @@ SECStatus pk11_RetrieveCrls(CERTCrlHeadNode *nodes, SECItem* issuer,
  */
 SECItem *
 PK11_FindCrlByName(PK11SlotInfo **slot, CK_OBJECT_HANDLE *crlHandle,
-					 SECItem *name, int type, char **url)
+					 SECItem *name, int type, char **pUrl)
 {
-    NSSCRL **crls, **crlp, *crl;
+    NSSCRL **crls, **crlp, *crl = NULL;
     NSSDER subject;
     SECItem *rvItem;
     NSSTrustDomain *td = STAN_GetDefaultTrustDomain();
+    char * url = NULL;
+
+    PORT_SetError(0);
     NSSITEM_FROM_SECITEM(&subject, name);
     if (*slot) {
 	nssCryptokiObject **instances;
@@ -443,7 +446,7 @@ PK11_FindCrlByName(PK11SlotInfo **slot, CK_OBJECT_HANDLE *crlHandle,
 	NSSToken *token = PK11Slot_GetNSSToken(*slot);
 	collection = nssCRLCollection_Create(td, NULL);
 	if (!collection) {
-	    return NULL;
+	    goto loser;
 	}
 	instances = nssToken_FindCRLsBySubject(token, NULL, &subject, 
 	                                       tokenOnly, 0, NULL);
@@ -461,9 +464,8 @@ PK11_FindCrlByName(PK11SlotInfo **slot, CK_OBJECT_HANDLE *crlHandle,
 	if (NSS_GetError() == NSS_ERROR_NOT_FOUND) {
 	    PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
 	}
-	return NULL;
+	goto loser;
     }
-    crl = NULL;
     for (crlp = crls; *crlp; crlp++) {
 	if ((!(*crlp)->isKRL && type == SEC_CRL_TYPE) ||
 	    ((*crlp)->isKRL && type != SEC_CRL_TYPE)) 
@@ -477,28 +479,34 @@ PK11_FindCrlByName(PK11SlotInfo **slot, CK_OBJECT_HANDLE *crlHandle,
 	/* CRL collection was found, but no interesting CRL's were on it.
 	 * Not an error */
 	PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
-	return NULL;
+	goto loser;
     }
     if (crl->url) {
-	*url = PORT_Strdup(crl->url);
-	if (!*url) {
-	    nssCRL_Destroy(crl);
-	    return NULL;
+	url = PORT_Strdup(crl->url);
+	if (!url) {
+	    goto loser;
 	}
-    } else {
-	*url = NULL;
     }
     rvItem = SECITEM_AllocItem(NULL, NULL, crl->encoding.size);
     if (!rvItem) {
-	PORT_Free(*url);
-	nssCRL_Destroy(crl);
-	return NULL;
+	goto loser;
     }
     memcpy(rvItem->data, crl->encoding.data, crl->encoding.size);
     *slot = PK11_ReferenceSlot(crl->object.instances[0]->token->pk11slot);
     *crlHandle = crl->object.instances[0]->handle;
+    *pUrl = url;
     nssCRL_Destroy(crl);
     return rvItem;
+
+loser:
+    if (url)
+    	PORT_Free(url);
+    if (crl)
+	nssCRL_Destroy(crl);
+    if (PORT_GetError() == 0) {
+	PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
+    }
+    return NULL;
 }
 
 CK_OBJECT_HANDLE