390542 - libpkix fails to validate a chain that consists only of one self issued, trusted cert

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

PKIX_PL_Cert structure has as a member: a pointer to cert store. If this pointer is null libpkix will treat the cert as untrusted. This means two things: either function that creates PKIX_PL_Cert from CERTCertificate should set pk11certstore as a default store for certs that libpkix validating or trust checker should assume that pk11certstore is a default store for all certs that have certstore pointer set to NULL.

The problem was found from by running all.sh: tools and fips tests fails with out fix for this problem.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

A chain that has only one cert, which is self-signed, is by definition,
valid ONLY if it is trusted.  And, being trusted overrides most other
considerations, except validity period.  

So I guess the real issue here is that we're not checking the one-and-only 
cert in the chain to see if it is trusted.  

For calls made with the CERT_VerifyCert* "wrappers", the check should be consistent with the trust domain.  For other cert verification APIs, 
who can say?  I think it should always be consistent with the trust 
domain.  but ... 

Comment 2

[Alexei Volkov]

Attachment: patch v1

all.sh->tools tests create self issued object signing certificate that also has critical NS Certificate Type extension. Lib pkix can not validate this cert for the two reasons: 1) when it tries to find an issuer, it detects a loop and aborts validation. 2) libpkix does not know about NS CERT TYPE extension.

To fix problems, two things have been ajusted:
    * Adding NS Cert Type to the list of processed cert extensions.
    * Delay reporting looping error on a trusted cert. Report an error later if cert is trusted, but fails any validation criteria.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279182 [details] [diff] [review]
patch v1

Was this patch attached to the wrong bug?
This patch has nothing to do with the subject of 
bug 390542.  It has to do with loop detection,
and with old netscape proprietary cert extensions,
which I thought we had agreed to drop.  

If there is a problem with loop detection, then there should
be a bug for that problem and the portion of this patch that
addresses that problem should be attached to that bug. 

I don't understand why we're looking at Netscape cert type now.
What problem does that fix? What bug exists about that problem?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279182 [details] [diff] [review]
patch v1

I'm giving this patch r- because it seems unrelated to the subject of this bug.  It seems to make two conceptually separate changes, neither of which is the subject of this bug.  Please file separate bugs and attach the patches to them.

Comment 5

[Alexei Volkov]

Splitting the bug into two. Patch related to NS cert type extension should be attached into bug 395224. 

Comment 6

[Alexei Volkov]

> If there is a problem with loop detection, then there should
> be a bug for that problem and the portion of this patch that
> addresses that problem should be attached to that bug. 
The early loop detection is actually in the roots of the problem of
libpkix rejecting selfsigning certs. When libpkix tries to build a patch for a such cert, it finds the cert's issuer and immediately checks for possible loops. 

Instead, I propose, the library should hold rejecting looped cert until the cert is validated and trust is verified. Only if the cert is not good, or not trusted  libpkix should report the loop error.   

> I don't understand why we're looking at Netscape cert type now.
> What problem does that fix? What bug exists about that problem?
The cert that we use for signtool tests(there the problem was detected) 
happened to have critical NS cert type extension that libpkix could not
processed. I should have filed a separate bug to this matter. 

Comment 7

[Alexei Volkov]

Attachment: patch v2(part 2 of patch v1 split)

The patch handles only issue with early cert loop reporting.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279978 [details] [diff] [review]
patch v2(part 2 of patch v1 split) 

Isn't PKIX_ERROR a goto?

>+                if (!trusted) {
>+                    PKIX_ERROR
>+                        (PKIX_LOOPDISCOVEREDDUPCERTSNOTALLOWED);
>+                } else {
>+                    state->certLoopingDetected = PKIX_TRUE;
>+                }

If it is, then the above code should be:

>+                if (!trusted) {
>+                    PKIX_ERROR
>+                        (PKIX_LOOPDISCOVEREDDUPCERTSNOTALLOWED);
>+                }
>+                state->certLoopingDetected = PKIX_TRUE;

Comment 9

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c,v  <--  pkix_build.c
new revision: 1.7; previous revision: /cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.h,v  <--  pkix_build.h
new revision: 1.4; previous revision: 1.3

Comment 10

[Alexei Volkov]

Remove invalid dependency.