Erroneous root CA tests in NSS Libraries

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
14 years ago
9 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(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,
                                    &subjectCert->derIssuer);
    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.
(Reporter)

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
(Reporter)

Updated

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

Updated

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

Updated

11 years ago
QA Contact: jason.m.reid → libraries
(Assignee)

Comment 2

10 years ago
This should be fixed for 3.12 .
(Assignee)

Comment 3

9 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 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
removed.

>-    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.
Thanks.
(Assignee)

Comment 5

9 years ago
Nelson,

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 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+
(Assignee)

Comment 7

9 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
done
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.46; previous revision: 1.45
done
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.162; previous revision: 1.161
done
Checking in ssl/cmpcert.c;
/cvsroot/mozilla/security/nss/lib/ssl/cmpcert.c,v  <--  cmpcert.c
new revision: 1.6; previous revision: 1.5
done
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.