The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 1 obsolete attachment)

8.48 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

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

Updated

10 years ago
Blocks: 390888
(Assignee)

Comment 2

10 years ago
Created attachment 279182 [details] [diff] [review]
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.
Attachment #279182 - Flags: review?(nelson)
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 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.
Attachment #279182 - Flags: review?(nelson) → review-
(Assignee)

Updated

10 years ago
Depends on: 395224
(Assignee)

Comment 5

10 years ago
Splitting the bug into two. Patch related to NS cert type extension should be attached into bug 395224. 
(Assignee)

Comment 6

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

Comment 7

10 years ago
Created attachment 279978 [details] [diff] [review]
patch v2(part 2 of patch v1 split) 

The patch handles only issue with early cert loop reporting.
Attachment #279182 - Attachment is obsolete: true
Attachment #279978 - Flags: review?(nelson)
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;
Attachment #279978 - Flags: review?(nelson) → review+
(Assignee)

Comment 9

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

Updated

10 years ago
Blocks: 396598
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
No longer blocks: 390888
(Assignee)

Comment 10

10 years ago
Remove invalid dependency.
No longer depends on: 395224
You need to log in before you can comment on or make changes to this bug.