67243 - CERT_ImportCAChain sometimes returns SECSuccess even when certs failed to install

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

I have some certs that don't have any CA bits set that I am trying to import 
into the cert database using this function. 
The function returns SECSucess, but I see by tracing through that it didn't 
actually import the cert because the "CA" extension was not set on the 
certificate. I feel that the function should return SECFailure when any of the 
certs in the chain fail to install.

There also should be a way to find how many and which certs failed to be 
imported. I would suggest an extra "PRInt32* installed" argument where the 
number of certs installed would be returned. The function would abort at the 
first failure in the chain, so you would always know that the first "installed" 
certs were successful, if the function returned SECFailure. That would also tell 
you that the cert at index "installed" is the one that failed.

Comment 1

[Wan-Teh Chang]

Julien, is this a bug that can be deferred to NSS 3.3?

Assigned the bug to relyea.

Comment 2

[Julien Pierre]

I guess so. We can still find out about the failure in the next call when we 
look for the DER cert and don't find it.

I am adding Nelson to cc list since I had told him about this problem on monday. 

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

There's another bug in this function.

Here's a code snippet:

        /*
         * Make sure that if this is an intermediate CA in the chain that
         * it was given permission by its signer to be a CA.
         */
        isca = CERT_IsCACert(newcert, &certtype);

        if ( !isca ) {
            goto endloop;
        }

The comment states what the code __should__ be doing.  It states 
"if this is an intermediate CA".  But the code doesn't check to
see whether the cert is intermediate or root (self signed). 
This test ought not to apply to a self-signed cert, but does.

I'm not sure whether the fix belongs in this function or in 
CERT_IsCACert().

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The more I look at various places that call CERT_IsCACert, the
more I become convinced that it is the right place to make the
exception that self-signed certs (or, certs whose issuer and
subject names are the same) need not have the CA extensions
to be recognized as CA certs.  There are many functions that use
CERT_IsCACert that probably don't do the right thing for 
self-signed CA certs that lack BasicConstraints.

Comment 5

[Julien Pierre]

Nelson,

I agree that for the particular cert I gave you monday, you may want to fix it 
in CERT_IsCACert . However, there is the general possibility that one cert in a 
chain of certs will fail to import in this function, for a variety of reasons. 
The API needs to enhanced to provide more status about failures.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Julien, the workaround for this is simply.  
Import the certs one at a time, not as a group.
It is best if you proceed from the root to the leaf, if you can.

However, I think you'd be better served by a different function,
namely CERT_ImportCerts.  I will write you about it privately.

Comment 7

[Robert Relyea]

Julian was able to get his code to work with the work around, it's still not
clear that CERT_ImportCAChain is doing the corrrect thing, though, so defer this
bug to 3.3 rather than close it now.

Comment 8

[Wan-Teh Chang]

Bob, is this fixed or should we move the target to 3.4?

Comment 9

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 10

[Julien Pierre]

cc'ing Terry and Miodrag since this is an issue that has come up recently . The
bug mentioned that I found a workaround for this bug. I can't remember which one
it was

Comment 11

[Julien Pierre]

Just to note, this function does not always return SECSuccess on failure. It
still fails in some cases. I have just seen it fail when importing a single CA
cert that already existed in the database for example. I don't know if it is
supposed to. It could be argued that it isn't an import failure if we already
have an identical cert. This would be especially useful for an application
trying to install multiple certs in one call.

For now, the only reliable way for an application to install a cert chain is to
call this function for each cert, ignore its return code, and then check whether
it can find the cert it just imported by doing a CERT_FindCertByDERCert.

Comment 12

[Julien Pierre]

There was a comment in the NES code that said that this function would return
SECSuccess and not actually import the cert into the perm DB if it had already
been imported into the temp DB. The workaround was to destroy the temp DB cert
prior to the import. This may in fact be the cause of this problem.
This bug no longer exists as there is no longer a temp db. I removed the
workaround code and it worked fine with NSS 3.4 - even without destroying the
CERTCertificate that had been previously imported temporarily with
CERT_ImportCerts, the CERT_ImportCAChain succeeded in permanently adding the
certs to the DB.
I think this bug can be closed. However I will open another one for the other
case where I got a failure adding an existing cert.

Comment 13

[Julien Pierre]

Fixed in 3.4