Open Bug 594297 Opened 14 years ago Updated 1 year ago

softoken imports certs with same nickname but different subjects, corrupts DB

Categories

(NSS :: Libraries, defect, P3)

3.12.7

Tracking

(Not tracked)

3.12.9

People

(Reporter: brauckmann, Unassigned)

Details

Attachments

(6 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.1.11) Gecko/20100714 SUSE/3.5.11-0.1.1 Firefox/3.5.11
Build Identifier: nss-3.12.7

I have a certificate database with two certificates. Both have nickname "test@example.com" and contain an email address "test@example.com". The certificates have different Subject-DNs.


When calling certuil with -L, both certificates are displayed.

When calling certutil with certutil -L -n test@example.com, just the first certificate is displayed.

Reproducible: Always

Steps to Reproduce:
1. certutil -L -d testdb

2. certutil -L -d testdb -n test@example.com
Actual Results:  
Just the first certificate is displayed

Expected Results:  
Both should be displayed

The problem seems to be certutils strategy to retrieve all certificates for a given nickname/email address:

In mozilla/security/nss/cms/certutil.c:470, one certificate with matching nickname/email address is retrieved. Some lines later, all certificates with a subject-dn matching the subject-dn of the first certificates are retrieved.

This is wrong, as there may be more certificates with identical email address but different subject dn.


Unfortunately, the same strategy is used by thunderbird. This strategy can prevent thunderbird from finding a valid recipient certificate for email encryption.
Version: unspecified → 3.12.7
One root certificate and two email certificates for test@example.com. Nickname test@example.com. The two email certificates have a different subject-dn.
The situation described in the first paragraph of comment 0 is the very 
definition of a "corrupt database".  In a proper database, ALL the 
certificates that share a nickname also shared a common subject DN. 

An attempt to add a certificate with a different subject DN but the same
nickname as an existing cert in the DB should have any of the following 
outcomes:

- failure
- cert added with a different nickname than the one requested, 
  one that is either:
  -- new and unique (if there are no other certs with the same subject), or 
  -- common to other certificates with the same subject DN as the one added.

If you can find a series of steps that take a non-corrupt database and 
add a certificate to it, making it be corrupt, that is, making it have 
two certs with the same nickname but different subject DNs, and can add 
those steps-to-reproduce to this bug, that would be VERY helpful. 
I don't doubt that they exist.  I've just never found them.
Attached file Test root certificate
Creating a database with certificates with equal nicknames but different subject DNs is actually quite easy:

- Use Firefox or Thunderbird 

- Import the certificates "Test root certificate", "Test user certificate 1" and "Test user certificate 2" with the certificate manager of Firefox/Thunderbird

- Thats all. After that, the cert8.db of that application contains two certificates with nickname test@example.com but with different subject DN (CN=Joe Test,OU=Org2,O=Test,C=DE and CN=Joe Test5,OU=Org1,O=Test2,C=US)


Tested with:

Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.9) Gecko/20100825 Thunderb
ird/3.1.3

Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.1.11) Gecko/20100714 SUSE/3.5.11-0.1.1 Firefox/3.5.11

It works with either an empty cert8.db or with an cert8.db that was already in use for several years.

Thunderbird/Firefox always sets the email-address of a certificate as its nickname, even if there already exists a certificate with same nickname but different subject DN.
Hm,  certutil uses a different way of importing a certificate than other Mozilla apps.

certutil.c, function AddCerts(), approx. line 187:
certutil uses PK11_ImportCert(), which expects a nickname and seems to complain if the nickname is recycled with a certificate with different SubjectDN.

Other mozilla apps seem to use the code from  mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp, method nsNSSCertificateDB::ImportEmailCertificate().
This method uses  CERT_ImportCerts(), which does not take a nickname as parameter and which seems to "compute" the nickname from the emailaddress.

I have modified certutil to use CERT_ImportCerts(), and it always adds certificates with their email addresses as nickname, regardless whether this nickname already exists or not. I can create the same type of corrupt database with the modified certutil that Firefox or Thunderbird creates when importing the certificates in comment 7.
Thanks very much for that research, Jürgen, especially for finding out how
mozilla apps do it differently from certutil.  It should be the case that,
ultimately, both PK11_ImportCert and CERT_ImportCerts use the same 
function in lib/softoken to actually import the cert.  It would be a function
called through a pointer, named NSC_<something>, such as NSC_CreateObject or
NSC_SetAttributeValue.  That function, and the functions it calls, are the 
code ultimately responsible for ensuring that the rules about nicknames 
(which are encoded as CKA_LABEL attributes) and subject names are followed.
Apparently, under some circumstances, that code fails to do its job, and you
have now provided a very simple test case, so it should not be too difficult
to "get to the bottom" of this problem now.
Jürgen, Please attach your patch to certutil, so that I can use it to 
reproduce your results without needing to use Thunderbird or Firefox.
Thanks.
I'm elevating this to P1, in anticipation of its confirmation.  It will be confirmed when an NSS team member can reproduce with a caller of 
CERT_ImportCerts.
Component: Tools → Libraries
Priority: -- → P1
QA Contact: tools → libraries
Summary: certutil does not list all matching certificates with -n → softoken imports certs with same nickname but different subjects
Target Milestone: --- → 3.12.9
Bob, I think you should take a look at this.  Very serious.
Assignee: nobody → rrelyea
Summary: softoken imports certs with same nickname but different subjects → softoken imports certs with same nickname but different subjects, corrupts DB
Attachment #473958 - Attachment description: Modified certutil.c to use CERT_ImportCerts instead of PK11_ImportCert → Modified certutil.c to use CERT_ImportCerts instead of PK11_ImportCert, based on NSS-3.12.7 source tar ball
Just another remark:

My original reason for opening this bug was a situation where thunderbird was not able to find a valid email receipient certificate in a certdb.

My hope was that it was able to reproduce a very similar situation with certutil, which seems much easier do debug than Thunderbird, thus this bug report against certutil

The certdb that triggered the situation contained two certificates with identical email addresses, identical nicknames, and different subject DNs. One certificate was expired, the other still valid.

Thunderbird did find only the first, expired certificate and refused mail encryption (naturally).

Currently, this bug tries to resolve the issue with the identical nicknams. I don't think that thats the main reason for my original problem. 

I think that there is a second issue, either in NSS or in Thunderbird.

Thunderbird and certutil use the same strategy for searching certificates. They both do the following (certutil.c  listCerts(), mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp nsNSSCertificateDB::FindCertByEmailAddress()):

the_cert = CERT_FindCertByNicknameOrEmailAddr( nickname/emailadress);

certs = CERT_CreateSubjectCertList(NULL, handle, &the_cert->derSubject,
		PR_Now(), ...);

The certutil man page documents that certutil -A -n < nickname> only finds certificates with matching nickname, but in fact the code also allows for finding certs with matching emailadress (independent of the nickname). 

Thunderbird uses this code for finding valid certificates with a matching email-address for email encryption.

This strategy does only work for Thunderbird if CERT_FindCertByNicknameOrEmailAddr() only returns valid certificates: If the first certificate that is found is invalid, and there are more certificates for the email address that are valid but have a different subject-DN, they are not found.

But CERT_FindCertByNicknameOrEmailAddr() also returns invalid certificates (verified with certutil). So either CERT_FindCertByNicknameOrEmailAddr() has a bug, or thunderbird makes invalid assumptions about CERT_FindCertByNicknameOrEmailAddr().

If this is not an NSS bug: Which Component/Product does mozilla/security/manager/ssl/ belong to?

Thanks for your patience :-)
(In reply to comment #14)
> But CERT_FindCertByNicknameOrEmailAddr() also returns invalid certificates
> (verified with certutil). So either CERT_FindCertByNicknameOrEmailAddr() has a
> bug, or thunderbird makes invalid assumptions about
> CERT_FindCertByNicknameOrEmailAddr().
> 
> If this is not an NSS bug: Which Component/Product does
> mozilla/security/manager/ssl/ belong to?

Core / Security: PSM (or for this specific issue, Security: SMIME). See bug 558337 for another problem with nsNSSCertificateDB::FindCertByEmailAddress(). And greetings to Hamburg, BTW ;-)
>  such as NSC_CreateObject or
> NSC_SetAttributeValue.  That function, and the functions it calls, are the 
> code ultimately responsible for ensuring that the rules about nicknames 
> (which are encoded as CKA_LABEL attributes) and subject names are followed.
> Apparently, under some circumstances, that code fails to do its job, and you
> have now provided a very simple test case, so it should not be too difficult
> to "get to the bottom" of this problem now.

Actually no, The upperlevel of NSS needs to enforce the guarantees. PKCS #11 modules are not required to have this semantic, so we need to make sure it's enforces at the NSS level. (Fixing softoken only fixes it part of the time).

One wonders how this can happen the DBM level itself. Nicknames are stored in the cert db as keys that contain subject entries. We can't physically store more than one nickname for a cert


> My original reason for opening this bug was a situation where thunderbird was
> not able to find a valid email receipient certificate in a certdb.

This is the real problem that needs to be fixed. Event though nicknames map one-for-one in Subjects, email addresses are allowed to map to more than one certificate. If thunderbird can't find them with the existing calls, We need to provide new ways to find them.

bob
(In reply to comment #15)
> Core / Security: PSM (or for this specific issue, Security: SMIME). See bug
> 558337 for another problem with nsNSSCertificateDB::FindCertByEmailAddress().

Thanks Kaspar for the pointer to that bug! The issue I'm describing really seems to be just another symptom of what is already reported in bug 558337 or bug 337430 (dating back to 2006).

I think that bug 337433 could solve the issue if the proposed  	CERT_FindCertByNicknameOrEmailAddrForUsage() only returns valid certificates. Otherwise Thunderbird would still not be able to find valid certificates if an email recipient has invalid and valid certificates in the certdb.

> And greetings to Hamburg, BTW ;-)

Greetings back!
(In reply to comment #17)
> I think that bug 337433 could solve the issue if the proposed     
> CERT_FindCertByNicknameOrEmailAddrForUsage() only returns valid certificates.

Correction: 
I think that bug 337433 could solve the issue if the proposed     
CERT_FindCertByNicknameOrEmailAddrForUsage() always returns a valid certificates if there is a mixture of expired and valid certificates for a given emailaddress/nickname n the database.
(In reply to comment #18)
> if the proposed     
> CERT_FindCertByNicknameOrEmailAddrForUsage() always returns a valid
> certificates if there is a mixture of expired and valid certificates for a
> given emailaddress/nickname n the database.

With the implementation proposed in attachment 467521 [details] [diff] [review], this would indeed be the case.

In theory, the code which tries to figure out the best cert from a list of candidate certs is already in place (and the NSSCryptoContext_FindBestCertificateBy* functions will prefer valid over expired certs, and certs with a later notAfter date over others - see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/pkibase.c&rev=1.33&mark=518-539#515 and http://mxr.mozilla.org/mozilla/ident?i=CERT_IsNewer).

However, my guess is that many of these problems would go away if there was actually an implementation of CERT_FindCertByEmailAddr, and nsNSSCertificateDB::FindCertByEmailAddress could use that function (instead of CERT_FindCertByNicknameOrEmailAddr). When looking for an e-mail cert, the nickname is more or less irrelevant - and in certain situations it even has adverse effects on the search results, as this bug illustrates.

(In reply to comment #16)
> If thunderbird can't find them with the existing calls, We need to
> provide new ways to find them.

I would therefore favor a smart implementation of CERT_FindCertByEmailAddress (http://mxr.mozilla.org/mozilla/ident?i=CERT_FindCertByEmailAddr - even its signature can still be changed, e.g. by adding a keyUsage argument, as it hasn't been exported yet).

Adding Kai to the Cc, as he has spent quite some time on code in this area in the past.
OS: Linux → All
Hardware: x86 → All
This bug report seems to have touched on several separate and distinct issues.
Separate bug reports should be filed for each one.  They include
1) Softoken should not allow multiple certs with different subject names to 
share a nickname (this bug).
2) Higher layers of NSS, outside of (above) softoken, should also enforce 
this, (See comment 16).
3) NSS should provide a way to find ALL valid certs for an email address
(also from comment 16).  
4) PSM should make use of that method to find email certs.

Please file separate bugs for items 2, 3 and 4.  Thanks.  

In comments below, I will specifically be addressing the first of these issues.
Attachment #473958 - Attachment mime type: text/x-csrc → text/plain
Attached patch certutil modification as a patch (obsolete) — Splinter Review
I converted the above attached modified certutil.c source file into a patch
against the NSS_3_12_7_RTM tag, in hopes that it would apply to the tip of 
the trunk.  Here is that patch.  I have not yet tested it.
Jürgen, what are the exact certutil commands you use to add the three test 
certs to a test DB to reproduce this problem?
I ran NSS's database checker program on the database attached to this bug
(first attachment), and it confirmed that the DB is self-inconsistent
(which we call "corrupt").  

Here's a bit of background on the Berkeley (cert7, cert8) DB schema.
The DB has effectively 4 different tables.  Each table has a single column 
taht is the "unique key" or index for that table, and one or more other data columns.  Each table is named after the index column/field for that table.
They are listed below.  The information given here is not complete in every 
detail.

DER Issuer Name and Serial Number table:
Index: DER Issuer Name and Serial Number of a cert
Data fields:  DER cert (entire cert)
              DER Subject Name table index

DER Subject Name table:
Index: DER Subject Name:
Data fields: 1) A list of (DER Issuer Name and Serial Number) table index values
             2) a nickname table index OR SMIME table index value

Nickname table
Index: UTF8 Nickname
Data Field: DER Subject Name

SMIME table:   (also sometimes called the email address table):
Index: RFC 822 email address:
Data Fields: 1) DER subject Name
             2) S/MIME capabilities structure from a signed message

Every Subject Name record must have zero or one nickname table records 
pointing to it, and zero or more email address records pointing to it.  
It must have AT LEAST ONE nickname or email address record pointing to it.

Originally, it was the intent that a subject name record would always have
exactly one record pointing to it, either a nickname OR an email address
record.  But I have reason to believe that we have not prevented a subject
name record from having both at once.

A nickname record can have an RFC 822 email address as the nickname.  This 
may be confusing.  For most purpsoes, SMIME records are treated just like
nicknames.  Most searches by nickname actually search both the nickname 
table and the SMIME table for a match.  Which each table has at most one 
entry of any value, it is possible for both tables to have an entry with 
the same value, which is an RFC 822 email address.  

In the case of the DB attached above, there are 3 IS+SN records, 3 subject
name records, one nickname record and one SMIME record.  Visually it is:

Cert 0 ----> Subject 0 ----> Nickname 0
Cert 1 ----> Subject 2 ----> S/MIME   0
Cert 2 ----> Subject 1 ----> **********
Here's a MUCH smaller patch to certutil that, I believe, produces the same 
series of database updates as the previous patch did.  It only changes the
behavior of certutil's -E command.

The commands I used were:
mkdir 594297
certutil -N -d 594297
certutil -A -d 594297 -n "Test-PKI Root"  -t ",C," -a -i /tmp/594297c.pem
certutil -E -d 594297 -n test@example.com -t ""    -a -i /tmp/594297d.pem
certutil -E -d 594297 -n test@example.com -t ""    -a -i /tmp/594297e.pem

where the c, d and e filres are the 3 certificates given as attachments above.

The result was a set of files that has similar properties as reported above:
3 certs, 3 subject records, 1 nickname and 1 SMIME record, and one cert and subject record for which there was no SMIME record.

But I'm not sure I reproduced exactly the original report, because in my case
the orphaned cert was the last one added, and as I understand it, in the 
reported case, the orphaned cert was the second one, not the last one, added.
Attachment #473958 - Attachment is obsolete: true
Attachment #474579 - Attachment is obsolete: true
I began debugging the execution of the last of the above certutil commands.

Suspicious observation #1)
At this point in the stack:

__CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 307
CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 358
CERT_ImportCerts(certdb=0x0c78018, usage=certUsageEmailRecipient, ncerts=0x01,
    derCerts=0x012fe24, retCerts=0x0, keepCerts=0x01, caOnly=0x0, nickname=0x0)
    Line 2578
AddCert(slot=0x0c05400, handle=0x0c78018, name=0x0c19680, trusts=0x0c260f6,
    inFile=0x0c19640, ascii=0x01, emailcert=0x01, pwdata=0x012fef4)  Line 184
certutil_main(argc=0x0b, argv=0x0c26060, initialize=0x01)  Line 2869
main(argc=0x0b, argv=0x0c26060)  Line 3026

Immediately following these lines of code:
    if (!stanNick && nickname) {
	stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, c->object.arena);
    }
stanNick is NULL. :-/
At this point in the stack:

sftkdb_checkConflicts(db=0x0c1e150, objectType=0x01, ptemplate=0x0c89010,
    len=0x0c, sourceID=0x0)  Line 922
sftkdb_write(handle=0x0c1a380, object=0x0c92000, objectID=0x0c9200c)  Line 1204
sftk_handleCertObject(session=0x0c1e4c0, object=0x0c92000)  Line 721
sftk_handleObject(object=0x0c92000, session=0x0c1e4c0)  Line 1503
NSC_CreateObject(hSession=0x01000003, pTemplate=0x012fcb8, ulCount=0x0a,
    phObject=0x012fc6c)  Line 3838
import_object(tok=0x0c54018, sessionOpt=0x0, objectTemplate=0x012fcb8,
    otsize=0x0a)  Line 248
nssToken_ImportCertificate(tok=0x0c54018, sessionOpt=0x0,
    certType=NSSCertificateType_PKIX, id=0x0c8a070, nickname=0x0,
    encoding=0x0c8a078, issuer=0x0c8a080, subject=0x0c8a088, serial=0x0c8a090,
    email=0x0c91070, asTokenObject=0x01)  Line 588
__CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 323
CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 358
CERT_ImportCerts(certdb=0x0c78018, usage=certUsageEmailRecipient, ncerts=0x01,
    derCerts=0x012fe24, retCerts=0x0, keepCerts=0x01, caOnly=0x0, nickname=0x0)
    Line 2578
AddCert(slot=0x0c05400, handle=0x0c78018, name=0x0c19680, trusts=0x0c260f6,
    inFile=0x0c19640, ascii=0x01, emailcert=0x01, pwdata=0x012fef4)  Line 184
certutil_main(argc=0x0b, argv=0x0c26060, initialize=0x01)  Line 2869
main(argc=0x0b, argv=0x0c26060)  Line 3026

We read this comment:
    /* Currently the only conflict is with nicknames pointing to the same 
     * subject when creating or modifying a certificate. */

I think SMIME address conflicts may have been overlooked.
Here's another place where we have a nickname conflict check, and no 
corresponding SMIME entry conflict check.

nsslowcert_CertNicknameConflict(nickname=0x0, derSubject=0x0c0f038,
    handle=0x0c3da40)  Line 3977
nsslowcert_UpdatePermCert(dbhandle=0x0c3da40, cert=0x0c0f000, nickname=0x0,
    trust=0x012fb28)  Line 4569
nsslowcert_AddPermCert(dbhandle=0x0c3da40, cert=0x0c0f000, nickname=0x0,
    trust=0x012fb28)  Line 4605
lg_createCertObject(sdb=0x0c1e150, handle=0x0c9200c, templ=0x0c89010,
    count=0x0c)  Line 130
lg_CreateObject(sdb=0x0c1e150, handle=0x0c9200c, templ=0x0c89010, count=0x0c)
    Line 988
sftkdb_CreateObject(arena=0x0c1d880, handle=0x0c1a380, db=0x0c1e150,
    objectID=0x0c9200c, template=0x0c89010, count=0x0c)  Line 614
sftkdb_write(handle=0x0c1a380, object=0x0c92000, objectID=0x0c9200c) Line 1214
sftk_handleCertObject(session=0x0c1e4c0, object=0x0c92000)  Line 721
sftk_handleObject(object=0x0c92000, session=0x0c1e4c0)  Line 1503
NSC_CreateObject(hSession=0x01000003, pTemplate=0x012fcb8, ulCount=0x0a,
    phObject=0x012fc6c)  Line 3838
import_object(tok=0x0c54018, sessionOpt=0x0, objectTemplate=0x012fcb8,
    otsize=0x0a)  Line 248
nssToken_ImportCertificate(tok=0x0c54018, sessionOpt=0x0,
    certType=NSSCertificateType_PKIX, id=0x0c8a070, nickname=0x0,
    encoding=0x0c8a078, issuer=0x0c8a080, subject=0x0c8a088, serial=0x0c8a090,
    email=0x0c91070, asTokenObject=0x01)  Line 588
__CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 323
CERT_AddTempCertToPerm(cert=0x0c90010, nickname=0x0, trust=0x0)  Line 358
CERT_ImportCerts(certdb=0x0c78018, usage=certUsageEmailRecipient, ncerts=0x01,
    derCerts=0x012fe24, retCerts=0x0, keepCerts=0x01, caOnly=0x0, nickname=0x0)
    Line 2578
AddCert(slot=0x0c05400, handle=0x0c78018, name=0x0c19680, trusts=0x0c260f6,
    inFile=0x0c19640, ascii=0x01, emailcert=0x01, pwdata=0x012fef4)  Line 184
certutil_main(argc=0x0b, argv=0x0c26060, initialize=0x01)  Line 2869
main(argc=0x0b, argv=0x0c26060)  Line 3026
There's a block of code in pcertdb.c after a (false) comment that reads;

	/* make a new subject entry - this case is only used when updating
	 * an old version of the database.  This is OK because the oldnickname
	 * db format didn't allow multiple certs with the same subject.

Perhaps it should not allow subject entries to be written with NULL nicknames.
In function lg_createCertObject (see stack above), at about lines 143-158, 
having just added the cert to the IN+SN and Subject Name tables, 
the code looks in the DB for an existing SMIME record for the email address 
in the cert. If it finds a matching SMIME record, it does not update the 
SMIME record (which is probably a good thing), but the result is that the 
newly created IN+SN and Subject Name table entries are orphans at birth.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #22)
> Jürgen, what are the exact certutil commands you use to add the three test 
> certs to a test DB to reproduce this problem?

Attachment 472980 [details] was created by thunderbird, not certutil. I deleted the original cert8.db and key.db from the thunderbird profile directory, and used thunderbirds certificate manager to manually import attachment 473425 [details], attachment 473427 [details] and attachment 473428 [details].

My experiments with the modified certutil were conducted with the same series of steps that you did:

certutil -N
certutil -A -d db -n "Test-PKI Root"  -t ",C," -a -i /tmp/594297c.pem
certutil -E -d db -n test@example.com -t ""    -a -i /tmp/594297d.pem
certutil -E -d db -n test@example.com -t ""    -a -i /tmp/594297e.pem


Your patch and my modification give different binary representations of cert8.db, but I think that is due to my modification using CERT_ImportCerts(handle, certUsageEmailRecipient,...) for the root certificate as well. So that is probably not relevant.
Thanks Nelson for structuring this bug!

(In reply to comment #20)
> This bug report seems to have touched on several separate and distinct issues.
> Separate bug reports should be filed for each one.  They include
> 1) Softoken should not allow multiple certs with different subject names to 
> share a nickname (this bug).
> 2) Higher layers of NSS, outside of (above) softoken, should also enforce 
> this, (See comment 16).

I don't know how to describe the problem sufficiently to create a bug report, so you should not expect me to do that. :-)

> 3) NSS should provide a way to find ALL valid certs for an email address
> (also from comment 16).  

I created Bug 596215 

> 4) PSM should make use of that method to find email certs.

I created Bug 596221

> Please file separate bugs for items 2, 3 and 4.  Thanks.
Severity: normal → S3
Assignee: rrelyea → nobody
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: