Bug 1672291 libpkix OCSP failures on SHA1 self-signed root certs when SHA1 signatures are disabled. r=mt

When libpkix is checking an OCSP cert, it can't use the passed in set of trust anchors as a base because only the single root that signed the leaf can sign the OCSP request. As a result it actually checks the signature of the self-signed root when processing an OCSP request. This fails of the root cert signature is invalid for any reason (including it's a sha1 self-signed root cert and we've disabled sha1 signatures (say, by policy)).

Further investigation indicates the difference between our classic code and the current code is the classic code only checks OCSP responses on leaf certs. In the real world, those responses are signed by intermediate certificates (who won't have sha1 signed certificates anymore), so our signature processing works just fine. pkix checks OCSP on the intermediate certificates as well, which are signed by the root cert. In this case the root cert is a chain of 1, and is effectively a leaf. This patch updates the OCSP response code to not check the signatures on the single cert if that cert is a selfsigned root cert. This requires bug 391476 so we still do the other validation checking on the certs (making sure it's trusted as a CA).

Differential Revision: https://phabricator.services.mozilla.com/D94661

4 files changed, 92 insertions, 1 deletions

diff --git a/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c b/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c
index 171427558..966c6f37e 100755
--- a/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c
+++ b/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c
@@ -54,6 +54,7 @@ PKIX_PL_NssContext_Create(
         context->crlReloadDelay = PKIX_DEFAULT_CRL_RELOAD_DELAY_SECONDS;
         context->badDerCrlReloadDelay =
                              PKIX_DEFAULT_BAD_CRL_RELOAD_DELAY_SECONDS;
+        context->certSignatureCheck = PKIX_TRUE;
         context->chainVerifyCallback.isChainValid = NULL;
         context->chainVerifyCallback.isChainValidArg = NULL;
         *pNssContext = context;
@@ -161,6 +162,75 @@ pkix_pl_NssContext_SetCertUsage(
 }
 
 /*
+ * FUNCTION: pkix_pl_NssContext_GetCertSignatureCheck
+ * DESCRIPTION:
+ *
+ *  This function obtains the platform-dependent flag to turn on or off
+ *  signature checks.
+ *
+ * PARAMETERS:
+ *  "nssContext"
+ *      The address of the context object whose wincx parameter is to be
+ *      obtained. Must be non-NULL.
+ *  "pCheckSig"
+ *      The address where the result is stored. Must be non-NULL.
+ * THREAD SAFETY:
+ *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
+ * RETURNS:
+ *  Returns NULL if the function succeeds.
+ *  Returns a Fatal Error if the function fails in an unrecoverable way.
+ */
+PKIX_Error *
+pkix_pl_NssContext_GetCertSignatureCheck(
+        PKIX_PL_NssContext *nssContext,
+        PKIX_Boolean *pCheckSig)
+{
+        void *plContext = NULL;
+
+        PKIX_ENTER(CONTEXT, "pkix_pl_NssContext_GetCertUsage");
+        PKIX_NULLCHECK_TWO(nssContext, pCheckSig);
+
+        *pCheckSig = nssContext->certSignatureCheck;
+
+        PKIX_RETURN(CONTEXT);
+}
+
+/*
+ * FUNCTION: pkix_pl_NssContext_SetCertSignatureCheck
+ * DESCRIPTION:
+ *
+ *  This function sets the check signature flag in
+ *  the context object pointed to by "nssContext" to the value provided in
+ *  "checkSig".
+ *
+ * PARAMETERS:
+ *  "checkSig"
+ *      Boolean that tells whether or not to check the signatues on certs.
+ *  "nssContext"
+ *      The address of the context object whose wincx parameter is to be
+ *      obtained. Must be non-NULL.
+ * THREAD SAFETY:
+ *  Thread Safe (see Thread Safety Definitions in Programmer's Guide)
+ * RETURNS:
+ *  Returns NULL if the function succeeds.
+ *  Returns a Fatal Error if the function fails in an unrecoverable way.
+ */
+PKIX_Error *
+pkix_pl_NssContext_SetCertSignatureCheck(
+        PKIX_Boolean checkSig,
+        PKIX_PL_NssContext *nssContext)
+{
+        void *plContext = NULL;
+
+        PKIX_ENTER(CONTEXT, "pkix_pl_NssContext_SetCertUsage");
+        PKIX_NULLCHECK_ONE(nssContext);
+
+        nssContext->certSignatureCheck =  checkSig;
+
+        PKIX_RETURN(CONTEXT);
+}
+
+/*
  * FUNCTION: pkix_pl_NssContext_GetWincx
  * DESCRIPTION:
  *
diff --git a/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h b/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h
index 04441c2b2..5de44214e 100755
--- a/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h
+++ b/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h
@@ -27,6 +27,7 @@ struct PKIX_PL_NssContextStruct {
         PRTime crlReloadDelay;
         PRTime badDerCrlReloadDelay;
         CERTChainVerifyCallback chainVerifyCallback;
+        PKIX_Boolean certSignatureCheck;
 };
 
 PKIX_Error *
@@ -39,6 +40,14 @@ pkix_pl_NssContext_SetCertUsage
         (SECCertificateUsage certUsage, PKIX_PL_NssContext *nssContext);
 
 PKIX_Error *
+pkix_pl_NssContext_GetCertSignatureCheck
+        (PKIX_PL_NssContext *nssContext, PKIX_Boolean *pCheckSig);
+
+PKIX_Error *
+pkix_pl_NssContext_SetCertSignatureCheck
+        (PKIX_Boolean checkSig, PKIX_PL_NssContext *nssContext);
+
+PKIX_Error *
 pkix_pl_NssContext_GetWincx(PKIX_PL_NssContext *nssContext, void **pWincx);
 
 /* XXX move the setter into the public header. */
diff --git a/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c b/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
index e5516505d..9dddd2e40 100644
--- a/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
+++ b/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
@@ -2811,14 +2811,23 @@ PKIX_PL_Cert_VerifySignature(
         PKIX_PL_Cert *cachedCert = NULL;
         PKIX_Error *verifySig = NULL;
         PKIX_Error *cachedSig = NULL;
+        PKIX_Error *checkSig = NULL;
         SECStatus status;
         PKIX_Boolean certEqual = PKIX_FALSE;
         PKIX_Boolean certInHash = PKIX_FALSE;
+        PKIX_Boolean checkCertSig = PKIX_TRUE;
         void* wincx = NULL;
 
         PKIX_ENTER(CERT, "PKIX_PL_Cert_VerifySignature");
         PKIX_NULLCHECK_THREE(cert, cert->nssCert, pubKey);
 
+        /* if the cert check flag is off, skip the check */
+        checkSig = pkix_pl_NssContext_GetCertSignatureCheck(
+                   (PKIX_PL_NssContext *)plContext, &checkCertSig);
+        if ((checkCertSig == PKIX_FALSE) && (checkSig == NULL)) {
+            goto cleanup;
+        }
+
         verifySig = PKIX_PL_HashTable_Lookup
                         (cachedCertSigTable,
                         (PKIX_PL_Object *) pubKey,
@@ -2879,6 +2888,7 @@ cleanup:
         }
 
         PKIX_DECREF(cachedCert);
+        PKIX_DECREF(checkSig);
         PKIX_DECREF(verifySig);
         PKIX_DECREF(cachedSig);
 
diff --git a/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c b/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
index fa5d6e9d5..fa41f8102 100644
--- a/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
+++ b/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
@@ -741,7 +741,9 @@ pkix_pl_OcspResponse_VerifyResponse(
             PKIX_CERTVERIFYKEYUSAGEFAILED);
         rv = SECSuccess;
     } else {
-        rv = CERT_VerifyCert(response->handle, response->signerCert, PKIX_TRUE,
+        /* checkSig is !isRoot */
+        PRBool checkSig = response->signerCert->isRoot ? PR_FALSE : PR_TRUE;
+        rv = CERT_VerifyCert(response->handle, response->signerCert, checkSig,
                              certUsage, response->producedAt, NULL, NULL);
         if (rv != SECSuccess) {
             PKIX_ERROR(PKIX_CERTVERIFYKEYUSAGEFAILED);