Bug 1682044 pkix_Build_GatherCerts() + pkix_CacheCert_Add() can corru…

…pt "cachedCertTable"

Patch by Andrew Cagney
Preliminary Review by Ryan Sleevie
Tested against all.sh rrelyea.
r=kjacobs

(this bug is old)

pkix_Build_GatherCerts() has two code paths for creating the list "certsFound":

    pkix_CacheCert_Lookup()
    this sets "certsFound" to a new list
    "certsFound" and "cachedCertTable" share items but not the list

    pkix_CacheCert_Add(pkix_pl_Pk11CertStore_CertQuery())
    this sets "certsFound" to a new list; and then adds the list to "cachedCertTable"
    "certsFound" and "cachedCertTable" share a linked list

Because the latter doesn't create a separate list, deleting list elements from "certsFound" can also delete list elements from within "cacheCertTable". And if this happens while pkix_CacheCert_Lookup() is trying to update the same element's reference, a core dump can result.

In detail (note that reference counts may occasionally seem off by 1, its because data is being captured before function local variables release their reference):

pkix_Build_GatherCerts() calls pkix_pl_Pk11CertStore_CertQuery() (via a pointer) to sets "certsFound":

                           PKIX_CHECK(getCerts
                                    (certStore,
                                    state->certSel,
                                    state->verifyNode,
                                    &nbioContext,
                                    &certsFound,
                                    plContext),
                                    PKIX_GETCERTSFAILED);

it then calls:

                            PKIX_CHECK(pkix_CacheCert_Add
                                    (certStore,
                                    certSelParams,
                                    certsFound,
                                    plContext),
                                    PKIX_CACHECERTADDFAILED);

lib/libpkix/pkix/util/pkix_tools.c

@@ -1163,6 +1163,7 @@ pkix_CacheCert_Add(
 {
         PKIX_List *cachedKeys = NULL;
         PKIX_List *cachedValues = NULL;
+        PKIX_List *cachedCerts = NULL;
         PKIX_PL_Date *cacheValidUntilDate = NULL;
         PKIX_PL_X500Name *subject = NULL;
         PKIX_Error *cachedCertError = NULL;
@@ -1219,9 +1220,12 @@ pkix_CacheCert_Add(
                 plContext),
                 PKIX_LISTAPPENDITEMFAILED);
 
+        PKIX_DUPLICATE(certs, &cachedCerts, plContext,
+                PKIX_OBJECTDUPLICATELISTFAILED);
+
         PKIX_CHECK(PKIX_List_AppendItem
                 (cachedValues,
-                (PKIX_PL_Object *)certs,
+                (PKIX_PL_Object *)cachedCerts,
                 plContext),
                 PKIX_LISTAPPENDITEMFAILED);
 
@@ -1243,6 +1247,7 @@ pkix_CacheCert_Add(
         PKIX_DECREF(subject);
         PKIX_DECREF(cachedKeys);
         PKIX_DECREF(cachedValues);
+        PKIX_DECREF(cachedCerts);
         PKIX_DECREF(cacheValidUntilDate);
         PKIX_DECREF(cachedCertError);