Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Erroneous root CA tests in NSS Libraries



14 years ago
10 years ago


(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Julien Pierre)


Firefox Tracking Flags

(Not tracked)



(1 attachment)

2.91 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
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

14 years ago
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.
Priority: -- → P2
Target Milestone: --- → 3.10


12 years ago
QA Contact: bishakhabanerjee → jason.m.reid


12 years ago
Assignee: wtchang → julien.pierre.bugs
Target Milestone: 3.10 → 3.12


12 years ago
QA Contact: jason.m.reid → libraries

Comment 2

10 years ago
This should be fixed for 3.12 .

Comment 3

10 years ago
Created attachment 300570 [details] [diff] [review]
Fix bogus root CA tests

These are all the ones I found.
Attachment #300570 - Flags: review?(nelson)

Comment 4

10 years ago
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

10 years ago

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

10 years ago
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
Attachment #300570 - Flags: review?(nelson) → review+

Comment 7

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.