786431 - libpkix should not re-import token certificates as temporary certificates

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch: do not re-import certificates returned by NSS as temporary certificates

When building the certification path, libpkix calls pkix_pl_Pk11CertStore_CertQuery
to get a list of issuer certificates of the current certificate from NSS.

However, it re-imports each of the certificates as a temporary certificate:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c&rev=1.20&mark=212-216,229-233,245-253#197

This is harmless in general because CERT_NewTempCertificate may return a permanent
certificate if the certificate exists in the (main) NSS database. But if the
certificate exists in a user database that NSS loads later, this could go wrong.
Here is the problematic scenario:

1. NSS is initialized with the (main) NSS database, which does NOT have the
issuer certificate in question.

2. The issuer certificate is imported as a temporary certificate by libSSL.

3. The application calls SECMOD_OpenUserDB to load a user database where this
issuer certificate exists and is marked trusted.

4. CERT_PKIXVerify is called to verify a certificate issued by the issuer
certificate.

5. pkix_pl_Pk11CertStore_CertQuery gets a list of two CERTCertificate objects
for the issuer certificate.

The first CERTCertificate object is a temporary certificate, originally created
in step 2. Note in particular that it is not marked trusted.

The second CERTCertificate object is a permanent certificate that represents
the issuer certificate in the user database.

6. pkix_pl_Pk11CertStore_CertQuery calls CERT_NewTempCertificate on each of
the two CERTCertificate objects. CERT_NewTempCertificate converts both to
the first CERTCertificate object, a temporary, untrusted certificate.

So, the trust settings on the second CERTCertificate object is lost in this
CERT_NewTempCertificate call.

My proposed fix is to replace the CERT_NewTempCertificate call by a
CERT_DupCertificate call (which merely acquires a reference). This preserves
the properties of the CERTCertificate objects returned by NSS.

I don't understand the purpose of the CERT_NewTempCertificate call. I can't
find an explanation in CVS history. That CERT_NewTempCertificate call already
existed in its current form when the file pkix_pl_pk11certstore.c was first
added to the NSS_LIBPKIX_BRANCH in rev. 1.1.2.1 by richard.freedman%sun.com.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 656203 [details] [diff] [review]
Proposed patch: do not re-import certificates returned by NSS as temporary certificates

The documentation for PKIX_PL_Cert_Create indicates that PKIX_PL_Cert objects are supposed to be immutable. However, CERTCertificate objects are not immutable. Perhaps, then, this is an attempt to support the immutability of PKIX_PL_Cert objects, by ensuring there's no mutable state shared between the internal CERTCertificates?

I see in PKIX_PL_Cert_CreateFromCERTCertificate, people were interested in trying to use CERT_DupCertificate as a performance improvement, but the use of CERT_DupCertificate is #ifdef'd out.

I really don't see how any kind of immutability guarantee from having a full copy of a CERTCertificate helps, because it seems like mutability is still an issue with CERT_NewTempCertificate when it is looking up certificates from the persistent databases.

Consequently, I'm giving a tentative r+ but I admit I'm not understanding the issues here as well as I'd like to. It would be good to hear from Bob.

Comment 2

[Wan-Teh Chang]

Brian: thanks for the review.

I found the comments you mentioned:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/include/pkix_pl_pki.h&rev=1.15&mark=47,50,58,63-64#46

I think PKIX_PL_Cert_CreateFromCERTCertificate should be
fixed to use CERT_DupCertificate as well:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c&rev=1.31&mark=1582,1602,1606-1607,1609#1577

Note that CERT_NewTempCertificate may do the equivalent of
Dup'ing an existing temporary or permanent certificate:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/stanpcertdb.c&rev=1.92&mark=349,361-363,365-367#348

So CERT_NewTempCertificate is not guaranteed to create a
brand new temporary certificate. In addition,
CERT_NewTempCertificate does not preserve the "meta data"
on the certificate, such as trust settings and the token
where the certificate resides. In this bug, the trust
settings are important to preserve.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in pkix_pl_pk11certstore.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certsto
re.c,v  <--  pkix_pl_pk11certstore.c
new revision: 1.21; previous revision: 1.20
done

Comment 3

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #1)
> 
> I really don't see how any kind of immutability guarantee from having a full
> copy of a CERTCertificate helps, because it seems like mutability is still
> an issue with CERT_NewTempCertificate when it is looking up certificates
> from the persistent databases.

Ah, I just noticed that you already knew about this. Sorry about that.
So we are in agreement.

Comment 4

[Robert Relyea]

Comment on attachment 656203 [details] [diff] [review]
Proposed patch: do not re-import certificates returned by NSS as temporary certificates

r+ even thought it's already checked in.

bob