433108 - Can't delete nssCertificate without calling fill_CERTCertificateFields

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Nelson Bolyard (seldom reads bugmail)]

There needs to be a way to destroy the nssCertificate and the corresponding
CERTCertificate, even if one or both are not completely filled in, without
completely filling them in, just to destroy them.  This is a HUGE waste
of cycles.  

We do this for every cert that we pull out of the cert DB in the course
of validating a cert chain.  

In Stan functions nssCertificateArray_Destroy() and cert_destroyObject(),
both of which try to destroy certificate objects, we see code like this:

    NSSCertificate *c = (NSSCertificate *)o;
    if (c->decoding) {
	CERTCertificate *cc = STAN_GetCERTCertificate(c);
	if (cc) {
	    CERT_DestroyCertificate(cc);
	    return;
	} /* else destroy it as NSSCertificate below */
    }
    nssCertificate_Destroy(c);

STAN_GetCERTCertificate() checks to see that c->decoding is non-NULL, 
and if so, it returns  cc = (CERTCertificate *)c->decoding->data;

But before returning, it checks cc->nssCertificate, and if that is NULL
the code calls fill_CERTCertificateFields() to fill in all sorts of 
fields in the nssCertificate.  This includes such things as looking 
through all the tokens for a matching private key, which in turn 
involves searching through all the private keys in the token, because
of bug 433105.  

So, we go through all this enormous waste of CPU cycles, to completely
fill in the nssCertificate, WHICH WILL BE IMMEDIATELY DESTROYED.

I am conducting some experiments to see if there is an easier way to 
do this destruction without all that waste.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

There are more functions that do this same thing.  The list now includes
nssCertificateArray_Destroy() 
cert_destroyObject(),
CERT_CertChainFromCert(),
STAN_GetCERTCertificateOrRelease(),

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The issue is seen when the nssCertificate and its corresponding 
CERTCertificate have the following relationship:

nssCertificate
  .object.refCount == 2
  ->decoding->data == & CertCertificate

CERTCertificate
  .referenceCount  == 1
  .nssCertificate  == NULL;

I don't know how they get into that state, nor what code leaves them in that 
state.  I think we should have a name for this state (besides "broken" :).

The code shown in comment 0 has the effect of increasing the referenceCOunt
in the CERTCertificate, and changing its nssCertificate pointer to point at
the corresponding nssCertificate.  By itself, that would be OK, I think, 
but the problem is all the other huge amount of time spent filling in all
sorts of fields in the two structures that is entirely wasted effort, and 
that can have unexpected side effects, such as setting the error code.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Two corrections to previous comments. 

a) In comment 2, I wrote:
> The code shown in comment 0 has the effect of increasing the referenceCount
> in the CERTCertificate, and changing its nssCertificate pointer to point at
> the corresponding nssCertificate. 

That is untrue.  It does NOT increase the CERTCertificate's referenceCount.
It makes no changes to the reference count in either the CERTCertificate nor
the nssCertificate.  It DOES change CERTCertificate's nssCertificate pointer 
to point at the corresponding nssCertificate, among other changes.  

b) in comment 1, I stated there are 4 functions that are candidates to be 
changed.  That is untrue.  STAN_GetCERTCertificateOrRelease() is not a 
candidate.  The correct list of functions that should stop filling in cert
fields just to destroy the cert is:

nssCertificateArray_Destroy() 
cert_destroyObject(),
CERT_CertChainFromCert()

I have been trying to build a new function to replace the common body of 
code shown in comment 0.  Nothing I have tried so far has had acceptable
results.  

Bob, you're our Stan code expert.  If you understand how/why the certs get
into this imbalanced state, where nssCertificate points at CERTCertificate,
but CERTCertificate does not point to nssCertificate, and understand how
to destroy such an nssCertificate without having to fill in all the fields
first, please take this bug, or at least share your thoughts.  Thanks.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I have found something that seems to work, partially.  It avoids the calls
that attempt to fill in all the cert fields just before the certs are 
destroyed.  But it doesn't solve the problem of CERT_VerifyCert reporting
SEC_ERROR_BAD_DATABASE when using an error log.  :(

The thing that seems to work is this:  
>- if (c->decoding) {
>+ if (c->decoding && (!c->decoding->data || 
>+     ((CERTCertificate*)(c->decoding->data))->nssCertificate != 0)) {

This has the effect that, when c->decoding is non-NULL, and 
c->decoding->data (which is really the CERTCertificate address) is non-NULL,
but that CERTCertificate's nssCertificate pointer IS NULL, then just fall
through to the line that calls nssCertificate_Destroy(c).  

Of course, this is not a good patch.  For one thing, accesses to parts of 
c->decoding should be protected by the c object lock.  But I can see writing
a function named STAN_GetCERTCertificateToDestroy that does the right thing.

I'd write that patch, but I still haven't solved the problem of the bogus 
SEC_ERROR_BAD_DATABASE errors, and that's what I really want to solve. 
I can't help but wonder if this is a new problem in 3.12.  Perhaps it is a 
consequence of having libNSS3 and libsoftokn3 both use a common underlying
libutil shared library?  

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.