The default bug view has changed. See this FAQ.

libpkix is unable to validate cert for certUsageStatusResponder

RESOLVED FIXED in 3.11.10

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

trunk
3.11.10

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX NSS312B2)

Attachments

(2 attachments, 1 obsolete attachment)

6.45 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
3.16 KB, patch
Julien Pierre
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Whiteboard: PKIX
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
Priority: -- → P1
Whiteboard: PKIX → PKIX NSS312B1
Whiteboard: PKIX NSS312B1 → PKIX NSS312B2
Version: 3.12 → trunk
(Assignee)

Comment 2

10 years ago
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.
Attachment #290772 - Flags: review?(nelson)
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);
> }
Attachment #290772 - Flags: review?(nelson) → review-
(Assignee)

Comment 4

9 years ago
Created attachment 291696 [details] [diff] [review]
Patch v2 (checked in on trunk)

Implementing suggested changes.
Attachment #290772 - Attachment is obsolete: true
Attachment #291696 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Attachment #291696 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #291696 - Attachment is patch: true
Comment on attachment 291696 [details] [diff] [review]
Patch v2 (checked in on trunk)

r=nelson
Thanks
Attachment #291696 - Flags: review?(nelson) → review+
(Assignee)

Comment 6

9 years ago
patch v2 is committed. 
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

8 years ago
Created attachment 351209 [details] [diff] [review]
Port the bug fix to the branch
Attachment #351209 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #351209 - Flags: superreview?(nelson)
Attachment #351209 - Flags: review?(julien.pierre.boogz)
Attachment #351209 - Flags: review?
(Assignee)

Comment 8

8 years ago
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.

Updated

8 years ago
Attachment #351209 - Flags: review?(julien.pierre.boogz) → review+
Attachment #291696 - Attachment description: Patch v2. → Patch v2 (checked in on trunk)
Attachment #351209 - Flags: superreview?(nelson) → superreview+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.12 → 3.11.10
(Assignee)

Comment 10

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

This patch is not needed. Closing the bug.
(Assignee)

Updated

8 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.