115954 - CERTCertListNode nickname field is empty after PK11_ListCerts

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

In NSS 3.4 the root certs returned from PK11_ListCerts don't have a slot set.
This makes it impossible to display the slot/token name and breaks the web
server admin UI.
Specifically, the code that displays the cert in the CGI UI does :

PRInt32 mgcrt :: printcert(const SecCert& acert)
{
    // prints an individual cert
    CERTCertificate* cert = acert.cert;
    char * typestr;
    char * datestr;
    char * nn = (char*)acert.nickname;
    char * jsnn;
    char * escname;
    char * escpass = "";
    char * keyfilepw = "";
    char   cgiVar[512];
    PRInt32 flags = acert.flags;
    SECStatus rv;
    SECCertTimeValidity certtimestatus;
        
    if (!nn)
    {
        nn = (char *) cmgGetNameDesc(&cert->subject);
        if (!nn)
        {
            return SECSuccess;
        }
    }
        
    if ((!strcmp(nn, "Server-Cert")) || (flags & (CERTDB_USER))) {
        typestr = "Own";
    }
    else if (flags & CERTDB_VALID_CA)
    {
        if (flags & CERTDB_TRUSTED_CLIENT_CA)
        {
            if (flags & CERTDB_TRUSTED_CA)
            {
                typestr = "Trusted Client & Server CA";
            }
            else
            {
                typestr = "Trusted Client CA";
            };
        }
        else
        {
            if (flags & CERTDB_TRUSTED_CA)
            {
                typestr = "Trusted Server CA";
            }
            else
            {
                typestr = "Untrusted CA";
            };
        };
    }
    else if (flags & CERTDB_VALID_PEER)
    {      
        typestr = (flags & CERTDB_TRUSTED) ? "Trusted Peer" : "Untrusted Peer";
    }
    else if (flags & CERTDB_TRUSTED)
        typestr = "Trusted";
    else
    {
        PR_ASSERT(0);
        typestr = "";
        //typestr = "Server CA";
    };

    datestr = (char *)DER_UTCTimeToAscii(&cert->validity.notAfter);
    certtimestatus = CERT_CheckCertValidTimes(cert, PR_Now(), PR_FALSE);
    rv = SECSuccess;
    if (certtimestatus == secCertTimeExpired) {
        rv = SECFailure;
    }

    jsnn = cmgJSString(nn);
    escname = util_uri_escape(NULL, nn);
    
    if ( cert->slot)
    {
        sprintf(cgiVar, "%s%s", PK11_GetSlotName(cert->slot),
PK11_GetTokenName(cert->slot));

        keyfilepw = get_cgi_var(cgiVar, NULL, NULL);
        if (keyfilepw)
        {
            escpass = util_uri_escape(NULL, keyfilepw);
        };
    };

    printf("<tr><td><a
href=javascript:pop_up(\"security?cmd=sec-ecrt&nickname=%s&keyfilepw=%s\") ",
           util_uri_escape(NULL, (char*)acert.nickname), escpass);

    printf(SHOW_CERT " >", jsnn);
    printf("%s</a></td>", nn);
    printf("<td>%s</td><td>%s%s</td></tr>", typestr, datestr,
           rv ? "[*]" : "");
    PORT_Free(escname);
    PORT_Free(jsnn);
    return SECSuccess;
};

It never goes to the case after if (cert->slot) for root certs, which it did
with 3.3.2 correctly but no longer does with 3.4 . As a result the certs are
assumed to be in the internal token rather than in a PKCS#11 module. When a user
clicks on the link which passes the nickname and is supposed to contain the
slot/token name, the web server admin UI tries to lookup the cert with
CERT_FindCertFromNickname instead of PK11_FindCertFromNickname, and thus fails
to find the cert subsequently.

Comment 1

[Ian McGreer]

I can't reproduce this.  I tried by putting a call to PK11_ListCerts in certutil
and then listing the builtins, they all had a slot.

Unless something bad has happened (the cert was prematurely destroyed?), the
only way for slot to be NULL in NSS 3.4 is for the cert to be a temp cert.

Comment 2

[Wan-Teh Chang]

Assigned the bug to Ian.

Comment 3

[Julien Pierre]

Never mind, the cert->slot is set. The problem is that the nickname field of the
CERTCertListNode structure from PK11_ListCerts is NULL for the root certs. It
needs to be filled with a nickname including the token name, as it was in 3.3.2
. This is what breaks the UI.

Comment 4

[Ian McGreer]

The only thing I notice is that the nickname does not include the token name, as
it used to.  But it is present in node->appData, and node->cert->nickname.  This
is from calling PK11_ListCerts with the type PK11CertListCA.  If PK11_ListCerts
is called with PK11CertListUnique or PK11CertListRootUnique, node->appData will
not be set.  This has not changed from 3.2.2, as far as I can tell.

For now, I'm going to make sure the token name is included.  Can you tell me
what type of list PK11_ListCerts is called with?

Comment 5

[Julien Pierre]

Ian,

The problem is indeed that the token name is missing from the nickname in the
list structure. It was supposed to be there in order to resolve issues with
duplicate certs in different tokens.

I'm making the calls the following way :

void certcb :: populate()
{
    // get all the user certs
    CERTCertList* clist;
    clist = PK11_ListCerts(PK11CertListUser, NULL);
    populate(clist);

    // then get all the root certs
    clist = PK11_ListCerts(PK11CertListRootUnique, NULL);
    populate(clist);
}

I believe the reason for having the root certs with the unique parameter was to
resolve trust issues with the built-in certs so that they wouldn't show twice in
the UI if the trust bits had been changed from the default.
There are several ways that the UI code would get the token name. Either it
would get it from the list structure, if present, or if missing, it would do a
PK11_GetTokenName . But the name from the list always had the priority.

Comment 6

[Julien Pierre]

Ian,

Another problem I have seen is that occasionally, the root certs are not
displayed at all in the web server UI. This is an intermittent, random problem,
most likely depending on the state of memory, so unfortunately I do not have a
test case for it. I can see it on my machine right now though.

Comment 7

[Julien Pierre]

FYI I opened another bug on PK11_ListCerts for the root certs not showing. The
bug is 116315 - http://bugzilla.mozilla.org/show_bug.cgi?id=116315 .

Comment 8

[Ian McGreer]

AFAIK, this bug is fixed.  The intermittent failures in PK11_ListCerts is
another bug, so I'll close this one.  Please reopen if this isn't actually fixed.

Comment 9

[Julien Pierre]

Reopening this.
Unfortunately, this is broken again in the latest build of NSS 3.4 , 20020329.
After changing the trust on a root certificate, the token name is no longer 
returned by PK11_ListCerts in the CERTCertListNode . So the certificate just 
shows with the nickname.

The relevant admin UI web server code is below :

        // walk the list and add the certs to our own lists
        CERTCertListNode *cln;

        for (cln = CERT_LIST_HEAD(clist); !CERT_LIST_END(cln,clist);
                cln = CERT_LIST_NEXT(cln))
        {
            CERTCertificate* cert = cln->cert;
            const char* nickname = (const char*) cln->appData;
            if (!nickname)
            {
                nickname = cert->nickname;
            }

The cln->appData is set for root certs if the trust has not changed and 
contains the proper "Builtin Object Token:xxx" string.
After trust is changed, the value is NULL, so we grab cert->nickname ...

Comment 10

[Julien Pierre]

There may be a workaround on the application side, suggested by Ian, which would 
be to look for the DER of all CA certs in the built-in slot, and then if 
matching, display the built-in prefix. This would only work for the specific 
case of the built-ins though, not for other CA certs living in multiple tokens.

Therefore, an NSS fix would be preferable. I talked to Bob about it and he said 
basically the cert->slot should be set to a token other than the internal 
cert/key db token if the cert existed in multiple tokens. That works for the 
case of a CA cert existing in any external token and the key db, which is the 
case of the root certs that we are concerned with here.

I have been investigating the code in mozilla/security/nss/lib/pki/pki3hack.c . 
I tried to fix fillCERTCertificateFields and set the right slot, but could not. 
That function only got called once on my root cert with changed trust, and the 
cc->slot got set to the key db slot. So I would need to set the slot somewhere 
else. Bob suggested there is some code in the cert cache that deals with 
nicknames which would be the proper place to set this also. I couldn't find the 
appropriate functions in that directory. Most of them dealt with NSSCertificates 
rather than CERTCertificates.

Comment 11

[Wan-Teh Chang]

Julien, bug 129298 is about nicknames.  It may have the
information you need.

Comment 12

[Julien Pierre]

Thanks. I did notice the NSSCertificate_GetNickname function. I already even put 
code of my own in my tree to try to iterate through every token and not just 
take the first nickname. But in no case was I able to get more than one token 
name returned. Maybe I didn't do it right. I have to look at it some more 
tomorrow.

Comment 13

[Ian McGreer]

Attachment: a proposed workaround

This is an application-level workaround for this bug, the one I mentioned. 
Here it is implemented in certutil.  This is what the output looks like:

[k@24-164-150-83 certutil]$ ./Linux2.4_x86_glibc_PTH_DBG.OBJ/certutil -L -d
test -h all
Builtin Object Token--v
E-Certify CA						     c,c,c
Builtin Object Token:Verisign/RSA Secure Server CA	     C,C,p
Builtin Object Token:GTE CyberTrust Root CA		     C,C,C


I agree that it is limited, but it does work for this particular problem.

Comment 14

[Ian McGreer]

Comment on attachment 77457 [details] [diff] [review]
a proposed workaround


Sigh.  It doesn't work.  PK11_FindCertFromDERCert will return a cert that is
not on the slot if the cert is cached.	So certs that are on the softoken but
*not* the builtins will return from it.

Comment 15

[Ian McGreer]

Attachment: patch to NSS 3.4

As I thought, the code already existed to always choose other tokens over the
softoken when there are multiple instances of a cert.  It just wasn't being
called when the instances were added, so if the softoken comes first, its
instance is used.  This patch forces an update of CERTCertificate fields once a
new instance of the cert is discovered.  Here's what it looks like with regular
certutil:

[k@24-164-150-83 certutil]$ ./Linux2.4_x86_glibc_PTH_DBG.OBJ/certutil -L -d
test -h all
negative						     c,c, 
Builtin Object Token:E-Certify CA			     c,c,c
Builtin Object Token:Verisign/RSA Secure Server CA	     C,C,p

"negative" is a softoken cert, "E-Certify CA" is a cert on both the softoken
and builtin token, with the trust "c,c," on the softoken.

Comment 16

[Robert Relyea]

Comment on attachment 77460 [details] [diff] [review]
patch to NSS 3.4

OK, I'm assuming the ForceCERT call is in the correct spot (It looks right to
me).

The logic for prefering the non-internal token is in get_cet_instance (which is
already existing).

The two other pointers that are overridden are nickname and trust, both are
allocated out of the arena, so there's no leak.

Approval based on positive purify output.

bob

Comment 17

[Ian McGreer]

Attachment: two changes to last patch

1.  Include pki3hack.h in order to get definition of
STAN_ForceCERTCertificateUpdate

2.  change the check to:

if (cert->decoding && inCache)

The previous patch would force the update for all certs, instead of just ones
that had already been created.	That is, the code did not match the comment. 
This code does.

I have verified this by stepping through the debugger for various cases.  I
have verified that this patch only affects certs with multiple instances.  I
have run the patch through purify and saw no leaks.

Comment 18

[Wan-Teh Chang]

Ian, please go ahead and check in your patch on the trunk
and NSS_3_4_BRANCH.

Julien, please test this fix with the web server.  Thanks.

Comment 19

[Ian McGreer]

checked in to both places.

Comment 20

[Julien Pierre]

I tested this fix with the web server and it did not resolve the problem.

Comment 21

[Julien Pierre]

Seems there was a problem with my NSS 3.4 build. With a full rebuild it resolved 
the problem.

Comment 22

[Julien Pierre]

verified