Closed Bug 227559 Opened 17 years ago Closed 17 years ago

__CERT_AddTempCertToPerm fails without setting error code

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

Details

There is a common error path in  __CERT_AddTempCertToPerm that returns
without setting any error code.  The following patch shows the path and
the suggested fix.

 SECStatus
 __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname,
                       CERTCertTrust *trust)
 {
     NSSUTF8 *stanNick;
     PK11SlotInfo *slot;
     NSSToken *internal;
     NSSCryptoContext *context;
     nssCryptokiObject *permInstance;
     NSSCertificate *c = STAN_GetNSSCertificate(cert);
     context = c->object.cryptoContext;
     if (!context) {
+       PORT_SetError(SEC_ERROR_ADDING_CERT);
        return SECFailure; /* wasn't a temp cert */
     }
r=wtc.  I hope there is a better error code than
SEC_ERROR_ADDING_CERT, but I can't find one that
means "the cert wasn't a temp cert".

By the way, there is another error path in that
function that doesn't set an error code:

    if (!trust) {
        return SECSuccess;
    }

For this one we can set SEC_ERROR_INVALID_ARGS
because 'trust' is an input argument.
Some comments in response to the above feedback.

1. IINM, This error code was created not to explain why an add failed, but
rather to explain that some higher level function failed because its attempt to
add the cert failed.  SO, it's not ideal, but I could find no better code.

2. Perhaps NSS should return SECSuccess in this path, rather than SECFailure.  
This code path observes that the cert is not tied to any "context", which it 
would be if it was a "temp" cert, but this code makes no attempt to understand
WHY that is so.  Possible explanations would seem to include:
   a) the cert is already "perm" (a token object in some token), or
   b) an NSS error has left this cert dangling, neither temp nor perm.

It seems to me that if the cert is already "perm", perhaps this function 
should return secsuccess, saying "ok, you wanted it to be perm, and now it 
is."   

One trouble with this, however, is that the caller may think "OK, I just 
made it perm, so now I should set the trust to some default", when the cert
already had some trust set for it.  This is exactly what happens in
CERT_ImportCerts, which is why I didn't previous choose to change this
path to return SECSucess.  

In comment 1, I wrote:
>
> By the way, there is another error path in that
> function that doesn't set an error code:
>
>     if (!trust) {
>         return SECSuccess;
>     }
>
> For this one we can set SEC_ERROR_INVALID_ARGS
> because 'trust' is an input argument.

I just realized that that code path returns SECSuccess
and is therefore not an error path.  Sorry about the
confusion and please ignore the above comment.
Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c

marking fixed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.