227559 - __CERT_AddTempCertToPerm fails without setting error code

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

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 */
     }

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c

marking fixed.