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)
Clearly this points out a deficiency in our regression test scripts.
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?
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.
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.
This regression has occurred since NSS_3_12_RTM. :(
Regression has occurred since NSS_3_12_2_RC1. :(
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.
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 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.
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
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 on attachment 352021 [details] [diff] [review] patch v1 r+ this patch in case you need a fix quickly.
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.
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