390499 - lib pkix does not check cached cert chain for revocation. r=nelson
authoralexei.volkov.bugs%sun.com
Thu, 27 Sep 2007 23:41:28 +0000
changeset 8082 a739206bcd71a6f9e22d54ba28faa6318cb9c277
parent 8081 3dbe531539e9a1cff04abf1444055c41634f7535
child 8083 9dd0290178dae20a738fe91272b55b2c7eb16d97
push idunknown
push userunknown
push dateunknown
reviewersnelson
bugs390499
390499 - lib pkix does not check cached cert chain for revocation. r=nelson
security/nss/lib/libpkix/include/pkix_pl_system.h
security/nss/lib/libpkix/pkix/top/pkix_build.c
security/nss/lib/libpkix/pkix/util/pkix_tools.c
security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
--- a/security/nss/lib/libpkix/include/pkix_pl_system.h
+++ b/security/nss/lib/libpkix/include/pkix_pl_system.h
@@ -1305,16 +1305,19 @@ PKIX_PL_HashTable_Create(
 
 /*
  * FUNCTION: PKIX_PL_HashTable_Add
  * DESCRIPTION:
  *
  *  Adds a key/value mapping using the Objects pointed to by "key" and "value"
  *  to the Hashtable pointed to by "ht".
  *
+ *  Function increments key/value reference counts. Caller is responsible to
+ *  to decrement(destroy) key/value ref counts(objects). 
+ *
  * PARAMETERS:
  *  "ht"
  *      Address of Hashtable to be added to. Must be non-NULL.
  *  "key"
  *      Address of Object to be associated with "value". Must be non-NULL.
  *  "value"
  *      Address of Object to be added to Hashtable. Must be non-NULL.
  *  "plContext"
@@ -1337,16 +1340,19 @@ PKIX_PL_HashTable_Add(
 /*
  * FUNCTION: PKIX_PL_HashTable_Remove
  * DESCRIPTION:
  *
  *  Removes the Object value whose key is equal to the Object pointed to by
  *  "key" from the Hashtable pointed to by "ht". If no such object exists,
  *  this function throws an Error.
  *
+ *  Function frees "value" object. Caller is responsible to free "key"
+ *  object.
+ *
  * PARAMETERS:
  *  "ht"
  *      Address of Hashtable to remove object from. Must be non-NULL.
  *  "key"
  *      Address of Object used for lookup. Must be non-NULL.
  *  "plContext"
  *      Platform-specific context pointer.
  * THREAD SAFETY:
@@ -1560,9 +1566,9 @@ PKIX_PL_BigInt_Create(
         PKIX_PL_String *stringRep,
         PKIX_PL_BigInt **pBigInt,
         void *plContext);
 
 #ifdef __cplusplus
 }
 #endif
 
-#endif /* _LIBPKIX_SYSTEM_CALLBACKS_H */
+#endif /* _LIBPKIX_SYSTEM_H */
--- a/security/nss/lib/libpkix/pkix/top/pkix_build.c
+++ b/security/nss/lib/libpkix/pkix/top/pkix_build.c
@@ -1052,17 +1052,16 @@ pkix_Build_VerifyCertificate(
         PKIX_Boolean dsaParamsNeeded = PKIX_FALSE;
         PKIX_Boolean isSelfIssued = PKIX_FALSE;
         PKIX_Boolean supportForwardChecking = PKIX_FALSE;
         PKIX_Boolean trusted = PKIX_FALSE;
         PKIX_PL_Cert *candidateCert = NULL;
         PKIX_PL_PublicKey *candidatePubKey = NULL;
         PKIX_CertChainChecker *userChecker = NULL;
         PKIX_CertChainChecker_CheckCallback checkerCheck = NULL;
-        PKIX_PL_Cert *cert = NULL;
         PKIX_Error *verifyError = NULL;
         void *nbioContext = NULL;
 
         PKIX_ENTER(BUILD, "pkix_Build_VerifyCertificate");
         PKIX_NULLCHECK_THREE(state, pTrusted, pNeedsCRLChecking);
         PKIX_NULLCHECK_THREE
                 (state->candidateCerts, state->prevCert, state->trustChain);
 
@@ -1677,16 +1676,19 @@ pkix_Build_ValidateEntireChain(
                         PKIX_RETURN(FATAL);
                 }
                 if (verifyNode != NULL) {
                         PKIX_CHECK_FATAL(pkix_VerifyNode_SetError
                                 (verifyNode, certCheckError, plContext),
                                 PKIX_VERIFYNODESETERRORFAILED);
                 }
                 pkixErrorResult = certCheckError;
+                /* getting SEGV if pkixErrorMsg is not set. Will be fixed
+                 * with 390527 */
+                pkixErrorMsg = PKIX_ErrorText[PKIX_CHECKCHAINFAILED];
                 goto cleanup;
         }
                 
 
         if (state->reasonCode != 0) {
                 PKIX_ERROR(PKIX_CHAINREJECTEDBYREVOCATIONCHECKER);
         }
 
@@ -2432,17 +2434,16 @@ pkix_BuildForwardDepthFirstSearch(
         PKIX_ForwardBuilderState *childState = NULL;
         PKIX_ForwardBuilderState *parentState = NULL;
         PKIX_PL_Object *crlCheckerState = NULL;
         PKIX_PL_PublicKey *candidatePubKey = NULL;
         PKIX_PL_PublicKey *trustedPubKey = NULL;
         PKIX_ComCertSelParams *certSelParams = NULL;
         PKIX_TrustAnchor *trustAnchor = NULL;
         PKIX_PL_Cert *trustedCert = NULL;
-        PKIX_PL_Cert *cert = NULL;
         PKIX_VerifyNode *verifyNode = NULL;
         PKIX_Error *verifyError = NULL;
         void *nbio = NULL;
 
         PKIX_ENTER(BUILD, "pkix_BuildForwardDepthFirstSearch");
         PKIX_NULLCHECK_FOUR(pNBIOContext, pState, *pState, pValResult);
 
         nbio = *pNBIOContext;
@@ -3553,16 +3554,178 @@ cleanup:
         PKIX_DECREF(trustedPubKey);
         PKIX_DECREF(crlCheckerState);
         PKIX_DECREF(anchor);
 
         PKIX_RETURN(BUILD);
 }
 
 /*
+ * FUNCTION: pkix_Build_CheckInCache
+ * DESCRIPTION:
+ *
+ * The function tries to locate a chain for a cert in the cert chain cache.
+ * If found, the chain goes through revocation chacking and returned back to
+ * caller. Chains that fail revocation check get removed from cache.
+ * 
+ * PARAMETERS:
+ *  "state"
+ *      Address of ForwardBuilderState to be used. Must be non-NULL.
+ *  "pBuildResult"
+ *      Address at which the BuildResult is stored, after a successful build.
+ *      Must be non-NULL.
+ *  "pNBIOContext"
+ *      Address at which the NBIOContext is stored indicating whether the
+ *      validation is complete. Must be non-NULL.
+ *  "plContext"
+ *      Platform-specific context pointer.
+ * THREAD SAFETY:
+ *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
+ * RETURNS:
+ *  Returns NULL if the function succeeds.
+ *  Returns a Build Error if the function fails in a non-fatal way
+ *  Returns a Fatal Error if the function fails in an unrecoverable way.
+ */
+static PKIX_Error*
+pkix_Build_CheckInCache(
+        PKIX_ForwardBuilderState *state,
+        PKIX_BuildResult **pBuildResult,
+        void **pNBIOContext,
+        void *plContext)
+{
+        PKIX_PL_Cert *targetCert = NULL;
+        PKIX_List *anchors = NULL;
+        PKIX_PL_Date *testDate = NULL;
+        PKIX_BuildResult *buildResult = NULL;
+        PKIX_ValidateResult *valResult = NULL;
+        PKIX_TrustAnchor *matchingAnchor = NULL;
+        PKIX_PL_Cert *trustedCert = NULL;
+        PKIX_List *certList = NULL;
+        PKIX_Boolean cacheHit = PKIX_FALSE;
+        PKIX_Boolean trusted = PKIX_FALSE;
+        PKIX_Boolean stillValid = PKIX_FALSE;
+        void *nbioContext = NULL;
+
+        PKIX_ENTER(BUILD, "pkix_Build_CheckInCache");
+
+        nbioContext = *pNBIOContext;
+        *pNBIOContext = NULL;
+
+        targetCert = state->buildConstants.targetCert;
+        anchors = state->buildConstants.anchors;
+        testDate = state->buildConstants.testDate;
+
+        /* Check whether this cert verification has been cached. */
+        PKIX_CHECK(pkix_CacheCertChain_Lookup
+                   (targetCert,
+                    anchors,
+                    testDate,
+                    &cacheHit,
+                    &buildResult,
+                    plContext),
+                   PKIX_CACHECERTCHAINLOOKUPFAILED);
+        
+        if (!cacheHit) {
+            goto cleanup;
+        }
+        
+        /*
+         * We found something in cache. Verify that the anchor
+         * cert is still trusted,
+         */
+        PKIX_CHECK(PKIX_BuildResult_GetValidateResult
+                   (buildResult, &valResult, plContext),
+                   PKIX_BUILDRESULTGETVALIDATERESULTFAILED);
+        
+        PKIX_CHECK(PKIX_ValidateResult_GetTrustAnchor
+                       (valResult, &matchingAnchor, plContext),
+                   PKIX_VALIDATERESULTGETTRUSTANCHORFAILED);
+        
+        PKIX_DECREF(valResult);
+        
+        PKIX_CHECK(PKIX_TrustAnchor_GetTrustedCert
+                   (matchingAnchor, &trustedCert, plContext),
+                   PKIX_TRUSTANCHORGETTRUSTEDCERTFAILED);
+        
+        PKIX_CHECK(PKIX_PL_Cert_IsCertTrusted
+                   (trustedCert, &trusted, plContext),
+                   PKIX_CERTISCERTTRUSTEDFAILED);
+        
+        if (!trusted) {
+            goto cleanup;
+        }
+        /*
+         * Since the key usage may vary for different
+         * applications, we need to verify the chain again.
+         * Reverification will be improved with a fix for 397805.
+         */
+        PKIX_CHECK(PKIX_BuildResult_GetCertChain
+                   (buildResult, &certList, plContext),
+                   PKIX_BUILDRESULTGETCERTCHAINFAILED);
+        
+        /* setting this variable will trigger addition rev
+         * checker into cert chain checker list */
+        state->revCheckDelayed = PKIX_TRUE;
+        
+        PKIX_CHECK(pkix_Build_ValidationCheckers
+                   (state,
+                    certList,
+                    matchingAnchor,
+                    plContext),
+                   PKIX_BUILDVALIDATIONCHECKERSFAILED);
+        
+        state->revCheckDelayed = PKIX_FALSE;
+
+        PKIX_CHECK_ONLY_FATAL(
+            pkix_Build_ValidateEntireChain(state, matchingAnchor,
+                                           &nbioContext, &valResult,
+                                           state->verifyNode, plContext),
+            PKIX_BUILDVALIDATEENTIRECHAINFAILED);
+
+        if (nbioContext != NULL) {
+            /* IO still pending, resume later */
+            *pNBIOContext = nbioContext;
+            goto cleanup;
+        }
+        PKIX_DECREF(state->reversedCertChain);
+        PKIX_DECREF(state->checkedCritExtOIDs);
+        PKIX_DECREF(state->checkerChain);
+        PKIX_DECREF(state->revCheckers);
+        
+        if (!PKIX_ERROR_RECEIVED) {
+            /* The result from cache is still valid. */
+            *pBuildResult = buildResult;
+            buildResult = NULL;
+            stillValid = PKIX_TRUE;
+        }
+
+cleanup:
+
+        if (!nbioContext && cacheHit && !(trusted && stillValid)) {
+            /* The anchor of this chain is no longer trusted or
+             * chain cert(s) has been revoked.
+             * Invalidate this result in the cache */
+            PKIX_CHECK(pkix_CacheCertChain_Remove
+                       (targetCert,
+                        anchors,
+                        plContext),
+                       PKIX_CACHECERTCHAINREMOVEFAILED);
+        }
+
+       PKIX_DECREF(buildResult);
+       PKIX_DECREF(valResult);
+       PKIX_DECREF(certList);
+       PKIX_DECREF(matchingAnchor);
+       PKIX_DECREF(trustedCert);
+
+       
+       PKIX_RETURN(BUILD);
+}
+
+/*
  * FUNCTION: pkix_Build_InitiateBuildChain
  * DESCRIPTION:
  *
  *  This function initiates the search for a BuildChain, using the parameters
  *  provided in "procParams" and, if continuing a search that was suspended
  *  for I/O, using the ForwardBuilderState pointed to by "state".
  *
  *  If a successful chain is built, this function stores the BuildResult at
@@ -3614,18 +3777,16 @@ pkix_Build_InitiateBuildChain(
 {
         PKIX_UInt32 numAnchors = 0;
         PKIX_UInt32 numCertStores = 0;
         PKIX_UInt32 numHintCerts = 0;
         PKIX_UInt32 i = 0;
         PKIX_Boolean dsaParamsNeeded = PKIX_FALSE;
         PKIX_Boolean isCrlEnabled = PKIX_FALSE;
         PKIX_Boolean nistCRLPolicyEnabled = PKIX_TRUE;
-        PKIX_Boolean cacheHit = PKIX_FALSE;
-        PKIX_Boolean trusted = PKIX_FALSE;
         PKIX_Boolean isDuplicate = PKIX_FALSE;
         PKIX_PL_Cert *trustedCert = NULL;
         PKIX_CertSelector *targetConstraints = NULL;
         PKIX_ComCertSelParams *targetParams = NULL;
         PKIX_List *anchors = NULL;
         PKIX_List *targetSubjNames = NULL;
         PKIX_PL_Cert *targetCert = NULL;
         PKIX_PL_Object *firstHintCert = NULL;
@@ -3642,17 +3803,16 @@ pkix_Build_InitiateBuildChain(
         PKIX_List *tentativeChain = NULL;
         PKIX_ValidateResult *valResult = NULL;
         PKIX_BuildResult *buildResult = NULL;
         PKIX_List *certList = NULL;
         PKIX_TrustAnchor *matchingAnchor = NULL;
         PKIX_ForwardBuilderState *state = NULL;
         PKIX_CertStore_CheckTrustCallback trustCallback = NULL;
         PKIX_PL_AIAMgr *aiaMgr = NULL;
-        PKIX_VerifyNode *verifyNode = NULL;
 
         PKIX_ENTER(BUILD, "pkix_Build_InitiateBuildChain");
         PKIX_NULLCHECK_FOUR(procParams, pNBIOContext, pState, pBuildResult);
 
         nbioContext = *pNBIOContext;
         *pNBIOContext = NULL;
 
         if (*pState != NULL) {
@@ -3952,116 +4112,33 @@ pkix_Build_InitiateBuildChain(
                             (targetCert,
                             0,
                             NULL,
                             &(state->verifyNode),
                             plContext),
                             PKIX_VERIFYNODECREATEFAILED);
             }
 
-            /* Check whether this cert verification has been cached. */
-            PKIX_CHECK(pkix_CacheCertChain_Lookup
-                    (targetCert,
-                    anchors,
-                    testDate,
-                    &cacheHit,
-                    &buildResult,
-                    plContext),
-                    PKIX_CACHECERTCHAINLOOKUPFAILED);
-    
-            if (cacheHit) {
-                    /*
-                     * We found something in cache. Verify that the anchor
-                     * cert is still trusted,
-                     */
-                    PKIX_CHECK(PKIX_BuildResult_GetValidateResult
-                            (buildResult, &valResult, plContext),
-                            PKIX_BUILDRESULTGETVALIDATERESULTFAILED);
-    
-                    PKIX_CHECK(PKIX_ValidateResult_GetTrustAnchor
-                            (valResult, &matchingAnchor, plContext),
-                            PKIX_VALIDATERESULTGETTRUSTANCHORFAILED);
-    
-                    PKIX_DECREF(valResult);
-    
-                    PKIX_CHECK(PKIX_TrustAnchor_GetTrustedCert
-                            (matchingAnchor, &trustedCert, plContext),
-                            PKIX_TRUSTANCHORGETTRUSTEDCERTFAILED);
-    
-                    PKIX_CHECK(PKIX_PL_Cert_IsCertTrusted
-                            (trustedCert, &trusted, plContext),
-                            PKIX_CERTISCERTTRUSTEDFAILED);
-    
-                    if (trusted == PKIX_TRUE) {
-                            /*
-                             * Since the key usage may vary for different
-                             * applications, we need to verify the chain again.
-                             */
-                            PKIX_CHECK(PKIX_BuildResult_GetCertChain
-                                    (buildResult, &certList, plContext),
-                                    PKIX_BUILDRESULTGETCERTCHAINFAILED);
-    
-                            PKIX_CHECK(pkix_Build_ValidationCheckers
-                                    (state,
-                                    certList,
-                                    matchingAnchor,
-                                    plContext),
-                                    PKIX_BUILDVALIDATIONCHECKERSFAILED);
-    
-                            PKIX_CHECK_ONLY_FATAL(pkix_Build_ValidateEntireChain
-                                    (state,
-                                    matchingAnchor,
-                                    &nbioContext,
-                                    &valResult,
-                                    verifyNode,
-                                    plContext),
-                                    PKIX_BUILDVALIDATEENTIRECHAINFAILED);
-
-                        if (nbioContext != NULL) {
-                                /* IO still pending, resume later */
-                                *pNBIOContext = nbioContext;
-                                goto cleanup;
-                            } else {
-                                PKIX_DECREF(state->reversedCertChain);
-                                PKIX_DECREF(state->checkedCritExtOIDs);
-                                PKIX_DECREF(state->checkerChain);
-                                PKIX_DECREF(state->revCheckers);
-                                if (state->verifyNode != NULL && verifyNode) {
-                                    PKIX_CHECK_FATAL(pkix_VerifyNode_AddToTree
-                                        (state->verifyNode,
-                                        verifyNode,
-                                        plContext),
-                                        PKIX_VERIFYNODEADDTOTREEFAILED);
-                                    PKIX_DECREF(verifyNode);
-                                }
-
-                                if (!PKIX_ERROR_RECEIVED) {
-                                    /* The result from cache is still valid. */
-                                    *pBuildResult = buildResult;
-                                    if (pVerifyNode != NULL) {
-                                        PKIX_INCREF(state->verifyNode);
-                                        *pVerifyNode = state->verifyNode;
-                                    }
-                                    goto cleanup;
-                                }
-                            }
-    
-                    } else {
-                            /* The anchor of this chain is no longer trusted. */
-                            /* Invalidate this result in the cache */
-                            PKIX_CHECK(pkix_CacheCertChain_Remove
-                                    (targetCert,
-                                    anchors,
-                                    plContext),
-                                    PKIX_CACHECERTCHAINREMOVEFAILED);
-                    }
-                    PKIX_DECREF(certList);
-                    PKIX_DECREF(matchingAnchor);
-                    PKIX_DECREF(trustedCert);
-                    PKIX_DECREF(buildResult);
+            PKIX_CHECK(
+                pkix_Build_CheckInCache(state, &buildResult,
+                                        &nbioContext, plContext),
+                PKIX_UNABLETOBUILDCHAIN);
+            if (nbioContext) {
+                *pNBIOContext = nbioContext;
+                *pState = state;
+                state = NULL;
+                goto cleanup;
+            }
+            if (buildResult) {
+                *pBuildResult = buildResult;
+                if (pVerifyNode != NULL) {
+                    *pVerifyNode = state->verifyNode;
+                    state->verifyNode = NULL;
+                }
+                goto cleanup;
             }
         }
 
         /* If we're resuming after non-blocking I/O we need to get SubjNames */
         if (targetSubjNames == NULL) {
             PKIX_CHECK(PKIX_PL_Cert_GetAllSubjectNames
                     (state->buildConstants.targetCert,
                     &targetSubjNames,
--- a/security/nss/lib/libpkix/pkix/util/pkix_tools.c
+++ b/security/nss/lib/libpkix/pkix/util/pkix_tools.c
@@ -607,18 +607,16 @@ pkix_CacheCertChain_Lookup(
                 *pFound = PKIX_FALSE;
 
                 /* out-dated item, remove it from cache */
                 PKIX_CHECK(PKIX_PL_HashTable_Remove
                     (cachedCertChainTable,
                     (PKIX_PL_Object *) cachedKeys,
                     plContext),
                     PKIX_HASHTABLEREMOVEFAILED);
-                cachedKeys = NULL; /* object destroy by hash remove */
-                cachedValues = NULL; /* object destroy by hash remove */
             }
         }
 
 cleanup:
 
         PKIX_DECREF(cachedValues);
         PKIX_DECREF(cachedKeys);
         PKIX_DECREF(cachedCertChainError);
@@ -1007,18 +1005,16 @@ pkix_CacheCert_Lookup(
                             *pFound = PKIX_FALSE;
 
                             /* one cert is out-dated, remove item from cache */
                             PKIX_CHECK(PKIX_PL_HashTable_Remove
                                     (cachedCertTable,
                                     (PKIX_PL_Object *) cachedKeys,
                                     plContext),
                                     PKIX_HASHTABLEREMOVEFAILED);
-                           cachedKeys = NULL; /* object destroy by remove */
-                           cachedValues = NULL; /* object destroy by remove */
                             goto cleanup;
                         }
 
                         PKIX_CHECK(selectorMatch
                                     (certSel,
                                     cert,
                                     &certMatch,
                                     plContext),
@@ -1050,18 +1046,16 @@ pkix_CacheCert_Lookup(
                     pkix_cRemoveCount++;
                     *pFound = PKIX_FALSE;
                     /* cache item is out-dated, remove it from cache */
                     PKIX_CHECK(PKIX_PL_HashTable_Remove
                                 (cachedCertTable,
                                 (PKIX_PL_Object *) cachedKeys,
                                 plContext),
                                 PKIX_HASHTABLEREMOVEFAILED);
-                    cachedKeys = NULL; /* object destroy by hash remove */
-                    cachedValues = NULL; /* object destroy by hash remove */
                 }
 
         } 
 
 cleanup:
 
         PKIX_DECREF(subject);
         PKIX_DECREF(certSel);
--- a/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
+++ b/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
@@ -289,17 +289,16 @@ PKIX_PL_HashTable_Remove(
                 (void **)&result,
                 plContext),
                 PKIX_PRIMHASHTABLEREMOVEFAILED);
 
         PKIX_MUTEX_UNLOCK(ht->tableLock);
         tableLocked = PKIX_FALSE;
 
         if (result != NULL) {
-                PKIX_DECREF(key);
                 PKIX_DECREF(result);
         } else {
                 PKIX_ERROR(PKIX_ATTEMPTTOREMOVENONEXISTANTITEM);
         }
 
         /*
          * we don't call PKIX_PL_InvalidateCache here b/c we have
          * not implemented toString or hashcode for this Object