202979 - CERT_ImportCerts always returns SECSuccess

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

CERT_ImportCerts always returns SECSuccess, even though some certs may fail to
be installed. I recall there was some discussion about this function, but can't
seem to remember what it was.

In any case, DS is running into the following case :

They are installing a single CA cert with a nickname of "Certificate Manager".
The nickname conflicts with an existing cert of a different subject.

Therefore, the CERT_AddTempCertToPerm call fails . However, the exported
function CERT_ImportCerts still returns SECSuccess . It even returns a pointer
to a CERTCertificate in my test - but one without a slot set (ie. not a
permanent cert, even though keepCerts = PR_TRUE was passed in).

I believe the correct behavior is to return SECFailure in this case. However
there is some added complexity because the function can import more than one
cert. So do we report an error if any cert failed to install in the chain ?

At the very least, when there is a single cert imported as in this case, there
is no ambiguity, we should report the correct status.

Comment 1

[Julien Pierre]

FYI, CERT_ImportCerts ends like this :

    return(SECSuccess);
    
#if 0	/* dead code here - why ?? XXX */
loser:
    if ( retCerts ) {
	*retCerts = NULL;
    }
    if ( certs ) {
	CERT_DestroyCertArray(certs, ncerts);
    }    
    return(SECFailure);
#endif

The "return SECSuccess" at the end is the only return in the function.
The ifdef'ed code sees to have been there for a long time - even as far back as
my NSS 3.3 tree.

DS reported running into this in NSS 3.4 and 3.7

Comment 2

[Julien Pierre]

There is another comment in the "keepCerts" part that says :

/* don't care if it fails - keep going */

I don't understand why. Maybe the legacy semantics of the function is that we
ignore the failure in importing certs to the permanent DB. I think we may need
Bob Relyea's insight on this one.

Comment 3

[Julien Pierre]

Ulf,

Until the issue is resolve, I suggest the following workaround :
Check if retCerts is non NULL. There will be a CERTCertificate returned .
Check if the "slot" field is NULL. If it is, then the certificate failed to
import, most likely because of a nickname conflict. You can try to generate
another nickname and import again.

Ideally, you should pass NULL as the nickname when importing CA certs. This will
let NSS pick the cert nickname.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I think the function in question was originally intended to import certs from
an email message.  If we couldn't import them all, it wasn't a big deal, you
just import what you can.  Sounds like we need a function with different 
semantics than this one.  

In any case, this is a P2 at least, maybe a P1.

Comment 5

[Julien Pierre]

Nelson,

It appears the behavior of this API was changed by you in lib/certdb/certdb.c

revision 1.57
date: 2003/12/06 06:52:53;  author: nelsonb%netscape.com;  state: Exp;  lines:
+3 -14
CERT_ImportCerts now returns SECFailure when NONE of the certs was succesfully
imported.  r=wtc.  Bugscape bug 54311.

It seems that the bugscape bug referenced in the comment was a duplicate of this
bugzilla bug. Unfortunately, the information about why the API was changed isn't
public. I may very well have been involved at the time, but I don't remember.
Would you mind refreshing my memory and explaining why this decision was finally
made, over your own objections in comment #4 ?

This bug should be marked appropriately (probably "fixed", since the code
appears to have implemented my suggestion when I opened this bug).

It turns out that this API semantic change may be what's making the Sun DS
"plug-in signature" feature fail. I'm still investigating that.

Comment 6

[Julien Pierre]

The failure is because the DS tries to import zero certs (!) . This showed up as
breaking binary compatibility, since in 3.3 this would silently succeed, but now
it fails because code checked in by Nelson checks that the number of certs
imported is non-zero, regardless of how many certs were passed to the function
to import.

I think clearly DS shouldn't be trying to import zero certs. As far as the NSS
behavior in this case, it would probably be a good thing to return SECSuccess
and be a no-op when importing 0 certs, rather than returning SECFailure .

Comment 7

[Julien Pierre]

The zero cert import was actually triggered from the NSS PKCS#7 signature
verification code, not directly from DS, as follows.

=>[1] CERT_ImportCerts(certdb = 0x100295ac0, usage = certUsageObjectSigner,
ncerts = 0, derCerts = (nil), retCerts = 0xffffffff7fff6e08, keepCerts = 0,
caOnly = 0, nickname = (nil)), line 2254 in "certdb.c"
 [2] sec_pkcs7_verify_signature(cinfo = 0x1002a57b0, certusage =
certUsageObjectSigner, detached_digest = 0xffffffff7fff80b8, digest_type =
HASH_AlgSHA1, keepcerts = 0), line 1558 in "p7decode.c"
 [3] SEC_PKCS7VerifyDetachedSignature(cinfo = 0x1002a57b0, certusage =
certUsageObjectSigner, detached_digest = 0xffffffff7fff80b8, digest_type =
HASH_AlgSHA1, keepcerts = 0), line 1960 in "p7decode.c" 

But it turns out DS linked libsmime statically instead of dynamically, so it's
still using old 3.3 libsmime code - together with 3.9 libnss3/libssl .I tried to
LD_PRELOAD libsmime3.so, but that couldn't stop DS from using the old statically
linked libsmime PKCS#7 code.

However, I looked at the diffs in p7decode.c and it doesn't appear that the
problem of trying to import zero certs was fixed for NSS 3.9. I think we may
have to make a fix either in CERT_ImportCerts for this case, in the PKCS#7 code,
or possibly in both.

Comment 8

[Julien Pierre]

Attachment: don't report error when importing zero cert

We need to have this fix to preserve binary compatibility when moving from NSS
3.3 to 3.9 . Unfortunately, we can't easily patch the calling code that tries
to import zero cert .

Comment 9

[Julien Pierre]

Comment on attachment 147668 [details] [diff] [review]
don't report error when importing zero cert

Nelson, Wan-Teh, please review for integration into 3.9.2 . I would also like
to know when it is OK to check this in.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 147668 [details] [diff] [review]
don't report error when importing zero cert

The patch above introduces a different binary incompatibility with the 
old code.  In the case where nCerts == 0, it would return SECSuccess
without first setting  *retCerts = NULL.  Consequently, the caller 
could rightly assume that the value in *retCerts was now a valid pointer
to the allocated certs.  

I suggest the following alternative patch.  

@@ -2307,15 +2313,15 @@
	*retCerts = certs;
     } else {
	if (certs) {
	    CERT_DestroyCertArray(certs, fcerts);
	}
     }

-    return (fcerts ? SECSuccess : SECFailure);
+    return ((fcerts || !ncerts) ? SECSuccess : SECFailure);
 }

Comment 11

[Wan-Teh Chang]

I agree that Nelson's patch is better.  r=wtc.

Comment 12

[Julien Pierre]

Attachment: Nelson's patch

Comment 13

[Julien Pierre]

Comment on attachment 147686 [details] [diff] [review]
Nelson's patch

I agree this patch is better. I have checked this in on the tip. Need the tree
to open for NSS_3_9_BRANCH .

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.67; previous revision: 1.66
done

Comment 14

[Julien Pierre]

Also fixed this on NSS_3_9_BRANCH for 3.9.2 .

Comment 15

[Julien Pierre]

I'd like to understand the part of the code in CERT_ImportCerts that
ignoresimport errors :

		/* don't care if it fails - keep going */

This means CERT_ImportCerts will always succeed as long as the certs passed in
are valid DER and decode using CERT_NewTempCertificate . They can still fail to
be added to the perm db, however, but no error is reported for that case.

Why do we do that ?

I'm seeing this behavior happen in a case where we try to import a cert with
CERT_ImportCerts and there is a nickname clash. This is the case of a bogus
PKCS#12 file where two different certs (user cert and CA cert) are given the
same friendlyName . The user cert is imported first successfully against its
key, then we try to import the CA cert which has the same friendlyname, so we
pass the same nickname for that other cert, but CERT_AddTempCertToPerm fails,
then CERT_ImportCert still returns success anyway ... So, pk12util ends up
claiming the import is successful. But of course only one of the two certs
actually imported.

Nelson, Bob, do you have any comments on whether we should change this behavior
and return failure if the CERT_AddTempCertToPerm failed, in the case where
ncerts == 1 ?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

I think you want a function that is different from this one.
This one has a definition.  It isn't the definition you want.
You want to change it.  I think you want a different function.

I think the function you want would import a list of certs, and return
an array of structures, each of which included a cert handle and an error
code.  That would allow the caller to know which cert failed to import,
and why, and do something intelligent about it.

The existing interface doesn't provide any way to return to the caller
enough useful info.  Say you get an error about invalid DER.  Then what?
You don't know which cert was at fault.  

Rather than changing the semantics of this function more, and possibly 
breaking some code that uses it, I'd prefer to see a new function designed
that returns USEFUL info about the individual imports.  That function
need not declare failure if any cert import succeeds.  Rather, it returns
the individual results, and the caller can then easily decide if it wants
an all-or-nothing result.

Comment 17

[Julien Pierre]

Nelson,

A new function would be helpful for the case of multiple certs to get the
failure details. The definition doesn't have a way of reporting it.
However, the definition does have a PRBool . This PRBool used to return
SECSuccess all the time. I am only suggesting we use this PRBool.

In your own check-in comment as reported in comment #5, you wrote :
"CERT_ImportCerts now returns SECFailure when NONE of the certs was succesfully
imported.  r=wtc.  Bugscape bug 54311."

Clearly there was no chance to report the individual errors for each cert in the
above case, but you still made the function fail. However, the code is not in
accord with the comment, since technically the function still succeeds even
though none of the certs were imported in the case I reported (and where ncerts==1).

I would argue that the function should still fail even if there is not a way to
get to the detailed reason why for each cert. Also, in the case I'm most
concerned about (ncerts == 1), since there is only one cert, one can get the
error, but the caller still needs to know if the import failed in order to know
to check the error code.

Comment 18

[Robert Relyea]

Short answer:

It's that way because applications depend on it being that way!

The issue is in some cases applications call this function with a chain of 
certificates and the application doesn't care if all of the certificates import 
or not.

I agree it's probably not the best semantics, but changing them would break 
those applications.

The solution is a new function which has the semantics Nelson describes.

bob

Comment 19

[Wan-Teh Chang]

Marked the bug fixed again.