CERT_SaveSMimeProfile only works if certificate is already in the certificate database

RESOLVED FIXED in 3.5

Status

NSS
Libraries
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Julien Pierre, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
CERT_SaveSMimeProfile is not doing the right thing when given a CERTCertificate
on a smartcard. It ends up in softoken trying to add an S/MIME record. But the
softoken code looks for a cert already stored in the certificate database before
adding an S/MIME record for it. The certificate is on the smartcard however, not
in the database, and therefore it isn't found in the database, and the addition
of the S/MIME record fails.

I believe the fix is to add a copy of the certificate to the database if the
certificate is not already there. This can be done in CERT_SaveSMimeProfile,
before going down into softoken.

Currently, it appears that the client is importing the certs manually into the
database before saving the S/MIME profile - but not for smartcards. Can anyone
think of a reason why we wouldn't want to always do it in this call ?
(Reporter)

Updated

16 years ago
Blocks: 114893
(Reporter)

Comment 1

16 years ago
I tried my suggested workaround of importing the cert into the database, but it
didn't work. The CERT_ImportCerts fails to save the cert to the database while
the smartcard is inserted. I have to remove the card for it to work.

Comment 2

16 years ago
Maybe PK11_ImportCert will work better?  Though I think that is what
CERT_ImportCerts ends up calling, so maybe not...
(Reporter)

Comment 3

16 years ago
Ian,

You are right, using the PK11 function resolved the problem.
(Reporter)

Comment 4

16 years ago
Created attachment 89187 [details] [diff] [review]
import certificate into the database when saving S/MIME profile

This patch imports certificates that aren't already in the database - ie. any
certs on external tokens, or temporary certs - which is a required step before
an S/MIME record can be created.
(Reporter)

Comment 5

16 years ago
Created attachment 89188 [details] [diff] [review]
correct patch

Remove unnecessary temp variable and insertion of PK11_ImportCert prototype in
the middle of the code (which actually compiled and ran happily with MS VC, not
even a warning, sigh ...)
Attachment #89187 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
The PK11_ImportCert prototype was in the declaration section,
which was legal.

Comment 7

16 years ago
Comment on attachment 89188 [details] [diff] [review]
correct patch


This patch leaks the internal slot twice.  You need to capture the result from
PK11_GetInternalKeySlot and free it.

Other than that, looks good to me.
Attachment #89188 - Flags: needs-work+
While the patch above may make things work, I wonder: 
Is this the right thing to do?

What value is served by copying the cert to the cert DB, other than 
making the code to create a PKCS7 profile work?  
The copied cert can be used to verify signatures, but not to create them.

Maybe we should change the PKCS#7 profile code so that it can create a 
profile even when the cert to which it points is on a different token.

Or maybe we should reconsider whether PKCS#7 profiles belong in PKCS#11 
tokens at all.
(Reporter)

Comment 9

16 years ago
Nelson's comments were answered in the NSS meeting today.
(Reporter)

Comment 10

16 years ago
Created attachment 89341 [details] [diff] [review]
patch not leaking slot, as checked in to NSS 3.5 and tip
Attachment #89188 - Attachment is obsolete: true
(Reporter)

Comment 11

16 years ago
Checked in to tip :

/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.50; previous revision: 1.49
done

And to NSS 3.5 :

Checking in stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.48.2.2; previous revision: 1.48.2.1
done
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

16 years ago
Moved the NSS_CLIENT_TAG so it will be in the trunk build.
Kai, Stephane, please add appropriate keywords for adt approval.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

16 years ago
Target Milestone: --- → 3.5
(Reporter)

Comment 13

16 years ago
Marking resolved. This fix is in NSS_3_5_BRANCH and the tip (NSS 3.6), but not
in mozilla 1.0.1 / netscape 7.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

16 years ago
Comment on attachment 89341 [details] [diff] [review]
patch not leaking slot, as checked in to NSS 3.5 and tip

>+    if (cert && cert->slot != internalslot) {
>+          /* this cert comes from an external source, we need to add it
>+          to the cert db before creating an S/MIME profile */
>+          rv = PK11_ImportCert(internalslot, cert,
>+              CK_INVALID_HANDLE, NULL, PR_FALSE);
>+
>+          PK11_FreeSlot(internalslot);
>+
>+          if (rv != SECSuccess )
>+          {
>+              return SECFailure;
>+          }
>+    } else {
>+        PK11_FreeSlot(internalslot);
>+    }

The body of the if statement seems to be indented by
six spaces.  The inner if has a different brace style.
(Assignee)

Comment 15

16 years ago
Comment on attachment 89341 [details] [diff] [review]
patch not leaking slot, as checked in to NSS 3.5 and tip

I am wondering if we can use the PK11_IsInternal function here
to avoid the else clause:

    if (cert && !PK11_IsInternal(cert->slot)) {
	PK11SlotInfo* internalslot = PK11_GetInternalKeySlot();

	/* this cert comes from an external source, we need to add it
	to the cert db before creating an S/MIME profile */
	rv = PK11_ImportCert(internalslot, cert,
	    CK_INVALID_HANDLE, NULL, PR_FALSE);

	PK11_FreeSlot(internalslot);

	if (rv != SECSuccess )
	    return SECFailure;
    }

Bob, Julien, is PK11_IsInternal(cert->slot) equivalent to
(cert->slot == PK11_GetInternalKeySlot()) here (ignoring the
slot reference leak)?
(Reporter)

Comment 16

16 years ago
Wan-Teh,

I don't think PK11_IsInternal provides the expected check. I believe it checks
if the slot is the internal slot for cryptographic operations, not the slot in
softoken that is holding private keys.

Comment 17

16 years ago
Not exactly, but close enought. PK11_IsInternal() will return true for the
crypto token as well as the key slot, but in this case the crypto token cannot
be the token we are looking at because the crypto token never stores certs (so
will never be placed on the cert->slot structure.)

Julian, using PK11_IsInternal would be the preferred way of doing this check
(it'll be both faster and more accurate and less chance of a slot leak.

bob

(Reporter)

Comment 18

16 years ago
Created attachment 90692 [details] [diff] [review]
updated patch

Here is a patch to apply to the tip (since the last one was already checked in)
that uses PK11_IsInternal for the check.
Note that it still has to call PK11_GetInternalKeySlot in the case where it
needs to do the import.
I believe the indentation is also fixed.
(Reporter)

Comment 19

16 years ago
Bob, please review the latest patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 20

16 years ago
Comment on attachment 90692 [details] [diff] [review]
updated patch

r=relyea
Attachment #90692 - Flags: review+
(Reporter)

Comment 21

16 years ago
Checked in the patch to the NSS tip.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

16 years ago
Comment on attachment 90692 [details] [diff] [review]
updated patch

>+    if (cert && cert->slot &&  !PK11_IsInternal(cert->slot)) {

I understand the purpose of the cert and cert->slot tests
is to prevent us from dereferencing a null pointer, but
there is no code that handles a null cert or cert->slot,
which I presume is an error condition.	This means a null
cert or cert->slot may still cause a crash later in the
function.  For example, if cert is null, we will skip the
if statement but will still crash in this line (which is
in the original code):
    emailAddr = cert->emailAddr;

I suggest that we either remove the cert and cert->slot
tests or add the error handling code if cert or cert->slot
is null.
(Reporter)

Comment 23

16 years ago
Wan-Teh,

You have a point about needing a check for a non-NULL cert argument, however I
think this was a pre-existing problem in the function not related to the new
block of code in this fix.
I will add a check for cert at the top of the function.
(Assignee)

Comment 24

16 years ago
I know that's a pre-existing problem.  I think it's a great
idea to test for null pointers, but it's also important to
return a failure status if a null pointer is an error
condition.  I know cert == NULL is an error condition, but
it's not obvious to me if cert->slot == NULL is an error
condition.

Thanks for your patience :-)
You need to log in before you can comment on or make changes to this bug.