399326 - libpkix is unable to validate cert for certUsageStatusResponder

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

old nss validation code deals with verifying certUsageStatusResponder as with exceptional case: function returns a success if detected that a cert has some trust.
Here is the code:
815         if ( issuerCert->trust ) {
...
821             if (certUsage == certUsageStatusResponder) {
822                 /* XXX NSS has done this for years, but it seems incorrect. */
823                 rv = rvFinal;
824                 goto done;
825             }
826 

IMO, it is incorrect, since it allow cert to be chained to an untrusted cert. 

Never the less, libpkix trust verification function does not address the problem of validating cert for certUsageStatusResponder, certUsageVerifyCA nor certUsageAnyCA usages at all.

The bug is the reason why ocsp designated responder and ocsp designated responder CA tests failed and were commented out from nightly and tinderbox run until a resolution to this bug is found.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

P1,  Beta1, if this means that OCSP checks won't work with libpKIX.
If OCSP generally works with libPKIX, then this is P2 or less, and not a 
NSS 3.12 Beta1 blocker

Comment 2

[Alexei Volkov]

Attachment: Patch v1

Check if a cert is a valid/trusted CA of any type when checking cert for AnyCA and StatusResponder usages.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 290772 [details] [diff] [review]
Patch v1

I htink there are two problems.

>Index: lib/certhigh/certvfy.c

>+	    } else {
>+                /* Check if we have any valid trust when cheching for certUsageAnyCA or
>+                 * certUsageStatusResponder. */
>+                for (trustType = trustSSL; trustType < trustTypeNone;
>+                     trustType++) {
>+                    flags = SEC_GET_TRUST_FLAGS(issuerCert->trust, trustType);
>+                    if ((flags & requiredFlags) == requiredFlags) {
>+	                rv = rvFinal; 
>+	                goto done;
>+                    }
>+                }
>+                validCAOverride = PR_TRUE;

I believe this code should not set validCAOverride unless 
(flags & CERTDB_VALID_CA) is true.  I suggest this change:  

>+                    flags = SEC_GET_TRUST_FLAGS(issuerCert->trust, trustType);
>+                    if ((flags & requiredFlags) == requiredFlags) {
>+	                rv = rvFinal; 
>+	                goto done;
>+                    }
>+                    if (flags & CERTDB_VALID_CA)
>+                        validCAOverride = PR_TRUE;
>+                }



>Index: lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c

>         rv = CERT_TrustFlagsForCACertUsage(certUsage, &requiredFlags, &trustType);
>         if (rv == SECSuccess) {
>                 rv = CERT_GetCertTrust(cert->nssCert, &trust);
>         }
> 
>+        if (rv == SECSuccess &&
>+            certUsage != certUsageAnyCA &&
>+            certUsage != certUsageStatusResponder) {
>+            unsigned int certFlags;
>+            CERTCertificate *nssCert = cert->nssCert;
>+
>+            if (certUsage == certUsageVerifyCA) {
>+                if (nssCert->nsCertType & NS_CERT_TYPE_EMAIL_CA) {
>+                    trustType = trustEmail;
>+                } else if (nssCert->nsCertType & NS_CERT_TYPE_SSL_CA) {
>+                    trustType = trustSSL;
>+                } else {
>+                    trustType = trustObjectSigning;
>+                }
>+            }
>+
>+            certFlags = SEC_GET_TRUST_FLAGS((&trust), trustType);
>+            if ((certFlags & requiredFlags) == requiredFlags) {
>+                trusted = PKIX_TRUE;
>+            }
>+        } else {

If rv != SECSuccess, this else case will be executed.  I think that's
wrong.  I think you want the if (rv == SECSuccess) to be a separate test
which, if true, executes a block of code that includes the 
          if (certUsage != certUsageAnyCA ...
and this else case.  

>+            for (trustType = trustSSL; trustType < trustTypeNone;
>+                 trustType++) {
>+                unsigned int certFlags =
>+                    SEC_GET_TRUST_FLAGS((&trust), trustType);
>                 if ((certFlags & requiredFlags) == requiredFlags) {
>+                    trusted = PKIX_TRUE;
>+                    break;
>                 }
>+            }
>         }
> 
>         *pTrusted = trusted;
> 
>         PKIX_RETURN(CERTSTORE);
> }

Comment 4

[Alexei Volkov]

Attachment: Patch v2 (checked in on trunk)

Implementing suggested changes.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 291696 [details] [diff] [review]
Patch v2 (checked in on trunk)

r=nelson
Thanks

Comment 6

[Alexei Volkov]

patch v2 is committed. 

Comment 7

[Alexei Volkov]

Attachment: Port the bug fix to the branch

Comment 8

[Alexei Volkov]

Comment on attachment 351209 [details] [diff] [review]
Port the bug fix to the branch

Identity Mgr customer reported an issuer related to the bug. Need to port the fix. Requesting two reviews for it.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 351209 [details] [diff] [review]
Port the bug fix to the branch

r=nelson, but let's discuss this before you check it in.

Comment 10

[Alexei Volkov]

Comment on attachment 351209 [details] [diff] [review]
Port the bug fix to the branch

This patch is not needed. Closing the bug.