164744 - rfe: nss 3.4.2 - pk12util do not support export for multiple certs

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P1)

Description

[Marc Sauton]

This is a request for enhancement.

In nss 3.4.2 (anr problably all versions), the pk12util tool do not export key
encipherment and signing certificates in a p12 file, while an export from
mozilla 1.0, Communicator 4.x, Nscp 7.0 do export my 2 certificates in a pkcs#12
file.
An "openssl pkcs12 -in testme.p12" do show one set of 2 keys, while in the
second case, I have 2 pairs, as expected.

In some cases, there is a need to get the 2 certs in a p12 file, without using a
browser.

Marc.

Comment 1

[Julien Pierre]

Attachment: quick patch to handle multiple certs

This patch solves the problem, but the error handling may not be optimal
currently. Reviews invited.

Comment 2

[Marc Sauton]

Attachment: simple test case for 164744

Comment 3

[Julien Pierre]

Attachment: updated patch

Cleaned up indentation
Use slot from first cert to create the export context (presumably for keygen)

Review welcome

Comment 4

[Julien Pierre]

I just realized that no one was cc'ed on this bug.
I'd appreciate a review of patch 980428.

Thanks.

Comment 5

[Robert Relyea]

Comment on attachment 98042 [details] [diff] [review]
updated patch

But not much...

We need to check that slot != NULL before we do the CreateExportContext() call.

I'm not sure how well this code will work with mixed token and user certs, but
it looks like the new code will behave at least as well or better than the old
code in this case.

One other thing: we should probably screen out non-user certs from the list so
that old certs without keys won't case the backup to fail.

Basically the rest looks good.

bob

Comment 6

[Julien Pierre]

Attachment: updated patch

This patch implements Bob's comments, as well as changes to apply successfully
to the current tree. Other changes to pk12util made a merge necessary.

Comment 7

[Wan-Teh Chang]

Raised priority to P1.

Comment 8

[Julien Pierre]

Attachment: patch update to filter certs in the list by user

Comment 9

[Julien Pierre]

Attachment: patch to export CERT_RemoveCertListNode

Comment 10

[Julien Pierre]

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.85; previous revision: 1.84
done

Checking in pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.23; previous revision: 1.22
done

Comment 11

[Wan-Teh Chang]

Attachment: patch to destroy the cert list correctly

This patch should be applied on top of Julien's patch
(attachment 101331 [details] [diff] [review]).  We were destroying the certs on
the cert list but not the cert list itself.  The
CERT_DestroyCertList function takes care of both.
We also need to "goto loser" if the cert_UserCertsOnly
call fails so that the cert list is destroyed.

Julien, I have a question about your patch.  You moved
the following code into the new for loop:
    
	keySafe = SEC_PKCS12CreateUnencryptedSafe(p12ecx);
	if(/*!SEC_PKCS12IsEncryptionAllowed() || */ PK11_IsFIPS()) {
	    certSafe = keySafe;
	} else {
	    certSafe = SEC_PKCS12CreatePasswordPrivSafe(p12ecx, pwitem,
		SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_40_BIT_RC2_CBC);
	}
   
	if(!certSafe || !keySafe) {
	    SECU_PrintError(progName,"key or cert safe creation failed");
	    pk12uErrno = PK12UERR_CERTKEYSAFE;
	    goto loser;
	}

Do we need to create keySafe and certSafe inside the
for loop?  Can we pass the same keySafe and certSafe
to SEC_PKCS12AddCertAndKey for all the certs on the
cert list?

Comment 12

[Wan-Teh Chang]

Comment on attachment 101331 [details] [diff] [review]
patch update to filter certs in the list by user

I have another comment about this patch.  The
cert_UserCertsOnly function was cut and pasted
from nss/lib/certhigh/certhigh.c.  While this
quick fix is fine for 3.6, we should consider
turning cert_UserCertsOnly into an exported
function, say CERT_FilterCertListForUserCerts,
in the next release.

Comment 13

[Julien Pierre]

Wan-Teh,

In response to comment #11 :

I remember doing some experiment with destroying the certlist, and I think I had
problems with double-freeing of the certs. I thought I had resolved that issue
but I can't find the code where I did that anymore. I'll look into it again. In
any case, the patch works as it is and the leak isn't that important since it
happens in the tool.

As far as the question of whether the 2 functions should be in the loop, I took
most of the code from the PSM "backup all" function. I'm not sure if it's a
requirement but I know it works that way. I will try changing it and see if it
breaks.

In response to comment #12 :

While I did copy and paste the function, I also modified it slightly to return
SECFailure if the returned list was empty in the tool.

Comment 14

[Robert Relyea]

The CERTCertList_Destroy() destroys the certlist and all the certs on the list.
You need to remove the incremental destruction of certs in your main loop. You
should only destroy a cert if you have removed it from the cert list and are now
through with it.

bob

Comment 15

[Wan-Teh Chang]

Julien, we can either implement the
CERT_FilterCertListForUserCerts function using the same
prototype as your version of cert_UserCertsOnly, or add a
new CERT_LIST_IS_EMPTY macro:
#define CERT_LIST_IS_EMPTY(certList) \
    CERT_LIST_END(CERT_LIST_HEAD(certList), certList)

In any case, I just wanted to propose that we share the
code because the logic of determining a user cert is
not obvious at all.

Comment 16

[Julien Pierre]

This is in response to comment #15 :

A CERT_LIST_IS_EMPTY macro is useful to have anyway, independent of this bug.

The core issue here is that we don't have a function that says :
PRBool CERT_IsUserCert(CERTCertificate*);

I think that's what should be implemented and exported, by looking at the trust
flags.

If we do that, then exposing a CERT_FilterCertListForUserCerts filtering
function becomes secondary. We could still do it, and if we do the
implementation would call CERT_IsUserCert to perform the test. I am not sure
that it should necessarily return an error if the resulting list is empty. There
could be other errors possibly, with the content of the list, etc - that are
real errors. By providing this function and the CERT_LIST_IS_EMPTY macro, the
functionality needed for pk12util is provided, so that would be my preferrd
solution.

Should I implement these for 3.6 rtm ?

Comment 17

[Julien Pierre]

Attachment: patch using and implementing CERT_LIST_EMPTY, CERT_IsUserCert; CERT_FilterCertListForUserCerts

Comment 18

[Julien Pierre]

FYI, I tested the patch with our QA script and all the pk12util tests passed. It
should be noted that dual-key certs aren't tested by the script currently, and
we probably should add that.

Comment 19

[Wan-Teh Chang]

Comment on attachment 101477 [details] [diff] [review]
patch using and implementing CERT_LIST_EMPTY, CERT_IsUserCert; CERT_FilterCertListForUserCerts

Bob, Nelson, what do you think of these new
functions?

Julien, thanks for putting together this patch.
This patch is correct.	I just wanted to suggest
two stylistic changes.

1. I think CERT_LIST_IS_EMPTY is better, but maybe it's
better to use the shorter CERT_LIST_EMPTY.

2. In lib/certdb/cert.h

>+/* is cert a user cert ? ie. does it have CERTDB_USER trust,
>+   ie. a private key
>+*/
>+PRBool CERT_IsUserCert(CERTCertificate* cert);

Please use the prevalent comment block style in this header,
which is either this:

/*
 * line 1
 * line 2
 */

or this:

/*
** line 1
** line 2
*/

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Does the new function CERT_IsUserCert perform a sufficient test?

The CERTDB_USER trust flags are a hint, a reminder that at one time NSS
believed that it possessed the private key that complements this cert,
and that we may still possess it.

I think it is possible for the CERTDB_USER flag to get out of sync with 
the token.  For example, if I remove my key3.db file, a new one will get
generated the next time I use NSS (initialized for read/write).  
Thereafter, my cert DB may continue to have the CERTDB_USER flag set,
but the private key will not actually be present.   

Perhaps under some circumstances we should double check, and confirm 
the accuracy of the CERTDB_USER flags by actually finding the private key
on the token.  

Should this function take an additional argument that causes it to do that
confirmation?  

Should there be a separate function named CERT_IsUserCertDoubleChecked
or CERT_ReallyIsUserCert?  :)

When, if ever, does the CERTDB_USER flag get updated to reflect the actual
presence of the private key?  

Also, seems to me that all or none of the 3 CERTDB_USER flags should be
set.  Either we have the private key or we don't.  There's no way to have
the private key for email but not for SSL, AFAIK.

I don't know the answers to the questions I just posed.  It may be that 
in NSS 3.4 the CERTDB_USER flag is confirmed each time NSS is run.  

Comment 21

[Julien Pierre]

Nelson,

You are correct that the trust bits may be out of sync as to whether we actually
have the private key. Though it is much less likely that we have the private key
yet the trust bit is unset, it is also possible if users mix & match databases.

Therefore this function depends on the bits matching the actual data. Otherwise,
the result will just be inaccurate.

As I understand, many of the performance improvements that Bob made to NSS in
3.6 come from bypass the actual check for the private key, and trusting the
trust bits to accurately reflect the content of the cert & key databases. So, it
is not just this function that would be affected in 3.6, but many others as
well. In fact, I merely moved this code that checked for a user cert from a
private function to an exported function - but that code was already in use in
certhigh in the filtering function cert_UserCertsOnly.

Over a month ago, I suggested in an NSS meeting that we should have a database
recovery API to reconcile the trust bits and the database content. According to
Bob, this required too much additional code and testing to implement for 3.6. I
haven't filed a bug about this problem yet, but it's probably time to file it.

You make a good point about the need to have two functions - one that would do
the strict check. However, I think that's just a little confusing for the
application programmer - having one fast function that might lie, and another
slow function that will always be correct. So I would prefer that we implement
the database reconciliation API as a solution.

As far as the possibility of having the private key for one purpose (e-mail) but
not another (SSL), my guess is that the trust bits are only set for valid usages
of the cert. So if you have an e-mail only cert you would need to check for the
CERTDB_USER flag on the email property. This would explain why Bob was checking
for the bit on all 3 cert usage types in his original code - which I basically
copied into CERT_IsUsercert.

Comment 22

[Julien Pierre]

I meant to add the following comment, which got caught unsbmitted in a "mid-air
collision" with Nelson. This was in response to Wan-Teh's review in comment #19 :

"I had renamed the macro CERT_LIST_EMPTY , dropping "_IS" because there was
another boolean macro called CERT_LIST_END just above.

I changed the comment in cert.h per your suggestion.

Patch is checked in."

Comment 23

[Ian McGreer]

> I think it is possible for the CERTDB_USER flag to get out of sync with 
> the token.  For example, if I remove my key3.db file, a new one will get
> generated the next time I use NSS (initialized for read/write).  
> Thereafter, my cert DB may continue to have the CERTDB_USER flag set,
> but the private key will not actually be present.   

In NSS 3.4+, the user trust bit stored in the database is ignored.  For a cert
to have user trust bits, it must have a private key in the database (or public
key, if he is not logged in).  Try using certutil to set the trust of a user
cert in your database to ",,", you will see the trust still show up as "u,u,u".
 That was not the case before 3.4, but I would consider the pre-3.4 behavior to
be a bug.

The function PK11_IsUserCert is used to determine the user trust bits.

It's possible that a cert had a private key when it was created, but has since
lost it, and thus the cached (in-memory) trust bits are incorrect.  But I'm not
sure that that case is worth pinging the token constantly for the existence of
the private key.

Comment 24

[Robert Relyea]

One small nit with Ian's description:

The bits in the trust DB are no longer ignored in NSS 3.5 and NSS 3.6. It
doesn't affect his description, however. As part of the performance work I did,
I made certificates which were not user certs have a NULL CKA_ID. If a cert had
a NULL CAK_ID, we did not try to find a key for it. This means if the bits
aren't set in NSS 3.5 or 3.6, the cert will not show up as a user cert (as it
did in NSS 3.4). However if the bits are set and the key does not exist, the
cert will also not show up as a user cert (were it would have in NSS 3.3 and
earlier). In our current system, the upper level trust buts are set if and only
if both the low level user bits are set and the key exists, which is the test I
think Nelson was asking for.

bob

Comment 25

[Ian McGreer]

When we import a key, do we set the CKA_ID for the corresponding cert (if it
exists)?  When this happens, do the user trust bits in the db get updated?  What
if a private key is deleted, but the cert remains present?

Comment 26

[Robert Relyea]

When import a cert or a key, we check for the presence of that key or cert. Also
when ever we try to set the CKA_ID on the key or cert, we update CERTDB_USER bits.

We really don't store the CKA_ID value in the database for certs, it's
calculated (and happens to be calculated to the same value that NSS would have
set it to).

I'm not sure if we turn the bits off if we delete the key. I don't believe any
of our apps currently delete the key without deleting the cert, so that could
easily be a missing corner case that we don't handle correctly.

Comment 27

[Julien Pierre]

Bob,

If I read your comment #24 correctly, that means that if somebody had a user
cert and deletes key3.db, then the cert will not show as a user cert. Is that
indeed the case ?

What about the opposite case ? Ie. somebody had the cert without the key, and
then copies over a key3.db that contains the key. Will the cert then show up as
user cert or not ? This is a much less common case but still one that exists.

As far as Ian's comment #23, it seems that PK11_IsUserCert already does what we
need, so it sounds like I should just remove CERT_IsUserCert and use
PK11_IsUserCert.

Comment 28

[Ian McGreer]

Except that PK11_IsUserCert always looks for the private key, which is costly. 
It   should really be called PK11_IsPrivateKeyAvailableForCert, or something
like that.

I think the CERT_IsUserCert is useful, as it just takes the cached entry.  But I
think it should be a macro, instead of a function.

Comment 29

[Robert Relyea]

Dropping in a new key3.db and expecting to to magically match up with certs that
happened to be in your certdb is pretty unlikely, so your second scenario is
pretty contrived. For completeness here is the difference in the various releases.

             deleting a key3.db        adding a key3.db*     restoring a key3.db%
NSS 3.3:      Cert is User Cert       Cert is not User Cert    Cert is User Cert
                (incorrect)              (incorrect)             (correct)
NSS 3.4:      Cert is not User Cert   Cert is User Cert        Cert is User Cert
                (correct)                (correct)                (correct)
NSS 3.6:      Cert is not User Cert   Cert is not User Cert    Cert is User Cert
                (correct)                (incorrect)              (correct)

* This assumes you are copying a key3.db to a database that had the cert
installed (somehow) without any key installed in the key3.db.
% This assumes that you restored a key3.db which was previously deleted. The
user cert will have the CERTDB_USER unless it was re imported between the time
the key3.db was removed and restored.

In addition 3.6 is self healing if you try to reimport the cert and/or key.

NOTE: the difference between 3.4 and 3.6 yeilds a drastic performance
improvement in dealing with certificates, particularly listing certificates.

bob

Comment 30

[Wan-Teh Chang]

I am a little overwhelmed by the discussion.  So, is
it okay to add the new functions CERT_IsUserCert and
CERT_FilterCertListForUserCerts in NSS 3.6?  Neither
of them is required for this bug, although the
CERT_FilterCertListForUserCerts function allows us
to avoid copying the cert_UserCertsOnly function
to pk12util.c.

Please let me know what you think is the right thing
for 3.6.

Comment 31

[Julien Pierre]

Wan-Teh,

Given the information posted today, I'd say that it is OK to leave everything
exported as checked in yesterday.

Comment 32

[Wan-Teh Chang]

Comment on attachment 101477 [details] [diff] [review]
patch using and implementing CERT_LIST_EMPTY, CERT_IsUserCert; CERT_FilterCertListForUserCerts

r=wtc.