Last Comment Bug 468532 - Trusted CA trust flags not being honored in CERT_VerifyCert
: Trusted CA trust flags not being honored in CERT_VerifyCert
Status: RESOLVED FIXED
SUN_MUST_HAVE
: regression, testcase
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 critical (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-08 15:56 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-02-08 23:52 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (1.68 KB, patch)
2008-12-08 17:59 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2008-12-08 15:56:06 PST
NSS 3.12 is not honoring the 'C' flag on certs in the certDB which doing a
cert chain validation with the old CERT_VerifyCertChain code.  

The 'C' flag (TRUSTED CA) should force any cert to be treated as a valid 
and trusted CA cert, regardless of whether it has a BasicConstraints 
extension or not, and regardless of whether it is X.509 v1 or v3.  

It is not having that effect in NSS 3.12.  It seems to work OK in NSS 3.11.

A simple reproducible test script shows this.  It creates an X.509 v3 cert
without any extensions, sets the 'C' flag on it, and uses it like a CA 
cert to issue a subordinate cert.  However, the attempt to validate the 
subordinate cert fails, reporting unknown issuer, because the cert is not 
recognized as a CA cert, despite having the flag set.  

echo passwd > pwfile
echo jfgdfgkjdflfdgopgpjergerpojggjgi54kgojgh-rt9-i-uy=50i5pgoj5r > noise
mkdir -p DB
certutil -N -d DB -f pwfile
# Create a self-signed certificate to act as CA
certutil -S -d DB -n test_ca -s cn=test_ca -x -t CT,, -f pwfile -z noise
# Generate a server certificate using that trusted CA
certutil -S -d DB -n test -s cn=test -c test_ca -t ,, -f pwfile -z noise
# List the created certificates
certutil -L -d DB
# Attempt to validate the certificate
certutil -V -d DB -n test -e -u V -f pwfile

(CR 6782276)
Comment 1 Nelson Bolyard (seldom reads bugmail) 2008-12-08 15:58:19 PST
Clearly this points out a deficiency in our regression test scripts.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-12-08 16:48:07 PST
The problem occurs in and below (functions called from) find_cert_issuer.
When we get to that function, the call stack is:


nss3.dll!find_cert_issuer()  		Line 442	
nss3.dll!nssCertificate_BuildChain()  	Line 513 + 0x1d bytes	
nss3.dll!NSSCertificate_BuildChain()  	Line 561 + 0x2d bytes	
nss3.dll!CERT_FindCertIssuer()  	Line 250 + 0x27 bytes	
nss3.dll!cert_VerifyCertChainOld()  	Line 626 + 0x15 bytes	
nss3.dll!cert_VerifyCertChain()  	Line 873 + 0x2d bytes	
nss3.dll!CERT_VerifyCertificate()  	Line 1287 + 0x2d bytes	
certutil.exe!ValidateCert()  		Line 657 + 0x2d bytes	
certutil.exe!certutil_main()  		Line 2601 + 0x43 bytes	
certutil.exe!main()  			Line 2934 + 0xf bytes	

find_cert_issuer calls two functions to get a list of NSSCertificates
whose subject names are the expected issuer name. Those functions are:
- nssCryptoContext_FindCertificatesBySubject and
- nssTrustDomain_FindCertificatesBySubject

It then calls filter_certs_for_valid_issuers() to filter that list. 

At NO TIME during this process does any code call either of the functions 
nssTrust_GetCERTCertTrustForCert or nssTrustDomain_FindTrustForCertificate
which would fetch the trust information for the cert and attach it to the 
cert.  So, when function cert_ComputeTrustOverrides() tries to apply the 
trust overrides, it finds no trust flags, and hence applies no overrides.

Some questions I am investigating are:
- How did this work in 3.11?
- did some code path fetch the trust info then, but not in 3.12? or
- did the code somehow conclude that the cert was a valid CA even 
without the trust flags?
Comment 3 Nelson Bolyard (seldom reads bugmail) 2008-12-08 17:18:36 PST
Another question is:  
Where *should* the call to fetch the trust information be made?

Since the trust information comes from the trust domain, perhaps 
nssTrustDomain_FindCertificatesBySubject should have done it before 
returning the array of NSSCertificates from the trust domain.

filter_certs_for_valid_issuers calls nssCertificate_GetDecoding()
which (as the name implies) is interested in decoding the cert,
which is different from getting the trust info from the trust domain.

filter_certs_for_valid_issuers then calls dcp->isValidIssuer(), 
which is nss3certificate_isValidIssuer(), which calls CERT_IsCACert().

I am thinking that perhaps function nssDecodedCert_Create should be
reimplemented as a call to STAN_GetCERTCertificate rather than as a call
to nssDecodedPKIXCertificate_Create().  STAN_GetCERTCertificate also calls 
nssDecodedPKIXCertificate_Create, but it also does the numerous other 
important steps.  I will explore this idea some more.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-12-08 17:59:16 PST
Created attachment 352021 [details] [diff] [review]
patch v1 

This patch makes the test pass.  It ensures that the trust information
is obtained from the trust domain before tests are made to the cert that
might need that trust information.  It also evaluates cert->isRoot before
that information is used.  

I'm going to test this some more, and also investigate how/why the code 
worked before, before asking for review.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2008-12-09 16:20:40 PST
This regression has occurred since NSS_3_12_RTM. :(
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-12-09 16:34:46 PST
Regression has occurred since NSS_3_12_2_RC1.  :(
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-12-09 18:21:31 PST
This regression appears to have been caused by the checkin of 
revision 1.93 of mozilla/security/nss/lib/certdb/certdb.c . 
Backing out that one revision seems to fix this bug, but it 
reintroduces bug 464406.  I am investigating.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-12-09 21:23:12 PST
I went and looked at how it worked before revision 1.93 of certdb.c was 
committed, and was unpleasently surprised by what I found.  

The relevant change was actually in CERT_IsCACert, which previously 
reported true for all self-issued certs (where subject and issuer name
are the same) regardless of the version of the cert (v1 or v3).  
In effect, all self-issued certs previously were treated as if the 
"valid CA" trust flag was set.  :(   Now, in NSS 3.12, only X.509 v1 certs 
are ever treated that way.  That is, a self-issued v1 cert is treated as a
CA cert, but a self-issued v3 cert is not.  This was done to fix bug 454437.

So, this was a case of one bug masking another one.  The valid CA override 
flag was not actually being consulted in all cases where it should have been,
and this would have been noticed LONG ago, except for the fact that the code
has been treating self-issued certs as valid CAs all along.  

So, I believe the patch attached to this bug is the right solution.  I do 
not believe the right solution is to go back and continue to treat all
self-issued certs as valid CA certs, but rather is to use the trust flags 
properly.  

However, I think I also need to test self-issued peer certs before making 
this decision final.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-12-10 12:11:00 PST
Comment on attachment 352021 [details] [diff] [review]
patch v1 

Bob, please review.  
I want you to review this because you are the one person remaining on NSS who knows how Stan was supposed to work.

The change made here to nssCertificate_GetDecoding feels like a hack, but it seems that Stan's architecture did not consider that trust flags would need to be considered when deciding if a cert is a valid issuer or not.  I don't even see trust anywhere in an NSSCertificate.  I see it only in a CERTCertificate.  By calling STAN_GetCERTCertificate in the Stan function whose purpose is to get the "decoding", I ensure that the trust flags will have been found, examined, and will be considered in the subsequent call to dcp->isValidIssuer().  I know of no other way to accomplish that.  

My only concern about this patch is that it may cause certain aspects of the cert to be evaluated more than once, and thus have a detrimental performance impact.  But in this case, correctness is more important to the security than performance.
Comment 10 Robert Relyea 2008-12-10 16:00:42 PST
In stan, trust is a different object from the cert, and certainly different from the Decoding (which is simply a cracked cert, no other information). The 3.X implementation  uses CERTCertificate as the decoded cert (IIRC), which, of course, has tons of other state (including trust). In stan, dcp->isValidIssuer should only return that the cert encoding itself meets the isValidIssuer test, it should have rolled up to a call that also tests the trust flags.

All that being said, the fact is CERTCertificate is more than just the decoding, so it seems reasonable to me to include the trust processing in our isValidIssuer.

Instead of adding the call to Stan code, it may be cleaner to move or include the extra STAN_GetCERTCertificate code to nssDecodedPKIXCertificate_Create instead (keeping it all in the pki3hack.c file).

If you are amiable to the change, I entertain a new patch. If not, I'll r+ this one.

bob
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-12-10 21:46:15 PST
The essence of this bug is that function filter_certs_for_valid_issuers in 
file lib/pki/certificate.c does not take trust flags into account when 
deciding which certs are, and which are not, valid issuers.  For each cert, 
that function calls nssCertificate_GetDecoding, and then calls 
dcp->isValidIssuer(dcp).  Since the trust information is not part of the 
decoded Stan certificate, this test is fundamentally flawed, because it 
ignores that trust information.  It answers the question "Does this cert
appear on its face to be a valid issuer?", but does not address the question "Does this cert have trust flags that make it be a valid issuer despite not appearing to be one?".

The question is: how should we fix that?  

I can think of numerous different approaches to it.  I don't know which 
(if any) of those is most aligned with the design intent of Stan.  

The idea which first came to me was to change dcp->isValidIssuer(dcp) so
that it takes trust info info account, despite that info not being part
of the decoded Stan cert.  

Maybe a superior answer is to modify filter_certs_for_valid_issuers so 
that it evaluates answers to both questions 
"Does this cert appear on its face to be a valid issuer?", and
"Does this cert have trust flags that make it be a valid issuer?"

Perhaps I should ask: Is there a Stan function that attempts to answer 
that latter function?  Or does one need to be written??  
(It would be nice if one already exists that could just be called as is.)
Comment 12 Robert Relyea 2008-12-17 17:25:24 PST
Comment on attachment 352021 [details] [diff] [review]
patch v1 

r+ this patch in case you need a fix quickly.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2008-12-17 17:57:59 PST
Bob, I went and looked into this some more, and found the NSSCertificates
do have a pointer to a trust domain, and an accessor function to return it.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/nsspki.h&rev=1.12&mark=258-267#258

I thought that in our discussion we had concluded that NSSCertificates exist 
apart from trust domains and may simultaneously exist in multiple domains.  
If an NSSCertificate has a trust domain associated with it, then I see no 
reason to avoid associating trust info with it too.  

So, I continue to want to "do the right thing" from a Stan perspective,
and I continue to not know what that is.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2009-02-08 23:52:40 PST
I'm not entirely happy with this patch, but it does seem to fix the 
problem, and we need a fix for it now.  Perhaps a better replacement
patch can be devised later.
Checking in pki/certificate.c; new revision: 1.66; previous revision: 1.65

Note You need to log in before you can comment on or make changes to this bug.