215182 - certutil prints incorrect nickname

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Julien Pierre]

When listing certs through PK11_ListCerts, a cert list is returned which has a
special appData field containing a string with the origin of the certificate .
This field may be different from cert->nickname if there are multiple instances
of the certificate.

Currently, certutil displays the cert->nickname. When multiple instances of the
cert exist, that means the same token:nickname string is displayed n times,
rather than token:nickname, token n:nickname, etc .

Comment 1

[Julien Pierre]

Attachment: use appData field of CERTCertListNode in SECU_PrintCertNickname

1) Change SECU_PrintCertNickname to take CERTCertListNode instead of
CERTCertificate

2) Try to use node->appData field before cert->nickname

3) fix tabs in the code section I modified

4) fix ugly "goto defualt" !

Comment 2

[Julien Pierre]

Attachment: Use new SECU_PrintCertNickname syntax with node instead of cert

Comment 3

[Wan-Teh Chang]

Comment on attachment 129245 [details] [diff] [review]
use appData field of CERTCertListNode in SECU_PrintCertNickname

r=wtc.

Please fix the indentation of the following comment
before you check in.  The comment should be moved
to the right by two spaces to be aligned with "case".

>+		case SEC_OID_X509_SUBJECT_ALT_NAME:
>+		case SEC_OID_X509_ISSUER_ALT_NAME:
>+	      /*
>+	       * We should add at least some of the more interesting cases
>+	       * here, but need to have subroutines to back them up.
>+	       */
>+		case SEC_OID_NS_CERT_EXT_NETSCAPE_OK:
>+		case SEC_OID_NS_CERT_EXT_ISSUER_LOGO:

Comment 4

[Wan-Teh Chang]

Comment on attachment 129246 [details] [diff] [review]
Use new SECU_PrintCertNickname syntax with node instead of cert

r=wtc

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

certutil takes a command line argument that tells it which token to list.
IIRC, by default it should only list the internal DB token. 
There is also a special value, which IIRC is "-h all" that tells it to examine
all tokens.  
And there is the curious behavior that an unrecognized name ("-h foo") seems 
to select the "built-in root" token, or the union of DB and built-in tokens.

It seems to me that, unless "-h all" has been specified, then all the certs 
output by certutil -L should have the exact same token name prefix (which may 
be empty).  

In the original comment above, it is reported that some certs are listed 
multiple times.  This is presumably because those certs appear in multiple
tokens.  Therefore, one would expect that the correct solution is to list
each cert only once (or as many times as it appears in the token being 
searched), except when -h all has been specified.  So, IMO, the bug is 
not necessarily that the cert names are not prefixed with the token name,
but rather that the certs from multiple tokens are appearing in the output
of a command that asks only for the certs in one token.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I just remembered another reason that a cert will sometimes appear more than
once in a list, even with the same token name.  There may be multiple certs
with the same subject name, and hence with the same nickname.  How does this
patch affect that behavior, if at all?

Comment 7

[Julien Pierre]

Nelson,

To answer your comment #6 : this patch will not change the behavior for the case
of multiple certs with the same subject in the same token.

To answer comment #5 :

There are 2 ways that certutil uses to list certs.

First, if a given slot is specified (or if the softoken is implicitly specified,
by not specifying -h), then PK11_ListCertsInSlot is used to find the certificate.
This returns a CERTCertList . Each node contains a CERTCertificate and an
appDate field. The appData field is set to be the same as cert->nickname . See
the listCertsCallback in pk11wrap/pk11cert.c . Therefore, this patch will not
affect the certutil for this case.

The second way is that certutil uses PK11_ListCerts(PK11CertListUnique, ...) .
This call currently lists only one instance of each cert accross even if it
exists in multiple tokens. That behavior needs to be fixed. See bugs 215214,
215186 and 72291. When all those are fixed, the CERTCertList returned to
PK11_ListCerts by certutil will contain all the instances of all certificates.
However, per Ian, we can't have multiple CERTCertificates for different
instances of the same DER cert. So the only way to differentiate their origin in
the appData field in the CERTCertList . Right now, this appData field is being
set to the same as cert->nickname . So, trying to print appData before
cert->nickname will not affect the output in anyway until the above mentioned
bugs are fixed.

But when they are, it will be possible for the node->appData and
node->cert->nickname to be different. The patch simply tries to print the more
accurate origin of the cert in node->appData, since we can't have a list of
tokens in the public CERTCertificate structure which we can't safely modify and
add a list of tokens to.

Comment 8

[Julien Pierre]

Checked in :

Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.47; previous revision: 1.46
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.11; previous revision: 1.10
done
Checking in certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.80; previous revision: 1.79
done

Comment 9

[Wan-Teh Chang]

Attachment: find_objects_by_template is not setting *statusOpt before one return statement

I found this bug while reviewing the code near Nelson's patch.

Comment 10

[Wan-Teh Chang]

Comment on attachment 131921 [details] [diff] [review]
find_objects_by_template is not setting *statusOpt before one return statement

I attached the patch to the wrong bug.	Sorry.