137645 - Cached public certificate does not get its nickname updated after P12 import of matching user certificate

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Charles Rosendahl]

Trunk/Branch builds 20020415.

If the recipient cert is already in the cert db, and the user imports the
corresponding keypair/cert into the client cert db via PSM, the cert is imported
(as verified by certUtil), but the cert does not show up in the user certificate
tab.  Instead, the other people cert displays all of the privileges of the user
cert.

Rationale:  I am userx, who also acts as [e-mail], [e-mail], etc.
 I set up multiple accounts within the mail client to support these roles.

test case:
1.  In browser 1 (ns4.79) create an account and set up secure mail settings. 
Send a signed message to browser 2.
2.  In browser 2 (latest builds), read the mail message - cert is now in 'Other
People' tab
3.  Export cert/keypair from browser 1.
4.  Import cert into Browser 2.

Serious certdb / client anomalies begin...certUtil verifies the presence of both
certs.

Comment 1

[Charles Rosendahl]

Rationale #2 (more compelling):  
My corporate CA only issues me one cert.
I use IMAP.
I go home and read mail messages that I have cc'd myself on.
Later I export my cert/keypair from work and go home and import it into my
client...I have to delete the cert db /key db to correct the problem.

Nominating for nsbeta1.

Comment 2

[Stephane Saux]

fix this for RTM

Read signed email you sent yourself with a software cert in a new profile, then
import the same cert -> import says it's successfull, yet as Charles reports,
the cert are not shown in the cert manager.

Comment 3

[Charles Rosendahl]

I noticed looking at before and after privileges in the cert db (certutil) that
the additional privileges are being added to the cert, but the cert is not
moving from one store to the other.

i.e.  if you have a cert/keypair and send yourself a message, the 'p' attribute
is added to your cert in the user cert store.  If you import a cert after you
already have the public cert in the recipient store, 'u,u,u' is added to the
recipient cert instead of moving the cert to the user store.

Comment 4

[Robert Relyea]

Charles, Certutil is showing the corrrect behavior. There is only one
certificate store. (Well actually there are many certificate stores, but there
is only one internal certificate store). Where the certificate is stored is the
'Security Device' field of PSM.

There are a number of different types of certs: The difference between these
various certificate types are simply difference in trust. Therefore certutil is
showing exactly what should be happening (cert goes from p,, (Peer cert) to
u,u,u (User cert)).

Comment 5

[Kai Engert (:KaiE:)]

Looking at the reported behaviour, it seems that the user cert has some internal
state in the NSS database, that prevents it from being identified as a user cert.

I'm unsure how I should debug this.

Comment 6

[Stephane Saux]

Bob Relyea should give us guidance here.

Comment 7

[Kai Engert (:KaiE:)]

Today I accidentially ran into this problem, and I think it is worse than what
his been reported.

I was using my private mail account in a separate profile, and with that profile
I had already received test messages from my corporate account. Therefore I had
the public key already, listed in the other's people tab.

In order to reproduce a bug, I imported my work certificate.

After I did I was able to decrypt a message encrypted for my work certificate.

Next I wanted to delete that cert. I did not found it in the list of my own
certificates, and remembered this bug. I opened the other's tab, and deleted my
cert.

But now, after restarting the application, I am still able to decrypt the
message, which I previously was not able to do.


Is it possible that my private key is still contained in the cert database?


(I already wondered in the past whether we should discuss, whether a user should
be able to view, which private keys are contained in a cert database - now seems
to be an appropriate occasion.)

Comment 8

[Robert Relyea]

In the general case you need both the cert and the key to decrypt the message.
That is because S/MIME describes the recipient in terms if issuer/serial number.
S/MIME v3 can use other means, which just identifies the key, but it's very
unlikely we are using them as v2 clients can't read the message.

The good news is we have a fix in the tip for this problem. The bad news is that
it requires several changes together to get it to work, and thus is not easy to
pull out as a patch (it's not likely to be a good beta candidate).

Kai, and you verify that this is working correctly with the tip?

bob

Comment 9

[Julien Pierre]

Stephane, following up on your test case in #2 , I sent myself signed 
(non-encrypted) email from Communicator. Read it in a brand new profile in 
mozilla trunk 2002050608 + NSS tip (without any certs). After I received the 
signed message and viewed it, my cert correctly showed up in "other people". So 
I can confirm that it works with the NSS tip. I have not tried the Mach V branch 
 and/or NSS 3.4 DLLs.

Comment 10

[Charles Rosendahl]

julien,

The test case is half complete.  You now need to import the certificate into
your cert db.  See if the recipient certificate moves from the Other People's
tab to 'Your' tab.

Comment 11

[Stephane Saux]

Julien,
Charles is right. The problem originally reported was about importing a cert
from p12 after the cert had already been added to the db as an email recipient cert.

Not only should the import be successful and the cert show up in the right
places (i.e., it should be in both the "Mine" and "Others" tabs), everything
should work correctly.

Comment 12

[Julien Pierre]

The import was successful and my cert showed both under mine and others. The 
security tab under mail server however would not let me select my certificate. A 
very strange UI came up with a blank drop-down list. I think this is related to 
the NULL nickname problem, see bugs 136920 / 142868 . 

Comment 13

[Julien Pierre]

There is definitely something fishy about doing a recipient cert install (public 
key) and then the private key (p12 import).
I created the following test case, which is reproducible :
1) sent myself signed e-mail from communicator
2) read it in mozilla with fresh cert7.db / key3.db 
3) imported all my certs from a single p12
4) went to window / mail / edit / mail preferences / security, and clicked on 
"security" to try to select a signing cert.
5) I got a crash in CERT_FindUserCertsByUsage . The line is :

		if ( CERT_MatchNickname(nnptr[n], node->cert->nickname) ) {

Debugging shows that the node->cert is correct, except that nickname is NULL.
This would tend to confirm my theory that the problems are due to NULL 
nicknames, except the other patch I made was for CA certs, not user certs.

If I skip step 2 in the test case above, the crash does not occur.
Also, if I exit the browser after step 2 and restart it, everything is OK too. 
This tells me that the P12 import is having a problem when the public key 
(certificate) is already in the temporary database.

Comment 14

[Robert Relyea]

Make sure you are using the most up-to-date versions of nss3.dll and
softokn3.dll (or libnss3.so and libsoftokn3.so).

There was a bug where we were not setting the nickname correctly that was fixed
in the last couple of weeks, but old binaries may still cause the problem.

bob

Comment 15

[Julien Pierre]

Bob,

I'm using my own build of the NSS DLLs, up-to-date. The problem still shows.
It is very hard to debug because every time I reproduce it, the entire mozilla 
profile gets corrupt - not just the cert and key dbs, and mozilla won't restart 
until I create a new one.

Comment 16

[Kai Engert (:KaiE:)]

>> every time I reproduce it, the entire mozilla profile gets corrupt

This sounds really evil. I have never seen something like that.

Do you know that you should never run Mozilla and a commercial Mozilla based
browser at the same time?

If you did not do that, I'd suspect that your NSS DLLs do no not fit the data
structures that Mozilla expects.

Comment 17

[Julien Pierre]

No, I'm not running mozilla and netscape6 at the same time.
I have done all my other patches to NSS for PSM using the same method of 
building - putting my own NSS and NSPR DLLs into the mozilla\bin directory. For 
all other purposes the browser has behaved fine.
I think the problem may be that at the time of the crash, when I try to select a 
signing cert, the preference file is open, and then it gets corrupt when the 
process crashes.

Comment 18

[Julien Pierre]

Further information :
The crash I get only occurs when I import a P12 containing a package of all my 
certificates (user certs for the past 3 years at netscape, and some test ones).

If I import a P12 containing only the most current cert and private key, the 
crash does not occur when looking up a signing certificate, but one is also not 
found in Mozilla (with no UI feedback in PSM - grr!).

It would appear that the problem is in the P12 import code, because my user 
certificate is not showing in PSM after import if I read the e-mail signed with 
the public cert before the import. Somehow the state of the temp DB is 
incorrect. If I close and restart mozilla, the user cert shows up as expected.

Comment 19

[Julien Pierre]

Changing the component to NSS.

I found the following code in the p12d.c P12 import code :

    testCert = PK11_FindCertFromDERCert(cert->slot, leafCert, wincx);
    CERT_DestroyCertificate(leafCert);
    /* if we can't find the certificate through the PKCS11 interface,
     * we should check the cert database directly, if we are
     * importing to an internal slot.
     */
    if(!testCert && PK11_IsInternal(cert->slot)) {
	testCert = CERT_FindCertByDERCert(CERT_GetDefaultCertDB(),
				 &cert->safeBagContent.certBag->value.x509Cert);
    }

    if(testCert) {
	if(!testCert->nickname) {
	    cert->removeExisting = PR_TRUE;
	}
	CERT_DestroyCertificate(testCert);
	if(cert->noInstall && !cert->removeExisting) {
	    return;
	}

The testCert is only found if the signed email was just read. The nickname is 
NULL and so cert->removeExisting gets set.

Later on, if that is set, the following function is called.

static SECStatus
sec_pkcs12_remove_existing_cert(sec_PKCS12SafeBag *cert, 
				void *wincx)
{
    SECItem *derCert = NULL;
    CERTCertificate *tempCert = NULL;
    CK_OBJECT_HANDLE certObj;
    PRBool removed = PR_FALSE;

    if(!cert) {
	return SECFailure;
    }

    PORT_Assert(cert->removeExisting);

    cert->removeExisting = PR_FALSE;
    derCert = &cert->safeBagContent.certBag->value.x509Cert;
    tempCert = CERT_DecodeDERCertificate(derCert, PR_FALSE, NULL);
    if(!tempCert) {
	return SECFailure;
    }

    certObj = PK11_FindCertInSlot(cert->slot, tempCert, wincx);
    CERT_DestroyCertificate(tempCert);
    tempCert = NULL;

    if(certObj != CK_INVALID_HANDLE) {
	PK11_DestroyObject(cert->slot, certObj);
	removed = PR_TRUE;
    } else if(PK11_IsInternal(cert->slot)) {
	tempCert = CERT_FindCertByDERCert(CERT_GetDefaultCertDB(), derCert);
	if(tempCert) {
	    if(SEC_DeletePermCertificate(tempCert) == SECSuccess) {
		removed = PR_TRUE;
	    } 
	    CERT_DestroyCertificate(tempCert);
	    tempCert = NULL;
	}
    }

    if(!removed) {
	cert->problem = PR_TRUE;
	cert->error = SEC_ERROR_NO_MEMORY;
	cert->noInstall = PR_TRUE;
    }
	
    if(tempCert) {
	CERT_DestroyCertificate(tempCert);
    }

    return ((removed) ? SECSuccess : SECFailure);
}

The function that ends up getting called is PK11_DestroyObject.
I'm not sure if that is correct.

I think the problem may be that the existing certificate is both in the perm DB 
and the temp db. It could also be a problem during the import of the public 
certificate when reading the email, and not setting the nickname properly.

Bob, your comments on this are welcome.

Comment 20

[Robert Relyea]

Hmmm. This looks like very old code used to get around the problem in NSS 3.3.
In NSS 3.4 we actually detect that the cert exists when we import it, then
update the cert nickname at that point.

I'm sure we aren't having the problem where the cert exists in the perm and temp
db because there is not temp db anymore. The certstore is completely different,
and certs in the certstore do not live in any token.

We may be running into a cache coherency problem where the PKCS #12 code is
deleting the underlying object, which may be preventing ian's code from making
changes to the object (because we've deleted it), so I think you are right, the
delete call is causing problems.

I think the best think to do is remove all of this detection code from PKCS #12
and let the pki layer handle it. Julian, try commenting out the fragment of code
you included in this bug report and see if this solves your problem.

bob

Comment 21

[Julien Pierre]

I commented the call to 
sec_pkcs12_remove_existing_cert that was made if removeExisting was set to true.
That did not make a difference.

I also tried to call that function twice, to see if it would fail the second 
time around, and it did not. 

The line
certObj = PK11_FindCertInSlot(cert->slot, tempCert, wincx);

still found the object both times, even though going through the rest of the 
function the first time should have deleted it.

Comment 22

[Julien Pierre]

Ian,

We think the cert cache may be getting out of sync here. Any idea ?

Comment 23

[Julien Pierre]

I just found out that my test also had the complication that I was using 
dual-key certs for S/MIME, ie. a separate keypair for signing and encrypting. 
But I was only using the signing half.

The part in my test case that receives the messages adds the signing cert to the 
database. The encryption cert did not get added. This may be because the message 
was sent from communicator which didn't know to add the separate encryption cert 
in the signature of the message.

The second part, the p12 import, was also an import of the signing certificate 
only, along with the private key - but without any encryption certs or private 
keys. It should just have added the private key for signing. In fact it does it 
permanently since this works after the browser is closed and restarted, but we 
need to keep the application running properly after the import.

Comment 24

[Ian McGreer]

This code is very confusing.  It looks like a series of workarounds and kluges
that are now conflicting with each other.

From what I can gather, the PKCS#12 library must remove an existing cert before
it can import the same cert with a key.  As Bob noted in comment #20, this is
not an NSS requirement.  If you attempt to import a cert that already exists in
NSS 3.5, NSS will only change the nickname if necessary.

But there are a lot of strange things in libpkcs12, so it is possible that if we
don't remove the cert, as Bob suggested, the key will not be imported.

The most glaring problem here is PK11_DestroyObject.  This call will completely
trash the cache for certs.  A cert in the cache will not know that it has been
removed, and further calls to do searches will still report the cert (as you
have noticed).  I *really* wish that call was not exported.  Fortunately, it is
marked as libsmime only, and its only use outside of libnss seems to be this one
here.

Therefore, I think the first step is to remove that call, and replace it with
SEC_DestroyPermCertificate.  I would rewrite sec_pkcs12_remove_existing_cert to
only do this:

	tempCert = CERT_FindCertByDERCert(CERT_GetDefaultCertDB(), derCert);
	if(tempCert) {
	    if(SEC_DeletePermCertificate(tempCert) == SECSuccess) {
		removed = PR_TRUE;
	    } 
	    CERT_DestroyCertificate(tempCert);
	    tempCert = NULL;
	}

Comment 25

[Julien Pierre]

Ian,

I tried that, but SEC_DeletePermCertificate fails.

Comment 26

[Ian McGreer]

Attachment: avoid uninitialized rv when deleting token object

I tried to figure out how SEC_DeletePermCertificate could fail, and I noticed
it is possible for NSS to return an uninitialized status variable in
devtoken.c.  This may be part of the problem.

Comment 27

[Julien Pierre]

Ian,

Thanks. Unfortunately that didn't fix the problem. Even though 
SEC_DeletePermCertificate now returns SECSuccess, what's displayed in the PSM UI 
is still inconsistent with the DB, and doesn't show the user certificate until 
after the browser is closed and restarted.

Comment 28

[Julien Pierre]

Bob helped me with a debug session today. It turns out this is a problem 
updating the certificate cache entry. The reason it showed up for me and not Bob 
is that I kept the mail window opened, which kept the cert in the cache, but Bob 
didn't.

The cert in the cache originally has no nickname. The problem is that the 
cert nickname fails to be updated after we import the private key. We thought we 
had nailed the problem to nssPKIObjectAddInstance failing to copy the label 
field. The call occurs in pk11cert.c at line 1764 .

However, I tried to make a patch to pkibase.c that makes it copy the label, but 
after I ran with it, I noticed the label in the source instance was not set 
either. This was different from what Bob and I had seen in the debugging 
session.

I was able to force and set it manually in the MSVC debugger and that got it to 
work right. I am wondering what the right course of action is at this point to 
fix the label. I think both the upper and lower levels are going to need to be 
patched. I'm including the patch to pkibase. It is not sufficient in itself 
since the instance passed to it still has a NULL label during import right now.

Comment 29

[Julien Pierre]

Attachment: patch to copy label in nssCryptokiObject

Comment 30

[Julien Pierre]

Attachment: patch to delete unnecessary code that removes existing certificate during p12 import

Comment 31

[Julien Pierre]

Comment on attachment 83836 [details] [diff] [review]
patch to delete unnecessary code that removes existing certificate during p12 import

Please review this patch.

Comment 32

[Ian McGreer]

Comment on attachment 83834 [details] [diff] [review]
patch to copy label in nssCryptokiObject

>Index: pkibase.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v
>retrieving revision 1.6
>diff -u -r1.6 pkibase.c
>--- pkibase.c	7 May 2002 14:58:12 -0000	1.6
>+++ pkibase.c	16 May 2002 03:45:06 -0000
>@@ -145,6 +145,14 @@
> 	for (i=0; i<object->numInstances; i++) {
> 	    if (nssCryptokiObject_Equal(object->instances[i], instance)) {
> 		PZ_Unlock(object->lock);
>+        if (!object->instances[i]->label && instance->label) {
>+            /* XXX need to copy label here */
>+            size_t labellen = strlen(instance->label)+1;
>+            object->instances[i]->label = nss_ZAlloc(NULL, labellen);
>+            if (object->instances[i]->label) {
>+                strcpy(object->instances[i]->label, instance->label);
>+            }
>+        }

The label lives in its own buffer.  Therefore, the copy is not necessary.
Also, we may need to handle a name change.  I would suggest this:

    if (instance->label)
	if (!object->instances[i]->label ||
	    !nssUTF8_Equal(instance->label, object->instances[i]->label, NULL))
{
	    /* Either the old instance did not have a label, or the label has
been updated */
	    nss_ZFreeIf(object->instances[i]->label);
	    object->instances[i]->label = instance->label;
	    instance->label = NULL;
	}
    } else if (!object->instances[i]->label) {
	/* The label was removed */
	nss_ZFreeIf(object->instances[i]->label);
	object->instances[i]->label = NULL;
    }

> 		nssCryptokiObject_Destroy(instance);
> 		return PR_SUCCESS;
> 	    }

Comment 33

[Ian McGreer]

Attachment: update object's label when it changes on token, rev 2

Here's my suggested patch, a little easier to read than the last comment.

Comment 34

[Ian McGreer]

Comment on attachment 83836 [details] [diff] [review]
patch to delete unnecessary code that removes existing certificate during p12 import

I think this is great.	What you propose to remove looks like a bunch of
piled-on ancient workarounds for bugs that no longer exist.  I, for one, am
glad to see that code go ;)

Comment 35

[Wan-Teh Chang]

Assigned the bug to Julien.

Comment 36

[Wan-Teh Chang]

Comment on attachment 83836 [details] [diff] [review]
patch to delete unnecessary code that removes existing certificate during p12 import

>Index: p12d.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v
>retrieving revision 1.13
>diff -u -r1.13 p12d.c
>--- p12d.c	10 May 2002 23:10:06 -0000	1.13
>+++ p12d.c	16 May 2002 03:53:14 -0000
>@@ -2240,7 +2240,7 @@
>     }
> 
>     cert->noInstall = PR_FALSE;
>-    cert->removeExisting = PR_FALSE;
>+    cert->removeExisting = PR_FALSE; /* this field no longer in use */
>     cert->problem = PR_FALSE;
>     cert->error = 0;
> 

If the removeExisting field is no longer in use, it would
be better to remove the field from the sec_PKCS12SafeBagStr
structure (defined in p12t.h).	If we can't remove the
field because of binary compatibility, we should also add a
"this field no longer in use" comment to the sec_PKCS12SafeBagStr
structure definition in p12t.h.

Comment 37

[Julien Pierre]

Wan-Teh, comment #36 :
Yes, it was for binary compatibility that I kept the field there. I agree to add 
it to the header and will add a patch for that.

Ian, comments #32, #33 :
The source instance gets destroyed in the code, therefore I don't think it's 
safe to just assign it as you propose.
The bigger problem still is that it's not getting passed from the higher level 
in my debug sessions right now - even the source instance has a NULL label ... 
So we still have to fix the PK11_ImportCert level.

Comment 38

[Ian McGreer]

Inside nssCryptokiObject_Destroy, there's a line:

    nss_ZFreeIf(instance->label);

By setting instance->label to NULL in the patch, after assigning its pointer
elsewhere, I prevent the string's memory from being freed.

Comment 39

[Julien Pierre]

Attachment: new patch for the pkcs12 library that renames removeExisting to unused and also removes one more obsolete usage with key->removeExisting

Comment 40

[Julien Pierre]

Ian, #38 :

Sorry, I had missed that.
I tried my test case again with the combination of all 3 patches. Unfortunately 
it asserts during the p12 import.

The stack is :

NTDLL! 77fa018c()
nsslowcert_AddPermNickname(NSSLOWCERTCertDBHandleStr * 0x03475ae8, 
NSSLOWCERTCertificateStr * 0x036a6880, char * 0x035d4fe0) line 3008 + 42 bytes
pk11_SetCertAttribute(PK11TokenObjectStr * 0x0355c320, unsigned long 3, void * 
0x034fdbc8, unsigned int 50) line 1300 + 26 bytes
pk11_forceTokenAttribute(PK11ObjectStr * 0x0355c320, unsigned long 3, void * 
0x034fdbc8, unsigned int 50) line 1428 + 21 bytes
pk11_forceAttribute(PK11ObjectStr * 0x0355c320, unsigned long 3, void * 
0x034fdbc8, unsigned int 50) line 1457 + 21 bytes
NSC_SetAttributeValue(unsigned long 16777228, unsigned long 4268362482, 
CK_ATTRIBUTE * 0x0012d098, unsigned long 2) line 3461 + 50 bytes
nssCKObject_SetAttributes(unsigned long 4268362482, CK_ATTRIBUTE * 0x0012d098, 
unsigned long 2, nssSessionStr * 0x03654e08, NSSSlotStr * 0x034a5e48) line 265 + 
25 bytes
nssToken_ImportCertificate(NSSTokenStr * 0x034a5490, nssSessionStr * 0x00000000, 
int 1, NSSItemStr * 0x0366784c, char * 0x034fdbc8, NSSItemStr * 0x03667854, 
NSSItemStr * 0x0366785c, NSSItemStr * 0x03667864, NSSItemStr * 0x0366786c, char 
* 0x00000000, int 1) line 592 + 36 bytes
PK11_ImportCert(PK11SlotInfoStr * 0x032b0e28, CERTCertificateStr * 0x03668018, 
unsigned long 0, char * 0x034fdbc8, int 1) line 1756 + 58 bytes
sec_pkcs12_add_cert(sec_PKCS12SafeBagStr * 0x03782ca0, int 0, void * 0x00000000) 
line 2352 + 24 bytes
sec_pkcs12_install_bags(sec_PKCS12SafeBagStr * * 0x01ee9938, void * 0x00000000) 
line 2821 + 32 bytes
SEC_PKCS12DecoderImportBags(SEC_PKCS12DecoderContextStr * 0x03782650) line 2850 
+ 22 bytes
PIPNSS! 03c4e94c()

The line of the assert is 3008 in pcertdb.c :

PORT_Assert(cert->nickname == NULL);

This is happening when trying to import the CA cert. The CA cert should already 
be present though, from the previous import of the e-mail cert.

Comment 41

[Julien Pierre]

Oops, sorry, my latest patch was all messed up, mix of previous works in the 
pkcs#12 library. Explains the crash.

Comment 42

[Julien Pierre]

Attachment: new patch for the pkcs12 library that renames removeExisting to unused and also removes one more obsolete using of key->removeExisting

Comment 43

[Julien Pierre]

Changing description to be more accurate.

OK, I put the correct patch now. With it doesn't crash.
But the original problem updating the nickname when adding the user cert is 
still not resolved.

Consider the code below.

    /* do the token import */
    certobj = nssToken_ImportCertificate(token, NULL,
                                         NSSCertificateType_PKIX,
                                         &c->id,
                                         nickname,
                                         &c->encoding,
                                         &c->issuer,
                                         &c->subject,
                                         &c->serial,
					 emailAddr,
                                         PR_TRUE);
    if (!certobj) {
	goto loser;
    }
    /* add the new instance to the cert, force an update of the
     * CERTCertificate, and finish
     */
    nssPKIObject_AddInstance(&c->object, certobj);

When we get to the last line, the certobj->label is NULL . So even though the 
function has been set to copy the label, it doesn't help.
There is probably a bug remaining in nssToken_ImportCertificate . The debugger 
shows the correct value for the nickname that's passed in to it.

So I traced that function and found this code :

	/* according to PKCS#11, label, ID, issuer, and serial number 
	 * may change after the object has been created.  For PKIX, the
	 * last two attributes can't change, so for now we'll only worry
	 * about the first two.
	 */
	NSS_CK_TEMPLATE_START(cert_tmpl, attr, ctsize);
	NSS_CK_SET_ATTRIBUTE_ITEM(attr, CKA_ID,    id);
	NSS_CK_SET_ATTRIBUTE_UTF8(attr, CKA_LABEL, nickname);
	NSS_CK_TEMPLATE_FINISH(cert_tmpl, attr, ctsize);
	/* reset the mutable attributes on the token */
	nssCKObject_SetAttributes(rvObject->handle, 
	                          cert_tmpl, ctsize,
	                          session, slot);


However, the rvObject->label field is left alone as NULL. I propose to add this 
just after the SetAttributes :

rvObject->label = nickname ;

I tried doing this, and it resolved the problem. I don't know what other side 
effects doing this may have. Bob, Ian, can you comment ?

Comment 44

[Julien Pierre]

Attachment: patch to set label in nssToken_ImportCertificate

obsoleting Ian's patch since this is in the same file

Comment 45

[Julien Pierre]

This last devtoken.c patch, which is a merge of Ian's and mine, finally takes 
care of it.
I tested all cases where I had issues before, importing the bundle of certs from 
the p12 or a single cert, and didn't encounter any odd situation.
All the certs showed up immediately after import without a refresh issue, and I 
was able to find the email certs immediately. S/MIME worked fine. So I think 
this is the end of it.

Bob, please review the latest attachment. Thanks !

Comment 46

[Ian McGreer]

Julien-
You marked attachment #83881 [details] [diff] [review] obsolete ("update object's label when it changes on
token, rev 2").  I think you may have meant to obsolete attachment #83792 [details] [diff] [review], since
your new patch subsumes it.

For the latest patch (attachment #83973 [details] [diff] [review]), you can use nssUTF8_Duplicate (see
base.h) instead of ZAlloc and strcpy.  That way it's more consistent with the
other Stan code.

Comment 47

[Julien Pierre]

Attachment: patch using nssUTF8_Duplicate as suggested by Ian

Comment 48

[Julien Pierre]

Cleaned up the patches. Ian, Bob, please review the 3 that are not marked 
obsolete.

Comment 49

[Robert Relyea]

Comment on attachment 84114 [details] [diff] [review]
patch using nssUTF8_Duplicate as suggested by Ian

r=relyea

Comment 50

[Ian McGreer]

Comment on attachment 84114 [details] [diff] [review]
patch using nssUTF8_Duplicate as suggested by Ian


not that we actually do super-reviews ;)

Comment 51

[Ian McGreer]

Julien, I submitted attachment #83881 [details] [diff] [review], so if you want to review it I think that
should suffice.

Comment 52

[Wan-Teh Chang]

Comment on attachment 84114 [details] [diff] [review]
patch using nssUTF8_Duplicate as suggested by Ian

>Index: devtoken.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v
>retrieving revision 1.22
>diff -u -r1.22 devtoken.c
>--- devtoken.c	7 May 2002 20:38:53 -0000	1.22
>+++ devtoken.c	17 May 2002 22:05:17 -0000
>@@ -301,9 +301,7 @@
>     if (createdSession) {
> 	nssSession_Destroy(session);
>     }
>-    if (ckrv != CKR_OK) {
>-	return PR_FAILURE;
>-    }
>+    status = (ckrv == CKR_OK) ? PR_SUCCESS : PR_FAILURE;
>     return status;
> }

One problem this change does not address is that 'status' may
have been set earlier:
    if (token->cache) {
	status = nssTokenObjectCache_RemoveObject(token->cache, instance);
    }
With this change, the return value of the nssTokenObjectCache_RemoveObject
call will be ignored. Is that OK?

Comment 53

[Ian McGreer]

Attachment: rev 2 of last patch

The function in question was removing an object from the token cache.  This
function should be void, because the cache has to handle any kind of failure
(either by self-healing or flushing).

Wan-Teh suggested we take this opportunity to make the function void (instead
of always returning PR_SUCCESS), so here is the last patch with that change.

Comment 54

[Julien Pierre]

I tested this latest patch and it is still OK with my test case.

Comment 55

[Julien Pierre]

I checked the patches to both NSS_3_5_BRANCH and the tip.

Branch :

Checking in devm.h;
/cvsroot/mozilla/security/nss/lib/dev/devm.h,v  <--  devm.h
new revision: 1.8.6.1; previous revision: 1.8
done
Checking in devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.22.2.1; previous revision: 1.22
done
Checking in devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.14.2.1; previous revision: 1.14
done
Checking in p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.13.2.1; previous revision: 1.13
done
Checking in p12t.h;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12t.h,v  <--  p12t.h
new revision: 1.1.178.1; previous revision: 1.1
done
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.6.2.2; previous revision: 1.6.2.1
done

Tip :

Checking in dev/devm.h;
/cvsroot/mozilla/security/nss/lib/dev/devm.h,v  <--  devm.h
new revision: 1.9; previous revision: 1.8
done
Checking in dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.23; previous revision: 1.22
done
Checking in dev/devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.15; previous revision: 1.14
done
Checking in pkcs12/p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.14; previous revision: 1.13
done
Checking in pkcs12/p12t.h;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12t.h,v  <--  p12t.h
new revision: 1.2; previous revision: 1.1
done
Checking in pki/pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.8; previous revision: 1.7
done

Comment 56

[Wan-Teh Chang]

Comment on attachment 83964 [details] [diff] [review]
new patch for the pkcs12 library that renames removeExisting to unused and also removes one more obsolete using of key->removeExisting

>Index: p12d.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v
>retrieving revision 1.13
>diff -u -r1.13 p12d.c
>--- p12d.c	10 May 2002 23:10:06 -0000	1.13
>+++ p12d.c	16 May 2002 23:16:02 -0000
>@@ -2240,7 +2240,7 @@
>     }
> 
>     cert->noInstall = PR_FALSE;
>-    cert->removeExisting = PR_FALSE;
>+    cert->unused = PR_FALSE;
>     cert->problem = PR_FALSE;
>     cert->error = 0;
> 

Julien, I think the "cert->unused = PR_FALSE;" assignment can be
deleted, correct?

Comment 57

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks! 

Comment 58

[lchiang]

charles - can you verify this bug fix in 1.01 branch?  When verified, pls
replace fixed1.0.1 keyword with verified1.0.1.  Thanks.

Comment 59

[Charles Rosendahl]

Verified - been fixed for a long time now.  Somehow the bug fell off of my radar
scope.