Last Comment Bug 399326 - libpkix is unable to validate cert for certUsageStatusResponder
: libpkix is unable to validate cert for certUsageStatusResponder
Status: RESOLVED FIXED
PKIX NSS312B2
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 normal (vote)
: 3.11.10
Assigned To: Alexei Volkov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-10 10:15 PDT by Alexei Volkov
Modified: 2009-04-19 22:02 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (5.76 KB, patch)
2007-11-29 15:27 PST, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v2 (checked in on trunk) (6.45 KB, patch)
2007-12-05 10:33 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
Port the bug fix to the branch (3.16 KB, patch)
2008-12-03 10:11 PST, Alexei Volkov
julien.pierre: review+
nelson: superreview+
Details | Diff | Splinter Review

Description Alexei Volkov 2007-10-10 10:15:23 PDT
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) 2007-11-08 18:40:14 PST
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 2007-11-29 15:27:15 PST
Created attachment 290772 [details] [diff] [review]
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) 2007-12-04 17:35:40 PST
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 2007-12-05 10:33:25 PST
Created attachment 291696 [details] [diff] [review]
Patch v2 (checked in on trunk)

Implementing suggested changes.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2007-12-05 10:58:06 PST
Comment on attachment 291696 [details] [diff] [review]
Patch v2 (checked in on trunk)

r=nelson
Thanks
Comment 6 Alexei Volkov 2007-12-05 11:28:37 PST
patch v2 is committed. 
Comment 7 Alexei Volkov 2008-12-03 10:11:49 PST
Created attachment 351209 [details] [diff] [review]
Port the bug fix to the branch
Comment 8 Alexei Volkov 2008-12-03 10:17:12 PST
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) 2008-12-04 10:09:07 PST
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 2009-04-19 22:02:40 PDT
Comment on attachment 351209 [details] [diff] [review]
Port the bug fix to the branch

This patch is not needed. Closing the bug.

Note You need to log in before you can comment on or make changes to this bug.