233038 - Chain validation fails when perm cert is issued by a temp cert

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSS fails NIST PKITS test 4.6.17, also known as test 
"Valid Self-Issued path Len Constraint Test 17".  

To simplfy the test, it may be run without any CRLs.  (CRLs are not the problem.)

The problem is that the function find_cert_issuer cannot find the issuer cert
when the subject cert is a "perm" cert (in the trust domain), and the issuer 
cert is a "temp" cert (in the crypto context).  

The test involves validating a chain of 6 certs, including 1 leaf cert,
1 root CA cert, and 4 intermediate CA certs.  This table shows the chain,
from leaf to root

   Cert #   Description          Other Info
   ------   ------------------   --------------
      0     leaf (EE)             temp
      1     self-issued sub CA    temp
      2     sub CA                perm
      3     self-issued CA        temp
      4     CA                    perm
      5     root CA               perm, trusted

All certs have unique subject Key ID extensions.  
All certs except the root have Authority Key ID extensions.
The "self issued" certs have the same subject name as their issuer.
However, I think the "self issued" nature of these certs is irrelevant.

Certs 0 through 4 are presented to the program as input.  
All 5 certs are passed to CERT_NewTempCertificate before validating the chain.

Certs 2, 4 and 5 are already in the cert db.  They were put there as a 
precondition to loading CRLs (which are not necessary to see this failure.)

The failure occurs when find_cert_issuer is called to find the issuer for
cert 2.  Cert 2 is in the trust domain.  Cert 3 is in the crypto context.

Because cert 2 is in the trust domain, its cryptoContext pointer is NULL.
That is, c->object.cryptoContext is NULL.  Consequently, find_cert_issuer
does not call nssCryptoContext_FindCertificatesBySubject, but only calls
nssTrustDomain_FindCertificatesBySubject.  

Removing cert 2 from the cert DB causes the chain to validate succesfully.
In that case, cert 2 is part of the crypto context, and so find_cert_issuer
finds its issuer.

As an experiment, I changed __CERT_NewTempCertificate so that, instead of
returning cc at about line 253, it jumps down to the call to
NSSCryptoContext_ImportCertificate near line 308 and continues from there.
I tested this with the original cert db, containing certs 2, 4 and 5.
This workaround also made the chain validate, but I strongly doubt that 
this is the solution we want.  

Suggestions on the right fix for this bug are invited!

Comment 1

[Julien Pierre]

It seems the logic in the NSSCertificate_BuildChain and find_cert_issuer
functions is just wrong. It only looks for certs in the global trust domain or
in the current cert's trustdomain and cryptocontext.

For it to work, it would need to also look up certs in the global cryptocontext
where the certs were just added by CERT_NewTempCertificate.

I would suggest adding extra arguments to the Stan function to pass them from
the NSS glue code.

Since it isn't clear what the right Stan semantics are supposed to be for these
functions, it would be good if Ian or Bob could comment.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch changes functions find_cert_issuer, nssCertificate_BuildChain, and
NSSCertificate_BuildChain to take explicit trustDomain and CryptoContext 
pointer arguments.  It changes the callers of NSSCertificate_BuildChain,
which are the old pre-stan functions CERT_CertChainFromCert and
CERT_FindCertIssuer to pass the values obtained from calls to
  td = STAN_GetDefaultTrustDomain();
  cc = STAN_GetDefaultCryptoContext();
It detects some previously undetected errors in  nssCertificate_BuildChain.
It ensures that we always clean up in error paths in nssCertificate_BuildChain.

And it adds some comments, and moves some previously misplaced comments.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

Bob, please review

Comment 4

[Ian McGreer]

I think this problem is due to our attempt to merge the ideologies of Stan and
3.X.  The Stan code wants to work with multiple temporary stores (crypto
contexts) and permanent stores (trust domains), and chain construction for a
given cert would presumably only occur in its context and domain.  However, I
imagine this code still would not have been totally correct (the code in
STAN_WORK_BRANCH is much more mature).

That said, it doesn't seem that Stan is going to happen, so we probably
shouldn't worry about its idioms.  I don't think it is necessary to pass the
NSSCryptoContext and NSSTrustDomain as arguments to the chain construction
function, since there will only be one of each for the foreseeable future.  But
that's just a stylistic comment, I agree with the patch in general.

Comment 5

[Robert Relyea]

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

My only concern is limited by the fact the functions actually changed their
prototypes, and I can see that all NSS v3 calls are passing in the default
trust domain. The new code is cleaner and easier to read, and still maintains
the old semantics. r=relyea

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Bob and Ian, 

I too am concerned about the API change.  Fortunately, the stan headers are 
(mostly) all still private.  But the question about which I'm still not 
confident is: what is the API for the stan code?

One of two things is wrong with the present implementation.  Either:

a) Stan's design did not intend that certs can have NULL context pointers 
and/or NULL domain pointers, and the proper fix would fix the NSSCertificate
to have non-NULL values for both of those, and leave BuildChain the API alone, 
or 

b) Stan was intended to allow certs to have NULL context and/or domain 
pointers, and was intended to take those values as function arguments. 
Hence it IS necessary to pass these values to BuildChain along with the 
cert that starts the chain.  

I addressed B, because I thought that was how things were intended to work 
in Stan (wouldn't a design document be GREAT?) but if A is what Stan intended.
please say so!

Comment 7

[Robert Relyea]

Don't we have such a document?

Such a document would also be important to remind ourselves of 2 year old
conversations.

On thing is clear, only one of the functions you modifed the signature on was
intended to be exportable. My feeling (which may have morphed from any original
decision, which would be lost now if we didn't write it down), would be that we
should explicitly provide the trust and context domains (as you have done), and
fall back to the trust and context domains found in the certs.

bob 

Comment 8

[Ian McGreer]

working Stan docs are here:
http://www.mozilla.org/projects/security/pki/nss/stan/nss_4_0_overview.html

They are not complete.  But maybe they will help.

I will try to get back to this bug later, but am very busy right now.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Bob, in answer to question 7, 
we have docs that are just copies of the header files, sans any comments.  
We have overview docs that contain nice diagrams that I found helpful.  

But I'm not aware of anything remotely close to the level of detail of
explaining when a stan object must/will have a CC pointer and when it will have
a TD pointer. 
I'd like to see something that explains what pointers a cert object will have when
a) it's been imported as was NOT found in any token object,
b) it's been imported and was found in a token object
c) the same cert exists in multiple CCs that share a TD 

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Marking fixed, but please review further if you wish.

/cvsroot/mozilla/security/nss/lib/pki/nsspki.h,v  <--  nsspki.h
new revision: 1.8; previous revision: 1.7

/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.50; previous revision: 1.49

/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.32; previous revision: 1.31

/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.41; previous revision: 1.40