Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 232392 - Erroneous root CA tests in NSS Libraries
: Erroneous root CA tests in NSS Libraries
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.9
: All All
: P2 normal (vote)
: 3.12
Assigned To: Julien Pierre
Depends on:
  Show dependency treegraph
Reported: 2004-01-27 23:03 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2008-02-01 14:09 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Fix bogus root CA tests (2.91 KB, patch)
2008-01-30 18:40 PST, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2004-01-27 23:03:46 PST
While reviewing the patch for bug 232377, I read and reviewed all the code
in function seckey_UpdateCertPQGChain, and noticed yet another place in 
NSS where the code attempts to determine if it has come to the end of a 
chain (to the self-signed root) by comparing the issuer name to the 
subject name.  e.g. 

    /* check if the cert is self-signed */
    rvCompare = (SECStatus)SECITEM_CompareItem(&subjectCert->derSubject,
    if (rvCompare == SECEqual) {

This code is wrong, because it ignores key ID extensions.  The NIST test
cases, which extensively test "self-issued" intermediate CA "roll-over" 
certs, do not detect this case, but it is still wrong.

Someone needs to go through all of NSS source code and find every place
that attempts to find the end of a CA with the above test, and fix every
one of them.

BTW, when we import a cert, we set a bit in the CERTCertificate, known
as cert->isRoot.  Most (if not all) of the places that do the test 
above should simply test that bit instead.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2004-01-27 23:06:43 PST
We shold fix this bug with the same priority, severity, anbd target release 
as the other NIST PKITS test failures.  I'm not sure if that's 3.9.1 or 3.10.
Comment 2 Julien Pierre 2007-07-10 22:04:25 PDT
This should be fixed for 3.12 .
Comment 3 Julien Pierre 2008-01-30 18:40:24 PST
Created attachment 300570 [details] [diff] [review]
Fix bogus root CA tests

These are all the ones I found.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-02-01 02:23:54 PST
Comment on attachment 300570 [details] [diff] [review]
Fix bogus root CA tests

As part of reviewing this patch, I need to ascertain whether the
isRoot variable can/will have been appropriately set in the code
path preceding each of these new tests.

BTW, this patch removes the only use of the variable rvCompare,
making that variable become unused.  So the variable should be

>-    rvCompare = (SECStatus)SECITEM_CompareItem(&subjectCert->derSubject,
>-				    &subjectCert->derIssuer);
>-    if (rvCompare == SECEqual) {
>+    if (subjectCert->isRoot) {

A request: please use the -p option in the cvs diff commands with 
which you generate patches, so that I can tell what function is 
being patched without having to greatly enlarge the diff context.
Comment 5 Julien Pierre 2008-02-01 02:44:43 PST

I spent much time checking that isRoot should be set previously before I submitted the patch. This is done by a call to CERT_DecodeDERCertificate . The only uncertainty I had was in the case of pk11cert.c , but I determined that there was no issue there either.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-02-01 12:02:55 PST
Comment on attachment 300570 [details] [diff] [review]
Fix bogus root CA tests

In reply to comment 5, OK, Julien, I'll take your work for it. :)  
Thanks for doing that check.

Remove the unused variable, then r=nelson
Comment 7 Julien Pierre 2008-02-01 14:09:24 PST
Thanks for the review, Nelson .

I checked this patch in to the trunk .

Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.64; previous revision: 1.63
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.46; previous revision: 1.45
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.162; previous revision: 1.161
Checking in ssl/cmpcert.c;
/cvsroot/mozilla/security/nss/lib/ssl/cmpcert.c,v  <--  cmpcert.c
new revision: 1.6; previous revision: 1.5

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