551429 - (Only when using libpkix and CERT_SetUsePKIXForValidation(PR_TRUE)): Stack overflow (infinite recursion) during revocation checking of OCSP response signing cert

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Kai Engert (:KaiE:)]

It appears there exist CA or OCSP responder certificates that rely on each in a circular dependency.

According to https://bugzilla.redhat.com/show_bug.cgi?id=562929
there exists such a dependency between
  http://ocsp.usertrust.com
and
  http://ocsp.comodoca.com

Should NSS detect that scenario, which leads to recursive verification attempts?

Comment 1

[Kai Engert (:KaiE:)]

The installed NSS version has probably been 3.12.4 or 3.12.5

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

In https://bugzilla.redhat.com/show_bug.cgi?id=562929#c5
Kai wrote
> We attempt to verify the signature on an OCSP response
>   CERT_VerifyOCSPResponseSignature
> which calls into
>   CERT_VerifyCert 
> probably to verify the cert that produced the signature,
> but this verification triggers another call to
>   CERT_CheckOCSPStatus
> then
>   ocsp_GetOCSPStatusFromNetwork
> then again going to
>   CERT_VerifyOCSPResponseSignature
> which causes us to repeat the same thing using the other cert.
> 
> When having a big recursive stack, we eventually crash while trying to 
> decode an OCSP response.

However, in CERT_VerifyCert, we read this comment, just prior to the 
call to the OCSP check:

1494      * Check revocation status, but only if the cert we are checking
1495      * is not a status reponder itself. 

The test is

1501     if (certUsage != certUsageStatusResponder && statusConfig != NULL) {

This depends on the caller passing in the certUsage value, telling us that
it is attempting to verify this cert as an OCSP responder cert.  

So, I'd guess that in the releveant case, the caller of CERT_VerifyCert 
did nto pass certUsageStatusResponder for the value of certUsage to 
CERT_VerifyCert.

Comment 3

[Kai Engert (:KaiE:)]

Yes, the caller stack tells us we're using certUsageAnyCA.

The code that triggers the call is this:

        SECCertUsage certUsage;
        if (CERT_IsCACert(signerCert, NULL)) {
            certUsage = certUsageAnyCA;
        } else {
            certUsage = certUsageStatusResponder;
        }
        rv = CERT_VerifyCert(handle, signerCert, PR_TRUE,
                             certUsage, producedAt, pwArg, NULL);

Both alternating signer cert pass the "IsCACert" test, 
in both alterating calls we use certUsageAnyCA.

Comment 4

[Rob Stradling]

Brian, thanks for cc'ing me on this bug just now.

We (ocsp.comodoca.com and ocsp.usertrust.com) always use non-delegated OCSP Response signing.  In other words, every OCSP Response is directly signed by the same CA name/key that issued the certificate that the OCSP Response relates to.  Therefore, I fail to see how our OCSP setup in itself could create a circular dependency.

Could this be yet another dup of Bug 479508 (just like Bug 634074) ?

Comment 5

[Rob Stradling]

(In reply to comment #4)
> Brian, thanks for cc'ing me on this bug just now.

Sorry, I misread two adjacent Bugzilla emails.  It was actually Robin who cc'd me.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In the non-delegated case, we shouldn't check for revocation because either the cert that signed the OCSP is a root cert (no revocation checking possible), or the cert that signed the OCSP response is an intermediate that will have its revocation status checked before/after.

In the delegated case, things are not so clear to me. How would a CA revoke the delegation? Only by revoking the (intermediate) CA cert associated with the responder?

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, we need to investigate how this works in libpkix in addition to Cert_VerifyCert.

Comment 8

[Rob Stradling]

(In reply to comment #6)
> In the non-delegated case, we shouldn't check for revocation because either the
> cert that signed the OCSP is a root cert (no revocation checking possible), or
> the cert that signed the OCSP response is an intermediate that will have its
> revocation status checked before/after.

Agreed.  I hope this is how it works.

> In the delegated case, things are not so clear to me. How would a CA revoke the
> delegation? Only by revoking the (intermediate) CA cert associated with the
> responder?

A delegated Responder cert contains the ocspSigning EKU OID.  There is no need for it to be a CA cert, because it only signs OCSP Responses (which are not certs).  Yes, if the CA wanted to revoke the delegation they would revoke this cert.  If this cert contains an AIA->OCSP URL, a circular dependency could easily arise.  Two ways to avoid this: 1) Include a CRL DP, but omit AIA->OCSP; 2) Include the id-pkix-ocsp-nocheck cert extension to indicate that the cert should be implicitly trusted for it's entire, hopefully-short lifetime.

For more info, read RFC 2560.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: zip archive for test case, certs sent by robin.eff.org server

Copying test case certs from other bug.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 534347 [details]
zip archive for test case, certs sent by robin.eff.org server

Attached to wrong bug. sorry for the noise.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This (only) occurs when OCSP revocation checking has been requested and CERT_SetUsePKIXForValidation(PR_TRUE) has been called (which happens automatically when NSS_ENABLE_PKIX_VERIFY=1). It may also occur in the analogous case for CRL revocation checking.

The short-term solution is to change cert_VerifyCertChainPkix so that it emulates the certUsage != certUsageStatusResponder check Nelson mentioned in comment 2. This is needed anyway, to ensure that the CERT_SetUsePKIXForValidation(PR_TRUE) behavior of CERT_VerifyCert* is more consistent with the CERT_SetUsePKIXForValidation(PR_FALSE) case.

After I fix that, I will file new enhancement requests for a way to enable revocation checking for the signers of OCSP responses processed during path building, and to remove the CERT_FindCertByName & CERT_VerifyCert dependencies from libpkix. (Removal of the CERT_FindCertIssuer dependency is bug 439505.)

Comment 13

[Kai Engert (:KaiE:)]

I would be helpful to have a test site that triggers this behaviour.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Prevent stack overflow and ensure addon compatibility when NSS_ENABLE_PKIX_VERIFY=1 by forcing CERT_SetUsePKIXForValidation(false)

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Regarding the addon compatbility issues, see bug 653572 comment 6.

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 596511 [details] [diff] [review]
Prevent stack overflow and ensure addon compatibility when NSS_ENABLE_PKIX_VERIFY=1 by forcing CERT_SetUsePKIXForValidation(false)

Brian, I think you misinterpret this bug report, I'm sorry if the bug report wasn't clear.

The summary of this bug says
  "we have a problem when we use CERT_SetUsePKIXForValidation(true)"

No code currently does this by default.

We need a real fix, not set CERT_SetUsePKIXForValidation(false).

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 596511 [details] [diff] [review]
Prevent stack overflow and ensure addon compatibility when NSS_ENABLE_PKIX_VERIFY=1 by forcing CERT_SetUsePKIXForValidation(false)

Moved this patch to bug 728321, since it is a patch for PSM and not a patch for NSS.

Comment 19

[Kyle Hamilton]

(In reply to Kai Engert (:kaie) from comment #3)
> Yes, the caller stack tells us we're using certUsageAnyCA.
> 
> The code that triggers the call is this:
> 
>         SECCertUsage certUsage;
>         if (CERT_IsCACert(signerCert, NULL)) {
>             certUsage = certUsageAnyCA;
>         } else {
>             certUsage = certUsageStatusResponder;
>         }
>         rv = CERT_VerifyCert(handle, signerCert, PR_TRUE,
>                              certUsage, producedAt, pwArg, NULL);
> 
> Both alternating signer cert pass the "IsCACert" test, 
> in both alterating calls we use certUsageAnyCA.

CERT_VerifyCertificate calls into nss/lib/certdb/certdb.c CERT_KeyUsageAndTypeForCertUsage.

certUsageStatusResponder requires EXT_KEY_USAGE_STATUS_RESPONDER, and will fail on a generic CA certificate when it isn't checking for a CA certificate.  (nss/lib/certhigh/certvfy.c:1301 for the PR_FALSE)

Why are we be calling here when PKIX is used for validation?  This is something that libpkix itself should be handling.  Is CERT_SetUsePKIXForValidation only checked in CERT_VerifyCert (not CERT_VerifyCertificate)?

Is there a "CERT_SetWorkaroundsForBugsInTheWild" function that might be set to change the PR_FALSE in certvfy.c:1301 to PR_TRUE?

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kyle Hamilton from comment #19)
> Why are we be calling here when PKIX is used for validation?  This is
> something that libpkix itself should be handling.

You are correct. I know how to fix this bug such that we will no longer call CERT_Verify[Certificate]. It is just a matter of priority.

Comment 21

[Kyle Hamilton]

If I understand correctly:

Gecko implements the pkix-by-default preference independently.  This patch is necessary to work around an infinite recursion which happens when libpkix is sent a certificate chain which refers to a CA as a responder (which should never happen under strict OCSP semantics), and is also needed to ensure that extensions (such as Sean Leonard's Penango) continue to use the old behavior until libpkix can do everything the current algorithm can.  Gecko itself will gain the benefit of libpkix enhanced chain-building algorithms, which will permit cross-certification initiatives to get started.  This is a temporary situation until schedule permits deep hacking into libpkix to remove the callouts that occur to CERT_Verify* functions.

FWIW, if my understanding is correct, I have no problem with this course of action.  If I misunderstand, please correct me?